Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src,etw: fix event 9 on 64 bit Windows #15563

Closed

Conversation

joaocgreis
Copy link
Member

The event manifest specifies the MethodID field as a 32 bit integer. The 32 bit node executable publishes this correctly, but the 64 bit executable publishes a 64 bit integer, making the event undecodable.

This fix makes the event decodable again by publishing it as 0 in 64 bit systems. This way the manifest is not changed, so this fix is semver-patch and can be backported to LTS. Note there are already other dummy values on this event and the address is already published correctly as a pointer in another field. In the future, this should probably be changed to a meaningful value provided by the engine in use.

cc @nodejs/platform-windows

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src, etw.

The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable publishes a 64 bit integer, making the event undecodable.
@joaocgreis joaocgreis added lts-watch-v6.x windows Issues and PRs related to the Windows platform. labels Sep 22, 2017
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. windows Issues and PRs related to the Windows platform. labels Sep 22, 2017
@joaocgreis
Copy link
Member Author

@refack
Copy link
Contributor

refack commented Sep 22, 2017

In the future, this should probably be changed to a meaningful value provided by the engine in use.

IMHO assigning addr1 to the ID is super arbitrary even on x32
JaneaSystems@66f64ae

@mscdex mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Sep 22, 2017
@@ -253,7 +247,11 @@ void NODE_V8SYMBOL_ADD(LPCSTR symbol,
}
void* context = nullptr;
INT64 size = (INT64)len;
INT_PTR id = (INT_PTR)addr1;
#if defined(_WIN64)
INT32 id = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not just use the lower 32 bits on 64 bit? That is, why not just have the non-64 bit version on 64 bit too?

Also agree with @refack that using addr1 as the id is somewhat arbitrary but I don't know if v8 has a better (and more stable/deterministic) id that we can use here- in ChakraCore we use our "function id" which I believe is somewhat stable across runs of node because it's incremented in order of in which the function is compiled- this allows us to collect a trace, and then run with additional tracing flags for just that function id to get more details on that function. Does v8 have something stable like that that we can use here? (cc @fhinkel)

@joaocgreis
Copy link
Member Author

Why do we not just use the lower 32 bits on 64 bit? That is, why not just have the non-64 bit version on 64 bit too?

Do we know that the lower 32 bits won't overlap? If there's something that limits the addresses to a continuous 4GB space we could do that. Either way, I'm not sure we should encourage the use of this ID in it's current form, and the address is already available in the field MethodStartAddress.

@digitalinfinity
Copy link
Contributor

We don't know that the bits wont overlap but in practice unless the app uses > 4GB, I believe the Windows memory manager would continue to allocate it in a contiguous address space (at least, that was my recollection from how ChakraCore organized its pages 😄)- but even if that assumption was incorrect, probabilistic overlapping is still probably better than the current behavior which is guaranteed collision- and while I agree we shouldn't encourage use of this ID, the Windows tools that decode JS stacks (Windows Performance Analyzer/VS etc) probably already have a dependency on this ID being more or less unique (I think the real solution here is to have a stable id that's provided by v8, but in the interim my instinct is that the lower 32 bits is better than nothing)

@joaocgreis
Copy link
Member Author

@digitalinfinity if you believe having the value is useful, I'm ok with having it. Updated, PTAL.

Copy link
Contributor

@digitalinfinity digitalinfinity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@refack
Copy link
Contributor

refack commented Sep 27, 2017

if you believe having the value is useful, I'm ok with having it. Updated, PTAL.

The upside I see is less code ;)

joaocgreis added a commit that referenced this pull request Sep 28, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@joaocgreis
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/10299/ , failures unrelated but ran linter again: https://ci.nodejs.org/job/node-test-linter/12074/ .

Landed in 510cab7

@joaocgreis joaocgreis closed this Sep 28, 2017
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Sep 28, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: nodejs#15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 30, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: nodejs/node#15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@MylesBorins MylesBorins mentioned this pull request Oct 3, 2017
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
MylesBorins pushed a commit that referenced this pull request Oct 17, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@MylesBorins MylesBorins mentioned this pull request Oct 17, 2017
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
The event manifest specifies the MethodID field as a 32 bit integer.
The 32 bit node executable publishes this correctly, but the 64 bit
executable published a 64 bit integer, making the event undecodable.

PR-URL: #15563
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants