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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:42
 using namespace llvm;
+using namespace llvm::object;
 
----------------
jhenderson wrote:
> Do you need this? The rest of the code in this file managed fine without it.
for example. in the code ,there is several place has code like.
isAIXBigArchive(Kind) ? sizeof(BigArchive::FixLenHdr) : Out.tell() + Size;





================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:204-207
+  // Big Archive has a different size for element of Member Header
+  // We cannot use printRestOfMemberHeader.
+  // Name is written at the end of the archive,
+  // followed by the cosmetic terminator "`\n".
----------------
jhenderson wrote:
> I'm not sure this comment adds much. Could you explain why you feel the need for it?
sorry that I just keep it as the original first  commit, I will delete the comment.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:221
+    if (NameLen % 2)
+      Out << (char)0; // Padding with null caracter
+  }
----------------
jhenderson wrote:
> `printBSDMemberHeader` uses `Out.write(uint8_t(0));` to write the padding. I'd probably do the same here for consistency.
thanks


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:359
+  } else if (isAIXBigArchive(Kind)) {
+    const char *Name = "";
+    printBigArchiveMemberHeader(Out, Out.tell(), Name, now(Deterministic), 0, 0,
----------------
jhenderson wrote:
> I don't think you need to declare the name as a separate variable. Just use the string literal directly at the call site.
thanks


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:370
                              bool Deterministic, ArrayRef<MemberData> Members,
-                             StringRef StringTable) {
+                             StringRef StringTable, uint64_t PreOffset = 0) {
   // We don't write a symbol table on an archive with no members -- except on
----------------
jhenderson wrote:
> Do you actually need `PreOffset` to be passed in here? This function calculates the `Pos` variable below already, and the `PreOffset` value could be calculated as part of this.
the calculate of pos happened after the writeSymbolTableHeader(Out, Kind, Deterministic, Size, PrevMemberOffset);  


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:647-649
+  uint64_t MaxOffset =
+      isAIXBigArchive(Kind) ? sizeof(BigArchive::FixLenHdr) : 8;
+  uint64_t LastOffset = MaxOffset;
----------------
jhenderson wrote:
> Previously, the scope of these two variables was quite limited. Now that it is used in a wider range of code, I'd recommend renaming them to clarify which offsets they're referring to. I think it should be `LastMemberHeaderOffset` and `LastMemberEndOffset` or something to that effect. Is that right?
I am not sure whether  "LastMemberHeaderOffset"(LastMemberEndOffset) is good variable name or not. for the gnu archive, it also include Symbol table.  but for big archive ,  MaxOffset/LastOffset  only include the file members(not include the Member table and global symbol table).


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:662
+    uint64_t SymtabSize = computeSymbolTableSize(
+        Kind, NumSyms, isAIXBigArchive(Kind) ? 8 : 4, SymNamesBuf);
     auto computeSymbolTableHeaderSize =
----------------
jhenderson wrote:
> I think think you need to modify this call? This code is already guarded by `!isAIXBigArchive(Kind)`.
yes, good catch. thanks


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:716
+    std::vector<StringRef> NameArray;
+    // Loop across object to find offset and names.
+    for (size_t it = 0, Size = NewMembers.size(); it != Size; ++it) {
----------------
jhenderson wrote:
> Could you avoid the need to loop through all members twice (i.e. above where `MaxOffset` and `LastOffset` are calculated, and here), by moving the first loop inside the first half of the `if` above (like it was before), and then calculating equivalent values as you go below?
for the gnu archive, all members include symbol table, but the big aix archive , it do not have  symbol table.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:717-729
+    for (size_t it = 0, Size = NewMembers.size(); it != Size; ++it) {
+      MembeTableNameStrTblSize += NewMembers[it].MemberName.size() + 1;
+      if (it == 0)
+        OffsetArray.push_back(sizeof(BigArchive::FixLenHdr));
+      else
+        // If current member is not last, fulfill offset of next member
+        // Curent offset + Header size  + Name length +
----------------
jhenderson wrote:
> jhenderson wrote:
> > `it` -> `Index` or `I`. `it` is not correct capitalization and could be confused with an iterator, which this isn't.
> Let's reorganise this loop a little, as I find it a little hard to follow.
> 
> Is there an off-by-one error here? The `MembeTableNameStrTblSize` increases by the name length plus a null terminator, but the offset only increases by the name length (possibly with a padding byte, but not always). If that's the case, I'd suggest adding the following line:
> ```
> size_t NameSize = Member.MemberName.size() + 1;
> ```
> and then using it in place of the name length lookups.
thanks for simplifying the code. The name in the member file header do not have a null terminator, but the name in the string table has the null terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123949