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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSectionELF.h:42
 
-  const MCSymbolELF *Group;
+  const PointerIntPair<const MCSymbolELF *, 1, bool> Group;
 
----------------
`/// The section group signature symbol (if not null) and a bool indicating whether this is within a GRP_COMDAT group.`


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:854
   const MCSymbolELF *LinkedToSym = nullptr;
-  if (F.hasComdat()) {
-    Group = F.getComdat()->getName();
+  StringRef Group = "";
+  bool IsComdat = false;
----------------
`StringRef Group`

StringRef initializes to an empty string.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:859
+    Group = C->getName();
+    IsComdat = C->getSelectionKind() == Comdat::Any;
   }
----------------
Can you check whether other selection kinds are rejected elsewhere?

If not, we should report an error for non-any-non-nodeduplicates kinds.


================
Comment at: llvm/lib/MC/MCParser/ELFAsmParser.cpp:428
+bool ELFAsmParser::parseGroup(StringRef &GroupName, bool &IsComdat) {
   MCAsmLexer &L = getLexer();
   if (L.isNot(AsmToken::Comma))
----------------
Perhaps assign `IsComdat` to false here to prevent pitfalls.


================
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()));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95851