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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/Archive.h:408
   uint64_t LastChildOffset = 0;
+  std::string MergedGloSymTblBuf;
 
----------------
jhenderson wrote:
> See my comments elsewhere about naming.
thanks


================
Comment at: llvm/lib/Object/Archive.cpp:1190
+  if (RawOffset.getAsInteger(10, Size))
+    return malformedError("malformed AIX big archive: " + Twine(BitMessage) +
+                          " global symbol table size \"" + RawOffset +
----------------
jhenderson wrote:
> You're being inconsistent with your "malformed AIX big archive" prefixes. You have it here, but not in the other error messages in the function.
there is " truncated or malformed archive" output for malformedError ,I delete the "malformed AIX big archive: " .

I will delete other "malformed AIX big archive:" in a separate patch.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:424
+
+static Expected<bool> is64BitSymbolicFile(const StringRef &ObjStringRef) {
+  MemoryBufferRef ObjMbf(ObjStringRef, "");
----------------
jhenderson wrote:
> I'm looking at this method again and I'm a little concerned that we end up potentially reading each member multiple times, which is not going to be a fast operation. I seem to remember (possibly in an unrelated patch - I've been looking at too many different ones to keep things straight) that you stored the SymbolicFile class as a member of the Archive member class, to avoid this issue elsewhere. Could you make use of that here, to avoid the additional reading in that this file?
I only added two new members for struct MemberData 

 
```
uint64_t PreHeadPadSize = 0;
 std::unique_ptr<SymbolicFile> SymFile = nullptr;
```

I think you maybe want to mention the comment https://reviews.llvm.org/D142479#inline-1379936





================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:470-471
+      Expected<bool> Is64BitOrErr = is64BitSymbolicFile(M.Data);
+      // If there is an error, the error will be emit when 'computeMemberData'
+      // call 'getSymbol' function, we use consumeError here.
+      if (!Is64BitOrErr)
----------------
jhenderson wrote:
> To clarify, will `getSymbol` definitely always have been called before this point (no matter the options/contents of file etc)? If not, is it definitely always called afterwards?
The function writeSymbolTable is called only when "WriteSymtab is true"

there is two places call the is64BitSymbolicFile(M.Data) before the function  writeSymbolTable() be called.
1. in function writeArchiveToStream()


```
  if (isAIXBigArchive(Kind) && WriteSymtab) {
      Expected<bool> Is64BitOrErr = is64BitSymbolicFile(M.Data);
      if (Error E = Is64BitOrErr.takeError())
        return E;

      if (!*Is64BitOrErr)
        NumSyms32 += M.Symbols.size();
    }
```

2. In computeMemberData 


```
    if (NeedSymbols) {
      Expected<std::vector<unsigned>> SymbolsOrErr =
          getSymbols(Buf, SymNames, HasObject);
      if (!SymbolsOrErr)
        return createFileError(M.MemberName, SymbolsOrErr.takeError());
      Symbols = std::move(*SymbolsOrErr);
    }
```


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:473
+      if (!Is64BitOrErr)
+        consumeError(Is64BitOrErr.takeError());
+      if (*Is64BitOrErr != Is64Bit) {
----------------
jhenderson wrote:
> Is there any code path that can hit this code when an error is present? If so, then you're going to get a crash on the line after this `consumeError` call, so you need to do something to fix that. If not, then this should be `cantFail`, not `consumeError`. The same goes for the `consumeError` call a little ways below.
The error should not be hit. I change to cantFail , thanks


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:730
   uint64_t NumSyms = 0;
+  uint64_t NumSyms32 = 0; // Store symbol number of 32-bits member files.
+
----------------
jhenderson wrote:
> Use "32-bit" when it is used as an adjective before the noun (as here). Use "32-bits" when it is not followed by a noun (e.g. "this file is 32-bits").
thanks for explain.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:739-744
+    // In AIX OS, the big archive file contains two global symbol tables. The
+    // first global symbol table locates 32-bit file members that define global
+    // symbols; the second global symbol table does the same for 64-bit file
+    // members. A big archive can has 32-bit and 64-bit file members, we need to
+    // know the symbol number of 32-bit global symbol table and symbol number of
+    // 64-bit global table.
----------------
jhenderson wrote:
> Some wording suggestions. Please reflow as appropriate.
> 
> I've added "may" before "contain", on the assumption that an AIX big archive with only 32-bit members won't contain a 64-bit symbol table. If it is always present, please delete the "may" from the first sentence.
> I've added "may" before "contain", on the assumption that an AIX big archive with only 32-bit members won't contain a 64-bit symbol table.
 Yes,  AIX big archive may only has 32-bit global symbol table. thanks



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:849-850
+
+    // In AIX OS, The 'GlobSymOffset' field in the fixed-length header contains
+    // the offset to the 32-bit global symbol table, and the 'GlobSym64Offset'
+    // contains the offset to the 64-bit global symbol table.
----------------
jhenderson wrote:
> Field names in the comment don't match the variable names. Are the field names different?
there are same.   the "GlobSymOffset" is the member of class "BigArchive"  


```
    char GlobSymOffset[20];                  ///< Offset to global symbol table.
    char
        GlobSym64Offset[20]; ///< Offset global symbol table for 64-bit objects
```.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142479