Limit symbols exported from proc macros #99944

bjorn3 · 2022-07-30T12:18:15Z

Only __rustc_proc_macro_decls_*__ and rust_metadata_* need to be
exported for proc macros to work. All other symbols only increase binary
size and have the potential to conflict with symbols from the host
compiler.

Fixes #99909
Fixes #59998

cc @eddyb

Only __rustc_proc_macro_decls_*__ and rust_metadata_* need to be exported for proc macros to work. All other symbols only increase binary size and have the potential to conflict with symbols from the host compiler.

rust-highfive · 2022-07-30T12:18:18Z

r? @jackh726

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

eddyb

Thanks for looking into this! ^{_{(and sorry I didn't mention the section stuff, was expecting that would get a bit in the way)}}

r=me with an issue for the #[no_mangle] stuff

compiler/rustc_codegen_ssa/src/back/linker.rs

eddyb · 2022-07-30T12:57:20Z

compiler/rustc_codegen_ssa/src/back/symbol_export.rs

-    if tcx.sess.crate_types().contains(&CrateType::Dylib) {
+    if tcx.sess.crate_types().contains(&CrateType::Dylib)
+        || tcx.sess.crate_types().contains(&CrateType::ProcMacro)
+    {
        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,
+                level: SymbolExportLevel::C,
                kind: SymbolExportKind::Data,
-                used: false,
+                used: true,


cc @michaelwoerister @wesleywiser @rust-lang/wg-llvm
Is anyone aware of any potential negative consequences of this?

I'm a bit surprised we even need a symbol (and don't find it by section instead).

If the symbol only exists to keep the section from being GC'd, shouldn't it be some form of unexported #[used]? (e.g. keep Rust export level, or go even lower? but always set used: true)

The symbol is needed on at least macOS to prevent the section from being discarded. The exact name is unique. I think it would make sense in the future to ensure that the symbol exists when using a dylib as dependency to ensure versions don't get mixed without recompilation.

Heh, right, the SVH enforcement ideas, but is this symbol accessible to the dynamic loader at all?
Also it might be good to have dedicated symbols in case the metadata ones get stripped.

EDIT: moved the rest of the comment below to #73917 (comment)

(click to open old version)

We could even have something like this:

struct LinkGuard { deps: &'static [&'static LinkGuard], } extern "Rust" { // foo[b7924b40b7ed5e7f]::{shim:LINK_GUARD#0}::<0x461e83de35f0b704f7e69b4cc741ad8eu128> #[link_name = "_RINSCsfL95rG4I7iB_3foo10LINK_GUARDKo461e83de35f0b704f7e69b4cc741ad8e_E"] static LINK_GUARD_DEP_FOO: LinkGuard; // bar[acb4b2d152c0bd2e]::{shim:LINK_GUARD#0}::<0xf8fc0fadc6a6e727eef4b916531abfe9u128> #[link_name = "_RINSCsePjaApBJGQA_3bar10LINK_GUARDKof8fc0fadc6a6e727eef4b916531abfe9_E"] static LINK_GUARD_DEP_BAR: LinkGuard; } // my_crate[78009e3fbfa2f6af]::{shim:LINK_GUARD#0}::<0xe538955c5950b59a598304a1e701c9fbu128> #[export_name = "_RINSCsaiLK1vfX74x_8my_crate10LINK_GUARDKoe538955c5950b59a598304a1e701c9fb_E"] pub static LINK_GUARD: LinkGuard { deps: unsafe { &[ &LINK_GUARD_DEP_FOO, &LINK_GUARD_DEP_BAR, ] } };

(and then use global constructors to ensure it gets kept in - potentially even doing some redundant SVH checking at runtime too...? or something more cursed where it's "activating" something that would be broken otherwise. but that's too much)

That makes sense. cc #73917

src/test/run-make-fulldeps/symbol-visibility/Makefile

eddyb · 2022-07-30T13:25:47Z

@bjorn3 Oh, almost forgot, can you look for // ignore-stage1 proc macro tests, and/or those that mention #59998, and try making them always run? I'm curious if export limiting can fix #59998.

bjorn3 · 2022-07-31T10:04:29Z

#59998 is indeed fixed by this it seems.

eddyb · 2022-07-31T11:26:21Z

@bors r+ rollup=never Thanks!

(#99944 (comment) might still worry me, but it's just the section for a holding the .rmeta, and dylib usage should be pretty limited, and it's easy to fix it it's just the C/Rust level difference)

bors · 2022-07-31T11:26:23Z

📌 Commit b87f8a4 has been approved by eddyb

It is now in the queue for this repository.

bors · 2022-08-01T03:58:56Z

⌛ Testing commit b87f8a4 with merge 25bb1c1...

bors · 2022-08-01T06:44:36Z

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing 25bb1c1 to master...

rust-timer · 2022-08-01T09:06:34Z

Finished benchmarking commit (25bb1c1): comparison url.

Instruction count

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-0.6%	-2.9%	41
Improvements 🎉 (secondary)	-2.4%	-10.7%	11
All 😿🎉 (primary)	-0.6%	-2.9%	41

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	-1.7%	-2.2%	8
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-1.7%	-2.2%	8

Cycles

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	3.7%	3.7%	1
Improvements 🎉 (primary)	-3.0%	-6.6%	6
Improvements 🎉 (secondary)	-5.3%	-9.8%	4
All 😿🎉 (primary)	-3.0%	-6.6%	6

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

Alexendoo · 2022-08-14T12:15:53Z

Following this PR rustc_private crates from rustup no longer work on aarch64-unknown-linux-gnu

#![feature(rustc_private)]

extern crate rustc_attr;

Gives the error:

error: /home/alex/.rustup/toolchains/nightly-2022-08-14-aarch64-unknown-linux-gnu/lib/rustlib/aarch64-unknown-linux-gnu/lib/libserde_derive-6dc9fea9ed4dba80.so: cannot allocate memory in static TLS block
 --> src/lib.rs:3:1
  |
3 | extern crate rustc_attr;
  | ^^^^^^^^^^^^^^^^^^^^^^^^

bjorn3 · 2022-08-14T12:29:37Z

I think this is because proc macros of rustc are incorrectly compiled with -Ztls-model=initial-exec. This tls model is not allowed for dlopen'ed dylibs. It just so happens to work by accident in certain cases. For example before this PR some tls variables would be incorrectly resolved in the host rustc instance instead of the proc macro itself (causing #59998) and thus not run into trouble. The fix would be to limit -Ztls-model=initial-exec to crates not compiled for the host. That is replacing

rust/src/bootstrap/builder.rs

Line 1853 in 05e678c

rustflags.arg("-Ztls-model=initial-exec");

with an env var that causes the argument to be passed in this if block:

rust/src/bootstrap/bin/rustc.rs

Line 84 in 05e678c

if target.is_some() {

eddyb · 2022-08-14T12:31:13Z

@Alexendoo Can you file a separate issue for that? Also, ignoring rustc_private, can you use serde_derive in general on that platform? I would assume so, which means this is a very strange interaction.

At most maybe this is misbehaving on some platforms (note the powerpc- exclusion):

rust/src/bootstrap/builder.rs

Lines 1848 to 1854 in f03ce30

    
           // Compile everything except libraries and proc macros with the more 
        
           // efficient initial-exec TLS model. This doesn't work with `dlopen`, 
        
           // so we can't use it by default in general, but we can use it for tools 
        
           // and our own internal libraries. 
        
           if !mode.must_support_dlopen() && !target.triple.starts_with("powerpc-") { 
        
               rustflags.arg("-Ztls-model=initial-exec"); 
        
           }

EDIT: ahh @bjorn3 replied while I was typing this, "only worked by accident" explains a lot, heh.

Alexendoo · 2022-08-14T13:17:25Z

Thanks! Opened #100530

Yeah serde_derive works fine as a regular dependency. I'll try to reproduce the issue without rustup + test the fix

In m-c we're running into linking issues in `uniffi_macros`, where it's missing symbols coming from `uniffi_core`: /bin/ld: uniffi_foreign_executor_callback_set: undefined version: /bin/ld: failed to set dynamic section sizes: bad value That's most likely this issue on Rust: rust-lang/rust#111888 It's called out that this likely actually broke because of another PR: rust-lang/rust#99944 Despite this bug there's a bit of an issue in UniFFI to begin with: We're exporting `extern "C"` functions from a crate that is a dependency of some other crates, including `uniffi_macros`, and thus the symbols land in a part where they are not needed. However for `uniffi_meta` in particular we don't need much from `uniffi_core`, so for now we just copy the necessary bits to get it all working.

…proc-macros-issue-99978, r=bjorn3 Restrict linker version script of proc-macro crates to just its two symbols Restrict linker version script of proc-macro crates to just the two symbols of each proc-macro crate. The main known effect of doing this is to stop including `#[no_mangle]` symbols in the linker version script. Background: The combination of a proc-macro crate with an import of another crate that itself exports a no_mangle function was broken for a period of time, because: * In PR rust-lang#99944 we stopped exporting no_mangle symbols from proc-macro crates; proc-macro crates have a very limited interface and are meant to be treated as a blackbox to everything except rustc itself. However: he constructed linker version script still referred to them, but resolving that discrepancy was left as a FIXME in the code, tagged with issue rust-lang#99978. * In PR rust-lang#108017 we started telling the linker to check (via the`--no-undefined-version` linker invocation flag) that every symbol referenced in the "linker version script" is provided as linker input. So the unresolved discrepancy from rust-lang#99978 started surfacing as a compile-time error (e.g. rust-lang#111888). Fix rust-lang#111888 Fix rust-lang#99978.

Limit symbols exported from proc macros

6098e79

Only __rustc_proc_macro_decls_*__ and rust_metadata_* need to be exported for proc macros to work. All other symbols only increase binary size and have the potential to conflict with symbols from the host compiler.

rust-highfive assigned jackh726 Jul 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 30, 2022

bjorn3 mentioned this pull request Jul 30, 2022

Proc macros should limit symbol exports #99909

Closed

eddyb approved these changes Jul 30, 2022

View reviewed changes

bjorn3 mentioned this pull request Jul 31, 2022

Don't export no_mangle symbols from proc macros #99978

Closed

This comment has been minimized.

Sign in to view

bjorn3 added 2 commits July 31, 2022 10:33

Remove workarounds for issue 59998

df13721

Add issue reference

b87f8a4

bjorn3 force-pushed the hide_proc_macro_symbols branch from 6fd93e9 to b87f8a4 Compare July 31, 2022 10:33

eddyb approved these changes Jul 31, 2022

View reviewed changes

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 31, 2022

eddyb mentioned this pull request Jul 31, 2022

Add SVH based symbol to dylibs #73917

Open

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 1, 2022

bors merged commit 25bb1c1 into rust-lang:master Aug 1, 2022

rustbot added this to the 1.64.0 milestone Aug 1, 2022

bjorn3 deleted the hide_proc_macro_symbols branch August 1, 2022 16:53

eddyb mentioned this pull request Aug 2, 2022

Panic inside panic when procedural macro is called with proc_macro::bridge::client #60593

Closed

Alexendoo mentioned this pull request Aug 14, 2022

Build failure with rustc_private crates on aarch64-unknown-linux-gnu #100530

Closed

This was referenced Mar 3, 2023

ICE : Proc macro : If Ident is invalid, panic occur #94913

Closed

"0_DISCRIMINANT" is not a valid identifier #98158

Closed

pnkfelix mentioned this pull request Jul 20, 2023

regression - nightly 2023-03-30 -> 2023-03-31 causes linker error when importing wasm_bindgen::JsValue in a proc_macro #111888

Closed

badboy mentioned this pull request Jul 25, 2023

Hack: Copy metadata module to avoid uniffi_core dependency mozilla/uniffi-rs#1665

Merged

badboy mentioned this pull request Jul 25, 2023

Hack: Copy metadata module to avoid uniffi_core dependency mozilla/uniffi-rs#1666

Merged

pnkfelix mentioned this pull request Aug 4, 2023

Restrict linker version script of proc-macro crates to just its two symbols #114470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit symbols exported from proc macros #99944

Limit symbols exported from proc macros #99944

bjorn3 commented Jul 30, 2022 •

edited by eddyb

Loading

rust-highfive commented Jul 30, 2022

eddyb left a comment

eddyb Jul 30, 2022

bjorn3 Jul 30, 2022 •

edited

Loading

eddyb Jul 31, 2022 •

edited

Loading

bjorn3 Jul 31, 2022

eddyb commented Jul 30, 2022

bjorn3 commented Jul 31, 2022

This comment has been minimized.

eddyb commented Jul 31, 2022

bors commented Jul 31, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

rust-timer commented Aug 1, 2022

Alexendoo commented Aug 14, 2022

bjorn3 commented Aug 14, 2022

eddyb commented Aug 14, 2022

Alexendoo commented Aug 14, 2022

Limit symbols exported from proc macros #99944

Limit symbols exported from proc macros #99944

Conversation

bjorn3 commented Jul 30, 2022 • edited by eddyb Loading

rust-highfive commented Jul 30, 2022

eddyb left a comment

Choose a reason for hiding this comment

eddyb Jul 30, 2022

Choose a reason for hiding this comment

bjorn3 Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

eddyb Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

bjorn3 Jul 31, 2022

Choose a reason for hiding this comment

eddyb commented Jul 30, 2022

bjorn3 commented Jul 31, 2022

This comment has been minimized.

eddyb commented Jul 31, 2022

bors commented Jul 31, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

rust-timer commented Aug 1, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Alexendoo commented Aug 14, 2022

bjorn3 commented Aug 14, 2022

eddyb commented Aug 14, 2022

Alexendoo commented Aug 14, 2022

bjorn3 commented Jul 30, 2022 •

edited by eddyb

Loading

bjorn3 Jul 30, 2022 •

edited

Loading

eddyb Jul 31, 2022 •

edited

Loading