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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

It looks like in the latest diff you've reinstated some XFAIL/`--format=gnu` commands. Why?

Not looked at everything quite today, as am out of time.



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:217
+    if (NameLen % 2)
+      Out.write(uint8_t(0)); // Padding with null caracter
+  }
----------------
If there are any similar comments in your patch, please make the same change.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:767
+      }
+      if (MemberTableNameStrTblSize % 3)
+        Out << '\0'; // Name table must be tail padded to an even number of
----------------
Why has this become `% 3`? This isn't correct for tail padding to an even size.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:42
 using namespace llvm;
+using namespace llvm::object;
 
----------------
DiggerLin wrote:
> 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;
> 
> 
> 
If you want to add it, please make a separate patch to remove the now-redundant `object::` prefixes in this file. It can be a successor patch, if you want, or a prerequisite.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:647-649
+  uint64_t MaxOffset =
+      isAIXBigArchive(Kind) ? sizeof(BigArchive::FixLenHdr) : 8;
+  uint64_t LastOffset = MaxOffset;
----------------
DiggerLin wrote:
> 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).
At the moment, I don't understand the difference between LastOffset and MaxOffset from just the variable names. I'd like either better names, and possibly even completely separate variables for AIX, to distinguish.


================
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 +
----------------
DiggerLin wrote:
> 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.
That's probably worth a comment or two in this code explaining why you add a null terminator in one place but not the other then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123949