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.

Sign up for GitHub

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

Jump to bottom

Don't use gc-sections with profile-generate. #87004

Merged
merged 1 commit into from
Jul 19, 2021
Merged

Don't use gc-sections with profile-generate. #87004

merged 1 commit into from
Jul 19, 2021

Conversation

JamieCunliffe
Copy link
Contributor

@JamieCunliffe JamieCunliffe commented Jul 9, 2021 •
edited
Loading

When building with profile-generate don't call gc_sections as this can
can sometimes strip out profile data. This missing information in the
prof files can then result in missing functions when using the profile
information.

#78226

r? @Mark-Simulacrum

@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2021
@JamieCunliffe
Copy link
Contributor Author

r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

OK -- I think we should land this for now, and I'm going to go ahead and beta-nominate it, as it is intending to fix #78226.

This could potentially have performance implications if the PGO on x86_64 for the compiler is also affected (just to a lesser extent) by the bugs observed in #78226. So, no rollup.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jul 12, 2021

📌 Commit a6fe9bc67ef1f4ded930b3f7ec7ad3bad08c7a0d has been approved by Mark-Simulacrum

@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 Jul 12, 2021
@bors
Copy link
Contributor

bors commented Jul 12, 2021

⌛ Testing commit a6fe9bc67ef1f4ded930b3f7ec7ad3bad08c7a0d with merge 069ca296f9a6a661da4170b84a6362c942e70fd9...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Mark-Simulacrum
Copy link
Member

Hm, CI failure looks likely to be non-spurious.

@Mark-Simulacrum Mark-Simulacrum added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 12, 2021
@bors
Copy link
Contributor

bors commented Jul 12, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 12, 2021
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 13, 2021
@Mark-Simulacrum
Copy link
Member

@JamieCunliffe -- happy to rerun CI if you disagree with my assumption, not sure what you meant by 👀 :)

It looks like a number of PGO tests failed, but it does seem a little surprising. Maybe we are generating unlinkable symbols or something...

@JamieCunliffe
Copy link
Contributor Author

JamieCunliffe commented Jul 13, 2021 •
edited
Loading

Sorry for the confusion, I meant I was looking into it with the 👀 :)

I have just managed to reproduce it locally with windows, so I don't think it's the CI.

@JamieCunliffe
Keep metadata when using gc-sections with profile-generate.
7c98b3c 
When building with profile-generate request that metadata is kept
during the gc_sections call, as this can sometimes strip out profile
data.
This missing information in the prof files can then result in missing
functions when using the profile information.
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 14, 2021
@JamieCunliffe
Copy link
Contributor Author

It looks like the binary that was built is seg faulting when run to generate the profile data. This also happens with master if using link-dead-code (unsurprisingly given the change/failure).

I have changed it so that it now uses the keep_metadata parameter, it seems in line with that parameter. Although I'm also happy to move the check into GccLinker if you would prefer.

@Mark-Simulacrum
Copy link
Member

Hm... I would not have expected us to need to keep metadata, which refers to Rust's metadata (basically equivalent of C headers, just with more information and embedded into rlibs/dylibs). It looks like that might just be masking the problem?

It looks like with the gcc linker at least passing keep_metadata = true will just turn gc_sections into a no-op (seemingly defeating the point of the patch entirely, since that's the linker I believe CI uses on aarch64-unknown-linux-gnu). On MSVC, (the platform that was failing) it has no effect.

@JamieCunliffe
Copy link
Contributor Author

The original patch didn't call gc_sections when performing PGO, which meant --gc-sections wasn't passed to the linker. Setting keep_metadata to true achieves the same thing with the gcc linker, by turning it into a no-op.

The gcc linker sometimes strips the __llvm_prf_data section and it then gets replaced with a dummy section later on in the process. If I understand correctly that section is only there so that it can be written out to the prof file at the end of the run, and it's designed to be used by the post processing tools. I hadn't realised that metadata there was referring to rustc metadata, but the __llvm_prf_data feels like metadata to me.

It does appear that different linkers handle that section differently, I suspect not passing --gc-sections would have been an alternative fix for #67829 (which is when x86 started to use gold as the linker for that test).

With the MSVC linker it seems not setting /OPT:REF,ICF breaks the binary that is built for profile generate. link-dead-code will also show this as it avoids the call to gc_sections, I also tried a debug build with PGO and got a different failure, although I'm not sure if that was just environmental. I don't know the MSVC linker well enough to really comment on it, but I was a little confused as https://docs.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-160 seems to suggest they are the defaults for an optimized build.

@apiraino
Copy link
Contributor

Beta backport discussed during the T-compiler team meeting on Zulip, decision is that this might be important for backport (next beta cut is in 2 weeks) but as PR looks now better wait a bit more before deciding on the backport

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

OK, let's try it out.

@bors
Copy link
Contributor

bors commented Jul 18, 2021

📌 Commit 7c98b3c has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 18, 2021
@bors
Copy link
Contributor

bors commented Jul 18, 2021

⌛ Testing commit 7c98b3c with merge b548d9f...

@bors
Copy link
Contributor

bors commented Jul 19, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing b548d9f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2021
@bors bors merged commit b548d9f into rust-lang:master Jul 19, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 19, 2021
@pnkfelix
Copy link
Member

discussed at T-compiler triage meeting. Declining for beta backport.

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jul 22, 2021
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmiasko tmiasko tmiasko left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.
10 participants
@JamieCunliffe @rust-highfive @Mark-Simulacrum @bors @rust-log-analyzer @apiraino @pnkfelix @tmiasko @LeSeulArtichaut @rustbot