[MC] Merge section flags for user defined sections

[tmatheson]

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

[peter.smith]

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]

• 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]

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]

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]

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]

Closing in favour of D100944

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

if (K.isMergeableCString() || K.isMergeableConst())
     Flags |= ELF::SHF_MERGE;
 
   if (K.isMergeableCString())
     Flags |= ELF::SHF_STRINGS;
 
   return Flags;
 }
 
+/// Return true if one of the default sections
+static bool isDefaultELFSection(const MCSectionELF &S) {
+  for (StringRef prefix :
+       {".bss", ".text", ".data", ".rodata", ".tdata", ".tbss",
+        ".gcc_except_table", ".debug_", ".apple_", ".llvm_", ".eh_frame",
+        ".stack_sizes", ".pseudo_probe"})
+    if (S.getName().startswith(prefix))
+      return true;
+  return false;
+}
+
+/// Return true if the section is compatible with the given flags.
+static bool flagsCompatible(const unsigned OldFlags, const unsigned NewFlags) {
+  // WRITE and EXECINSTR are not compatible
+  if (OldFlags & ELF::SHF_EXECINSTR && NewFlags & ELF::SHF_WRITE)
+    return false;
+  if (OldFlags & ELF::SHF_WRITE && NewFlags & ELF::SHF_EXECINSTR)
+    return false;
+
+  // TLS and non-TLS are not compatible
+  if ((OldFlags & ELF::SHF_TLS) != (NewFlags & ELF::SHF_TLS))
+    return false;
+
+  return true;
+}
+
 static const Comdat *getELFComdat(const GlobalValue *GV) {
   const Comdat *C = GV->getComdat();
   if (!C)
     return nullptr;
 
   if (C->getSelectionKind() != Comdat::Any)
     report_fatal_error("ELF COMDATs only support SelectionKind::Any, '" +
                        C->getName() + "' cannot be lowered.");
MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
   Kind = getELFKindForNamedSection(SectionName, Kind);
 
   StringRef Group = "";
   unsigned Flags = getELFSectionFlags(Kind);
   if (const Comdat *C = getELFComdat(GO)) {
     Group = C->getName();
     Flags |= ELF::SHF_GROUP;
   }
 
   unsigned EntrySize = getEntrySizeForKind(Kind);
 
   // A section can have at most one associated section. Put each global with
   // MD_associated in a unique section.
   unsigned UniqueID = MCContext::GenericSectionID;
   const MCSymbolELF *LinkedToSym = getLinkedToSymbol(GO, TM);
   if (GO->getMetadata(LLVMContext::MD_associated)) {
     UniqueID = NextUniqueID++;
MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
   MCSectionELF *Section = getContext().getELFSection(
       SectionName, getELFSectionType(SectionName, Kind), Flags,
       EntrySize, Group, UniqueID, LinkedToSym);
   // Make sure that we did not get some other section with incompatible sh_link.
   // This should not be possible due to UniqueID code above.
   assert(Section->getLinkedToSymbol() == LinkedToSym &&
          "Associated symbol mismatch between sections");
 
+  // Update the seen flags of user defined sections, raise an error if invalid
+  // combinations are seen. Don't update flags of standard sections (.text etc).
+  if (Section->getFlags() != Flags && !isDefaultELFSection(*Section)) {
+    if (flagsCompatible(Section->getFlags(), Flags)) {
+      // If all flags are compatible then add any new ones to the section.
+      const auto MergedFlags = Section->getFlags() | Flags;
+      if (MergedFlags != Section->getFlags()) {
+        Section->setFlags(MergedFlags);
+        GO->getContext().diagnose(LoweringDiagnosticInfo(
+            "Symbol " + GO->getName() + ": updating section flags: " +
+                SectionName + " 0x" + utohexstr(MergedFlags),
+            DS_Remark));
+      }
+    } else {
+      GO->getContext().diagnose(LoweringDiagnosticInfo(
+          "Symbol '" + GO->getName() +
+          "' implies incompatible flags for section '" + SectionName +
+          "'. Old: 0x" + utohexstr(Section->getFlags()) + ", New: 0x" +
+          utohexstr(Flags)));
+    }
+  }
+
   if (!getContext().getAsmInfo()->useIntegratedAssembler()) {
     // If we are not using the integrated assembler then this symbol might have
     // been placed in an incompatible mergeable section. Emit an error if this
     // is the case to avoid creating broken output.
     if ((Section->getFlags() & ELF::SHF_MERGE) &&
         (Section->getEntrySize() != getEntrySizeForKind(Kind)))
       GO->getContext().diagnose(LoweringDiagnosticInfo(
           "Symbol '" + GO->getName() + "' from module '" +

llvm/test/CodeGen/Generic/elf-incompatible-section-flags.ll

+; RUN: not llc -mtriple=aarch64-none-eabi %s -o - 2>&1 | FileCheck %s
+; RUN: not llc -mtriple=aarch64-none-eabi %s -o - 2> /dev/null | FileCheck %s --check-prefix=ASSEMBLY
+
+; User creates globals with SHF_WRITE and SHF_EXECINSTR
+@rw_s1 = global i32 10, section "s1_ax", align 4
+define i32 @fn_s1() section "s1_ax" {
+    entry:
+    ret i32 0
+}
+; CHECK: error: Symbol 'rw_s1' implies incompatible flags for section 's1_ax'. Old: 0x6, New: 0x3
+
+; This is fine; constant can live in SHF_EXECINSTR section
+@ro_s2 = constant i32 10, section "s2_ax", align 4
+define i32 @fn_s2() section "s2_ax" {
+    entry:
+    ret i32 0
+}
+; CHECK-NOT: error: Symbol 'ro_s2' implies incompatible flags for section 's2_ax'.
+; CHECK-NOT: remark: Symbol ro_s2: updating section flags
+; ASSEMBLY: .section s2_ax,"ax"
+; ASSEMBLY: .section s2_ax,"ax"
+
+; This is also fine; constant can live in SHF_WRITE
+@ro_s3 = constant i32 10, section "s3_aw", align 4
+@rw_s3 = global i32 10, section "s3_aw", align 4
+; CHECK-NOT: error: Symbol 'rw_s3' implies incompatible flags for section 's3_aw'.
+; CHECK: remark: Symbol rw_s3: updating section flags: s3_aw 0x3
+; ASSEMBLY:  .section s3_aw,"a"
+
+; This is fine; .text is read only, but the flags shouldn't change
+@ro_text = constant i32 10, section ".text", align 4
+; CHECK-NOT: error: Symbol 'ro_text' implies incompatible flags for section '.text'.
+; CHECK-NOT: remark: Symbol ro_text: updating section flags: .text
+
+; This is fine; .text is read only, but mutable globals are not necessarily written to
+@rw_text = global i32 10, section ".text", align 4
+; CHECK-NOT: error: Symbol 'rw_text' implies incompatible flags for section '.text'. Old: 0x6, New: 0x3
+; CHECK-NOT: remark: Symbol rw_text: updating section flags: .text
+
+; This is fine; .data is writeable, but the flags shouldn't change and constants are allowed
+@ro_data = constant i32 10, section ".data", align 4
+; CHECK-NOT: error: Symbol 'ro_data' implies incompatible flags for section '.data'.
+; CHECK-NOT: remark: Symbol ro_data: updating section flags: .data
+
+; TLS and non-TLS symbols cannot live in the same section
+@tls_var = thread_local global i32 10, section "s4", align 4
+@non_tls_var = global i32 10, section "s4", align 4
+; CHECK: error: Symbol 'non_tls_var' implies incompatible flags for section 's4'. Old: 0x403, New: 0x3
+; CHECK-NOT: remark: Symbol non_tls_var: updating section flags: s4
+
+; TLS and non-TLS symbols cannot live in the same section
+@non_tls_var2 = global i32 10, section "s5", align 4
+@tls_var2 = thread_local global i32 10, section "s5", align 4
+; CHECK: error: Symbol 'tls_var2' implies incompatible flags for section 's5'. Old: 0x3, New: 0x403
+; CHECK-NOT: remark: Symbol tls_var2: updating section flags: s5