[NFC][XCOFF][AIX] Refactor get/setContainingCsect

[jasonliu]

Summary

For current architect, we always require setContainingCsect to be called on every MCSymbol got used in XCOFF context.
This is very hard to achieve because symbols gets created everywhere and other MCSymbol types(ELF, COFF) do not have similar rules.
It's very easy to miss setting the containing csect, and we would need to add a lot of XCOFF specialized code around some common code area.

This patch intendeds to do

1. Rely on getFragment().getParent() to get csect from labels.
2. Only use get/setRepresentedCsect (was get/setContainingCsect) if symbol itself represents a csect.

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1609

I think we still need GVSym->setContainingCsect(Csect) for extern variable . when we emitlLnkage for the extern Variable, the GVSym is not a qualName symbol, So we get the SMC from GVSym's containningCsect.

for example:
extern int b_e;
we need to emit linkage as
.extern b_e[UA]

[DiggerLin]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1609

I think we can change to
if ( GV->isDeclaration()) {

GVSym->setContainingCsect(Csect);

}

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1609

Symbol b_e[UA] will have getCsectMCSectionXCOFF returning the csect you want. It's a qualname, so it will have the old "containingCsect", or the new "CsectMCSectionXCOFF".

[jasonliu]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

1609

The GVSym you there better be b_e[UA].

[DiggerLin]

LGTM

[hubert.reinterpretcast]

llvm/include/llvm/MC/MCSymbolXCOFF.h

58

Suggestion: RepresentedCsect.

[daltenty]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5146

It feels like this breaks the constraint you had listed under 2, since the symbol S is not actually the symbol representing the csect (i.e. the qualname symbol). Would we be able to wrap this whole business in a call to an new TLOF function like TargetLoweringObjectFileXCOFF::getSectionForUndefinedFunction(StringRef) and just use the qualname symbol we get back instead (so we don't need to call setCsectMCSectionXCOFF() seperately)?

If we can do this we could restrict setCsectMCSectionXCOFF() so it can only be called from MCSectionXCOFF's constructor, enforcing the constraint that this only valid for symbols representing csects (and hasCsectMCSectionXCOFF() could become isCsect() ).

[hubert.reinterpretcast]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1849

Can we expect that hasCsectMCSectionXCOFF is true for an undefined XCOFF symbol? Do we want to assert that?

llvm/lib/CodeGen/MachineModuleInfo.cpp

107–108

This is unused except by the assert.

llvm/lib/MC/MCSymbolXCOFF.cpp

17

Just noting that the setter already checks this, which explains why the assert does not need to be written in terms of what the callee is trying to do--it can be written in terms of the internal state of this object.

19

Suggestion:
Symbol does not represent a csect; MCSectionXCOFF that represents the symbol should not be (but is) set.

24

Should also assert that C is not null.

29

Suggestion:
Symbol does not represent a csect; can only set a MCSectionXCOFF representation for a csect.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5146

Since the symbol is undefined. It must represent the csect. I also would prefer if we can have a stronger correspondence between being a QualName symbol and being the representation of a csect, but I am not sure how much that entails.

[jasonliu]

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

5146

I think I'm going to use the same reason why this haven't been done in D72347: we do not have the access to TLOF here.
Aside from that, I had the same debate that you have. The conclusion I came up with is that all the undefined symbol are essentially csects (even though that undefined symbol is actually a label in the other compilation unit). So in here, although we do not have a qualname, this symbol still represents a csect.
I agree with you that it might be better if we have a qualname .bl .foo[PR] here when foo is an undefined function. So that we know if we have a qualname, it must be csect. But right now, I don't see an immediate need for that and I kinda want to leave things as it is until there are concrete reasons to change it.

[jasonliu]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1849

The original purpose of hasCsectMCSectionXCOFF should really be hasCsectMCSectionXCOFFSet. So it won't always be true. I will change the name to hasCsectMCSectionXCOFFSet to avoid confusion. I'm not sure if we really need hasCsectMCSectionXCOFF though, maybe not for this patch until we find the reason that we need to.

[jasonliu]

Rename and address comment.

[hubert.reinterpretcast]

LGTM. Thanks.

[daltenty]

LGTM

llvm/include/llvm/MC/MCSectionXCOFF.h

MCSectionXCOFF(StringRef Section, XCOFF::StorageMappingClass SMC,
                  XCOFF::SymbolType ST, XCOFF::StorageClass SC, SectionKind K,
                  MCSymbolXCOFF *QualName, MCSymbol *Begin)
       : MCSection(SV_XCOFF, K, Begin), Name(Section), MappingClass(SMC),
         Type(ST), StorageClass(SC), QualName(QualName) {
     assert((ST == XCOFF::XTY_SD || ST == XCOFF::XTY_CM || ST == XCOFF::XTY_ER) &&
            "Invalid or unhandled type for csect.");
     assert(QualName != nullptr && "QualName is needed.");
     QualName->setStorageClass(SC);
-    QualName->setContainingCsect(this);
+    QualName->setRepresentedCsect(this);
   }
 
 public:
   ~MCSectionXCOFF();
 
   static bool classof(const MCSection *S) {
     return S->getVariant() == SV_XCOFF;
   }

llvm/include/llvm/MC/MCSymbolXCOFF.h

public:
   };
 
   XCOFF::StorageClass getStorageClass() const {
     assert(StorageClass.hasValue() &&
            "StorageClass not set on XCOFF MCSymbol.");
     return StorageClass.getValue();
   }
 
-  void setContainingCsect(MCSectionXCOFF *C) {
-    assert((!ContainingCsect || ContainingCsect == C) &&
-           "Trying to set a containing csect that doesn't match the one that"
-           "this symbol is already mapped to.");
-    ContainingCsect = C;
-  }
-
-  MCSectionXCOFF *getContainingCsect() const {
-    assert(ContainingCsect &&
-           "Trying to get containing csect but none was set.");
-    return ContainingCsect;
-  }
-
-  bool hasContainingCsect() const { return ContainingCsect != nullptr; }
-
   StringRef getUnqualifiedName() const {
     const StringRef name = getName();
     if (name.back() == ']') {
       StringRef lhs, rhs;
       std::tie(lhs, rhs) = name.rsplit('[');
       assert(!rhs.empty() && "Invalid SMC format in XCOFF symbol.");
       return lhs;
     }
     return name;
   }
 
+  bool hasRepresentedCsectSet() const { return RepresentedCsect != nullptr; }
+
+  MCSectionXCOFF *getRepresentedCsect() const;
+
+  void setRepresentedCsect(MCSectionXCOFF *C);
+
 private:
   Optional<XCOFF::StorageClass> StorageClass;
-  MCSectionXCOFF *ContainingCsect = nullptr;
+  MCSectionXCOFF *RepresentedCsect = nullptr;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCSYMBOLXCOFF_H

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (!MAI->needsFunctionDescriptors()) {
     // function body does not have the same name as the source function's
     // C-linkage name.
     assert(CurrentFnDescSym && "The function descriptor symbol needs to be"
                                " initalized first.");
 
     // Get the function entry point symbol.
     CurrentFnSym = OutContext.getOrCreateSymbol(
         "." + cast<MCSymbolXCOFF>(CurrentFnDescSym)->getUnqualifiedName());
-
-    // Set the containing csect.
-    MCSectionXCOFF *FnEntryPointSec =
-        cast<MCSectionXCOFF>(getObjFileLowering().SectionForGlobal(&F, TM));
-    cast<MCSymbolXCOFF>(CurrentFnSym)->setContainingCsect(FnEntryPointSec);
   }
 
   CurrentFnSymForSize = CurrentFnSym;
   CurrentFnBegin = nullptr;
   CurExceptionSym = nullptr;
   bool NeedsLocalForSize = MAI->needsLocalForSize();
   if (F.hasFnAttribute("patchable-function-entry") ||
       needFuncLabelsForEHOrDebugInfo(MF, MMI) || NeedsLocalForSize ||
void AsmPrinter::emitConstantPool() {
   unsigned Offset = 0;
   for (unsigned i = 0, e = CPSections.size(); i != e; ++i) {
     for (unsigned j = 0, ee = CPSections[i].CPEs.size(); j != ee; ++j) {
       unsigned CPI = CPSections[i].CPEs[j];
       MCSymbol *Sym = GetCPISymbol(CPI);
       if (!Sym->isUndefined())
         continue;
 
-      if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
-        cast<MCSymbolXCOFF>(Sym)->setContainingCsect(
-            cast<MCSectionXCOFF>(CPSections[i].S));
-      }
-
       if (CurSection != CPSections[i].S) {
         OutStreamer->SwitchSection(CPSections[i].S);
         emitAlignment(Align(CPSections[i].Alignment));
         CurSection = CPSections[i].S;
         Offset = 0;
       }
 
       MachineConstantPoolEntry CPE = CP[CPI];
for (unsigned JTI = 0, e = JT.size(); JTI != e; ++JTI) {
     // second label is actually referenced by the code.
     if (JTInDiffSection && DL.hasLinkerPrivateGlobalPrefix())
       // FIXME: This doesn't have to have any specific name, just any randomly
       // named and numbered local label started with 'l' would work.  Simplify
       // GetJTISymbol.
       OutStreamer->emitLabel(GetJTISymbol(JTI, true));
 
     MCSymbol* JTISymbol = GetJTISymbol(JTI);
-    if (TM.getTargetTriple().isOSBinFormatXCOFF()) {
-      cast<MCSymbolXCOFF>(JTISymbol)->setContainingCsect(
-          cast<MCSectionXCOFF>(TLOF.getSectionForJumpTable(F, TM)));
-    }
     OutStreamer->emitLabel(JTISymbol);
 
     for (unsigned ii = 0, ee = JTBBs.size(); ii != ee; ++ii)
       emitJumpTableEntry(MJTI, JTBBs[ii], JTI);
   }
   if (!JTInDiffSection)
     OutStreamer->emitDataRegion(MCDR_DataRegionEnd);
 }

llvm/lib/CodeGen/MachineModuleInfo.cpp

ArrayRef<MCSymbol *> MMIAddrLabelMap::getAddrLabelSymbolToEmit(BasicBlock *BB) {
   }
 
   // Otherwise, this is a new entry, create a new symbol for it and add an
   // entry to BBCallbacks so we can be notified if the BB is deleted or RAUWd.
   BBCallbacks.emplace_back(BB);
   BBCallbacks.back().setMap(this);
   Entry.Index = BBCallbacks.size() - 1;
   Entry.Fn = BB->getParent();
   MCSymbol *Sym = Context.createTempSymbol(!BB->hasAddressTaken());
-  if (Context.getObjectFileInfo()->getTargetTriple().isOSBinFormatXCOFF()) {
-    MCSymbol *FnEntryPointSym =
-        Context.lookupSymbol("." + Entry.Fn->getName());
-    assert(FnEntryPointSym && "The function entry pointer symbol should have"
-                  " already been initialized.");
-    MCSectionXCOFF *Csect =
-        cast<MCSymbolXCOFF>(FnEntryPointSym)->getContainingCsect();
-    cast<MCSymbolXCOFF>(Sym)->setContainingCsect(Csect);
-  }
   Entry.Symbols.push_back(Sym);
   return Entry.Symbols;
 }
 
 void MMIAddrLabelMap::UpdateForDeletedBlock(BasicBlock *BB) {
   // If the block got deleted, there is no need for the symbol.  If the symbol
   // was already emitted, we can just forget about it, otherwise we need to
   // queue it up for later emission when the function is output.
   AddrLabelSymEntry Entry = std::move(AddrLabelSymbols[BB]);

llvm/lib/MC/CMakeLists.txt

add_llvm_component_library(LLVMMC
   MCSectionELF.cpp
   MCSectionMachO.cpp
   MCSectionWasm.cpp
   MCSectionXCOFF.cpp
   MCStreamer.cpp
   MCSubtargetInfo.cpp
   MCSymbol.cpp
   MCSymbolELF.cpp
+  MCSymbolXCOFF.cpp
   MCTargetOptions.cpp
   MCTargetOptionsCommandFlags.cpp
   MCValue.cpp
   MCWasmObjectTargetWriter.cpp
   MCWasmStreamer.cpp
   MCWin64EH.cpp
   MCWinCOFFStreamer.cpp
   MCWinEH.cpp

llvm/lib/MC/MCSymbolXCOFF.cpp

+//===- lib/MC/MCSymbolXCOFF.cpp - XCOFF Code Symbol Representation --------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/MC/MCSectionXCOFF.h"
+
+using namespace llvm;
+
+MCSectionXCOFF *MCSymbolXCOFF::getRepresentedCsect() const {
+  assert(RepresentedCsect &&
+         "Trying to get csect representation of this symbol but none was set.");
+  assert((!getName().equals(getUnqualifiedName()) ||
+          RepresentedCsect->getCSectType() == XCOFF::XTY_ER) &&
+         "Symbol does not represent a csect; MCSectionXCOFF that represents "
+         "the symbol should not be (but is) set.");
+  return RepresentedCsect;
+}
+
+void MCSymbolXCOFF::setRepresentedCsect(MCSectionXCOFF *C) {
+  assert(C && "Assigned csect should not be null.");
+  assert((!RepresentedCsect || RepresentedCsect == C) &&
+         "Trying to set a csect that doesn't match the one that"
+         "this symbol is already mapped to.");
+  assert((!getName().equals(getUnqualifiedName()) ||
+          C->getCSectType() == XCOFF::XTY_ER) &&
+         "Symbol does not represent a csect; can only set a MCSectionXCOFF "
+         "representation for a csect.");
+  RepresentedCsect = C;
+}

llvm/lib/MC/XCOFFObjectWriter.cpp

case XCOFF::XMC_TC:
     assert(!TOCCsects.empty() &&
            "We should at least have a TOC-base in this CsectGroup.");
     return TOCCsects;
   default:
     report_fatal_error("Unhandled mapping of csect to section.");
   }
 }
 
+static MCSectionXCOFF *getContainingCsect(const MCSymbolXCOFF *XSym) {
+  if (XSym->isDefined())
+    return cast<MCSectionXCOFF>(XSym->getFragment()->getParent());
+  return XSym->getRepresentedCsect();
+}
+
 void XCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
                                                  const MCAsmLayout &Layout) {
   if (TargetObjectWriter->is64Bit())
     report_fatal_error("64-bit XCOFF object files are not supported yet.");
 
   for (const auto &S : Asm) {
     const auto *MCSec = cast<const MCSectionXCOFF>(&S);
     assert(SectionMap.find(MCSec) == SectionMap.end() &&
void XCOFFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
   }
 
   for (const MCSymbol &S : Asm.symbols()) {
     // Nothing to do for temporary symbols.
     if (S.isTemporary())
       continue;
 
     const MCSymbolXCOFF *XSym = cast<MCSymbolXCOFF>(&S);
-    const MCSectionXCOFF *ContainingCsect = XSym->getContainingCsect();
+    const MCSectionXCOFF *ContainingCsect = getContainingCsect(XSym);
 
     if (ContainingCsect->getCSectType() == XCOFF::XTY_ER) {
       // Handle undefined symbol.
       UndefinedCsects.emplace_back(ContainingCsect);
       SectionMap[ContainingCsect] = &UndefinedCsects.back();
     } else {
       // If the symbol is the csect itself, we don't need to put the symbol
       // into csect's Syms.
bool IsPCRel = Backend.getFixupKindInfo(Fixup.getKind()).Flags &
                  MCFixupKindInfo::FKF_IsPCRel;
 
   uint8_t Type;
   uint8_t SignAndSize;
   std::tie(Type, SignAndSize) =
       TargetObjectWriter->getRelocTypeAndSignSize(Target, Fixup, IsPCRel);
 
   const MCSectionXCOFF *SymASec =
-      cast<MCSymbolXCOFF>(SymA).getContainingCsect();
+      getContainingCsect(cast<MCSymbolXCOFF>(&SymA));
   assert(SectionMap.find(SymASec) != SectionMap.end() &&
          "Expected containing csect to exist in map.");
 
   // If we could not find SymA directly in SymbolIndexMap, this symbol could
   // either be a temporary symbol or an undefined symbol. In this case, we
   // would need to have the relocation reference its csect instead.
   uint32_t Index = SymbolIndexMap.find(&SymA) != SymbolIndexMap.end()
                        ? SymbolIndexMap[&SymA]

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

void PPCAIXAsmPrinter::emitGlobalVariable(const GlobalVariable *GV) {
   // Create the containing csect and set it. We set it for externals as well,
   // since this may not have been set elsewhere depending on how they are used.
   MCSectionXCOFF *Csect = cast<MCSectionXCOFF>(
       GV->isDeclaration()
           ? getObjFileLowering().getSectionForExternalReference(GV, TM)
           : getObjFileLowering().SectionForGlobal(
                 GV, GVKind = getObjFileLowering().getKindForGlobal(GV, TM),
                 TM));
-  GVSym->setContainingCsect(Csect);
 
   // External global variables are already handled.
   if (GV->isDeclaration())
     return;
 
   if (!GVKind.isGlobalWriteableData() && !GVKind.isReadOnly())
     report_fatal_error("Encountered a global variable kind that is "
                        "not supported yet.");

 
 void PPCAIXAsmPrinter::emitFunctionDescriptor() {
   const DataLayout &DL = getDataLayout();
   const unsigned PointerSize = DL.getPointerSizeInBits() == 64 ? 8 : 4;
 
   MCSectionSubPair Current = OutStreamer->getCurrentSection();
   // Emit function descriptor.
   OutStreamer->SwitchSection(
-      cast<MCSymbolXCOFF>(CurrentFnDescSym)->getContainingCsect());
+      cast<MCSymbolXCOFF>(CurrentFnDescSym)->getRepresentedCsect());
   // Emit function entry point address.
   OutStreamer->emitValue(MCSymbolRefExpr::create(CurrentFnSym, OutContext),
                          PointerSize);
   // Emit TOC base address.
   const MCSymbol *TOCBaseSym =
       cast<MCSectionXCOFF>(getObjFileLowering().getTOCBaseSection())
           ->getQualNameSymbol();
   OutStreamer->emitValue(MCSymbolRefExpr::create(TOCBaseSym, OutContext),
if (TOCEntriesByteSize > 32767) {
     report_fatal_error("Handling of TOC entry displacement larger than 32767 "
                        "is not yet implemented.");
   }
 
   for (auto &I : TOC) {
     // Setup the csect for the current TC entry.
     MCSectionXCOFF *TCEntry = cast<MCSectionXCOFF>(
         getObjFileLowering().getSectionForTOCEntry(I.first));
-    cast<MCSymbolXCOFF>(I.second)->setContainingCsect(TCEntry);
     OutStreamer->SwitchSection(TCEntry);
 
     OutStreamer->emitLabel(I.second);
     TS.emitTCEntry(*I.first);
   }
 }
 
 MCSymbol *

llvm/lib/Target/PowerPC/PPCISelLowering.cpp

static SDValue transformCallee(const SDValue &Callee, SelectionDAG &DAG,
   const auto getAIXFuncEntryPointSymbolSDNode =
       [&](StringRef FuncName, bool IsDeclaration,
           const XCOFF::StorageClass &SC) {
         auto &Context = DAG.getMachineFunction().getMMI().getContext();
 
         MCSymbolXCOFF *S = cast<MCSymbolXCOFF>(
             Context.getOrCreateSymbol(Twine(".") + Twine(FuncName)));
 
-        if (IsDeclaration && !S->hasContainingCsect()) {
+        if (IsDeclaration && !S->hasRepresentedCsectSet()) {
           // On AIX, an undefined symbol needs to be associated with a
           // MCSectionXCOFF to get the correct storage mapping class.
           // In this case, XCOFF::XMC_PR.
           MCSectionXCOFF *Sec = Context.getXCOFFSection(
               S->getName(), XCOFF::XMC_PR, XCOFF::XTY_ER, SC,
               SectionKind::getMetadata());
-          S->setContainingCsect(Sec);
+          S->setRepresentedCsect(Sec);
         }
 
         MVT PtrVT =
             DAG.getTargetLoweringInfo().getPointerTy(DAG.getDataLayout());
         return DAG.getMCSymbol(S, PtrVT);
       };
 
   if (isFunctionGlobalAddress(Callee)) {