[PATCH] D93948: [MC] Merge section flags for user defined sections

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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 <https://reviews.llvm.org/D93102>) and the assembly parser (D73999 <https://reviews.llvm.org/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.

[X] 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.



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:511
+/// Return true if one of the default sections
+static bool isDefaultELFSection(const MCContext &C, const MCSectionELF &S) {
+  const MCObjectFileInfo &OFI = *C.getObjectFileInfo();
----------------
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.






================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:693
+/// 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
----------------
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.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:922
 
+  // 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).
----------------
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.




================
Comment at: llvm/test/CodeGen/AArch64/aarch64-section-flags-incompatible.ll:1
+; RUN: not llc -mtriple=aarch64-arm-none-eabi %s -o - 2>&1 | FileCheck %s
+; RUN: not llc -mtriple=aarch64-arm-none-eabi %s -o - 2> /dev/null | FileCheck %s --check-prefix=ASSEMBLY
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93948/new/

https://reviews.llvm.org/D93948