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

[Peter Smith via Phabricator via llvm-commits]

psmith added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:760
+      // If all flags are compatible then add any new ones to the section.
+      const auto MergedFlags = Section->getFlags() | Flags;
+      if (MergedFlags != Section->getFlags()) {
----------------
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.




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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93948