[PATCH] D65159: [PowerPC][XCOFF] Adds support for writing the .bss section for object files.

Wed Aug 14 13:44:23 PDT 2019

sfertile added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:152
+  uint16_t Magic;
+  uint16_t SectionCount;
+  int32_t TimeStamp;
----------------
DiggerLin wrote:
> I think we should use the same name st NumberOfSections for same as in the XCOFFObjectFile.h ?
Sorry I missed this comment yesterday and didn't address it when responding to the others. I've updated it now. Not all the names are exactly the same, for example I 've used  `SymbolTableFileOffset` instead `SymbolTableOffset` and skipped the comment explaining 'Offset` means 'FileOffset` in this context.

================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:155
+  uint32_t SymbolTableFileOffset;
+  int32_t SymbolTableEntryCount;
+  uint16_t AuxiliaryHeaderSize;
----------------
DiggerLin wrote:
> ditto, NumberOfSymTableEntries in the XCOFFObjectFile.h
Updated.

================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:27
+private:
+  XCOFF::StorageClass StorageClass;
 };
----------------
hubert.reinterpretcast wrote:
> If possible, we should have this initialized in the constructor. Failing that, we should use have a way to check that it has been initialized and assert in the accessor when it is not.
Because of the way MCSymbols are created we won't be able to pass in a value to the constructor, so we will need a way to check if the Storage class has been set. I don't want to add an 'Uninitialized` enum value and we can't just use the obvious -1 since that is `C_EFCN`.  What are peoples thoughts on using an  optional<StorageClass> as the member?

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:44

+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
hubert.reinterpretcast wrote:
> My observations of the XLC and GCC behaviour on AIX does not support what this patch does with this value. Namely, it is observed that the physical/virtual address assigned to the `.bss` section is not necessarily 16-byte aligned.
I think we chose a default of 16 for all sections to keep it simple, after seeing that gcc aligned .data to 16 bytes, but other sections only to 4. 

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:285

-  // TODO FIXME Assign section numbers/finalize sections.
+  writeFileHeader();
+  writeSectionHeaderTable();
----------------
hubert.reinterpretcast wrote:
> Is the plan to have 64-bit versions of the functions? If the plan is to use the same functions, then having 64-bit guards on all of them now might make sense.
I haven't given any consideration on how we want to split the 32-bit/64-bit functionality yet. 

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:405
+  // BSS Section third.
+  if (!BSSCSects.empty()) {
+    Sections.push_back(&BSS);
----------------
DiggerLin wrote:
> Can we use a function for the code line 405~432, I think the Text section and Data section maybe use the same code later
We can, but the handling is subtly different for different sections: symbol index assignment is different between the .bss sections and other sections. .data will have multiple sets of different csects that get mapped into it, etc.  We will likely need to separate address/offset assignments from symbol table building, and I think we will want to have other sections implemented before trying to common up the code.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:422
+             +"Csects in the BSS can only contain a single symbol.");
+      Csect.Syms[0].SymbolTableIndex = Csect.SymbolTableIndex;
+    }
----------------
DiggerLin wrote:
> it looks Csect.Syms[0].SymbolTableIndex only assigned here, but never been used anywhere, it is I think we delete it here and delete the define of SymbolTableIndex in the Symbol structure.
In general a symbol needs a symbol table index, however the symbols we map to the .bss section are special in that they don't get a label-def so their symbol table index is the index of their containing csect.  I've included setting the SymbolTableIndex because it shows that a symbol in the .bss not getting a unique symbol table entry is the desired behavior, as opposed to it was simply forgotten. . If you feel strongly about it I can remove it from this patch, and we will need to add it back when we either add support for .text/,data sections or when we add support for emitting relocations.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:426
+    // Each of which has 2 symbol table entries.
+    SymbolTableEntryCount += BSSCSects.size() * 2;
+
----------------
hubert.reinterpretcast wrote:
> Please indicate why `+=` and why the multiplication instead of using `SymbolTableIndex`.
Changed to use `SymbolTableIndex`.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:456
+uint8_t getEncodedType(const MCSectionXCOFF *Sec) {
+  unsigned Align = Sec->getAlignment();
+  assert(isPowerOf2_32(Align) && "Alignment must be a power of 2.");
----------------
hubert.reinterpretcast wrote:
> I'm not sure where we should address this, but it seems both XLC and GCC generate at least 4-byte alignment by default.
I believe @xingxue  looked into this and may have an answer to where/how we intend to address this.

================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:458
+  assert(isPowerOf2_32(Align) && "Alignment must be a power of 2.");
+  assert(((Sec->getCSectType() & 0xF8) == 0) && "Csect type exceeds 3-bits");
+  unsigned Log2Align = Log2_32(Align);
----------------
hubert.reinterpretcast wrote:
> I believe it is more robust to use `Sec->getCSectType() <= 0x07u`.
> 
> Also:
> s/Csect/CSect/;
> s/3-bits/3 bits./;
Updated. Most of the tools and documentation use either `CSECT` or `csect`.  In these error messages 'csect' is the first word so I am capitalizing it. Should we always stick to 'CSect', and why?

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1673
+  XSym->setStorageClass(
+     TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GV));
   const DataLayout &DL = GV->getParent()->getDataLayout();
----------------
hubert.reinterpretcast wrote:
> Use `clang-format`.
I did: I use clang format enabled in vim. I also tried formatting with git-clang-format and end up with the same formating as my vim integration. What formatting do you get when you format this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159