[PATCH] D123949: [AIX] support write operation of big archive.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:209-210
+  printWithSpacePadding(Out, sys::toTimeT(ModTime), 12); // File member date
+  printWithSpacePadding(Out, UID % 1000000000000, 12);   // UID
+  printWithSpacePadding(Out, GID % 1000000000000, 12);   // GID
+  printWithSpacePadding(Out, format("%o", Perms), 12);   // Permission
----------------
I'm not sure I understand this modulo here. I'm guessing it's intended to mirror the same thing in `printRestOfMemberHeader`, but in that case, it should have a comment (just like in that code).


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:354-357
+  } else if (isAIXBigArchive(Kind))
+    printBigArchiveMemberHeader(Out, "", now(Deterministic), 0, 0,
+                                0, Size, PrevMemberOffset, 0);
+  else {
----------------
Once using braces for part of an if/else, use it for all parts.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:451-452
 
   // This ignores the symbol table, but we only need the value mod 8 and the
   // symbol table is aligned to be a multiple of 8 bytes
+  uint64_t Pos =
----------------
I feel like this comment is stale for Big Archive format.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:514
 
+  // Big archive needs to know offset of the previous member header
+  unsigned PrevOffset = 0;
----------------
(and reflow with clang-format if needed)


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:554-556
+    } else
+      printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, M,
+                        ModTime, Size);
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:656
 
+  if (WriteSymtab && !isAIXBigArchive(Kind)) {
     // We assume 32-bit offsets to see if 32-bit symbols are possible or not.
----------------
You should probably have a comment explaining why no symbol table for Big archive.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:698
 
-  if (WriteSymtab)
-    writeSymbolTable(Out, Kind, Deterministic, Data, SymNamesBuf);
+  if (!isAIXBigArchive(Kind)) {
+    if (WriteSymtab)
----------------
Ditto - please add a comment explaining why Big archive format is different.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:704
+  } else {
+    // For big archive (AIX), compute a table of member names and offset,
+    // used in the member table.
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:724-725
+    // AIX member table size.
+    unsigned MemberTableSize =
+        20 * (MemberOffsets.size() + 1) +
+        MemberTableNameStrTblSize; // Size of Global symbol table
----------------
I'm assuming the "+ 1" is for the "number of members" field? If so, I'd recommend pulling it out of the calculation, and adding a comment for it e.g.:

```
    unsigned MemberTableSize =
        20 + // Number of members field
        20 * MemberOffsets.size() +
        MemberTableNameStrTblSize;
```
Conversely, I don't think you need to label the second or third parameters, since they are already named variables (plus the comment doesn't line up with my understanding of what this is).


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:737
+                          20); // Offset to member table
+    printWithSpacePadding(Out, NewMembers.size() ? GlobalSymbolOffset : 0, 20);
+    printWithSpacePadding(
----------------
Missing comment? Also, I don't understandwhy the global symbol table offset is dependent on the size of `NewMembers`?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:747
+    printWithSpacePadding(
+        Out, "0",
+        20); // Offset to first member of free list - Not supported yet
----------------
Why is `"0"` quoted here, unlike in the other cases?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:761
+                                  GlobalSymbolOffset);
+      printWithSpacePadding(Out, MemberOffsets.size(), 20); // Number of member
+      for (uint64_t MemberOffset : MemberOffsets)
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:766
+      for (StringRef MemberName : MemberNames)
+        Out << MemberName << '\0' ; // Mmember file names, null byte padding.
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123949