[PATCH] D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I think I now understand why you've changed the NewMember loop stuff at least, but I wanted to raise one thing before I make any further comments: the spec https://www.ibm.com/docs/en/aix/7.2?topic=formats-ar-file-format-big makes no comment about all this complicated additional alignment stuff, and refers simply to aligning on even byte boundaries. From a traditional GNU-like archive format, this makes sense: the ar tool is designed for creating static archvies that are linked by the static linker. Loadability is not a thing that needs thinking about.

Are you suggesting that AIX Big Archives can be used to store shared objects in an archive, and then that the runtime loader can directly load shared objects from these archives?



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:431
+// Log2 of PAGESIZE(4096) on an AIX system.
+static const uint32_t Log2OfPageSize = 12;
+
----------------
This and the next constant are only applicable to Big Archives/AIX, but that isn't communicated by the variable name. Could this one be renamed `Log2OfAIXPageSize`?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:433
+
+// In AIX big archive, Since the data content follows the member
+// file name, if the name ends on an odd byte, an extra byte will be
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:437
+// file starts at an even byte.
+static const uint32_t MinMemDataAlign = 2;
+
----------------
Similar to above, perhaps `MinBigArchiveMemDataAlign`?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:440
+template <typename AuxiliaryHeader>
+uint16_t GetAuxMaxAlignment(uint16_t AuxHeaderSize, AuxiliaryHeader *AuxHeader,
+                            uint16_t Log2OfMaxAlign) {
----------------
Function names start with lower-case letters...


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:443
+  // If the member doesn't have an auxiliary header, it isn't a
+  // loadable object and so it just need align at even.
+  if (AuxHeader == nullptr)
----------------
Don't specifically say "even" here, because the variable name is talking about "minimum".


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:448
+  // If the auxiliary header does not have both MaxAlignOfData and
+  // MaxAlignOfText field, align at even.
+  if (AuxHeaderSize <
----------------
This comment doesn't explain the "why" of what you're doing. I think it's important that you explain this for this case.

This was the point I was trying to get across with my previous comment, related to the casting style. Why is this the right thing to do here (in particular why the "2")?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:459-460
+  // The content of the loadable member file needs to be aligned at
+  //  MAX(maximum alignment of .text, maximum alignment of .data) if there is
+  //  two fields. If desired alignment is > PAGESIZE, 32-bit members are aligned
+  //  on a word boundary, while 64-bit members are aligned on a
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:314
+  uint64_t PreHeadPadSize = 0;
+  std::unique_ptr<SymbolicFile> SymFile = nullptr;
 };
----------------
DiggerLin wrote:
> jhenderson wrote:
> > No need for explicit assignment of `nullptr` - `unique_ptr`'s default constructor leaves it in an empty state.
> it need it , otherwise there is compiler error on 
> line 327
>  "return {{}, std::move(Header), Names, Pad ? "\n" : ""};" 
> 
> as "missing field 'SymFile' initializer [-Werror,-Wmissing-field-initializers]"
Ah, I didn't see you used it like that. In which case, this is fine.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:453
+
+      // If the XCOFF object file have not a loader section, align at 2.
+      if (AuxHeader->SecNumOfLoader <= 0)
----------------
jhenderson wrote:
> 
Any reason you didn't adopt this comment suggestion I made?

(You can change "so align at 2." in my suggestion to "so align at the minimum value."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872