-
Notifications
You must be signed in to change notification settings - Fork 30k
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
tickprocessor: pass proper arguments to /bin/sh
#8480
Conversation
cc @nodejs/v8 |
Should be backported everywhere cc @thealphanerd |
One more patch is needed to make this thing work properly on all systems, but I have to file it to V8 team first. Will link it here in a bit. |
Ah, actually this patch should be for node only... |
I'll push it to this branch too in a moment. |
`/bin/sh -c` trick wasn't working for several reasons: * `/bin/sh -c "..."` expects the first argument after `"..."` to be a `$0`, not a `$1`. Previously `-n` wasn't passed to `nm` because of this, and many symbols were ordered improperly * `c++filt` was applied not only to the names of the functions but to their `nm` prefixes like `t` and `a` (`t xxx` turns into `unsigned char xxx`). Instead of applying `c++filt` wide and using `sh -c`, execute `nm` as requested by `deps/v8/tools/tickprocessor.js` and apply `c++filt` to all matching entries manually. Included test demonstrates where previous approach failed: all builtins were merged into `v8::internal::Builtins::~Builtins`, because they were prefixed by `t` in `nm` output.
b3b244a
to
97bd7cd
Compare
Force-pushed proper fix, PTAL |
CI is green with unrelated |
Is there any chance at all that this fixes the flakiness on nearly all platforms of the existing test-tick-processor test? (See #4427.) I know nearly nothing about the V8 tick processor, so I'm asking from a point of near-complete ignorance. |
@Trott unlikely, this affects only macs. |
@bnoordhuis ping ;) |
ping @nodejs/v8 too |
LGTM |
Thank you, landing. |
Landed in 15d72c8. |
`/bin/sh -c` trick wasn't working for several reasons: * `/bin/sh -c "..."` expects the first argument after `"..."` to be a `$0`, not a `$1`. Previously `-n` wasn't passed to `nm` because of this, and many symbols were ordered improperly * `c++filt` was applied not only to the names of the functions but to their `nm` prefixes like `t` and `a` (`t xxx` turns into `unsigned char xxx`). Instead of applying `c++filt` wide and using `sh -c`, execute `nm` as requested by `deps/v8/tools/tickprocessor.js` and apply `c++filt` to all matching entries manually. Included test demonstrates where previous approach failed: all builtins were merged into `v8::internal::Builtins::~Builtins`, because they were prefixed by `t` in `nm` output. PR-URL: #8480 Reviewed-By: Matthew Loring <[email protected]>
|
||
function macCppfiltNm(out) { | ||
// Re-grouped copy-paste from `tickprocessor.js` | ||
const FUNC_RE = /^([0-9a-fA-F]{8,16} [iItT] )(.*)$/gm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a comment, just a note: this matches any hex digit string >= 8 && <= 16 characters long, not just strings of size 8 or 16. Ditto below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a copy-paste from V8, but I agree with you. It doesn't matter that much in the end, though.
Belated LGTM. |
`/bin/sh -c` trick wasn't working for several reasons: * `/bin/sh -c "..."` expects the first argument after `"..."` to be a `$0`, not a `$1`. Previously `-n` wasn't passed to `nm` because of this, and many symbols were ordered improperly * `c++filt` was applied not only to the names of the functions but to their `nm` prefixes like `t` and `a` (`t xxx` turns into `unsigned char xxx`). Instead of applying `c++filt` wide and using `sh -c`, execute `nm` as requested by `deps/v8/tools/tickprocessor.js` and apply `c++filt` to all matching entries manually. Included test demonstrates where previous approach failed: all builtins were merged into `v8::internal::Builtins::~Builtins`, because they were prefixed by `t` in `nm` output. PR-URL: #8480 Reviewed-By: Matthew Loring <[email protected]>
@indutny this lands cleanly on If I replace all instances of
Thoughts? |
I think the regex should be changed: Builtin_DateNow => Runtime_DateCurrentTime |
`/bin/sh -c` trick wasn't working for several reasons: * `/bin/sh -c "..."` expects the first argument after `"..."` to be a `$0`, not a `$1`. Previously `-n` wasn't passed to `nm` because of this, and many symbols were ordered improperly * `c++filt` was applied not only to the names of the functions but to their `nm` prefixes like `t` and `a` (`t xxx` turns into `unsigned char xxx`). Instead of applying `c++filt` wide and using `sh -c`, execute `nm` as requested by `deps/v8/tools/tickprocessor.js` and apply `c++filt` to all matching entries manually. Included test demonstrates where previous approach failed: all builtins were merged into `v8::internal::Builtins::~Builtins`, because they were prefixed by `t` in `nm` output. PR-URL: #8480 Reviewed-By: Matthew Loring <[email protected]>
landed in v4.x as 54c38eb changing the regex and let -> var got the tests passing. |
`/bin/sh -c` trick wasn't working for several reasons: * `/bin/sh -c "..."` expects the first argument after `"..."` to be a `$0`, not a `$1`. Previously `-n` wasn't passed to `nm` because of this, and many symbols were ordered improperly * `c++filt` was applied not only to the names of the functions but to their `nm` prefixes like `t` and `a` (`t xxx` turns into `unsigned char xxx`). Instead of applying `c++filt` wide and using `sh -c`, execute `nm` as requested by `deps/v8/tools/tickprocessor.js` and apply `c++filt` to all matching entries manually. Included test demonstrates where previous approach failed: all builtins were merged into `v8::internal::Builtins::~Builtins`, because they were prefixed by `t` in `nm` output. PR-URL: #8480 Reviewed-By: Matthew Loring <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
tickprocessor
Description of change
/bin/sh -c
trick wasn't working for several reasons:/bin/sh -c "..."
expects the first argument after"..."
to be a$0
, not a$1
. Previously-n
wasn't passed tonm
because ofthis, and many symbols were ordered improperly
c++filt
was applied not only to the names of the functions but totheir
nm
prefixes liket
anda
(t xxx
turns intounsigned char xxx
).Instead of applying
c++filt
wide and usingsh -c
, executenm
asrequested by
deps/v8/tools/tickprocessor.js
and applyc++filt
to allmatching entries manually.
Included test demonstrates where previous approach failed: all builtins
were merged into
v8::internal::Builtins::~Builtins
, because they wereprefixed by
t
innm
output.R= @bnoordhuis