[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 inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:448
+  // If the auxiliary header does not have both MaxAlignOfData and
+  // MaxAlignOfText field, it is not loadable share object file,
+  // align at the minimum value.
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:452
+      reinterpret_cast<const char *>(&AuxHeader->MaxAlignOfData) -
+          reinterpret_cast<const char *>(AuxHeader) + 2)
+    return MinBigArchiveMemDataAlign;
----------------
I still don't understand specifically why "2". Should this use some form of `offsetof` to get the actual position in the struct or similar?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:567
   static char PaddingData[8] = {'\n', '\n', '\n', '\n', '\n', '\n', '\n', '\n'};
-
+  static uint64_t MemHeadPadSize = 0;
   uint64_t Pos =
----------------
We've had this sort of discussion before: you can't use static local variables like this that will change each time this function is called, because this function could be called from multiple threads.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:636
+
+  for (auto M = NewMembers.begin(); M < NewMembers.end(); M++) {
     std::string Header;
----------------
Please re-review the LLVM coding standards. You keep making mistakes, despite being told before, that are covered in the coding standards.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:668
 
     if (isAIXBigArchive(Kind)) {
+      uint64_t OffsetToMemData = Pos + sizeof(object::BigArMemHdrType) +
----------------
Some more comments explaining why you're doing what you're doing in this block would be good.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:687-688
+      Pos += MemHeadPadSize;
+      uint64_t NextOffset = Pos + sizeof(object::BigArMemHdrType) +
+                            alignTo(M->MemberName.size(), 2) + alignTo(Size, 2);
+
----------------
Is this value correct for the last member in an archive?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:695-696
+            getSymbolFile((M + 1)->Buf->getMemBufferRef(), Context);
+        if (!SymFileOrErr)
+          return createFileError((M + 1)->MemberName, SymFileOrErr.takeError());
+
----------------
If I'm not mistaken, the implication of this code is that you will no longer be able to store non-symbolic-files in BigArchives. Is that intended (and if so, is it correct)? For other formats, you can store non-symbolic files as archive members. They just don't contribute anything to the symbol table.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:720-724
+        Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
+            getSymbolFile(Buf, Context);
+        if (!SymFileOrErr)
+          return createFileError(M->MemberName, SymFileOrErr.takeError());
+        CurSymFile = std::move(*SymFileOrErr);
----------------
If I'm not mistaken, you could avoid a lot of this duplicate logic about the SymbolicFile by pulling it out of the isAIXBigArchive code. Something like this outline:

```
for (auto M = NewMembers.begin(); M < NewMembers.end(); ++N) {
  if (NeedSymbols || isAIXBigArchive(Kind)) {
    if (M == NewMembers.begin() {
      CurSymFile = // load symbolic file for first member
    } else {
      CurSymFile = std::move(NextSymFile);
    }
    if (M + 1 != NewMembers.end()) {
      NextSymFile = // load symbolic file for next member
    }

    // Do all of loop logic
  }
}
```
You'll need to handle non-symbolic files somehow, perhaps by just resetting the relevant std::unique_ptr to leave it empty, and then checking whether the pointer is empty before trying to read it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872