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

Thu Mar 16 02:23:45 PDT 2023

jhenderson added inline comments.

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:314
+  uint64_t PreHeadPadSize = 0;
+  std::unique_ptr<SymbolicFile> SymFile = nullptr;
 };
----------------
No need for explicit assignment of `nullptr` - `unique_ptr`'s default constructor leaves it in an empty state.

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:430-439
+// For the AIX Big Archive format, if a member file is an XCOFF object file that
+// are loadable.  Alignmentis required by the OS for 64-bit members and
+// recommended for 32-bit members. AIX allows shared objects to be archive
+// members. When archive members are loaded, they are mapped into memory. If the
+// members aren't aligned properly in the archive, they won't be aligned in
+// memory. The loadable XCOFF Object file has Auxiliary header and loader
+// section. The content of the loadable member file needs to be aligned at
----------------
At the moment, I'm struggling to follow parts of this comment so I'd like to propose rewording it as follows:

"AIX Big Archives may contain shared object members. The AIX OS requires these members to be aligned if they are 64-bit and recommends it for 32-bit members. This ensures that when these members are loaded they are aligned in memory."

I think the rest of the comment can be moved into the method body, and I'll comment as appropriate.

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:441
+static uint32_t getMemberAlignment(SymbolicFile *SymObj) {
+  if (XCOFFObjectFile *XCOFFObj = dyn_cast_or_null<XCOFFObjectFile>(SymObj)) {
+    auto GetAuxMaxAlignment = [](uint16_t Size, auto *AuxHeader,
----------------
Perhaps you could flip this on its head. Something like:

```
XCOFFObjectFile *XCOFFObj = dyn_cast_or_null<XCOFFObjectFile>(SymObj);
if (!XCOFFObj)
  // Replace this comment with a comment that says why "2" is the right value.
  return 2;
...
```

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:442
+  if (XCOFFObjectFile *XCOFFObj = dyn_cast_or_null<XCOFFObjectFile>(SymObj)) {
+    auto GetAuxMaxAlignment = [](uint16_t Size, auto *AuxHeader,
+                                 uint16_t MaxAlignSize) {
----------------
I think I'd make this a free-standing function. The body is long and it doesn't capture any variables, so I don't think making it a lambda is particularly helpful for readability.

I'd also rename `Size` to be clearer what size it represents (e.g. "AuxHeaderSize" if that is correct).

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:444
+                                 uint16_t MaxAlignSize) {
+      if (AuxHeader == nullptr)
+        return 2;
----------------
Add a comment like: "If the member doesn't have an auxiliary header, it isn't a loadable object and so doesn't need aligning."

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:447-448
+
+      // If the auxiliary header have not both MaxAlignOfData and MaxAlignOfText
+      // field, align at 2.
+      if (Size < ((const char *)(&(AuxHeader->MaxAlignOfData)) -
----------------
It's not clear to me why if it is missing one or other of these that 2 is the right choice. Why is it not the other alignment value?

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:449-450
+      // field, align at 2.
+      if (Size < ((const char *)(&(AuxHeader->MaxAlignOfData)) -
+                  (const char *)(AuxHeader) + 2))
+        return 2;
----------------
Please use static_cast or reinterpret_cast, not C-style casts. That being said, I'm struggling to follow the logic here. Is this essentially testing if the Header is too small to contain the MaxAlignOfData field? If so, is that actually a permitted case? The `+ 2` in particular is throwing me off though.

================
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)
----------------

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:457-459
+      uint16_t AlignSize = AuxHeader->MaxAlignOfText > AuxHeader->MaxAlignOfData
+                               ? AuxHeader->MaxAlignOfText
+                               : AuxHeader->MaxAlignOfData;
----------------

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:461
+
+      return 1 << (AlignSize > 12 ? MaxAlignSize : AlignSize);
+    };
----------------
This value of 12 is a magic number that is rather meaningless to a reader of the code. Please stick it in a named constant somewhere.

Also, is `AlignSize` (and `MaxAlignSize`) really an appropriate name for the variable? An alignment value isn't a size, so unless you are aligning a size field or something, it doesn't really make sense as a name.

This line also needs a comment explaining what it is doing and why.

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:465
+    // If the desired alignment is > PAGESIZE, 32-bit members are aligned on a
+    // word boundary, while 64-bit members are aligned on a PAGESIZE boundary
+    return XCOFFObj->is64Bit()
----------------

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:595

-  // The big archive format needs to know the offset of the previous member
   // header.
----------------
Why has this comment changed only to ADD a typo??

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:597
   // header.
-  unsigned PrevOffset = 0;
-  for (const NewArchiveMember &M : NewMembers) {
----------------
Why has this variable been renamed?

================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:618
+
+  for (auto M = NewMembers.begin(); M < NewMembers.end(); M++) {
     std::string Header;
----------------
I'm not sure I understand the changes to this loop. Why do you need to know anything about the next member to know how much padding the current member requires?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872