Skip to content

add codegen option strip#71825

Merged
bors merged 1 commit intorust-lang:masterfrom
contrun:cg-option-strip
May 11, 2020
Merged

add codegen option strip#71825
bors merged 1 commit intorust-lang:masterfrom
contrun:cg-option-strip

Conversation

@contrun
Copy link
Copy Markdown
Contributor

@contrun contrun commented May 3, 2020 •
edited
Loading

closes #71757

I don't know if the flags added here works for all linkers. I only tested on my Linux pc. I also don't know what is the best for emlinker, PtxLinker, MsvcLinker. The option for WasmLd is copied from https://aransentin.github.io/cwasm/.

@rust-highfive
Copy link
Copy Markdown
Contributor

rust-highfive commented May 3, 2020

r? @cramertj

(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 May 3, 2020
@GabrielMajeri
Copy link
Copy Markdown
Contributor

GabrielMajeri commented May 3, 2020

Looks good to me, but I think according to the comment in the options.rs file, the following should also be updated:

// If you add a new option, please update:
// - src/librustc_interface/tests.rs
// - src/doc/rustc/src/codegen-options/index.md

@contrun contrun force-pushed the cg-option-strip branch from 335a242 to 994e075 Compare May 3, 2020 11:37
@contrun
Copy link
Copy Markdown
Contributor Author

contrun commented May 3, 2020

@GabrielMajeri Thanks. I added the relevant sections in aforementioned files. I don't know if there are still things overlooked.

@rust-highfive

This comment has been minimized.

Sign in to view
@contrun contrun force-pushed the cg-option-strip branch 2 times, most recently from d188309 to 942a16e Compare May 3, 2020 15:28
josephlr
josephlr reviewed May 5, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@josephlr josephlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks reasonable, but I think we can do better than noop for emscripten and MSVC

Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
Copy link
Copy Markdown
Contributor

@josephlr josephlr May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For emscripten, you should try to use --llvm-opts to pass -strip-all to the LLVM toolchain. See the emcc documentation

You may also need something like -Wl to make this work, I'm not sure.

Copy link
Copy Markdown
Contributor Author

@contrun contrun May 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know if --llvm-opts can be specified multiple times. Can we just add two --llvm-opts and ['--strip-all'] here? Or we should add an field llvmOpts in Emlinker?

Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
petrochenkov
petrochenkov reviewed May 5, 2020
View reviewed changes
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 5, 2020

This PR introduces the option as immediately stable (-C strip).
New options are usually introduced as unstable (-Z strip) instead.

@contrun contrun force-pushed the cg-option-strip branch from 2263797 to 1ee3e7c Compare May 5, 2020 11:10
@cramertj
Copy link
Copy Markdown
Member

cramertj commented May 8, 2020

r? @petrochenkov

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 8, 2020

Upon some closer investigation, I think this should be added in a slightly different form.

  • -Z strip-debuginfo-if-disabled should be removed.
  • A new option -Z strip=[none|debuginfo|symbols] should be added.
  • From implementation point of view Linker::strip should be merged with Linker::debuginfo.

cc #46034 #49212

@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 8, 2020

Effect of the option on different platforms:

  • ELF

    • -Z strip=debuginfo -> --strip-debug, strips debuginfo sections and debuginfo symbols from the symbol table section at link time.
    • -Z strip=symbols -> --strip-all, same as -Z strip=debuginfo but also strips the rest of the symbol table section. Dynamic symbol table is not stripped, so if we want to load shared libraries we are still good.

  • PE/COFF + DWARF

    • -Z strip=debuginfo -> --strip-debug strips debuginfo sections and debuginfo symbols from the symbol table section at link time.
    • -Z strip=symbols -> --strip-all same as -Z strip=debuginfo, but also strips the rest of the symbol table section.

  • PE/COFF + CodeView

    • -Z strip=debuginfo -> /DEBUG:NONE strips debuginfo sections (not copies them to PDB) at link time.
    • -Z strip=symbols -> /DEBUG:NONE, same as -Z strip=debuginfo
    • We may want so add one more option for /PDBSTRIPPED in the future, but I haven't seen any demand for it so far.

I wouldn't care too much about other target flavors right now, it's safe to do nothing if these options are passed.
If people care about PTX linker etc, they will contribute the proper implementation eventually.

This was referenced May 8, 2020
Closed
Closed
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 8, 2020 •
edited
Loading

GCC doc says that -s (which translates to --strip-all) "removes all ... relocation information from the executable", but ld doc doesn't say that, so I'm not sure what they mean.

The relocations required by dynamic linker are still there, and I assumed that all other relocations are removed during linking anyway as unnecessary.

cc @mati865

@petrochenkov petrochenkov 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 May 8, 2020
@tesuji
Copy link
Copy Markdown
Contributor

tesuji commented May 9, 2020

I wouldn't care too much about other target flavors right now, it's safe to do nothing if these options are passed.

Would adding a waring about doing nothing for those targets/linkers is better?

@mati865
Copy link
Copy Markdown
Member

mati865 commented May 9, 2020

Clang will just pass -s to the linker for both Linux and MinGW.
I've tried to find code for that in GCC but had to give up because I've already wasted too much time looking at this mess.

petrochenkov
petrochenkov reviewed May 9, 2020
View reviewed changes
Comment thread src/librustc_session/config.rs Outdated
Comment thread src/librustc_session/options.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
@rust-highfive

This comment has been minimized.

Sign in to view
petrochenkov
petrochenkov reviewed May 9, 2020
View reviewed changes
Comment thread src/librustc_session/options.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
Comment thread src/librustc_codegen_ssa/back/linker.rs Outdated
@rust-highfive

This comment has been minimized.

Sign in to view
@contrun contrun force-pushed the cg-option-strip branch from cc075e3 to 4fe6ba2 Compare May 10, 2020 03:26
petrochenkov
petrochenkov reviewed May 10, 2020
View reviewed changes
Comment thread src/librustc_codegen_ssa/back/link.rs Outdated
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 10, 2020

Great!
r=me after squashing the commits

@contrun contrun force-pushed the cg-option-strip branch from 4fe6ba2 to 169661e Compare May 10, 2020 08:37
@contrun
add linking option strip
a6c2f73 
move strip option to "Z"

add more strip options, remove strip-debuginfo-if-disabled

merge strip and debuginfo
@contrun contrun force-pushed the cg-option-strip branch from 169661e to a6c2f73 Compare May 10, 2020 08:45
@petrochenkov
Copy link
Copy Markdown
Contributor

petrochenkov commented May 10, 2020

Thanks!
@bors r+

@bors
Copy link
Copy Markdown
Collaborator

bors commented May 10, 2020

📌 Commit a6c2f73 has been approved by petrochenkov

@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 May 10, 2020
@bors
Copy link
Copy Markdown
Collaborator

bors commented May 10, 2020

⌛ Testing commit a6c2f73 with merge 0f73d7656803c0a3f9b88595e0dc9590eac4c573...

@Dylan-DPC-zz
Copy link
Copy Markdown

Dylan-DPC-zz commented May 10, 2020

@bors retry yield

@bors
Copy link
Copy Markdown
Collaborator

bors commented May 10, 2020

⌛ Testing commit a6c2f73 with merge cb49bb804985ac5e7d2df2250f9980bfd306ac42...

@Dylan-DPC-zz
Copy link
Copy Markdown

Dylan-DPC-zz commented May 10, 2020

@bors retry yield

@bors
Copy link
Copy Markdown
Collaborator

bors commented May 10, 2020

⌛ Testing commit a6c2f73 with merge 69da478ebbb84dd58aabace4bd3c803392cff8cb...

@Dylan-DPC-zz
Copy link
Copy Markdown

Dylan-DPC-zz commented May 10, 2020

@bors retry yield

@bors
Copy link
Copy Markdown
Collaborator

bors commented May 10, 2020

⌛ Testing commit a6c2f73 with merge aeb4738...

@bors
Copy link
Copy Markdown
Collaborator

bors commented May 11, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing aeb4738 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 11, 2020
@bors bors merged commit aeb4738 into rust-lang:master May 11, 2020
@petrochenkov petrochenkov mentioned this pull request May 11, 2020
Closed
@retep998 retep998 mentioned this pull request Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

+1 more reviewer

@josephlr josephlr josephlr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@petrochenkov petrochenkov

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add compiler option to strip symbols from generated binaries

10 participants

@contrun @rust-highfive @GabrielMajeri @petrochenkov @cramertj @tesuji @mati865 @bors @Dylan-DPC-zz @josephlr