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

Limit symbols exported from proc macros #99944

Merged
merged 3 commits into from
Aug 1, 2022
Merged

Limit symbols exported from proc macros #99944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6098e79
Limit symbols exported from proc macros
bjorn3 Jul 30, 2022
df13721
Remove workarounds for issue 59998
bjorn3 Jul 31, 2022
b87f8a4
Add issue reference
bjorn3 Jul 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions compiler/rustc_codegen_ssa/src/back/linker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -656,9 +656,7 @@ impl<'a> Linker for GccLinker<'a> {
return;
}

if crate_type == CrateType::ProcMacro {
return;
}
// FIXME(#99978) hide #[no_mangle] symbols for proc-macros

let is_windows = self.sess.target.is_like_windows;
let path = tmpdir.join(if is_windows { "list.def" } else { "list" });
Expand Down
8 changes: 5 additions & 3 deletions compiler/rustc_codegen_ssa/src/back/symbol_export.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,16 +257,18 @@ fn exported_symbols_provider_local<'tcx>(
}));
}

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,
Comment on lines -260 to +271
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@bjorn3 bjorn3 Jul 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eddyb eddyb Jul 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. cc #73917

},
));
}
Expand Down
21 changes: 21 additions & 0 deletions src/test/run-make-fulldeps/symbol-visibility/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ include ../tools.mk
NM=nm -D
CDYLIB_NAME=liba_cdylib.so
RDYLIB_NAME=liba_rust_dylib.so
PROC_MACRO_NAME=liba_proc_macro.so
EXE_NAME=an_executable
COMBINED_CDYLIB_NAME=libcombined_rlib_dylib.so

ifeq ($(UNAME),Darwin)
NM=nm -gU
CDYLIB_NAME=liba_cdylib.dylib
RDYLIB_NAME=liba_rust_dylib.dylib
PROC_MACRO_NAME=liba_proc_macro.dylib
EXE_NAME=an_executable
COMBINED_CDYLIB_NAME=libcombined_rlib_dylib.dylib
endif
Expand All @@ -20,6 +22,7 @@ ifdef IS_WINDOWS
NM=nm -g
CDYLIB_NAME=liba_cdylib.dll.a
RDYLIB_NAME=liba_rust_dylib.dll.a
PROC_MACRO_NAME=liba_proc_macro.dll
EXE_NAME=an_executable.exe
COMBINED_CDYLIB_NAME=libcombined_rlib_dylib.dll.a
endif
Expand All @@ -31,6 +34,7 @@ all:
$(RUSTC) -Zshare-generics=no an_rlib.rs
$(RUSTC) -Zshare-generics=no a_cdylib.rs
$(RUSTC) -Zshare-generics=no a_rust_dylib.rs
$(RUSTC) -Zshare-generics=no a_proc_macro.rs
$(RUSTC) -Zshare-generics=no an_executable.rs
$(RUSTC) -Zshare-generics=no a_cdylib.rs --crate-name combined_rlib_dylib --crate-type=rlib,cdylib

Expand All @@ -54,6 +58,14 @@ all:
# Check that a Rust dylib does not export generics if -Zshare-generics=no
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -v __imp_ | grep -c public_generic_function_from_rlib)" -eq "0" ]

# Check that a proc macro exports its public #[no_mangle] functions
# FIXME(#99978) avoid exporting #[no_mangle] symbols for proc macros
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c public_c_function_from_cdylib)" -eq "1" ]
# Check that a proc macro exports the public #[no_mangle] functions of dependencies
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c public_c_function_from_rlib)" -eq "1" ]
# Check that a proc macro DOES NOT export any public Rust functions
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c $(RE_ANY_RUST_SYMBOL))" -eq "0" ]

# FIXME(nbdd0121): This is broken in MinGW, see https://github.com/rust-lang/rust/pull/95604#issuecomment-1101564032
ifndef IS_WINDOWS
# Check that an executable does not export any dynamic symbols
Expand All @@ -75,6 +87,7 @@ endif
$(RUSTC) -Zshare-generics=yes an_rlib.rs
$(RUSTC) -Zshare-generics=yes a_cdylib.rs
$(RUSTC) -Zshare-generics=yes a_rust_dylib.rs
$(RUSTC) -Zshare-generics=yes a_proc_macro.rs
$(RUSTC) -Zshare-generics=yes an_executable.rs

# Check that a cdylib exports its public #[no_mangle] functions
Expand All @@ -94,6 +107,14 @@ endif
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -v __imp_ | grep -c public_rust_function_from_rlib)" -eq "1" ]
[ "$$($(NM) $(TMPDIR)/$(RDYLIB_NAME) | grep -v __imp_ | grep -c public_generic_function_from_rlib)" -eq "1" ]

# Check that a proc macro exports its public #[no_mangle] functions
# FIXME(#99978) avoid exporting #[no_mangle] symbols for proc macros
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c public_c_function_from_cdylib)" -eq "1" ]
# Check that a proc macro exports the public #[no_mangle] functions of dependencies
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c public_c_function_from_rlib)" -eq "1" ]
# Check that a proc macro DOES NOT export any public Rust functions
[ "$$($(NM) $(TMPDIR)/$(CDYLIB_NAME) | grep -v __imp_ | grep -c $(RE_ANY_RUST_SYMBOL))" -eq "0" ]

ifndef IS_WINDOWS
# Check that an executable does not export any dynamic symbols
[ "$$($(NM) $(TMPDIR)/$(EXE_NAME) | grep -v __imp_ | grep -c public_c_function_from_rlib)" -eq "0" ]
Expand Down
9 changes: 9 additions & 0 deletions src/test/run-make-fulldeps/symbol-visibility/a_proc_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![crate_type = "proc-macro"]

extern crate an_rlib;

// This should not be exported
#[no_mangle]
extern "C" fn public_c_function_from_cdylib() {
an_rlib::public_c_function_from_rlib();
}
0 ...ui-fulldeps/auxiliary/proc-macro-panic.rs → .../proc-macro/auxiliary/proc-macro-panic.rs
View file
Open in desktop
File renamed without changes.
5 changes: 0 additions & 5 deletions src/test/ui/proc-macro/invalid-punct-ident-1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
// aux-build:invalid-punct-ident.rs
// ignore-stage1
// only-linux
//
// FIXME: This should be a normal (stage1, all platforms) test in
// src/test/ui/proc-macro once issue #59998 is fixed.

#[macro_use]
extern crate invalid_punct_ident;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/invalid-punct-ident-1.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: proc macro panicked
--> $DIR/invalid-punct-ident-1.rs:11:1
--> $DIR/invalid-punct-ident-1.rs:6:1
|
LL | invalid_punct!();
| ^^^^^^^^^^^^^^^^
Expand Down
5 changes: 0 additions & 5 deletions src/test/ui/proc-macro/invalid-punct-ident-2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
// aux-build:invalid-punct-ident.rs
// ignore-stage1
// only-linux
//
// FIXME: This should be a normal (stage1, all platforms) test in
// src/test/ui/proc-macro once issue #59998 is fixed.

#[macro_use]
extern crate invalid_punct_ident;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/invalid-punct-ident-2.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: proc macro panicked
--> $DIR/invalid-punct-ident-2.rs:11:1
--> $DIR/invalid-punct-ident-2.rs:6:1
|
LL | invalid_ident!();
| ^^^^^^^^^^^^^^^^
Expand Down
5 changes: 0 additions & 5 deletions src/test/ui/proc-macro/invalid-punct-ident-3.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,4 @@
// aux-build:invalid-punct-ident.rs
// ignore-stage1
// only-linux
//
// FIXME: This should be a normal (stage1, all platforms) test in
// src/test/ui/proc-macro once issue #59998 is fixed.

#[macro_use]
extern crate invalid_punct_ident;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/invalid-punct-ident-3.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: proc macro panicked
--> $DIR/invalid-punct-ident-3.rs:11:1
--> $DIR/invalid-punct-ident-3.rs:6:1
|
LL | invalid_raw_ident!();
| ^^^^^^^^^^^^^^^^^^^^
Expand Down
5 changes: 0 additions & 5 deletions ...eps/issue-76270-panic-in-libproc-macro.rs → ...cro/issue-76270-panic-in-libproc-macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
// aux-build:proc-macro-panic.rs
// edition:2018
// ignore-stage1
// only-linux
//
// FIXME: This should be a normal (stage1, all platforms) test in
// src/test/ui/proc-macro once issue #59998 is fixed.

// Regression test for issue #76270
// Tests that we don't print an ICE message when a panic
Expand Down
2 changes: 1 addition & 1 deletion ...issue-76270-panic-in-libproc-macro.stderr → ...issue-76270-panic-in-libproc-macro.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: proc macro panicked
--> $DIR/issue-76270-panic-in-libproc-macro.rs:15:1
--> $DIR/issue-76270-panic-in-libproc-macro.rs:10:1
|
LL | proc_macro_panic::panic_in_libproc_macro!();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
10 changes: 0 additions & 10 deletions src/test/ui/proc-macro/load-panic-backtrace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,8 @@
// aux-build:test-macros.rs
// compile-flags: -Z proc-macro-backtrace
// rustc-env:RUST_BACKTRACE=0

// FIXME https://github.com/rust-lang/rust/issues/59998
// normalize-stderr-test "thread '.*' panicked " -> ""
// normalize-stderr-test "note:.*RUST_BACKTRACE=1.*\n" -> ""
// normalize-stderr-test "\nerror: internal compiler error.*\n\n" -> ""
// normalize-stderr-test "note:.*unexpectedly panicked.*\n\n" -> ""
// normalize-stderr-test "note: we would appreciate a bug report.*\n\n" -> ""
// normalize-stderr-test "note: compiler flags.*\n\n" -> ""
// normalize-stderr-test "note: rustc.*running on.*\n\n" -> ""
// normalize-stderr-test "query stack during panic:\n" -> ""
// normalize-stderr-test "we're just showing a limited slice of the query stack\n" -> ""
// normalize-stderr-test "end of query stack\n" -> ""

#[macro_use]
extern crate test_macros;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/load-panic-backtrace.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
at 'panic-derive', $DIR/auxiliary/test-macros.rs:43:5
error: proc-macro derive panicked
--> $DIR/load-panic-backtrace.rs:20:10
--> $DIR/load-panic-backtrace.rs:10:10
|
LL | #[derive(Panic)]
| ^^^^^
Expand Down