Page MenuHomePhabricator

[MC] Merge section flags for user defined sections
AbandonedPublic

Authored by tmatheson on Dec 30 2020, 9:57 AM.

Details

Summary

When placing global variables in a previously encountered user defined
section, check that the flags for the new global variable are compatible
with the existing flags on the section, and emit an error if not.
Otherwise, update the section flags with the union of the new and old
flags and emit a remark.

I am not sure if this is the correct approach to take and would welcome
any feedback.

There is already some mitigation for this in clang (e.g.
https://reviews.llvm.org/D93102). However, the problem could still arise
when the global variables are in separate translation units and LTO was enabled.

Some previous discussion about merging sections with values of different size
can be found here:

https://bugs.llvm.org/show_bug.cgi?id=43627
https://reviews.llvm.org/D72194
https://reviews.llvm.org/D68101

Diff Detail

Event Timeline

tmatheson created this revision.Dec 30 2020, 9:57 AM
tmatheson requested review of this revision.Dec 30 2020, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 9:57 AM

Another change that is worth highlighting in the description is: https://reviews.llvm.org/D73999 [MC][ELF] Error for sh_type, sh_flags or sh_entsize change with its associated LLVM-DEV message: https://lists.llvm.org/pipermail/llvm-dev/2020-February/139390.html

As I understand it we have protection against redefinition of a section with different type or flags within the same .c file or .s file with error messages implemented in clang (D93102) and the assembly parser (D73999) but not when reading LLVM-IR directly. Normally this isn't a problem as people write in a high-level language or assembly but if two independent LLVM-IR files with conflicting section flags are combined during LTO the backend will combine all the sections together with flags that are not always correct[*]. If the two files were compiled independently we'd end up with two independent ELF sections which would be handled by the linker (behaviour dependent on linker, module(section) input section descriptions can handle these cases if necessary). To summarise:

  • Non-LTO, linker sees independent objects, merges input sections with the same name according to linker script if present.
  • LTO, linker sees a single object that LLVM has already merged sections with the same name (sometimes incorrectly).

In an ideal world we'd want these cases to be the same, but I don't think that this is possible as section names need to be unique within an object, and renaming sections with an additional suffix to make them unique may also cause problems. I'm personally in favour of an error message to match the clang and assembler behaviour as LTO already has inconsistencies with linker scripts that need a lot of work to sort out.

The Qualcomm proposal for LTO + linker-scripts had what I think is the only alternative to an error message https://lists.llvm.org/pipermail/llvm-dev/2018-May/123252.html this augmented the LLVM-IR section with a module_id that could be used to disambiguate sections coming from separate files. If that proposal or some equivalent were implemented then I think we could have a way of making this case work consistently.

  • If a function (read-only, executable) is assigned to a section with name S1 and read-write data is assigned to a section S1 the resulting section in the ELF file is (read-only, executable) which could break if the read-write data is written to.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
511

Although precise, this looks like it could be difficult to maintain as more section types are added. Reserved section names typically follow a naming convention with a prefix such as .debug for all of the dwarf sections. For example see StringRef getSectionPrefixForGlobal(SectionKind Kind) it may be more maintainable to base this on a prefix.

693

The assembler is stricter than this. It checks that the section flags match (see ELFAsmParser.cpp::ParseSectionArguments).
As I understand it this would permit a RO (absence of SHF_WRITE) and RW section to be merged. There are also other incompatibilities such as SHF_TLS and non SHF_TLS.

755

Is there a reason to exclude standard sections from the check? If a user makes a custom section with these names and an incompatible then this is just as likely to cause problems if they are incompatible? Standard sections should all have the same type and flags so I wouldn't expect this to be a problem. If we removed the check this could get rid of the somewhat difficult to maintain isDefaultELFSection.

Personally I'd prefer to be consistent with the assembler and fault mismatched flags. Would be interested in second opinions here.

Section types can also be inconsistent (SHT_PROGBITS) and (SHT_NOBITS). See https://reviews.llvm.org/D73999 ELFAsmParser for what the assembler does.

llvm/test/CodeGen/AArch64/aarch64-section-flags-incompatible.ll
1 ↗(On Diff #314130)

the triple with arm vendor is not in open source clang. I suggest using aarch64-none-eabi instead. Given that this behaviour is not specific to AArch64 it would be good to have an X86_64 example as the majority of the upstream buildbots implement the backend.

tmatheson updated this revision to Diff 314649.Jan 5 2021, 10:06 AM
  • Added check for incompatible SHF_TLS flags
  • Moved test to llvm/test/CodeGen/Generic
  • Removed arm vendor from triple in test
  • Changed isDefaultELFSection to check section by name rather than pointer value
tmatheson marked an inline comment as done.Jan 5 2021, 10:52 AM
tmatheson added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
693

The assembler is stricter than this. It checks that the section flags match

It is more difficult to be this strict in CodeGen since the section flags are implied by the global with the section keyword, and not set explicitly by the user. The assembler doesn't have this problem since every .section it sees comes with the full set of flags. e.g.:

@ro_s3 = constant i32 10, section "s3_aw", align 4
@rw_s3 = global i32 10, section "s3_aw", align 4

The first global is read only, but that should not prevent it being stored in a writable section.

With a list of checks for things that are definitely incompatible the checks can be added to in future.

There are also other incompatibilities such as SHF_TLS and non SHF_TLS.

I've added a check and test for this.

755

Standard sections could be included in the check for flag compatibility, but the behaviour would still need to differ so that flags for user defined sections can be updated (merged) but the flags of standard sections remain unchanged.

Sometimes symbols imply SHF_ALLOC but the section they are assigned to is not allocatable, and the section flags should not be updated. For example. llvm/test/CodeGen/X86/explicit-section-mergeable.ll includes the following test and comment:

;; Non-allocatable "default" sections can have allocatable mergeable symbols with compatible entry sizes assigned to them.
@explicit_default_3 = unnamed_addr constant [2 x i8] [i8 1, i8 0], section ".debug_str"

Another example where we want to avoid the check is when a non-constant global is assigned to a read-only section such as ".text" but never written to (one of the test cases). This seems valid and we would want neither an error nor a remark about section flags changing.

psmith added a subscriber: psmith.Jan 6 2021, 2:30 AM
psmith added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
755

Thanks for the update. Just to check my understanding. In the cases in your comment (adding a mergeable string to .debug_str and adding a non-constant global that happens to never be written to a .text section) I would expect the section flags to not be updated. I guess the next question is whether we know that LLVM will only do this for "default" sections? If it can do this for "named" sections then the whole approach may be flawed.

760

The main reason for my preference for the assembler policy of identical flags is that I don't think it is as simple as adding all the flags.

I think we'd likely need to make a check for each individual flag. For example:

  • Combination of SHF_MERGE and non-SHF_MERGE is non-SHF_MERGE.
  • Even if both sections have SHF_LINKORDER flag we'd need to check that both sections had a link-order dependency on the same Section as the combined section can only have one link-order dependency.

These will be similar to the checks made by a linker when combining ELF input sections into output sections, they won't be exactly the same as some flags only have meaning in a relocatable object and not an output image.

There may be other bits of code in LLVM that check that these cases can't happen, if that is the case then we can give an error message here.

MaskRay added a comment.EditedJan 6 2021, 12:10 PM

For sections with incompatible flags, we have 4 choices.

  • Error. For certain combinations, they have been errored by Clang (D93102) but the check is not comprehensive. (Note: SHF_WRITE and SHF_EXECINSTR are not fundamentally incompatible. For instance, ppc32 BSS-PLT .plt section is RWX, but several tools may warn for them (asan security check, selinux and other Linux hardening, OpenBSD hardening, etc))
  • Pick the first one. IIRC this is the current behavior in the backend which is not ideal.
  • Merge the flags.
  • Emit multiple sections.

It is not super clear where to draw a line between error and non-error (this is dependent on the binary format and probably platform preference), but for non-error cases, I feel that we should go toward the direction of D72194 (emit multiple sections for different sh_entsize of SHF_MERGE sections) for consistency and simplicity. If we err, multiple sections look better than arbitrarily picking the first (current behavior): if there is an issue, it is at least more noticeable.

tmatheson abandoned this revision.May 5 2021, 10:21 AM

Closing in favour of D100944