[PATCH] D152945: [SystemZ][z/OS] Implement executePostLayoutBinding for GOFFWriter

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolGOFF.h:21
 class MCSymbolGOFF : public MCSymbol {
+  mutable Align Alignment;
+  enum SymbolFlags : uint16_t {
----------------
I am not sure the purpose of using mutable here ,just want to keep  as MCSymbol did?

if we do not use the  mutable here, we do not need the "const" in
 
void setAlignment(Align Value) const


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:249
+
+  GOFFSymbol(StringRef Name, GOFF::ESDSymbolType Type, uint32_t EsdID,
+             uint32_t ParentEsdID)
----------------
suggest to change the construct to 

 GOFFSymbol(const std::string& Name, ......) 

Since in most of your cases, when you create GOFFSymbol, the parameter is std::string , using const std::string& Name
 in your construct, it can avoid from std::string to StringRef and from StringRef to std::string again.

for example in your following test case:
RootSD = createSDSymbol(FileName.str().append("#C")); 
of course ,
you also need to change createSDSymbol(StringRef Name) to 
:createSDSymbol(const std::string Name)


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:484
+  GOFFSymbol *ADAED;
+  ADAED = createWSASymbol(RootSD->EsdId);
+  if (CsectNames.second.empty()) {
----------------
change to 
  GOFFSymbol *ADAED = createWSASymbol(RootSD->EsdId); 


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:512
+  if (Kind.isText()) {
+    GOFFSymbol *SD = RootSD;
+    const char *CodeClassName = "C_CODE64";
----------------
SD to SDSym ?
and also for ED to EDSym? 
...


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:533
+    // Statics and globals that are defined.
+    std::string SectionName = Section.getName().str();
+    GOFFSymbol *SD = createSDSymbol(SectionName);
----------------
change to
StringRef SectionName = Section.getName(); to avoid string construct and copy.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:558
+
+void GOFFObjectWriter::processSymbolDefinedInModule(const MCSymbolGOFF &Symbol,
+                                                    MCAssembler &Asm,
----------------
Symbol too generic
change to 
const MCSymbolGOFF &MCSym ? 


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:597
+    else
+      LD->ADAEsdId = 0;
+
----------------
LD->ADAEsdId = (ADA && ADA->SectionLength > 0 ? ADA->EsdId : 0);


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:620
+    GSectionSym->MC = &Symbol;
+    GSection->SD->MC = &Symbol;
+    SymbolMap[&Symbol] = GSectionSym;
----------------
GSection->SD , the SD is to generic , I do not know the type of GSection->SD at first glance, I have to go back and check the Type the GSection->SD, if change the name SD to SDSym in the definition, from the name, at least we know it is a symbol related

and add assert(GSection->SD ) , otherwise if  GSection->SD is nullptr, it will be crash.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:705
+    Flags Attr[10];
+    void setAmode(GOFF::ESDAmode Amode) { Attr[0].set(0, 8, Amode); }
+    void setRmode(GOFF::ESDRmode Rmode) { Attr[1].set(0, 8, Rmode); }
----------------
I am expressing the difficulty in understanding the meaning or significance of the values 0, 8 in a particular context. it's possible to use a "static constexpr" to define these values to provide more clarity and context? 

if can , please change all the following .

for example: in xcoff.h

constexpr size_t FileNamePadSize = 6;
constexpr size_t NameSize = 8;
constexpr size_t FileHeaderSize32 = 20;
constexpr size_t FileHeaderSize64 = 24;



================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:739
+  } BehavAttr;
+
+  uint32_t AdaEsdId = 0;
----------------
do not use inline struct here, using lambda functions here.


================
Comment at: llvm/lib/MC/GOFFObjectWriter.cpp:848
+  ConverterEBCDIC::convertToEBCDIC(Symbol.Name, Res);
+  Name = Res.str();
+
----------------
change to 
StringRef Name = Res.str(); 

and delete line 845
  StringRef Name;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152945