[MC][ARM] Don't create multiple .ARM.exidx associated to one .text

[MaskRay]

Summary

Fixed an issue exposed by D74006.

In clang cc1as, MCContext::UseNamesOnTempLabels is true.
When parsing a .fnstart directive, FnStart gets redefined to a temporary symbol of a different name (.Ltmp0, .Ltmp1, ...).
MCContext::getELFSection() called by SwitchToEHSection() will create a different .ARM.exidx each time.

llvm-mc uses Ctx.setUseNamesOnTempLabels(false); and FnStart is unnamed.
MCContext::getELFSection() called by SwitchToEHSection() will reuse the same .ARM.exidx .

[MaskRay]

This can only be tested by clang -c a.s => a.o, which (I think) is generally not recommended...

[efriedma]

Should we add an assertion that getELFSection isn't called with an unnamed symbol? Or is there some legitimate use-case I'm missing?

[MaskRay]

Add

assert(!(LinkedToSym && LinkedToSym->getName().empty()));

[efriedma]

LGTM

[maxim-kuvyrkov]

Hi @MaskRay ,

Quite surprisingly, this patch increases code size by 3-4% on several SPEC CPU2006 benchmarks for arm-linux-gnueabihf at -Oz optimization level:

1. 473.astar,astar_base.default regressed by 103
2. 450.soplex,soplex_base.default regressed by 103
3. 483.xalancbmk,Xalan_base.default regressed by 104

With -Os I see 2-3% code size increase for same benchmarks:

1. 473.astar,astar_base.default regressed by 102
2. 450.soplex,soplex_base.default regressed by 102
3. 483.xalancbmk,Xalan_base.default regressed by 103

Would you please check if the code size increase expected for this patch?

Let me know if you need help reproducing the issue. Thanks!

[MaskRay]

In D75095#1923866, @maxim-kuvyrkov wrote:

Hi @MaskRay ,

Quite surprisingly, this patch increases code size by 3-4% on several SPEC CPU2006 benchmarks for arm-linux-gnueabihf at -Oz optimization level:

1. 473.astar,astar_base.default regressed by 103
2. 450.soplex,soplex_base.default regressed by 103
3. 483.xalancbmk,Xalan_base.default regressed by 104

With -Os I see 2-3% code size increase for same benchmarks:

1. 473.astar,astar_base.default regressed by 102
2. 450.soplex,soplex_base.default regressed by 102
3. 483.xalancbmk,Xalan_base.default regressed by 103

Would you please check if the code size increase expected for this patch?

Let me know if you need help reproducing the issue. Thanks!

@maxim-kuvyrkov D75095 fixed a misuse and could be applied before D74006. I can't think of how it could increase code size.

Echo the comment from https://reviews.llvm.org/D74006#1920702
Could you try to figure out what's actually different about the object files/executables before/after this patch, using objdump and readelf -S?

llvm/lib/MC/MCContext.cpp

 MCSectionELF *MCContext::getELFSection(const Twine &Section, unsigned Type,
                                        unsigned Flags, unsigned EntrySize,
                                        const MCSymbolELF *GroupSym,
                                        unsigned UniqueID,
                                        const MCSymbolELF *LinkedToSym) {
   StringRef Group = "";
   if (GroupSym)
     Group = GroupSym->getName();
+  assert(!(LinkedToSym && LinkedToSym->getName().empty()));
   // Do the lookup, if we have a hit, return it.
   auto IterBool = ELFUniquingMap.insert(std::make_pair(
       ELFSectionKey{Section.str(), Group,
                     LinkedToSym ? LinkedToSym->getName() : "", UniqueID},
       nullptr));
   auto &Entry = *IterBool.first;
   if (!IterBool.second)
     return Entry.second;

llvm/lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

inline void ARMELFStreamer::SwitchToEHSection(StringRef Prefix,
   }
 
   // Get .ARM.extab or .ARM.exidx section
   const MCSymbolELF *Group = FnSection.getGroup();
   if (Group)
     Flags |= ELF::SHF_GROUP;
   MCSectionELF *EHSection = getContext().getELFSection(
       EHSecName, Type, Flags, 0, Group, FnSection.getUniqueID(),
-      static_cast<const MCSymbolELF *>(&Fn));
+      static_cast<const MCSymbolELF *>(FnSection.getBeginSymbol()));
 
   assert(EHSection && "Failed to get the required EH section");
 
   // Switch to .ARM.extab or .ARM.exidx section
   SwitchSection(EHSection);
   emitCodeAlignment(4);
 }