[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/lib/Object/Archive.cpp:1181
                          RawOffset + "\" is not a number");
+    return;
+  }
----------------
jhenderson wrote:
> 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.
OK, I will do it.


================
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);
----------------
jhenderson wrote:
> 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;
> };
> ```
when class Archive , it need set two member
 
```
 StringRef SymbolTable;
  StringRef StringTable;
```

 for symbol iterating.

we do not need a variable "SymbolOffsets" here.

and for  getStringTable() in your comment, also only use once for setting the "StringTable" of class Archive.

And the code use in my patch only 18 lines which including the merging of 32bit global symbol table and 64 bit global symbol table into a one and setting the two variable of Archive class (SymbolTable and StringTable), I think it is easy to understand.

but I get some idea from your comment to introduce the a new structure to reduce the variables.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142479