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

[Tomas Matheson via Phabricator via llvm-commits]

tmatheson marked an inline comment as done.
tmatheson added inline comments.


================
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
----------------
peter.smith wrote:
> 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.
> 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.


================
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).
----------------
peter.smith wrote:
> 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.
> 
> 
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93948