[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/lib/MC/XCOFFObjectWriter.cpp:44
 
+constexpr unsigned DefaultSectionAlign = 16;
+
----------------
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.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:285
 
-  // TODO FIXME Assign section numbers/finalize sections.
+  writeFileHeader();
+  writeSectionHeaderTable();
----------------
sfertile wrote:
> 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.
Whether or not named as being 32-bit specific, please locally have the assertions for each function. For types, prevent the creation of objects of that type when targeting 64-bit XCOFF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159