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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 4 inline comments as done.
sfertile added a comment.

Related to the 32-bit vs 64-bit naming and assertions: I can appreciate being defensive about this, but I think trying to disable being able to create the various types and having an assertion (or error) in every function is overkill. Too much defensiveness is just clutter. I've added an earlier fatal_error in `executePostlayoutBinding` which blocks the ObjectWriter from doing anything interesting. When we are ready to proceed with 64-bit support we can remove that error and either rename the types (and guard all the appropriate functions), or modify the types for both 64-bit and 32-bit support, whichever is appropriate for the way we intend to add 64-bit support.



================
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:
> > > 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.
> Agreed on consistency. Unfortunately following the "most official" way does not help with consistency. The Assembler Language Reference capitalizes as CSECT; however, my understanding of the LLVM style is that it means we should use CSECT in comments/messages, and `Csect` in code. `as` does generate at least one message where the capitalization is "Csect". In any case, "CSect" seems to be the odd one out, which is unfortunate, because then we'd have to rename functions.
I've replaced 'CSect' with 'Csect' in all new code added in this patch, and updated any of the error messages or comments that refer to a singular csect to use 'CSECT'. I wasn't sure about how to treat the plural 'csects' though. To me it seems like we are using plain English when talking a bout multiple csects so they should not be capitalized. Let me know if `CSECTs` is more appropriate.

I can clean up any other `CSect` usage in an NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65159