[PATCH] D100651: [AIX] Support of Big archive (read)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D100651#3058324 <https://reviews.llvm.org/D100651#3058324>, @jsji wrote:

>> In my opinion, this PR is complex enough. It might be accepted with as few changes as possible. So, without freelist, undocumented features... In a second time, you can create a new PR, to extend support of Big Archive, with new tests.
>
> As @EGuesnet mentioned above, I think he won't have time to address the comments in time.
> However, we do depend on this patch for AIX in near future, 
> so I would suggest that we *accept* this patch *as it is* for now, and commit it as @EGuesnet suggested.
>
> And @DiggerLin will post follow up patches immediately.
>
> Is that OK for reviewers? @jhenderson @rupprecht @Esme ?

This patch is not in a suitable state ready for commit. I've highlighted a high number of issues, and I think there are some fundamental issues with the approach, which need addressing before it is acceptable to land this in LLVM. I haven't finished reviewing the whole patch yet either. I'm sorry if you need the patch in the near future. I can only suggest that you try implementing something yourself, perhaps by adopting and improving this patch, or starting an alternative version (inspired by this patch, and with credit given to @EGuesnet for their initial work, in the commit message).



================
Comment at: llvm/include/llvm/Object/Archive.h:40
   friend class Archive;
-
-  ArchiveMemberHeader(Archive const *Parent, const char *RawHeaderPtr,
-                      uint64_t Size, Error *Err);
-  // ArchiveMemberHeader() = default;
-
+  // clone() is used to create a new object identical to original.
+  virtual std::unique_ptr<AbstractArchiveMemberHeader> clone() const = 0;
----------------
This comment is superfluous. `clone` always means this.


================
Comment at: llvm/include/llvm/Object/Archive.h:51
 
-  Expected<uint64_t> getSize() const;
+  // Raw access and helper getters
+  virtual uint64_t getOffset() const = 0;
----------------
`getOffset` isn't named `getRawOffset`, so doesn't look like it belong in this block.

`getRawName` on the other hand looks like it does belong here.

Please make sure all comments follow the existing style. At the moment, comments are doxygen style (see e.g. `getName`). LLVM style also uses trailing full stops on comments - please add them where they are missing. That being said, don't bother with comments that provide no useful benefit beyond the method names. For example, you do not need this comment or the "Non-raw getters" comment: what benefit do they provide?


================
Comment at: llvm/include/llvm/Object/Archive.h:106
+public:
+  BigArchiveMemberHeader(Archive const *Parent, const char *RawHeaderPtr,
+                         uint64_t Size, Error *Err);
----------------
Please order `const` according to standard LLVM style, i.e. `const T *`.

If you're changing a function anyway, it makes sense to fix the style.


================
Comment at: llvm/include/llvm/Object/Archive.h:108
+                         uint64_t Size, Error *Err);
+  std::unique_ptr<AbstractArchiveMemberHeader> clone() const override {
+    return std::unique_ptr<AbstractArchiveMemberHeader>(
----------------
Please separate unrelated method declarations, or methods with non-single-line definitions with a blank line.


================
Comment at: llvm/include/llvm/Object/Archive.h:166
     Child(const Archive *Parent, StringRef Data, uint16_t StartOfFile);
+    Child(const Child &C)
+        : Parent(C.Parent), Data(C.Data), StartOfFile(C.StartOfFile) {
----------------
Do we actually need all these new constructors and assignment operators? Are they used?


================
Comment at: llvm/include/llvm/Object/Archive.h:168
+        : Parent(C.Parent), Data(C.Data), StartOfFile(C.StartOfFile) {
+      if (C.Header)
+        Header = C.Header->clone();
----------------
Under what circumstances can C.Header be a nullptr?


================
Comment at: llvm/include/llvm/Object/Archive.h:171
+    }
+    Child(Child &&C) {
+      Parent = std::move(C.Parent);
----------------
Nit: add new line before this method.


================
Comment at: llvm/include/llvm/Object/Archive.h:172
+    Child(Child &&C) {
+      Parent = std::move(C.Parent);
+      Header = std::move(C.Header);
----------------
`Parent` is a raw pointer, there's no need to std::move it.


================
Comment at: llvm/include/llvm/Object/Archive.h:360
+  // Fixed-Length Header (without magic)
+  struct ArFixLenHdrType {
+    char MemOffset[20];       /*Offset to member table */
----------------
Nit: please use `//` comments in this struct, as per the rest of the code.

As I understand it, this is a specific BigArchive class. Please rename it to be clear, e.g. `BigArchiveFixLenHdr`.


================
Comment at: llvm/include/llvm/Object/Archive.h:368
+  };
+  ArFixLenHdrType const *ArFixLenHdr;
+
----------------
I think the presence of this member shows that the design is not right.

`BigArchive` is a kind of `Archive`. The members specific to BigArchive types (as opposed to e.g. traditional BSD or GNU archives) should not be present in a type that should be generic.

I think rather than, or possibly as well as, having this member, you really want to have concrete Archive subclasses that implement the relevant logic, and make Archive an abstract class.


================
Comment at: llvm/lib/Object/Archive.cpp:46
+// All magic are 8 caractere long
+#define MAGIC_LEN (uint)8
 
----------------
I don't like this define. It is not guaranteed that all future archive magic is 8 characters long. I don't think it would be necessary if we made the Archive class a class hierarchy as suggested above.

I'm also slightly surprised that BigMagic doesn't start with `!`, but I assume that is correct and intentional.


================
Comment at: llvm/lib/Object/Archive.cpp:64
 
+  ArMemHdr = reinterpret_cast<const ArMemHdrType *>(RawHeaderPtr);
+
----------------
Why has this code moved?


================
Comment at: llvm/lib/Object/Archive.cpp:127
+
+  // Terminator is cosmetic only for AIX Big Archive
+}
----------------
Terminator characters are cosmetic only, really, in regular archives too, but we still check them.


================
Comment at: llvm/lib/Object/Archive.cpp:159-160
+Expected<StringRef> BigArchiveMemberHeader::getRawName() const {
+  // Name is outside ArMemHdr, and there is no end caracter
+  // name lenght is in NameLen field
+  // The two first char of name are already in ArMemHdrType
----------------
This comment has several typos in, and I'm not really sure what it is actually trying to say.


================
Comment at: llvm/lib/Object/Archive.cpp:305-307
+  if (StringRef(ArMemHdr->Size, sizeof(ArMemHdr->Size))
+          .rtrim(" ")
+          .getAsInteger(10, Size)) {
----------------
I'd prefer that this calculation be factored out into at least a variable, as I don't follow what it represents.


================
Comment at: llvm/lib/Object/Archive.cpp:311
     OS.write_escaped(
-        StringRef(ArMemHdr->AccessMode, sizeof(ArMemHdr->AccessMode))
-            .rtrim(" "));
+        StringRef(ArMemHdr->Size, sizeof(ArMemHdr->Size)).rtrim(" "));
     OS.flush();
----------------
This pattern is a duplicate of most of the earlier expression. Put it in a variable rather than duplicating code.


================
Comment at: llvm/lib/Object/Archive.cpp:412
+// This gets the raw UID from the ArMemHdr->UID field.
+StringRef ArchiveMemberHeader::getRawUID() const {
+  return StringRef(ArMemHdr->UID, sizeof(ArMemHdr->UID)).rtrim(' ');
----------------
`getRawName` appears not to trim the trailing spaces, but `getRawUID` does. This seems like a logical inconsistency that will likely lead to bugs.


================
Comment at: llvm/lib/Object/Archive.cpp:488
+  // Create the right concrete archive member as a function of Kind.
+  if (Parent->kind() != K_AIXBIG) {
+    Header = std::make_unique<ArchiveMemberHeader>(ArchiveMemberHeader(
----------------
These if/else checks clearly indicate that Child should really be `BigArchiveChild` and something else, sharing a common `Child` abstract base class.


================
Comment at: llvm/lib/Object/Archive.cpp:646-648
+  if (NextLoc == Parent->Data.getBufferEnd()) {
     return Child(nullptr, nullptr, nullptr);
+  }
----------------
Revert this change.


================
Comment at: llvm/lib/Object/Archive.cpp:682-683
   uint64_t RawSize = RawSizeOrErr.get();
-  Expected<StringRef> NameOrErr = Header.getName(Header.getSizeOf() + RawSize);
+  Expected<StringRef> NameOrErr =
+      Header->getName(Header->getSizeOf() + RawSize);
   if (!NameOrErr)
----------------
This looks like an unrelated formatting change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100651