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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSymbolXCOFF.h:27
+private:
+  XCOFF::StorageClass StorageClass;
 };
----------------
sfertile wrote:
> 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?
I would be in favour of doing that.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:44
 
+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
sfertile wrote:
> 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. 
This is from a GCC-compiled object file; the `.data` section is not 16-byte aligned:
```
                         Section Header for .data
PHYaddr      VTRaddr     SCTsiz      RAWptr      RELptr
0x00000004  0x00000004  0x00000014  0x00000068  0x0000007c
```



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:285
 
-  // TODO FIXME Assign section numbers/finalize sections.
+  writeFileHeader();
+  writeSectionHeaderTable();
----------------
sfertile wrote:
> 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. 
Being clear in each function about its applicability to the 64-bit support is something that factors into the review. We can name the functions as being 32-bit specific, and then we are clear that the function has not been evaluated for 64-bit support. Even if we decide later that some of these functions should be common between the 32-bit and 64-bit paths, I believe it is less harmful to have a clearly 32-bit specific version of the function in the first place than to have cases with ambiguous scope.

If we know up front that some cases will be common, then we should be clear about those cases too (by adding the appropriate TODOs).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:422
+             +"Csects in the BSS can only contain a single symbol.");
+      Csect.Syms[0].SymbolTableIndex = Csect.SymbolTableIndex;
+    }
----------------
sfertile wrote:
> 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.
I prefer to keep the assignment in.


================
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);
----------------
sfertile wrote:
> 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?
Why did we name the functions `CSect`? I don't see why we need different styles for how to capitalize "csect".


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:38
+// into a section based on its storage-mapping class, with the exception of
+// XMC_RW which gets mapped to either .data or .bss based on if its explicitly
+// initialized or not.
----------------
s/of its/whether it's/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:182
+    assert(!needsAuxiliaryHeader() &&
+           "Auxiliary header support not implemented");
+    return 0;
----------------
Period at the end of the sentence.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1673
+  XSym->setStorageClass(
+     TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GV));
   const DataLayout &DL = GV->getParent()->getDataLayout();
----------------
sfertile wrote:
> 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?
You did, and the formatting did change. There is now one more space on the second line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159