tools: pass explicit section list to objdump by citrus-it · Pull Request #49992 · nodejs/node

citrus-it · 2023-10-01T11:41:57Z

Objdump in recent versions of GNU binutils ignores bss sections unless they are specifically selected with the '-j' option. Symbols from these sections are needed for dtrace support. To fix this while retaining compatibility with previous versions of binutils, change to a two pass approach. The first pass extracts the list of unique sections, and the second pass invokes objdump with '-j' options for each of them.

Fixes: #49991

Objdump in recent versions of GNU binutils ignores bss sections unless they are specifically selected with the '-j' option. Symbols from these sections are needed for dtrace support. To fix this while retaining compatibility with previous versions of binutils, change to a two pass approach. The first pass extracts the list of unique sections, and the second pass invokes objdump with '-j' options for each of them. Fixes: nodejs#49991

citrus-it · 2023-10-01T11:44:05Z

I have tested this change on OmniOS r151046, which uses binutils 2.40, and on OmniOS r151048, which has binutils 2.41.

On both systems, it is now possible to compile nodejs.
On r151046, the generated content of out/Release/obj/gen/v8constants.h was the same with and without this patch.

nodejs-github-bot · 2023-10-01T12:44:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/54439/

bnoordhuis · 2023-10-01T18:56:18Z

You can remove the file, it's no longer used. I removed the dtrace support last year but I left in genv8constants.py because I had this vague notion it was used by llnode but that's the gen-postmortem-metadata.py script.

bnoordhuis · 2023-10-03T10:50:24Z

Replaced by #50023. Thanks anyway for the pull request.

yasuhirokimura · 2023-10-29T06:50:37Z

Hello,

I'm one of FreeBSD Ports Collection's maintainers.
I'm working to update our binutils port from 2.40 to 2.41 and facing issue reported as #49991 now.
Currently there are version 16, 18 and 20 of Node.js ports in our ports collection (I know version 16 has already reached it's EoL but we still need it as dependency of some other ports), and build of version 16 and 18 fails because of same error as one described in #49991.
IIUC support of DTrace was removed with version 19 so the change in binutils 2.41 doesn't affect to version 20 but still affects to version 18 and 16.
I googled the error message, found this pull request and tried the change in citrus-it@4ecd40f with our Node.js version 16 and 18 ports.
But unfortunately build of them still fails with same error.
Does this mean the change doesn't fix the issue if version of Node.js is 16 or 18?
Or did I do something wrong?

bnoordhuis · 2023-10-29T09:25:08Z

@yasuhirokimura you build with ./configure --with-dtrace? Does it build correctly without that flag? What errors, if any, do you get in both cases?

yasuhirokimura · 2023-10-29T20:06:06Z

@bnoordhuis

Our Node.js version 16 and 18 ports have DTRACE option, and it provides user with the way to toggle support of DTrace.
If DTRACE option is enabled/disabled, configure script is invoked with --with-dtrace/--without-dtrace respectively.
DTRACE option is enabled by default, and if it is enabled build of them with binutils 2.41 fails with following error message: dtrace: failed to compile script src/v8ustack.d: line 1: failed to resolve V8DBG_OFF_FP_CONTEXT: Unknown variable name.
If the option is disabled, build of both ports with binutils 2.41 finishes successfully.

bnoordhuis · 2023-10-29T21:34:15Z

Right, that does indeed look like the issue this PR tried to address. I don't really have a good suggestion how to fix it except to build with --without-dtrace going forward.

yasuhirokimura · 2023-10-30T01:19:47Z

@bnoordhuis Thanks for suggestion. I'll propose the removal of DTrace support to the maintainer of Node.js ports unless the change of citrus-it@4ecd40f fixes the issue.

@citrus-it Would you mind my asking which version of Node.js you used to test the change you propose with this pull request?

… 2.41 * With binutils 2.41 build fails as following. LD_LIBRARY_PATH=/wrkdirs/usr/ports/www/node16/work/node-v16.20.1/out/Release/lib.host:/wrkdirs/usr/ports/www/node16/work/node-v16.20.1/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /wrkdirs/usr/ports/www/node16/work/node-v16.20.1/out/Release/obj.target/libnode/src; dtrace -64 "-I/wrkdirs/usr/ports/www/node16/work/node-v16.20.1/out/Release/obj/gen" -Isrc -C -G -s src/v8ustack.d -o "/wrkdirs/usr/ports/www/node16/work/node-v16.20.1/out/Release/obj.target/libnode/src/node_dtrace_ustack.o" dtrace: failed to compile script src/v8ustack.d: line 1: failed to resolve V8DBG_SMITAG: Unknown variable name * This is because behaviour of objdump has changes with binutils 2.41. [1][2] * On upstream DTrace support itself has been removed with Node.js 19.x. * There is a pull request that says it fixes build error. [3] I tried it but unfortunately it doesn't work as is expected. * Upstream suggests me to remove DTrace support. [4] Reference: nodejs/node#49991 (comment) [1] Reference: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=0a3137ce4c4b38ee8 [2] Reference: nodejs/node#49992 [3] Reference: nodejs/node#49992 (comment) [4]

… 2.41 * With binutils 2.41 build fails as following. LD_LIBRARY_PATH=/wrkdirs/usr/ports/www/node18/work/node-v18.18.2/out/Release/lib.host:/wrkdirs/usr/ports/www/node18/work/node-v18.18.2/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /wrkdirs/usr/ports/www/node18/work/node-v18.18.2/out/Release/obj.target/libnode/src; dtrace -64 "-I/wrkdirs/usr/ports/www/node18/work/node-v18.18.2/out/Release/obj/gen" -Isrc -C -G -s src/v8ustack.d -o "/wrkdirs/usr/ports/www/node18/work/node-v18.18.2/out/Release/obj.target/libnode/src/node_dtrace_ustack.o" dtrace: failed to compile script src/v8ustack.d: line 1: failed to resolve V8DBG_SMITAG: Unknown variable name * This is because behaviour of objdump has changes with binutils 2.41. [1][2] * On upstream DTrace support itself has been removed with Node.js 19.x. * There is a pull request that says it fixes build error. [3] I tried it but unfortunately it doesn't work as is expected. * Upstream suggests me to remove DTrace support. [4] Reference: nodejs/node#49991 (comment) [1] Reference: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=0a3137ce4c4b38ee8 [2] Reference: nodejs/node#49992 [3] Reference: nodejs/node#49992 (comment) [4]

citrus-it · 2023-11-01T14:41:25Z

@bnoordhuis Thanks for suggestion. I'll propose the removal of DTrace support to the maintainer of Node.js ports unless the change of citrus-it@4ecd40f fixes the issue.
@citrus-it Would you mind my asking which version of Node.js you used to test the change you propose with this pull request?

We are using this change to continue building various nodejs versions with dtrace support - you can see an example for version 18 at https://github.com/omniosorg/omnios-extra/blob/master/build/nodejs/patches-18/genv8constants.patch

We have had confirmation from users of the packages that the dtrace support is still working for them.

I'm not sure what we will do for the newer versions of nodejs that have removed dtrace support, we may end up patching it back in depending on feedback from our users.