GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — THP ELF assembly: Add .note.GNU-stack section by jayaddison · Pull Request #37688 · nodejs/node

Conversation

jayaddison commented Mar 10, 2021

Copy link

Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


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

b4f3e39

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 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 commented Mar 10, 2021

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jayaddison commented Mar 10, 2021

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Move .note.GNU-stack section to beginning of assembly file

bd478f3

nodejs-github-bot commented Mar 10, 2021

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev approved these changes

Mar 10, 2021

View reviewed changes

nodejs-github-bot commented Mar 10, 2021

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jayaddison mentioned this pull request

Mar 10, 2021

FreeBSD infra failure nodejs/build#2563

Closed

jayaddison commented Mar 11, 2021

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Mar 11, 2021

Copy link

Copy Markdown

Collaborator

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

jayaddison reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev pushed a commit that referenced this pull request

Mar 17, 2021


          src: add .note.GNU-stack section

90008f8

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 <[email protected]>

danbev commented Mar 17, 2021

Copy link

Copy Markdown

Contributor

Landed in 90008f8.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

danbev closed this

Mar 17, 2021

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


          src: add .note.GNU-stack section

836cb67

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 <[email protected]>

ruyadorno mentioned this pull request

Mar 30, 2021

v15.13.0 proposal #37977

Merged

sgallagher mentioned this pull request

Apr 20, 2021

src: Use %progbits instead of @progbits #38312

Closed

targos pushed a commit that referenced this pull request

Apr 27, 2021


          src: use %progbits instead of @progbits

47207e5

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

targos pushed a commit that referenced this pull request

Apr 29, 2021


          src: use %progbits instead of @progbits

7d8cc2a

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

targos pushed a commit that referenced this pull request

May 1, 2021


          src: add .note.GNU-stack section

eb0faa1

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 <[email protected]>

danielleadams mentioned this pull request

May 3, 2021

v14.17.0 proposal #38507

Merged

targos pushed a commit that referenced this pull request

May 30, 2021


          src: use %progbits instead of @progbits

398cde3

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

targos pushed a commit that referenced this pull request

Jun 5, 2021


          src: use %progbits instead of @progbits

2975b9e

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

targos pushed a commit that referenced this pull request

Jun 5, 2021


          src: use %progbits instead of @progbits

b568e4c

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

targos pushed a commit that referenced this pull request

Jun 11, 2021


          src: use %progbits instead of @progbits

c170026

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 <[email protected]>

PR-URL: #38312
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request

May 5, 2024


          src: Use %progbits instead of @progbits

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 <[email protected]>

Reviewers

1 more reviewer

danbev

danbev approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.