[PATCH] D142479: [AIX] support 64bit global symbol table for big archive

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'm out of time for now. Please take a look at my inline comments and I'll come back to this at some point after you've addressed them (I have a big backlog to catch up on, so it may be a while).



================
Comment at: llvm/include/llvm/Object/Archive.h:408
   uint64_t LastChildOffset = 0;
+  SmallString<0> GlobalSymbolTableBuf;
 
----------------
I think it's unlikely that this buffer is ever going to be "small", so it seems likely that it would just be better to store this a `std::string` ultimately.


================
Comment at: llvm/lib/Object/Archive.cpp:1181
                          RawOffset + "\" is not a number");
+    return;
+  }
----------------
These new returns seem like they should be a separate patch. They aren't really anything to do with 64-bit global symbol table support.


================
Comment at: llvm/lib/Object/Archive.cpp:1195
+    Err = malformedError("malformed AIX big archive: global symbol table "
+                         "offset of 32-bits members \"" +
+                         RawOffset + "\" is not a number");
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:1214-1216
+  auto GetGlobSymTblLocAndSize = [&](uint64_t GlobalSymOffset,
+                                     const char *&GlobSymTblLoc, uint64_t &Size,
+                                     const char *BitMessage) {
----------------
I would avoid long lambdas like this, as they make it hard to follow the code flow. Instead, I'd pull it into a separate function. You can have it return `Error` too, rather than passing in the error as a reference.


================
Comment at: llvm/lib/Object/Archive.cpp:1252
+    GetGlobSymTblLocAndSize(GlobSym32Offset, GlobSym32TblLoc, GlobSym32Size,
+                            "32-bits");
+    if (Err)
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:1259
+    GetGlobSymTblLocAndSize(GlobSym64Offset, GlobSym64TblLoc, GlobSym64Size,
+                            "64-bits");
+    if (Err)
----------------



================
Comment at: llvm/lib/Object/Archive.cpp:1273
+
+  auto GetGlobalSymbolTableInfo = [](const char *GlobSymTblLoc,
+                                     StringRef &SymbolTable,
----------------
Again, I'd prefer this to be a separate function.


================
Comment at: llvm/lib/Object/Archive.cpp:1296
+  // global symbol table, we need to combine two global symbol table  into one.
+  if (GlobSym64Offset && GlobSym32Offset) {
+    raw_svector_ostream Out(GlobalSymbolTableBuf);
----------------
This code all feels a bit too procedural. I think you could make things a little neater by introducing a small SymbolTableReader class. Something like the following rough outline:

```
class SymbolTableReader {
public:
  void readTable(...) {
    assert(MergedSymbolOffsets.empty() && MergedStringTables.empty()); // readTable should only be called before the getters.
    SymbolOffsets.push_back(...); // Add the offsets for this individual table
    StringTable.push_back(...); // Add the strings for this individual table.
  }

  StringRef getSymbolOffsetTable() const {
    if (SymbolOffsets.size() == 1)
      return SymbolOffsets[0];
    if (!MergedSymbolOffsets.empty())
      return MergedSymbolOffsets;

    uint64_t SymCount = 0;
    for (StringRef SymbolOffsetTable : SymbolOffsets)
      SymCount += /* read sym count */;
    raw_svector_ostream Stream(MergedSymbolOffsets);
    // Write the SymCount to the stream.
    // Loop over and write the symbol offsets to the stream.
    return MergedSymbolOffsets;
  }

  StringRef getStringTable() const {
    if (StringTables.size() == 1)
      return StringTables[0];
    if (!MergedStringTables.empty())
      return MergedStringTables;

    // Combine all the strings in StringTables and store in MergedStringTables
    return MergedStringTables;
  }

private:
  StringRefVector SymbolOffsets;
  StringRefVector StringTables;

  mutable std::string MergedSymbolOffsets;
  mutable std::string MergedStringTables;
};
```


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:423
+
+  // The is64BitSymbolicFile() in the llvm-ar.cpp will be run before this
+  // function. If there is unsupported file format, it will be fail at that
----------------
The fact that there's another `is64BitSymbolFile()` should tell you that you need to reorganise some code to avoid code duplication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142479