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

THP ELF assembly: Add .note.GNU-stack section #37688

Closed
wants to merge 2 commits into from
Closed

THP ELF assembly: Add .note.GNU-stack section #37688

wants to merge 2 commits into from

Conversation

jayaddison
Copy link
Contributor

This indicates to GNU binutils that it can unset the executable stack flag on the binary that it is building.

Refs: #17933

cc @gabrielschulhof @bnoordhuis (from the git shortlog in this part of the codebase)
cc @danbev (as discussed, for review)

Two developer notes to aid review:

  • Some other examples on the web instruct to use %progbits with a percentage symbol. The approach here uses @progbits instead, which is the form documented in the binutils documentation here.
  • The conditional block does not check for a Linux-based build environment; this is intentional since node is a cross-platform binary and I believe this section could be relevant for other platforms.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

This indicates to GNU binutils that it can unset the executable stack
flag on the binary that it is building.

Refs: #17933
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 10, 2021
@nodejs-github-bot
Copy link
Collaborator

@jayaddison
Copy link
Contributor Author

It looks like clang is complaining when building binaries from b4f3e39, and I think the cause may be related to the ordering of the sections within the .S file.

Theory, based on the error message: clang considers that the __node_text_start: reference should be placed within the.note.GNU-stack section (since it is appears below it in the file); but this section is then discarded since it is a note, meaning that the (required) object reference is lost.

For reference since the build logs may disappear, the error message is:

`__node_text_start' referenced in section `.text' of /home/runner/work/node/node/out/Release/obj.target/libnode/src/large_pages/node_large_page.o: defined in discarded section `.note.GNU-stack' of /home/runner/work/node/node/out/Release/obj.target/node_text_start/src/large_pages/node_text_start.o

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@jayaddison
Copy link
Contributor Author

@danbev Could you try re-running the NodeJS CI tests, if that's possible? The failure for bd478f3 seems related to a flaky test, so perhaps a re-run might clear it.

@nodejs-github-bot
Copy link
Collaborator

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

danbev pushed a commit that referenced this pull request Mar 17, 2021
This indicates to GNU binutils that it can unset the executable stack
flag on the binary that it is building.

PR-URL: #37688
Refs: #17933
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@danbev
Copy link
Contributor

danbev commented Mar 17, 2021

Landed in 90008f8.

@danbev danbev closed this Mar 17, 2021
@jayaddison jayaddison deleted the build/large-pages-assembly-gnu-stack branch March 17, 2021 23:20
ruyadorno pushed a commit that referenced this pull request Mar 19, 2021
This indicates to GNU binutils that it can unset the executable stack
flag on the binary that it is building.

PR-URL: #37688
Refs: #17933
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
targos pushed a commit that referenced this pull request Apr 27, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Apr 29, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
This indicates to GNU binutils that it can unset the executable stack
flag on the binary that it is building.

PR-URL: #37688
Refs: #17933
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
targos pushed a commit that referenced this pull request May 30, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: #17933
Related: #37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>

PR-URL: #38312
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
While @progbits is preferred for most architectures, there are some
(notably 32-bit ARM) for which it does not. %progbits is effective
everywhere.

See https://bugzilla.redhat.com/show_bug.cgi?id=1950528 for more
details.

Related: nodejs/node#17933
Related: nodejs/node#37688

Signed-off-by: Stephen Gallagher <sgallagh@redhat.com>
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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants