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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:44
 
+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
hubert.reinterpretcast wrote:
> 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
> ```
> 
I'm thoroughly confused at this point. gcc emits assembly and for a bunch of different test cases and it consistently emits `.csect .data[RW], 4`. According the the AIX assembly language reference, the expression after the csect name and storage mapping class is the log base-2 of alignment. Even with this alignment, some tools report the .data sections alignment as 2^3, some as 16, and the virtual address assigned to it typically ends up being **only** 4-byte aligned as you observed. I'm probably mistaking the .data csect for the .data sections alignment in some of these cases, but even if the .data section contains a 16 byte aligned .data csect, why is it not 16 byte aligned? sigh.

The binary output  llvm writes, and the binary output the system assembler will produce for the assembly llvm produces from the same input should semantically match as close as possible. At this point I think choosing the correct long term behavior is outside the scope of this patch, whose intent to add //experimental //support. If an alignment of 16 on the .data csect  was chosen only for matching gcc's assembly output then I am fine changing both assembly output and binary output to some other default (4-byte ?) alignment . If there was some other important limitation causing us to choose 16-byte aligned for assembly output then I think we keep this patch as is, until we can find out if hte system assemblers alignment treatment is intentional or not.

 @xingxue I would appreciate your thoughts on relaxing the default alignment in the assembly output.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:285
 
-  // TODO FIXME Assign section numbers/finalize sections.
+  writeFileHeader();
+  writeSectionHeaderTable();
----------------
hubert.reinterpretcast wrote:
> 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).
My problem with this is its implying more detailed though on 64-bit object support then I put into this patch. I haven't considered 64-bit support at all other then blocking writing 64-bit objects, so everything is strictly 32-bit. I don't think trying to mark up any of the structures or functions with '32' serves much purpose. Whoever adds 64-bit support will need to decide how they intend to proceed and we can evaluate the direction then.


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


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1673
+  XSym->setStorageClass(
+     TargetLoweringObjectFileXCOFF::getStorageClassForGlobal(GV));
   const DataLayout &DL = GV->getParent()->getDataLayout();
----------------
hubert.reinterpretcast wrote:
> 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.
Sorry I didn't realize it changed between the 2 patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159