Support for retrieving type name on a Stackframe

[Dominik Gruber]

Dear v8 community,

Background: The native API is lacking the functionality "GetTypeName" on StackFrame objects contrary to Callsites where this feature is available to the managed API.

We have implemented and tested this feature with Nodejs and would like to contribute those changes to the v8 source and get that feature into Nodejs as soon as possible.

Can someone guide me through the contribution process regarding issue creation, code review, merge to Nodejs?

Thanks,

Dominik

[Jakob Gruber]

+Simon Zünd +Yang Guo 

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/7b6bdc89-ec2f-4594-8c4f-7b7af4b426d2%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Simon Zünd]

Hi Dominik,

for general V8 contribution guidelines, please follow https://v8.dev/docs/contribute. There is also currently some work going regarding stack traces. Feel free to use the same tracking bug (https://crbug.com/v8/8742) for your CLs. You can send your CLs to me for review and I will add additional reviewers as necessary.

AFAIK Node 12 will stay at V8 API compatibility of 7.5 (the current V8 version being worked on is 7.6). Meaning your change won't make it into Node 12. Note that the contribution process for Node itself differs from V8.

Cheers,

Simon

[Yang Guo]

I think additive changes to V8's API are fine. E.g. adding a new API does not break native modules compiled against a version of V8 that does not yet have that new API.

Cheers,

Yang

Yang Guo | Google Germany GmbH | Erika-Mann-Str. 33 | 80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind, leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen Sie die E-Mail und alle Anhänge. Vielen Dank. This e-mail is confidential. If you are not the right addressee please do not forward it, please inform the sender, and please erase this e-mail including any attachments. Thanks.

[Ben Noordhuis]

On Thu, May 16, 2019 at 2:10 PM 'Simon Zünd' via v8-dev
<v8-...@googlegroups.com> wrote:
> AFAIK Node 12 will stay at V8 API compatibility of 7.5 (the current V8 version being worked on is 7.6). Meaning your change won't make it into Node 12. Note that the contribution process for Node itself differs from V8.

Additive V8 changes are acceptable in Node.js provided they've been
merged in upstream V8 first and API and ABI are maintained.

A new, non-virtual `StackFrame::GetTypeName()` method should be no
problem and could land in a minor v12.x release.

@Dominik: https://github.com/nodejs/node/pull/27729 is an example of
cherry-picking a patch from upstream. Target the master branch,
releasers will take care of back-porting it to release branches.

[Dominik Gruber]

Hi Simon,

Dynatrace has already signed the CLA.

I am currently porting the changeset to our v8 fork. Is it sufficient if I provide the link to the git repo with the changelist for reviewing?

Thanks,

Dominik

On Thursday, May 16, 2019 at 2:10:09 PM UTC+2, Simon Zünd wrote:

Hi Dominik,

for general V8 contribution guidelines, please follow https://v8.dev/docs/contribute. There is also currently some work going regarding stack traces. Feel free to use the same tracking bug (https://crbug.com/v8/8742) for your CLs. You can send your CLs to me for review and I will add additional reviewers as necessary.

AFAIK Node 12 will stay at V8 API compatibility of 7.5 (the current V8 version being worked on is 7.6). Meaning your change won't make it into Node 12. Note that the contribution process for Node itself differs from V8.

Cheers,

Simon

From: Jakob Gruber <jgr...@chromium.org>
Date: Thu, May 16, 2019 at 2:01 PM
To: <v8-...@googlegroups.com>, Simon Zünd, Yang Guo

+Simon Zünd +Yang Guo 

From: Dominik Gruber <domini...@dynatrace.com>
Date: Thu, May 16, 2019 at 1:58 PM
To: v8-dev

Dear v8 community,

Background: The native API is lacking the functionality "GetTypeName" on StackFrame objects contrary to Callsites where this feature is available to the managed API.

We have implemented and tested this feature with Nodejs and would like to contribute those changes to the v8 source and get that feature into Nodejs as soon as possible.

Can someone guide me through the contribution process regarding issue creation, code review, merge to Nodejs?

Thanks,

Dominik

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to v8-...@googlegroups.com.

[Simon Zünd]

Unfortunately we don't accept patches from Github PRs and we don't review code on Github. Please follow the instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/contributing.md#Uploading-a-change-for-review. This way, the CL can run against our Try bots and the CL can properly be reviewed in Gerrit.

To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/2eecbe69-7b7d-42f5-8437-f4fe7a2b1158%40googlegroups.com.

[Dominik Gruber]

Hi Simon,

can you give me an example on how to run a test procedure from "test-api-stack-traces.cc"? Running "tools/run-tests.py --outdir=out/x64.release cctest" does not result in any test runs. I guess I have to define a target, but I am struggling to find the correct parameters.

Thanks,

Dominik

On Monday, May 20, 2019 at 1:40:40 PM UTC+2, Simon Zünd wrote:

Unfortunately we don't accept patches from Github PRs and we don't review code on Github. Please follow the instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/contributing.md#Uploading-a-change-for-review. This way, the CL can run against our Try bots and the CL can properly be reviewed in Gerrit.

[Jakob Gruber]

Shot in the dark: are you building with `v8_static_library = true`? cctests unfortunately don't work in that config.  

To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/363c0022-4d1d-4aee-ae36-6a7589b7fb1e%40googlegroups.com.

[Jakob Kummerow]

On Wed, May 22, 2019 at 10:22 AM Dominik Gruber <dominik...@dynatrace.com> wrote:

can you give me an example on how to run a test procedure from "test-api-stack-traces.cc"? Running "tools/run-tests.py --outdir=out/x64.release cctest" does not result in any test runs.

That should work, as in: it works for me. Make sure you have compiled cctests first, either with ninja -C out/x64.release cctest or with tools/dev/gm.py x64.release cctest.

You can also save some time by only running the tests you care about: tools/run-tests.py --outdir=out/x64.release cctest/test-api-stack-traces/*. With gm.py, you can even conveniently build and run with a single command: tools/dev/gm.py x64.release cctest/test-api-stack-traces takes care of everything.

[Dominik Gruber]

Indeed, good catch. Did a rebuild and tests run now. Thanks!

On Wednesday, May 22, 2019 at 10:34:00 AM UTC+2, Jakob Gruber wrote:

Shot in the dark: are you building with `v8_static_library = true`? cctests unfortunately don't work in that config.  

[Dominik Gruber]

I did add a test and stumbled upon the following issue:

// following test code to test the GetTypeName feature:
function bar() {

};

bar.prototype.x = function foo() {

    AnalyzeStackInNativeCode();

}

var baz = new bar();

baz.x(); // returned typename = "bar" as expected

baz.x.call("aString"); // typename is "" but I had expected it to be "String"

I tracked the missing code for getting the typename down to "factory.cc" and the "NewStackFrameInfo"-ctor where I reused the already exisiting "GetTypeName" function from the FrameArrayIterator. The same API call is also used in CallSitePrototypeGetTypeName so I assumed that would work here as well. Was my assumption false? Or is that test code not meant to run in this test context? 

Any help pushing me in the right direction is highly appreciated.

Thanks,

Dominik

[Dominik Gruber]

I hope I have not been too unconcise in my description of the issue.

Just wanted to figure if the underlying API calls suffice any use cases of getting the typename:

In the Factory::NewStackFrameInfo-ctor I use the already in-place function "it.Frame()->GetTypeName()" in order to retrieve the typename.

Initial testing with the updated v8 and a custom built Node had shown this change did work fine (did not test changing the "this" though).

Unit testing has revealed that if I use "call" on a function object and set another "this" no typename is delivered at all (in contrast to using the Callsites-API).

Is "it.Frame()->GetTypeName()" supposed to deliver the typename also in the use-case of changing the "this"? If the answer is yes it looks like a bug and I am more than willing to track it down.

If the answer is no it looks like I am missing something in my code. Any pointers welcome.

It just does not feel right as "it.Frame()->GetTypeName()" delivers the correct typename if I keep the "this" unchanged.

Thanks,

Dominik

[Simon Zünd]

This seems like a bug.

One thing that could interfere here is the stack frame cache. Stack frames collected via the API are cached. I encountered another bug in the stack frame caching code while working on some other unrelated stack trace part, but I haven't gotten around to investigate and fix it.

To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/1bf1b206-45d6-4bf2-82ed-ae64b988acc6%40googlegroups.com.