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

deps: cherry-pick 4229ca2 from V8 upstream #14538

Closed

Conversation

jaimecbernardo
Copy link
Contributor

Running the profiler processor (--prof-process) with a profiler output file generated on Windows (with --prof) results in "UNKNOWN" code dominating the statistics. This is caused by the processor not correctly parsing the output of the "%p" format specifier on Windows.

This PR cherry-picks the upstream commit v8/v8@4229ca2 , that makes the output format be the same on Windows so it can be correctly parsed by --prof-process.

Original commit message:
[profiler] Fix logging addresses on Windows.

Change-Id: Iff0dcec95d04b85d31a452fed31b1500ad17a9f0
Reviewed-on: https://chromium-review.googlesource.com/591373
Commit-Queue: Jaroslav Sevcik [email protected]
Reviewed-by: Camillo Bruni [email protected]
Cr-Commit-Position: refs/heads/master@{#46976}

Fixes: #8221
Refs: v8/v8@4229ca2
Refs: #14510

/cc @nodejs/v8

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

deps,v8,win

Running the profiler processor (--prof-process) with a profiler
output file generated on Windows (with --prof) results in "UNKNOWN"
code dominating the statistics. This is caused by the processor not
correctly parsing the output of the "%p" format specifier on Windows.

This commit makes the output format be the same on Windows so it can
be correctly parsed by --prof-process.

Original commit message:
  [profiler] Fix logging addresses on Windows.

  Change-Id: Iff0dcec95d04b85d31a452fed31b1500ad17a9f0
  Reviewed-on: https://chromium-review.googlesource.com/591373
  Commit-Queue: Jaroslav Sevcik <[email protected]>
  Reviewed-by: Camillo Bruni <[email protected]>
  Cr-Commit-Position: refs/heads/master@{nodejs#46976}

Fixes: nodejs#8221
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jul 29, 2017
@tniessen
Copy link
Member

cc @addaleax

@addaleax
Copy link
Member

@jaimecbernardo Did you open a merge request upstream so that this might get into V8 6.0 upstream? That would be easiest because it would mean we’d pick it up anyway. There’s some info on how to do that in https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md.

If not, it might be easiest to rebase this PR on top of #14004 since it seems to have become consensus that we won’t do a release with the version of V8 that’s currently in master (5.9).

/cc @nodejs/v8

@jaimecbernardo
Copy link
Contributor Author

@addaleax Thank you. Will look into those first.

@jaimecbernardo
Copy link
Contributor Author

Opened an issue for a backport merge request upstream: https://bugs.chromium.org/p/v8/issues/detail?id=6650

@bnoordhuis
Copy link
Member

It was accepted for back-merge to 6.1 and 6.0. Does that mean this can be closed?

@jaimecbernardo
Copy link
Contributor Author

Not sure. While approved, it doesn't seem to have been merged.
I've asked if some more action could be done merge it, but I haven't had an answer yet.
https://bugs.chromium.org/p/v8/issues/detail?id=6650#c4

@targos
Copy link
Member

targos commented Aug 28, 2017

/cc @fhinkel @ofrobots can you help to get this merged?

@ofrobots
Copy link
Contributor

Apologies for the delay. I've merged to 6.1: https://github.com/v8/v8/commits/889f5cc1e9574a18ee54d44f2643e7db284612bc. Unfortunately, during this time, the 6.0 branch moved from stable to unsupported, and I am not sure if it is going to be possible to merge it to 6.0 upstream.

@BridgeAR
Copy link
Member

Closing this as the patch is now on master due to the upstream merge.

@jaimecbernardo thanks a lot for your contribution anyway!

@BridgeAR BridgeAR closed this Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

32bit address given for 64bit builds using --prof on Windows
10 participants