search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.61k stars 12.48k forks source link

__asan_globals_registered is not comdat when building a staticlib with LTO #113404

Closed glandium closed 1 year ago

glandium commented 1 year ago

Disclaimer: I tried to create a testcase from scratch, but for some reason I wasn't able to find a way to trigger the use of __asan_register_elf_globals instead of __asan_globals_register.

STR:

Actual output:

0000000000000000 l     O .bss.___asan_globals_registered    0000000000000008 ___asan_globals_registered
0000000000000000 l    d  .bss.___asan_globals_registered    0000000000000000 .bss.___asan_globals_registered

Expected output: Something like:

0000000000000008       O *COM*  0000000000000008 .hidden ___asan_globals_registered

This doesn't happen without LTO. The unfortunate consequence is that when the resulting static library is linked with C or C++ code compiled with clang with -fsanitize=address -fsanitize-address-globals-dead-stripping (that latter flag is now default in clang trunk), which also uses __asan_register_elf_globals/__asan_globals_registered, ODR violation detection kick in complaining about globals defined multiple times, because both the clang-side asan constructor and the rust asan constructor register all the globals. Normally, what happens is that they both use the same __asan_globals_registered (thus it normally being *COM*), and set its value, so that only one constructor registers the globals. With the LTOed staticlib, what happens is that there are two distinct __asan_globals_registered, so both constructors go through.

rustc +nightly --version --verbose:

rustc 1.72.0-nightly (d9c13cd45 2023-07-05)
binary: rustc
commit-hash: d9c13cd4531649c2028a8384cb4d4e54f985380e
commit-date: 2023-07-05
host: x86_64-unknown-linux-gnu
release: 1.72.0-nightly
LLVM version: 16.0.5

(Edit: fixed typos, changed the peculiar setup with -Clto and -Cembed-bitcode=yes to the more normal LTO, which shows the problem too)

danakj commented 1 year ago

https://bugs.chromium.org/p/chromium/issues/detail?id=1459233 is tracking this for Chromium as it causes our asan bots to fail.

rnk commented 1 year ago

Here is the code which creates the __asan_globals_registered global: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L2216

It's interesting that it uses common linkage (*COM* as you said). Common linkage should work, but to me it is a surprising choice. I would've expected this global to use linkonce_odr linkage and be marked comdat, which is what you would get for a C++17 inline global.

I don't know how Rust is producing LTOed static libraries, but maybe somewhere along the way there is a bug in how LTO is handling common linkage globals.

One possible summary of this issue is that the ODR violation detector is suffering from an ODR violation, we have two flags when we should have one.

glandium commented 1 year ago

One possible summary of this issue is that the ODR violation detector is suffering from an ODR violation, we have two flags when we should have one.

I like this take :)

MaskRay commented 1 year ago

I consider myself quite familiar with asan, LTO, Clang Driver, but I know very little about Rust.

rm -r target/release/libbuiltins_static.a
RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release --verbose

gives me this command (after removing --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat):

rustc --crate-name builtins_static --edition=2021 src/lib.rs --diagnostic-width=159 --crate-type staticlib --emit=dep-info,link -C opt-level=3 -C lto -C metadata=9ef9aaa79a14308e -C extra-filename=-9ef9aaa79a14308e --out-dir /tmp/p/nss-builtins/target/release/deps -L dependency=/tmp/p/nss-builtins/target/release/deps --extern pkcs11_bindings=/tmp/p/nss-builtins/target/release/deps/libpkcs11_bindings-cb6ed583361f31fa.rlib --extern smallvec=/tmp/p/nss-builtins/target/release/deps/libsmallvec-e6a97ae5d501d626.rlib -Zsanitizer=address
% ar t /tmp/p/nss-builtins/target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a | wc -l
177
% ar x /tmp/p/nss-builtins/target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o
% readelf -Ws builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o | grep ___asan_globals_registered
   117: 0000000000000000     8 OBJECT  LOCAL  DEFAULT 3732 ___asan_globals_registered
  2944: 0000000000000000     0 SECTION LOCAL  DEFAULT 3732 .bss.___asan_globals_registered
% llvm-nm -gU builtins_static-9ef9aaa79a14308e.builtins_static.7af90105-cgu.0.rcgu.o
0000000000000000 T BUILTINSC_GetFunctionList
0000000000000000 V DW.ref.rust_eh_personality
0000000000000000 D _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17h244e25de9c1d3c88E
0000000000000000 T rust_eh_personality

Most defined symbols are localized. I do not know why the 4 symbols are special and rustc lto doesn't localize them.

I don't know how to rerun the rustc with a locally built (./x.py build with config.toml containing [rust]\ndebug=true). I suspect that preventing ___asan_globals_registered from being localized will fix this bug.

MaskRay commented 1 year ago

-fcommon is consider bad nowadays but the ___asan_globals_registered COMMON symbol use case is fine. It is like a COMDAT group containing just a variable. COMMON is more size efficient than using a COMDAT group (there is a size overhead due to a 64 byte Elf64_Shdr header).

If we use a COMDAT for ___asan_globals_registered but rustc compiler/rustc_codegen_llvm/src/back/lto.rs still localizes the symbol, then we'd still have this bug.

anforowicz commented 1 year ago

Let me try to help move this bug forward by attempting to answer some of the questions above. I have limited experience with linkers, asan, and compilers, so please shout if there are any mistakes below.

RE: @MaskRay: I don't know how to rerun the rustc with a locally built (./x.py build)

I tweaked the original cargo cmdline (from the first comment/report on this bug) by adding RUSTC=<path to locally built rustc> and adding -Zbuild-std --target x86_64-unknown-linux-gnu (and other than that I've rerun the original repro steps + debugging steps from your earlier comment at https://github.com/rust-lang/rust/issues/113404#issuecomment-1633307926):

RUSTC=$HOME/src/github/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release -Zbuild-std --target x86_64-unknown-linux-gnu

(Note that this changes the path where the build artifacts are - e.g. target/x86_64-unknown-linux-gnu/release/deps/libbuiltins_static-9bce9c00959dd948.a instead of target/release/deps/libbuiltins_static-9ef9aaa79a14308e.a)

RE: @MaskRay: I do not know why the 4 symbols are special and rustc lto doesn't localize them.

I am not sure if these are the reasons, but this is what I've found for some of the symbols emitted by llvm-nm -gU ...:

RE: @MaskRay: I suspect that preventing ___asan_globals_registered from being localized will fix this bug.

Can you please elaborate on that? From your https://github.com/rust-lang/rust/issues/113404#issuecomment-1633307926, it seems that you are saying that a change in rustc is needed - did I get that right?

OTOH, it seems that ___asan_globals_registered comes from outside of rustc sources - it comes from llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp and therefore it seems that maybe we need to change how it is marked for linking by LLVM? Interestingly the comment there mentions "a local symbol" - not sure if this is relevant:

// ASan version script has __asan_* wildcard. Triple underscore prevents a
// linker (gold) warning about attempting to export a local symbol.
const char kAsanGlobalsRegisteredFlagName[] = "___asan_globals_registered";
anforowicz commented 1 year ago

/cc @eugenis who AFAICT added the kAsanGlobalsRegisteredFlagName comment above in https://github.com/llvm/llvm-project/commit/964f4663c42c42fb958b14cb3b510daa5ce54252#diff-74cc02fff013e3ab08aefcad70e555b1177549f8403d03395c856c3f8c5b5875

zmodem commented 1 year ago

/cc @tru fyi. Although this isn't fully understood and it may well be that the fix will be on the rust side, this is a rustc/clang interop issue that's new with the upcoming clang 17 release, so perhaps there should at least be a "known issues" release note.

tru commented 1 year ago

cc @nikic

nikic commented 1 year ago

Probably the symbol needs to be added to the symbol export list, see existing handling for __llvm_profile and __msan symbols in https://github.com/rust-lang/rust/blob/6742e2b18502afa9d27b0e02d0cfa36aa93aa2ee/compiler/rustc_codegen_ssa/src/back/symbol_export.rs#L248.

anforowicz commented 1 year ago

@nikic - thanks for the pointer! After adding the following to compiler/rustc_codegen_ssa/src/back/symbol_export.rs:

if tcx.sess.opts.unstable_opts.sanitizer.contains(SanitizerSet::ADDRESS) {
    // Similar to profiling, preserve weak asan symbols during LTO.
    const ASAN_WEAK_SYMBOLS: [&str; 1] = ["___asan_globals_registered"];

    symbols.extend(ASAN_WEAK_SYMBOLS.into_iter().map(|sym| {
        let exported_symbol = ExportedSymbol::NoDefId(SymbolName::new(tcx, sym));
        (
            exported_symbol,
            SymbolExportInfo {
                level: SymbolExportLevel::C,
                kind: SymbolExportKind::Data,
                used: false,
            },
        )
    }));
}

I am now seeing ___asan_globals_registered in the output of llvm-nm (which I assume means that the problem is fixed based on https://github.com/rust-lang/rust/issues/113404#issuecomment-1633307926):

$ $HOME/src/github/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-nm -gU builtins_static-9bce9c00959dd948.builtins_static.29256d2b2790aa9a-cgu.1.rcgu.o
0000000000000000 T BUILTINSC_GetFunctionList
0000000000000000 V DW.ref.rust_eh_personality
0000000000000000 D _ZN3std3sys4unix4args3imp15ARGV_INIT_ARRAY17h45459e0562778ae4E
0000000000000008 C ___asan_globals_registered
0000000000000000 T rust_eh_personality

Repro steps for completeness:

  1. cd ~/src/github/rust
  2. edit compiler/rustc_codegen_ssa/src/back/symbol_export.rs
  3. ./x build
  4. cd ~/scratch/rustc-test/nss-builtins
  5. rm -rf target
  6. RUSTC=$HOME/src/github/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc RUSTFLAGS="-Zsanitizer=address" CARGO_PROFILE_RELEASE_LTO=true cargo +nightly build --release -Zbuild-std --target x86_64-unknown-linux-gnu
  7. ar t target/x86_64-unknown-linux-gnu/release/deps/libbuiltins_static-9bce9c00959dd948.a | grep builtins_static
  8. ar x target/x86_64-unknown-linux-gnu/release/deps/libbuiltins_static-9bce9c00959dd948.a builtins_static-9bce9c00959dd948.builtins_static.29256d2b2790aa9a-cgu.1.rcgu.o
  9. $HOME/src/github/rust/build/x86_64-unknown-linux-gnu/llvm/bin/llvm-nm -gU builtins_static-9bce9c00959dd948.builtins_static.29256d2b2790aa9a-cgu.1.rcgu.o

I'll try to put together a PR with the changes above.

I think one of the first steps is to figure out if/how to add tests for this. I'll try to see if I can cargo cult something from either e2acaee8bb364197af2ab197f0f641e8f988ae04 (Add codegen test that makes sure PGO instrumentation is emitted as expected), 4053e25bfb55b6e1bf6766158ccd06cb87de79c7 (librustc_trans: Mark some profiler symbols as exported to avoid LTO removing them), d8c661a88644ad710e309d3a8f0f27c5f74d705f (Mark msan_keep_going as an exported symbol for LTO), or 2c0845c6ccfdee7fb255756918a22101376efa7e (Mark msan_track_origins as an exported symbol for LTO by @nikic - thanks again for the code pointer!),

FWIW, I think it would be nice to consolidate the code from the 3 similar cases: instrument_coverage() || profile_generate.enabled(), SanitizerSet::MEMORY, and SanitizerSet::ADDRESS. I plan to have a separate commit that refactors gathering all symbol names into a single vector and then uses a single/shared symbols.extend(...iter().map(|sym| { ... }).

Finally, I am a bit surprised that compiler/rustc_codegen_ssa/src/back/symbol_export.rs needs to be aware of symbol names used in a far-away LLVM land where AFAIU ASAN and MSAN are implemented. This arrangement seems fragile. OTOH, it seems that this does fix the issue at hand, so it seems like a reasonable way to proceed.

nikic commented 1 year ago

Finally, I am a bit surprised that compiler/rustc_codegen_ssa/src/back/symbol_export.rs needs to be aware of symbol names used in a far-away LLVM land where AFAIU ASAN and MSAN are implemented. This arrangement seems fragile. OTOH, it seems that this does fix the issue at hand, so it seems like a reasonable way to proceed.

This is because Rust assumes that all code provided to (non-plugin) LTO comes from Rust, so it knows about all symbols that are involved. This doesn't hold up for symbols that get injected by LLVM, so they need to be special-cased.

anforowicz commented 1 year ago

TL;DR: I would appreciate help with creating a regression test for this.

I have trouble creating a regression test for this. Based on the contributor docs, FileCheck tests under tests/codegen run with --emit=llvm-ir and it seems that exactly the same LLVM IR is present in rust/build/x86_64-unknown-linux-gnu/test/codegen/sanitizer/address-sanitizer-globals-tracking/address-sanitizer-globals-tracking.ll before and after the changes from https://github.com/rust-lang/rust/issues/113404#issuecomment-1669979127. In other words, the following test (say, added in tests/codegen/sanitizer/address-sanitizer-globals-tracking.rs) passes before and after the change:

// Verifies that AddressSanitizer symbols show up as expected in LLVM IR
// with -Zsanitizer (DO NOT SUBMIT: consider adding non-LTO and fat LTO test flavours/revisions?)
//
// needs-sanitizer-address
// compile-flags: -Zsanitizer=address -C lto

#![crate_type="lib"]

// The test below mimics `CACHED_POW10` from `library/core/src/num/flt2dec/strategy/grisu.rs` which
// (because of incorrect handling of `___asan_globals_registered` during LTO) was incorrectly
// reported as an ODR violation in https://crbug.com/1459233#c1.
//
// See https://github.com/rust-lang/rust/issues/113404 for more discussion.
//
// CHECK: @___asan_globals_registered = common hidden global i64 0
// CHECK: @__start_asan_globals = extern_weak hidden global i64
// CHECK: @__stop_asan_globals = extern_weak hidden global i64
pub static CACHED_POW10: [(u64, i16, i16); 4] = [
    (0xe61acf033d1a45df, -1087, -308),
    (0xab70fe17c79ac6ca, -1060, -300),
    (0xff77b1fcbebcdc4f, -1034, -292),
    (0xbe5691ef416bd60c, -1007, -284),
];

AFAICT the test above replicates the following rustc cmdline flags from https://github.com/rust-lang/rust/issues/113404#issuecomment-1633307926: -Zsanitizer=address, -C lto. It doesn't replicate --crate-type staticlib, but I haven't pursued this direction further, because #![crate_type="staticlib"] necessitates -C prefer-dynamic=false and no longer includes ___asan_globals_registered in the LLVM IR. The presence or absence of -C opt-level=3 doesn't seem to change the result here.

I notice that https://github.com/rust-lang/rust/issues/113404#issuecomment-1633307926 (and my repro steps in https://github.com/rust-lang/rust/issues/113404#issuecomment-1669979127) reported seeing --emit=dep-info,link rather than --emit=llvm-ir. So maybe my changes only have effect on stages after LLVM-IR generation? I dunno... Not sure if this hypothesis makes sense...

tmiasko commented 1 year ago

When a module doesn't exports any symbols, getUniqueModuleId returns an empty string and instrumentation no longer uses ___asan_globals_registered:

https://github.com/rust-lang/llvm-project/blob/7c612e1732f3976fcfe29526ad796cbb6174b829/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L2524-L2526

You can export an arbitrary symbol. For example mark CACHED_POW10 as #[no_mangle] and then use --crate-type=staticlib.

anforowicz commented 1 year ago

@tmiasko - thanks! I have a working test now :-).

rnk commented 1 year ago

When a module doesn't exports any symbols, getUniqueModuleId returns an empty string and instrumentation no longer uses ___asan_globals_registered:

I have a patch lying around to try to simplify this logic, but I haven't prioritized it because this issue is still outstanding and I don't want to complicate matters while we look for a solution.

anforowicz commented 1 year ago

I may need some extra help with the tests please. The latest version of the PR fails on x86_64-apple-1 (see https://github.com/rust-lang/rust/pull/114642#issuecomment-1677822766) and it seems that the FileCheck's input file (i.e. LLVM-IR) doesn't contain any mentions of ___asan_globals_registered.

I am not sure how to handle the test failure above.

tmiasko commented 1 year ago

@anforowicz since the instrumentation strategy is target specific, limiting exercised targets seems appropriate.

Since we don't have a option to run tests only on ELF targets (which would use InstrumentGlobalsELF code path and ___asan_globals_registered), maybe use only-linux header (it will be sufficient to exercise test on CI).

anforowicz commented 1 year ago

Thanks @tmiasko! I've added only-linux (plus an explanatory comment) to the new test. Feel free to ask bors to commit if this LGTY.

I note that, since we are saying that ___asan_globals_registered is ELF-specific, then maybe we should consider only applying the product code changes to ELF targets. Still, maybe recognizing the ___asan_globals_registered symbol on other targets won't hurt (and it definitely simplifies the code). FWIW, I see that codegen_attrs.rs has some is_like_elf-dependent behavior, but I am not sure if this should also be used in the ___asan_globals_registered PR.

anforowicz commented 1 year ago

The latest test failure (see https://github.com/rust-lang/rust/pull/114642#issuecomment-1680112624) on x86_64-apple-1 is:

---- [run-make] tests/run-make/sanitizer-cdylib-link stdout ----
--- stdout -------------------------------
...
  = note: Undefined symbols for architecture x86_64:
            "____asan_globals_registered", referenced from:
               -exported_symbol[s_list] command line option
          ld: symbol(s) not found for architecture x86_64
          clang: error: linker command failed with exit code 1 (use -v to see invocation)
...

This seems to indicate that the product code changes should not unconditionally expect the ___asan_globals_registered symbols whenever ASAN is used, but instead the presence of the symbol should depend on the target architecture. This is a little bit tricky - I see that llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp refers to kAsanGlobalsRegisteredFlagName from:

void ModuleAddressSanitizer::InstrumentGlobalsELF(...) {
  ...
  // RegisteredFlag serves two purposes. First, we can pass it to dladdr()
  // to look up the loaded image that contains it. Second, we can store in it
  // whether registration has already occurred, to prevent duplicate
  // registration.
  //
  // Common linkage ensures that there is only one global per shared library.
  GlobalVariable *RegisteredFlag = new GlobalVariable(
      M, IntptrTy, false, GlobalVariable::CommonLinkage,
      ConstantInt::get(IntptrTy, 0), kAsanGlobalsRegisteredFlagName);
  RegisteredFlag->setVisibility(GlobalVariable::HiddenVisibility);
  ...
}
...
void ModuleAddressSanitizer::InstrumentGlobalsMachO(...) {
  ...
  // RegisteredFlag serves two purposes. First, we can pass it to dladdr()
  // to look up the loaded image that contains it. Second, we can store in it
  // whether registration has already occurred, to prevent duplicate
  // registration.
  //
  // common linkage ensures that there is only one global per shared library.
  GlobalVariable *RegisteredFlag = new GlobalVariable(
      M, IntptrTy, false, GlobalVariable::CommonLinkage,
      ConstantInt::get(IntptrTy, 0), kAsanGlobalsRegisteredFlagName);
  RegisteredFlag->setVisibility(GlobalVariable::HiddenVisibility);
  ...
}

So, based on the above https://github.com/rust-lang/rust/pull/114642/commits/bf9820e6fef8f8c06604b5673f36e33d94ed4c25#diff-6d46444b86c506127f8dd251a0c949845de78baec01de25b7524b3b10d90efd7 should maybe be modified to only recognize ___asan_globals_registered for ELF and MachO targets.

anforowicz commented 1 year ago

I don't know how to detect ELF or MachO targets based on tcx (this would be required if we wanted to pursue the direction outlined at the end of https://github.com/rust-lang/rust/issues/113404#issuecomment-1681200130). There is tcx.sess.target (Target which also implements a Deref for TargetOptions), but it's unclear to me how to map this into ELF-or-MachO decision.

This hurdle also reinforces the feeling that I've expressed in https://github.com/rust-lang/rust/issues/113404#issuecomment-1669979127: it feels fragile that we have to keep rustc aware of how instrumentation symbols are used in a far-away LLVM land. I would hope that the information about these symbols can be communicated by LLVM (instead of having to duplicate/replicate this information in rustc sources). OTOH, I have very little experience working with linkers, so I don't know if a viable alternative exists.

@tmiasko, @nikic, @rnk, and/or @MaskRay - could you please comment on the above? Do we really want/need rust/compiler/rustc_codegen_ssa/src/back/symbol_export.rs to replicate the knowledge from the (far-away land of) llvm-project/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp that the ___asan_globals_registered symbol exists on ELF and MachO targets and that rustc should handle it with specific SymbolExportInfo?

anforowicz commented 1 year ago

RE: @nikic: https://github.com/rust-lang/rust/issues/113404#issuecomment-1670163459: Rust assumes that all code provided to (non-plugin) LTO comes from Rust

Would it be possible for rustc to detect symbols injected by LLVM (like ___asan_globals_registered, __msan_keep_going, or __llvm_profile_filename) more generically (without knowing about specific symbol names)? If there was a way to see the list of all symbols (in compiler/rustc_codegen_ssa/src/back/symbol_export.rs? in compiler/rustc_codegen_llvm/src/back/lto.rs?) then maybe these LLVM-origin symbols could be detected by special-casing their prefix (__llvm, __msan, __asan, or ___asan) rather than their exact names?

I tried staring for a while at fn exported_symbols_provider_local and I don't currently see a way to go from its input (i.e. tcx: TyCtxt) to the list of all symbols (including ones injected by LLVM). FWIW, I also don't understand how symbols injected by LLVM (e.g. https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L2216) present themselves at the rustc API level (I guess they are not "items" in rustc sense and don't have a DefId).

rnk commented 1 year ago

it feels fragile that we have to keep rustc aware of how instrumentation symbols are used in a far-away LLVM land.

With an understanding that we want to work together and find a practical solution, let me first represent the perspective of the instrumentation tool writer. From the PoV of one writing instrumentation passes, it is surprising that Rust assumes that all symbols originate from Rust, and that instrumentation passes cannot add symbols and rely on the standard linkage rules. From the ASan PoV, all we are doing is inventing a common symbol and assuming that linking will proceed as normal, meaning the common symbol will be deduplicated, and there will be one per DSO. I would argue that this Rust pass is breaking fairly reasonable assumptions.

But I'm more interested in practical solutions, so yes, I think using those prefixes would be a good place to start. I'm not sure that list is complete, however.

We could go the direction of having LLVM instrumentation passes annotate symbols somehow, but it would be difficult to enforce that as a requirement for instrumentation passes. Anyone writing a new instrumentation pass is likely to discover the annotation requirement when they first try to instrument Rust code, which doesn't reduce the cost of this issue, it just distributes the fixes from export_symbols.rs to each instrumentation pass.

You may be able to put together some heuristic by looking at global variables with weak linkages (common, linkonce_odr, weak, weak_odr, etc). If Rust never or rarely produces weak symbols, you could teach the exporter to leave those alone. That would be the simplest.

bjorn3 commented 1 year ago

I would argue that this Rust pass is breaking fairly reasonable assumptions.

For the staticlib and cdylib crate types we need to hide all symbols except those marked with #[no_mangle] to prevent conflicts between multiple copies of rust crates (like the standard library itself) that are statically linked into them.[^1] We are using the same list of symbols to be exported for LTO as these symbols definitively need to be exported and not exporting the rest as far as LTO is concerned will allow for the most optimizations. The staticlib and cdylib crate types use a closed world model: All rust code is contained in them and only an explicitly specified C interface is exported. https://github.com/rust-lang/rust/pull/114642 simply extends the C interface it exports.

[^1]: For staticlibs we currently don't do this as there is no way to do this without editing the object files themself it seems. (hidden visibility wouldn't work as applying hidden visibility to the standard library would break dynamic linking) This currently makes it to be impossible to link two rust staticlibs into the same executable except when using a linker which allows multiple identical definitions of a symbol and even then multiple versions of a crate would break.

anforowicz commented 1 year ago

AFAICT the initial fix attempt at https://github.com/rust-lang/rust/pull/114642/commits/bf9820e6fef8f8c06604b5673f36e33d94ed4c25 influences whether prepare_lto from rust/compiler/rustc_codegen_llvm/src/back/lto.rs contains ___asan_globals_registered in the symbols_below_threshold that get returned as the first tuple item. symbols_below_threshold are then passed to LLVMRustRunRestrictionPass in rust/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp and used in the PreserveFunction predicate.

So, let me try a different fix approach and recognize ASAN/MSAN/etc-related symbols in the PreserveFunction predicate (rather than in exported_symbols_provider_local as in the original fix attempt). See: https://github.com/rust-lang/rust/pull/114946

anforowicz commented 1 year ago

I guess one could argue about pushing the knowledge about __asan, __msan, etc. prefixes even further down - e.g. into the implementation of InternalizePass::internalizeModule or InternalizePass::shouldPreserveGV in LLVM. @rnk and/or @tmiasko - any strong opinions on continuing with the rustc PR at https://github.com/rust-lang/rust/pull/114946 VS authoring an LLVM PR instead?

rnk commented 1 year ago

Putting this list of symbols in LLVM sounds practical to me. Is it possible to emulate what Rust is doing using opt -internalize? If so, we could make a small regression test out of this, and put it in the asan test suite.

anforowicz commented 1 year ago

Hmmm... I now think that it still makes sense to review and attempt to land the rustc PR at https://github.com/rust-lang/rust/pull/114946I even if we (eventually) preserve these ASAN symbols via a separate LLVM PR:

RE: @rnk: https://github.com/rust-lang/rust/issues/113404#issuecomment-1683041051: Is it possible to emulate what Rust is doing using opt -internalize?

Maybe. I don't know how to check. Sorry.

FWIW, I see that LLVM-level tests that use opt -internalize take LLVM-IR as input. This probably means that ___asan_globals_registered would have to be hardcoded/simulated in the test input (rather than generated by the real ASAN). Maybe this is ok.

RE: @rnk: https://github.com/rust-lang/rust/issues/113404#issuecomment-1683041051: Putting this list of symbols in LLVM sounds practical to me.

The llvm-project/llvm/lib/Transforms/IPO/Internalize.cpp source file has quite elaborate code for preserving some symbols. I wonder if ASAN / MSAN / etc symbols should be protected by any of the existing mechanisms:

anforowicz commented 1 year ago

Status update / summary (I edited this comment on 2023-08-25 at 10:47 PST to add one other potential next step at the very end of the comment):

Can we discuss the next steps?:

anforowicz commented 1 year ago

@bjorn3, in https://github.com/rust-lang/rust/pull/114946#issuecomment-1693765740 you've suggested that there might be issue with Option 2 of the fix. Let me partially reply here, because I have some questions that are not related to Option 2.

Do you think we should proceed with Option 1 (or is there another approach that you'd suggest)? If so, then I assume that you agree that to avoid the x86_64-apple-1 test failures we should only inject __asan_globals_registered in exported_symbols_provider_local when the symbol is actually present (i.e. when ModuleAddressSanitizer::InstrumentGlobalsELF and/or ModuleAddressSanitizer::InstrumentGlobalsMachO inject the symbol). Could you please help me understand how to tweak https://github.com/rust-lang/rust/pull/114642 to do this? How can exported_symbols_provider_local detect ELF and/or MachO targets? Do you think it would be okay to copy-and-paste let is_like_elf = ... from codegen_attrs.rs? What would you suggest for detecting MachO targets? (I don't see code for detecting MachO targets outside of compiler/rustc_codegen_cranelift/src/lib.rs which I assume can't be used outside of cranelist.)

bjorn3 commented 1 year ago

Do you think we should proceed with Option 1

I think so, but I may be misunderstanding what exactly address sanitizer needs in terms of linkage.

How can exported_symbols_provider_local detect ELF and/or MachO targets?

Like this:

https://github.com/rust-lang/rust/blob/22d41ae90facbffdef9115809e8b6c1f71ebbf7c/compiler/rustc_codegen_ssa/src/back/metadata.rs#L217-L225

Maybe extracting this into a function to ensure it is kept in sync makes sense.

anforowicz commented 1 year ago

How can exported_symbols_provider_local detect ELF and/or MachO targets?

Like this:

...

Maybe extracting this into a function to ensure it is kept in sync makes sense.

Thanks! That helps :-). Not sure how I managed to miss this piece of code when greepping the source code for BinaryFormat... :-/

FWIW I've applied the changes suggested above to the newly pushed version of https://github.com/rust-lang/rust/pull/114642 (e.g. see https://github.com/rust-lang/rust/pull/114642/commits/d9abc46a680750c63e0f14e19d900fa51cfbd094#diff-aa810a3be0834da891b171a8b04b09d0d3bbb76ac27c757cd232235099e062d2)