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

test: fix DebugSymbolsTest.ReqWrapList on PPC64LE #44341

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danbev
Copy link
Member

@danbev danbev commented Aug 22, 2022

Currently, DebugSymbolsTest.ReqWrapList fails on PPC64LE when Node has
been configured with Link Time Optimization (LTO) and using RHEL 8.5
and gcc:

$ . /opt/rh/gcc-toolset-11/enable
$ export CC='ccache gcc'
$ export CXX='ccache g++'
$ ./configure --enable-lto
$ make -j8 cctest
...
21:52:27 [ RUN      ] DebugSymbolsTest.ReqWrapList
21:52:27 ../test/cctest/test_node_postmortem_metadata.cc:203: Failure
21:52:27 Expected equality of these values:
21:52:27   expected
21:52:27     Which is: 140736537072320
21:52:27   calculated
21:52:27     Which is: 1099680328560
21:52:27 [  FAILED  ] DebugSymbolsTest.ReqWrapList (43 ms)

After looking into this is seems that the compiler is tampering with one
or more of the queue, head, tail, and last variables when
compiling with LTO enabled. This commit suggests adding volatile to
these variables to prevent the compiler from tampering with them.


Some additional details can be found in these notes.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 22, 2022
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Aug 22, 2022

@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Aug 22, 2022

@richardlau
Copy link
Member

richardlau commented Aug 22, 2022

hmm I ran an LTO build with this and while the PPC64LE failure is gone, the same test now fails on s390x (where it wasn't before 😞): https://ci.nodejs.org/job/node-test-commit-linux-lto/28/
Retrying again: https://ci.nodejs.org/job/node-test-commit-linux-lto/29/

Currently, DebugSymbolsTest.ReqWrapList fails on PPC64LE when Node has
been configured with Link Time Optimization (LTO) and using RHEL 8.5
and gcc:
```console
$ . /opt/rh/gcc-toolset-11/enable
$ export CC='ccache gcc'
$ export CXX='ccache g++'
$ ./configure --enable-lto
$ make -j8 cctest
...
21:52:27 [ RUN      ] DebugSymbolsTest.ReqWrapList
21:52:27 ../test/cctest/test_node_postmortem_metadata.cc:203: Failure
21:52:27 Expected equality of these values:
21:52:27   expected
21:52:27     Which is: 140736537072320
21:52:27   calculated
21:52:27     Which is: 1099680328560
21:52:27 [  FAILED  ] DebugSymbolsTest.ReqWrapList (43 ms)
```

After looking into this is seems that the compiler is tampering with the
`last` variable when compiling with LTO enabled. This commit suggests
adding volatile to this variable to prevent the compiler from tampering
with it.
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Aug 24, 2022

@danbev
Copy link
Member Author

danbev commented Aug 24, 2022

Re-run of failing LTO builds: https://ci.nodejs.org/job/node-test-commit-linux-lto/30/

@danbev danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants