This is an archive of the discontinued LLVM Phabricator instance.

Revert [MC][ELF] Emit separate unique sections for different flags
AbandonedPublic

Authored by nikic on Jul 31 2021, 1:16 AM.

Details

Summary

D100944 broke the ability to specify custom section flags in LLVM IR by including an empty section with the desired flags in module asm. This functionality is used by rustc to embed LTO bitcode with the SHF_EXCLUDE flag, and currently blocks our evaluation of the LLVM 13 release. As the post-commit discussion on D100944 indicates, other people were using this functionality as well: https://reviews.llvm.org/D100944#2840948

This reverts D100944 to restore the ability to set section flags. The change from D100944 can be reapplied once LLVM provides an alternative way to specify section flags -- it would certainly be better if it were possible to do this using a dedicated IR construct. But I don't think it's possible to add such a construct for the LLVM 13 release anymore, so this reverts to the status quo.

Fixes https://bugs.llvm.org/show_bug.cgi?id=51207.

Diff Detail

Unit TestsFailed

Event Timeline

nikic created this revision.Jul 31 2021, 1:16 AM
nikic requested review of this revision.Jul 31 2021, 1:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2021, 1:16 AM
MaskRay requested changes to this revision.Jul 31 2021, 1:19 AM
This revision now requires changes to proceed.Jul 31 2021, 1:19 AM

This functionality is used by rustc to embed LTO bitcode with the SHF_EXCLUDE flag, and currently blocks our evaluation of the LLVM 13 release.

But Rust usage is technically incorrect. I think Rust can carry its own patch to for the LLVM 13 release.

As the post-commit discussion on D100944 indicates, other people were using this functionality as well: https://reviews.llvm.org/D100944#2840948

I think @bd1976llvm have found a way forward.

This cannot be used as an argument favoring the revert: the code abuses a property which cannot be guaranteed by the assembler.

nikic added a subscriber: tstellar.Jul 31 2021, 1:57 AM

This functionality is used by rustc to embed LTO bitcode with the SHF_EXCLUDE flag, and currently blocks our evaluation of the LLVM 13 release.

But Rust usage is technically incorrect. I think Rust can carry its own patch to for the LLVM 13 release.

I mean, we're happy to carry the patch, but that does mean that all distros are going to carry it as well (as they ship rust linked against system LLVM). I prefer upstreaming the fix over this being a "de facto" LLVM 13 patch that everyone is required to apply independently. I'll leave it to @tstellar to decide.

To be clear, I don't particularly care about the change that D100944 did, the only thing I care about is that it resulted in an effective loss of functionality. If it's possible to add a first-class way to set section flags in IR and backport it to LLVM 13, I will be perfectly happy with that.

MaskRay added a comment.EditedJul 31 2021, 10:49 AM

This functionality is used by rustc to embed LTO bitcode with the SHF_EXCLUDE flag, and currently blocks our evaluation of the LLVM 13 release.

But Rust usage is technically incorrect. I think Rust can carry its own patch to for the LLVM 13 release.

I mean, we're happy to carry the patch, but that does mean that all distros are going to carry it as well (as they ship rust linked against system LLVM). I prefer upstreaming the fix over this being a "de facto" LLVM 13 patch that everyone is required to apply independently. I'll leave it to @tstellar to decide.

To be clear, I don't particularly care about the change that D100944 did, the only thing I care about is that it resulted in an effective loss of functionality. If it's possible to add a first-class way to set section flags in IR and backport it to LLVM 13, I will be perfectly happy with that.

Rust's current is flawed in another way: it assumes that module asm is emitted before rustc.embedded.module
The Rust workaround is weighted against breaking correctness and @tmatheson 's workflow.

(The Rust comment says "* Mach-O - this is for macOS. Inspecting the source code for the native ... ". I am not sure how important the feature is to ELF.)

Rust can try other approaches, e.g. replacing mixed module asm & rustc.embedded.module with pure module asm (.ascii "abc" or .incbin "file").
That will not break correctness and @tmatheson 's workflow.

I think @bd1976llvm have found a way forward.

We haven't yet solved this satisfactorily and we are still investigating; however, we don't want LLVM's behaviour compromised to support our use-case.
We would support a proper feature to assign symbols to sections with specified flags (especially as GNU appear to offer this as a feature/*horrible hack* : see comments on https://reviews.llvm.org/D100944).

cuviper added a subscriber: cuviper.Aug 2 2021, 6:04 PM

Rust's LLVM fork has the revert, but this is still going to be a problem for Rust with system LLVM 13. Do we have a way forward for this release? A revert seems to be a safer path than trying to develop something new in short time.

Even if the former behavior wasn't guaranteed, it still worked well enough in practice, so right now we have a functional regression.

MaskRay added a comment.EditedSep 8 2021, 4:07 PM

Rust's LLVM fork has the revert, but this is still going to be a problem for Rust with system LLVM 13. Do we have a way forward for this release? A revert seems to be a safer path than trying to develop something new in short time.

Even if the former behavior wasn't guaranteed, it still worked well enough in practice, so right now we have a functional regression.

Rust abused the previous LLVM behavior when an inline asm .section directive can override the section attributes of section ".llvmbc".
LLVM doesn't provide such guarantee.
In addition, the Rust comment mentions that it knows the approach is a hack.

I have made a suggestion that: Rust can try other approaches, e.g. replacing mixed module asm & rustc.embedded.module with pure module asm (.ascii "abc" or .incbin "file").
Have you tried it?

@cuviper Not exploring a proper approach forward, but just trigger-happily reverting a correct change and breaking the original author (and others)' workflow is not IMHO the correct approach.

What if we reverted the original change in the release/13.x branch only and kept the patch in trunk? This would give rust ~4 months to work on a proper solution which should be enough time.

Reverting *only* in release/13.x looks good to me.

What's the correct way to fix this in the release/13.x branch? Should I apply this patch or revert 165321b3d27de5349520b5fdb7e08cbd238c880f?

Applying this patch. git revert -n 165321b3d27de5349520b5fdb7e08cbd238c880f seems to have merge conflicts.

But I'd definitely reword the commit message since the current one is unfair to the functional fix IMO:

Rust has a fragile embed-bitcode implementation (https://github.com/rust-lang/rust/blob/bddb59cf07efcf6e606f16b87f85e3ecd2c1ca69/compiler/rustc_codegen_llvm/src/back/write.rs#L970-L1017) which relied on the previous LLVM MC behavior.
Rust's LLVM fork has carried a revert. This commit made the similar revert to help distributions since they would otherwise probably carry a similar patch (as they ship rust linked against system LLVM).

nikic abandoned this revision.Dec 13 2021, 5:42 AM

Abandoning this now that rustc has implemented the necessary workaround (see https://github.com/rust-lang/rust/issues/90326). The revert should no longer be needed from our side.

llvm/lib/MC/MCContext.cpp