[MC][ELF] Fix printing group signature symbols.

[kosarev]

They may be not known.

[llvmbot]

@llvm/pr-subscribers-mc

Author: Ivan Kosarev (kosarev)

Changes

They may be not known.

Change-Id: I66543240af2d10d77b4f7f23c06553356b3ec767

---

Full diff: https://github.com/llvm/llvm-project/pull/112543.diff

1 Files Affected:

• (modified) llvm/lib/MC/MCSectionELF.cpp (+4-2)

diff --git a/llvm/lib/MC/MCSectionELF.cpp b/llvm/lib/MC/MCSectionELF.cpp
index 25e62b70b5e2a0..3dc9a67e63939a 100644
--- a/llvm/lib/MC/MCSectionELF.cpp
+++ b/llvm/lib/MC/MCSectionELF.cpp
@@ -191,8 +191,10 @@ void MCSectionELF::printSwitchToSection(const MCAsmInfo &MAI, const Triple &T,
   }
 
   if (Flags & ELF::SHF_GROUP) {
-    OS << ",";
-    printName(OS, Group.getPointer()->getName());
+    if (const MCSymbolELF *Signature = Group.getPointer()) {
+      OS << ",";
+      printName(OS, Signature->getName());
+    }
     if (isComdat())
       OS << ",comdat";
   }

[kosarev]

@MaskRay If this needs a test, can you give some pointers as to what it might look like and where we would want it, please? Not quite my area.

[MaskRay]

When is it unknown? We need a test.
If this only triggers in downstream code, I don't think we can change the code.

[MaskRay]

.

[kosarev]

Caught downstream, yes. The comment for the Group field in MCSectionELF.h reads like the pointer can be null:

  /// The section group signature symbol (if not null) and a bool indicating
  /// whether this is a GRP_COMDAT group.
  const PointerIntPair<const MCSymbolELF *, 1, bool> Group;

There's also a guarding if for the pointer in MCSectionELF constructor:

  MCSectionELF(StringRef Name, unsigned type, unsigned flags,
...
        Group(group, IsComdat), LinkedToSym(LinkedToSym) {
    if (Group.getPointer())
      Group.getPointer()->setIsSignature();
  }

[kosarev]

Added test.

[MaskRay]

In the test you added

  // Test that switching to a group section with no associated signature name
  // doesn't crash.
  C.Streamer->switchSection(
      C.Ctx->getELFSection("foo", ELF::SHT_PROGBITS, ELF::SHF_GROUP));

I am still not convinced we should take the change. Perhaps we should disallow SHF_GROUP without a signature in switchSection. This isn't valid in ELF.

[kosarev]

Perhaps we should disallow SHF_GROUP without a signature in switchSection. This isn't valid in ELF.

Should the ELF loading code do the check then?

[MaskRay]

Perhaps we should disallow SHF_GROUP without a signature in switchSection. This isn't valid in ELF.

Should the ELF loading code do the check then?

The section creation code can have an assert

[kosarev]

Perhaps we should disallow SHF_GROUP without a signature in switchSection. This isn't valid in ELF.

Should the ELF loading code do the check then?

The section creation code can have an assert

Having the assert without the loading code erroring out signature-less group sections means we'll just crash on loading such ELFs.

[MaskRay]

We should avoid this construct when a SHF_GROUP section without a signature symbol is constructed. Factory functions can have this precondition.