[PATCH] D95851: [MC][ELF] Support for zero flag section groups

[Petr Hosek via Phabricator via llvm-commits]

phosek added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:859
+    Group = C->getName();
+    IsComdat = C->getSelectionKind() == Comdat::Any;
   }
----------------
MaskRay wrote:
> Can you check whether other selection kinds are rejected elsewhere?
> 
> If not, we should report an error for non-any-non-nodeduplicates kinds.
They're checked in `getELFComdat` which reports an error for unsupported kinds (see the lines 525-529).


================
Comment at: llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp:1206
   MCSectionELF *EHSection = getContext().getELFSection(
-      EHSecName, Type, Flags, 0, Group, FnSection.getUniqueID(),
+      EHSecName, Type, Flags, 0, Group, false, FnSection.getUniqueID(),
       static_cast<const MCSymbolELF *>(FnSection.getBeginSymbol()));
----------------
MaskRay wrote:
> This is likely wrong, but the existing tests are not good enough to catch the issue.
> 
> ```
> void ext();
> inline void foo() {
>   try {
>     ext();
>   } catch (...) {
>   }
> }
> void bar() {
>   foo();
> }
> ```
> 
> `clang -target armv7-linux-gnueabi -c a.cc`
> 
> `.ARM.extab.text._Z3foov` and `.ARM.exidx.text._Z3foov` should be in GRP_COMDAT groups, not zero flag groups.
> 
> Given the risk, consider making a precursor commit dropping `, ""` so that the patch can be more focused on functionality changes.
Thanks for catching this, I made this change in earlier revision when the meaning of the boolean was flipped and I forgot to update this instance. I've also included comment.


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

https://reviews.llvm.org/D95851