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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 30 inline comments as done.
sfertile added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:44
 
+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
xingxue wrote:
> hubert.reinterpretcast wrote:
> > sfertile wrote:
> > > 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.
> > I am not aware of a way to refer to the XCOFF sections in assembly. The belief that XCOFF sections are low-level artifacts below the level of assembly is one of the reasons why `MCSectionXCOFF` represents a CSECT and not a section.
> > 
> > The `.data[RW], 4` //does// modify the symbol table entry for the CSECT to indicate 16-byte alignment. CSECTs in object files are aligned in relation to the physical/virtual address allocation and not in relation to the base address of their containing section. The snippet I posted above is for a case where the `.data` section contains a 16-byte aligned CSECT at 0x10. The offset of the CSECT data in relation to the start of the data section image in the object file is 0xC, which is not a multiple of 16.
> What Hubert says makes sense.  Variables in `.csect .data[RW]` are contained in the `.data` section where the alignments specified for `csect` are honored. 
Yeah I get it now. The section itself only has 4 byte alignment; its starting address has to be 4 byte aligned, and the size has to be a multiple of 4 bytes. Then we pad out the virtual address when allocating an address to any of the contained csects so that they are properly aligned. We aren't actually wasting any space in the sense that linking will happen at the csect granularity, not by copying an objects entire data section, so the extra padding is inconsequential to the linked module.


================
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:
> 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".
I honestly don't know. I've seen all 3 version used recently (and pretty sure I've used all 3 myself). If there is an 'official' way to capitalize it then lets use that, other wise lets pick one and use it consistently through out llvm codebase. I don't care which format, as long as we are consistent.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:100
+
+  uint32_t Alignment;
+  uint16_t Index;
----------------
hubert.reinterpretcast wrote:
> This `Alignment` is not a representation of an XCOFF field, so it should follow the convention for types representing alignment values in LLVM. See also D64790.
Regarding our other section alignment related discussion, we no longer need this field. Instead we use the `DefaultSectionAlign` of 4 regardless of the alignment of any contained csects. (Thanks for pointing out D64790 though, I wasn't aware of it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159