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

Proc macros should limit symbol exports #99909

Closed
bjorn3 opened this issue Jul 29, 2022 · 8 comments · Fixed by #99944
Closed

Proc macros should limit symbol exports #99909

bjorn3 opened this issue Jul 29, 2022 · 8 comments · Fixed by #99944
Labels
A-linkage Area: linking into static, shared libraries and binaries A-proc-macros Area: Procedural macros I-heavy Issue: Problems and improvements with respect to binary size of generated code.

Comments

@bjorn3
Copy link
Member

bjorn3 commented Jul 29, 2022

readelf --wide --syms build/x86_64-unknown-linux-gnu/stage0-rustc/release/deps/libproc_macro_hack-cdb4209108fe6fa9.so | grep GLOBAL | grep -v UND | wc -l shows over 5500 exported symbols. It should limit exports to only those that are used, just like for cdylib's. This should decrease the size of proc macros by allowing more effecting garbage collection of dead code.

@bjorn3 bjorn3 added A-linkage Area: linking into static, shared libraries and binaries I-heavy Issue: Problems and improvements with respect to binary size of generated code. A-proc-macros Area: Procedural macros labels Jul 29, 2022
@eddyb
Copy link
Member

eddyb commented Jul 29, 2022

I just about almost opened my own issue, as this looks like it could cause e.g. #59998:

$ nm -C -D target/debug/deps/libserde_derive-eb8144d61a68af88.so | rg PANIC_
00000000005d31b8 B std::panicking::panic_count::GLOBAL_PANIC_COUNT

(noticed the excess of exports in @fasterthanlime's latest blog post, about RA/proc macros)

Frankly I think I got confused at some point and expected proc macros to get more cdylib treatment than they actually do, I even described them in the past as "cdylib+rmeta".

And there are certainly places where it looks like that was the intention:

fn crate_export_threshold(crate_type: CrateType) -> SymbolExportLevel {
match crate_type {
CrateType::Executable | CrateType::Staticlib | CrateType::ProcMacro | CrateType::Cdylib => {
SymbolExportLevel::C
if let Some(id) = tcx.proc_macro_decls_static(()) {
reachable_non_generics.insert(
id.to_def_id(),
SymbolExportInfo {
level: SymbolExportLevel::C,

But also this seems to apply? So what's the actual crate type for a proc macro?

if tcx.sess.crate_types().contains(&CrateType::Dylib) {
let symbol_name = metadata_symbol_name(tcx);
let exported_symbol = ExportedSymbol::NoDefId(SymbolName::new(tcx, &symbol_name));
symbols.push((
exported_symbol,
SymbolExportInfo {
level: SymbolExportLevel::Rust,


Ah there it is! For some reason we just completely bypass symbol-hiding for proc macros:

// We manually create a list of exported symbols to ensure we don't expose any more.
// The object files have far more public symbols than we actually want to export,
// so we hide them all here.
if !self.sess.target.limit_rdylib_exports {
return;
}
if crate_type == CrateType::ProcMacro {
return;
}

@bjorn3
Copy link
Member Author

bjorn3 commented Jul 29, 2022

(noticed the excess of exports in @fasterthanlime's latest blog post, about RA/proc macros)

That is exactly what I was reading when I opened this issue :)

@eddyb
Copy link
Member

eddyb commented Jul 29, 2022

(i.e. the original proc macro impl) is what appears to have added the proc macro case here (which seems to have been called RustcMacro back then):

// If we're compiling a dylib, then we let symbol visibility in object
// files to take care of whether they're exported or not.
//
// If we're compiling a cdylib, however, we manually create a list of
// exported symbols to ensure we don't expose any more. The object files
// have far more public symbols than we actually want to export, so we
// hide them all here.
if crate_type == CrateType::CrateTypeDylib ||
crate_type == CrateType::CrateTypeRustcMacro {

So what happened was that proc macros were a lot more like dylibs than cdylibs originally, and this was consistently implemented.

However, at some point, @michaelwoerister refactored symbol_export, in:

But that decision couldn't have changed anything (at least not in the way we might expect) because of the bail-out from export hiding that proc macros have been hit by.

Nowadays not even dylib crates complete ignore export hiding, only proc macros do.

I believe this also tripped me up in the past, since crate_export_threshold looked correct, and I guess I never looked in depth at what's actually being exported, welp.

@bjorn3
Copy link
Member Author

bjorn3 commented Jul 30, 2022

Removing the gate at

// We manually create a list of exported symbols to ensure we don't expose any more.
// The object files have far more public symbols than we actually want to export,
// so we hide them all here.
if !self.sess.target.limit_rdylib_exports {
return;
}
if crate_type == CrateType::ProcMacro {
return;
}
seems to cause the .rustc section to be omitted.

Edit: Symbol visibility for rust_metadata_* was set to Rust instead of C.
Edit2: Changing that doesn't seem to be enough.
Edit3: Needed to add the symbol for proc macros too. It was limited to dylibs. It works now.

@bjorn3
Copy link
Member Author

bjorn3 commented Jul 30, 2022

Opened #99944 with a fix.

@bors bors closed this as completed in 25bb1c1 Aug 1, 2022
@eddyb
Copy link
Member

eddyb commented Aug 2, 2022

$ rustup update nightly
info: syncing channel updates for 'nightly-x86_64-unknown-linux-gnu'
info: latest update on 2022-08-02, rust version 1.64.0-nightly (fe3342816 2022-08-01)
...
  nightly-x86_64-unknown-linux-gnu updated - rustc 1.64.0-nightly (fe3342816 2022-08-01) (from rustc 1.64.0-nightly (9067d5277 2022-07-28))
$ cargo build -p serde_derive
$ nm -D --defined-only -S target/debug/deps/libserde_derive-*.so
0000000000315f10 0000000000000010 D __rustc_proc_macro_decls_284a6d00fbf74d9e__
0000000000000000 0000000000008ca2 N rust_metadata_serde_derive_284a6d00fbf74d9e

Beautiful, thanks @bjorn3! ✨

@est31
Copy link
Member

est31 commented Aug 2, 2022

This should decrease the size of proc macros by allowing more effecting garbage collection of dead code.

I wonder what the impact of this reduction has been.

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 2, 2022

#99944 (comment) shows quite significant build time improvements. serde_derive debug got almost 3MB smaller (down to 23MB). Same for serde_derive opt (down to 8MB). I don't think we benchmark compilation of any other proc macro on perf.rust-lang.org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries A-proc-macros Area: Procedural macros I-heavy Issue: Problems and improvements with respect to binary size of generated code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants