Skip to content

Remove the use of -use-gnu-stack when BOLTing LLVM#109945

Merged
bors merged 1 commit intorust-lang:masterfrom
Kobzol:llvm-bolt-gnu-stack
Apr 5, 2023
Merged

Remove the use of -use-gnu-stack when BOLTing LLVM#109945
bors merged 1 commit intorust-lang:masterfrom
Kobzol:llvm-bolt-gnu-stack

Conversation

@Kobzol
Copy link
Copy Markdown
Member

@Kobzol Kobzol commented Apr 4, 2023 •
edited
Loading

This flag was (counterintuitively) removing the GNU_STACK ELF attribute, which caused the optimized libLLVM.so file to be flagged as having an executable stack on SELinux.

Removing the flag might cause issues with strip. I'm not aware that we're stripping libLLVM.so though. Does it happen anywhere?

Fixes: #105783

@Kobzol
Remove the use of -use-gnu-stack when BOLTing LLVM
c32953f 
This flag (counterintuitively) was removing the `GNU_STACK` ELF attribute, which caused the optimized `libLLVM.so` file to be flagged as having an executable stack on SELinux.
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 4, 2023

r? @albertlarsan68

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 4, 2023
@Noratrieb
Copy link
Copy Markdown
Member

Noratrieb commented Apr 4, 2023

This is weird..
https://github.com/llvm/llvm-project/blob/09ab1f335bce8bb4d14c28b9adad6d3bec8dfed9/bolt/lib/Rewrite/RewriteInstance.cpp#L4322-L4324
https://github.com/llvm/llvm-project/blob/09ab1f335bce8bb4d14c28b9adad6d3bec8dfed9/bolt/lib/Rewrite/RewriteInstance.cpp#L4267
When the -use-gnu-stack option is set, it appears to be replacing the PT_GNU_STACK program header with an empty PT_LOAD header, effectively deleting it. If it's not set it goes into another branch where it keeps the type.

The description of the flag is indeed very confusing.
llvm/llvm-project@7f7d4af does also talk about "issues with strip/objcopy" but it's very unclear what these issues are.

https://github.com/llvm/llvm-project/blob/09ab1f335bce8bb4d14c28b9adad6d3bec8dfed9/bolt/docs/OptimizingClang.md?plain=1#L68 Even the BOLT docs use -use-gnu-stack when optimizing clang, so something seems to be up. Probably worth asking upstream.

@Kobzol
Copy link
Copy Markdown
Member Author

Kobzol commented Apr 4, 2023

Yeah the name of the flag is weird. I asked about this on the BOLT Discord: https://discord.com/channels/636084430946959380/930647188944613406/1092884016627200081

@Noratrieb
Copy link
Copy Markdown
Member

Noratrieb commented Apr 4, 2023 •
edited
Loading

Summary from discord:
-use-gnu-stack actually means "use the program header for GNU_STACK as a slot for adding new program headers, effectively deleting it". This exists because some tools like strip and objcopy are unable to deal with the appended program headers and reusing the existing slot is fine for them. Since we aren't doing that on our libLLVM.so, it should be fine.

@bors try

@bors
Copy link
Copy Markdown
Collaborator

bors commented Apr 4, 2023

⌛ Trying commit c32953f with merge 7a2984f94b9b2cc9bcbfc178ffaec12878b7fd65...

@bors
Copy link
Copy Markdown
Collaborator

bors commented Apr 4, 2023

☀️ Try build successful - checks-actions
Build commit: 7a2984f94b9b2cc9bcbfc178ffaec12878b7fd65 (7a2984f94b9b2cc9bcbfc178ffaec12878b7fd65)

@Kobzol
Copy link
Copy Markdown
Member Author

Kobzol commented Apr 5, 2023

@rust-timer build 7a2984f94b9b2cc9bcbfc178ffaec12878b7fd65

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented Apr 5, 2023

Finished benchmarking commit (7a2984f94b9b2cc9bcbfc178ffaec12878b7fd65): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 1.9% [1.9%, 1.9%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.4% [1.1%, 4.5%] 4
Improvements ✅
(primary)		 -2.7% [-3.2%, -2.3%] 2
Improvements ✅
(secondary)		 -1.9% [-3.2%, -1.1%] 9
All ❌✅ (primary) -2.7% [-3.2%, -2.3%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

@Noratrieb
Copy link
Copy Markdown
Member

Noratrieb commented Apr 5, 2023

Seems like it all still works, great!

@nikic
Copy link
Copy Markdown
Contributor

nikic commented Apr 5, 2023

@bors r+

@bors
Copy link
Copy Markdown
Collaborator

bors commented Apr 5, 2023

📌 Commit c32953f has been approved by nikic

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 5, 2023
@bors
Copy link
Copy Markdown
Collaborator

bors commented Apr 5, 2023

⌛ Testing commit c32953f with merge 90a9f69...

@bors
Copy link
Copy Markdown
Collaborator

bors commented Apr 5, 2023

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 90a9f69 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2023
@bors bors merged commit 90a9f69 into rust-lang:master Apr 5, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 5, 2023
@Kobzol Kobzol deleted the llvm-bolt-gnu-stack branch April 5, 2023 10:40
@rust-timer
Copy link
Copy Markdown
Collaborator

rust-timer commented Apr 5, 2023

Finished benchmarking commit (90a9f69): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 2.8% [2.4%, 3.2%] 2
Regressions ❌
(secondary)		 3.0% [1.7%, 5.1%] 4
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.4% [-1.9%, -0.9%] 3
All ❌✅ (primary) 2.8% [2.4%, 3.2%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@albertlarsan68 albertlarsan68

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)

Projects

None yet

Milestone

1.70.0

Development

Successfully merging this pull request may close these issues.

rustc 1.66.0 requires execstack to load libLLVM.so

7 participants

@Kobzol @rustbot @Noratrieb @bors @rust-timer @nikic @albertlarsan68