Is passing -Clink-dead-code by default still a good idea? #391
Comments
|
I can run the cited quiche fuzzer on Debian bullseye using the provided command So my only guess is that the reason this now works even if I follow all the directions for the reproducer is that Debian bullseye shipped a fix to the GNU ld linker, my bullseye container says |
|
I'm in favor of removing this flag! |
Update docs for `-Clink-dead-code` to discourage its use The `-Clink-dead-code` flag was originally added way back in rust-lang#31368, apparently to help improve the output of some older forms of code coverage measurement, and also to address some use-cases for wanting to suppress linker flags like `-dead_strip` and `--gc-section`. In the past it might have also been useful in conjunction with `-Cinstrument-coverage`, but subsequent improvements to coverage instrumentation have made it unnecessary there. [It is also currently used by cargo-fuzz by default](rust-fuzz/cargo-fuzz#391), for reasons that are possibly no longer relevant. --- The flag currently does more than its name suggests, affecting not just linker flags, but also monomorphization decisions. It has also contributed to ICEs (e.g. rust-lang#135515) that would not have occurred without link-dead-code. --- For now, this PR just updates the documentation to be more realistic about what the flag does, and when it should be used (approximately never). In the future, it might be worth looking into properly deprecating this flag, and perhaps making it a no-op if feasible.
Rollup merge of rust-lang#135561 - Zalathar:link-dead-code, r=saethlin Update docs for `-Clink-dead-code` to discourage its use The `-Clink-dead-code` flag was originally added way back in rust-lang#31368, apparently to help improve the output of some older forms of code coverage measurement, and also to address some use-cases for wanting to suppress linker flags like `-dead_strip` and `--gc-section`. In the past it might have also been useful in conjunction with `-Cinstrument-coverage`, but subsequent improvements to coverage instrumentation have made it unnecessary there. [It is also currently used by cargo-fuzz by default](rust-fuzz/cargo-fuzz#391), for reasons that are possibly no longer relevant. --- The flag currently does more than its name suggests, affecting not just linker flags, but also monomorphization decisions. It has also contributed to ICEs (e.g. rust-lang#135515) that would not have occurred without link-dead-code. --- For now, this PR just updates the documentation to be more realistic about what the flag does, and when it should be used (approximately never). In the future, it might be worth looking into properly deprecating this flag, and perhaps making it a no-op if feasible.
|
Hi, I'm happy to help with the implementation. To avoid disrupting existing users, perhaps we could simply change the default value of |
Sounds great 👍 |
|
Nice, please assign this issue to me. I will give it fixed. |
I work on the monomorphization code in the compiler a fair bit, and any use of
-Clink-dead-codemakes me uncomfortable because that flag isn't just about linkage, it has to toggle off other dead code pruning mechanisms in the compiler in order to produce all the dead code. And many of those code paths that it exercises are not well-tested and certainly are not well-exercised in production use-cases.cargo-fuzz has run into two ICEs that I know of as a result of using this flag:
rust-lang/rust#135515
#323
(both of these are bugs in the compiler that wouldn't be hit without
-Clink-dead-code)The LLVM issue that motivated using
-Clink-dead-codewhen also using coverage instrumentation is llvm/llvm-project#33984, and it has been closed for years now. It sounds like this is fully fixed with LLD which is the default on nightly toolchains, but not on stable.Someone should check if GNU's linker still causes trouble if used with
--strip-dead-code.It looks like in some builds,
-Clink-dead-codecan have a dramatic impact on disk space: bytecodealliance/wasmtime#3875 which might be another motivation for swapping the default behavior of cargo-fuzz.The text was updated successfully, but these errors were encountered: