[PATCH] D142479: [AIX] support 64bit global symbol table for big archive
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 29 01:51:42 PDT 2023
jhenderson added a comment.
Sorry for taking a long time to review this. Also, it will be important that somebody with AIX familiarity reviews this.
================
Comment at: llvm/include/llvm/Object/Archive.h:408
uint64_t LastChildOffset = 0;
+ std::string MergedGloSymTblBuf;
----------------
See my comments elsewhere about naming.
================
Comment at: llvm/lib/Object/Archive.cpp:1171
+static Error GetGlobSymTblLocAndSize(const MemoryBufferRef &Data,
+ uint64_t GlobalSymOffset,
----------------
Use lower-case for function names (as stated before), and don't abbreviate things if the meaning can become ambiguous ("Glob" could refer to "Glob patterns" and is not obviously referring to Global).
Also, "Symtab" is a more common way of shortening "symbol table", so you may wish to change all your "SymTbl" names to that, although I'm less bothered by that one.
================
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 +
----------------
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.
================
Comment at: llvm/lib/Object/Archive.cpp:1210
+
+static void GetGlobalSymbolTableInfo(SmallVector<GlobalSymTblInfo> &SymTblInfos,
+ const char *GlobSymTblLoc, uint64_t Size) {
----------------
Either return a vector from the function, rather than pass it by reference, or change the function name to `appendGlobalSymbolTableInfo` or something to that effect, to better describe what it is doing.
================
Comment at: llvm/lib/Object/Archive.cpp:1290-1292
+ // In order to let the Archive::Symbol::getNext() work for 32-bit and 64-bit
+ // global symbol table, we need to merge two global symbol table into
+ // one.
----------------
================
Comment at: llvm/lib/Object/Archive.cpp:1303
+ SymbolTable = MergedGloSymTblBuf;
+ StringTable = StringRef(SymbolTable.begin() + (SymNum + 1) * 8,
+ SymTblInfos[0].StringTable.size() +
----------------
Why "8"? It would be better to avoid "magic numbers" like this. Give them names, or better yet directly use sizeof or similar to derive the value.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:404
+static Expected<std::unique_ptr<SymbolicFile>>
+getSymbolFile(MemoryBufferRef Buf, LLVMContext &Context) {
+ std::unique_ptr<object::SymbolicFile> Obj;
----------------
I'd rename this to match the type you're fetching.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:405
+getSymbolFile(MemoryBufferRef Buf, LLVMContext &Context) {
+ std::unique_ptr<object::SymbolicFile> Obj;
+ const file_magic Type = identify_magic(Buf.getBuffer());
----------------
Move this variable declaration to where it is used for the first time.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:407
+ const file_magic Type = identify_magic(Buf.getBuffer());
+ // Treat non symbolic file types as nullptr.
+ if (!object::SymbolicFile::isSymbolicFile(Type, &Context))
----------------
I suggest this should be "Don't attempt to read non-symbolic file types."
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:424
+
+static Expected<bool> is64BitSymbolicFile(const StringRef &ObjStringRef) {
+ MemoryBufferRef ObjMbf(ObjStringRef, "");
----------------
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?
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:434
+
+ // Treated non symbolic file types as false.
+ if (!*ObjOrErr)
----------------
================
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)
----------------
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?
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:473
+ if (!Is64BitOrErr)
+ consumeError(Is64BitOrErr.takeError());
+ if (*Is64BitOrErr != Is64Bit) {
----------------
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.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:509
+
+ // If the member is not a symbolic file, treated as no symbol.
+ if (!*ObjOrErr)
----------------
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:730
uint64_t NumSyms = 0;
+ uint64_t NumSyms32 = 0; // Store symbol number of 32-bits member files.
+
----------------
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").
================
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.
----------------
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.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:835
+ if (WriteSymtab && NumSyms)
+ // Generate the 32-bits object member symbol name string and 64-bits' one.
+ for (const NewArchiveMember &M : NewMembers) {
----------------
================
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.
----------------
Field names in the comment don't match the variable names. Are the field names different?
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:866-867
+ else
+ // If there is global symbol table for 32-bit member file,
+ // the 64 bit global symbol table is after for 32-bit one.
+ GlobalSymbolOffset64 =
----------------
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:914
+ if (WriteSymtab) {
+ // Write global table for 32-bit file members.
+ if (GlobalSymbolOffset) {
----------------
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:919-920
+ GlobalSymbolOffset64);
+ // Align the 32-bit global symbol table for 64-bit global symbol table
+ // if there is.
+ if (GlobalSymbolOffset64 && (SymNamesBuf32.size() % 2))
----------------
I think this comment would be a little clearer.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:922
+ if (GlobalSymbolOffset64 && (SymNamesBuf32.size() % 2))
+ Out.write(uint8_t(0));
+ }
----------------
The style elsewhere uses the suggested edit.
================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:925
+
+ // Write global table for 64-bit file members.
+ if (GlobalSymbolOffset64)
----------------
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142479/new/
https://reviews.llvm.org/D142479
More information about the llvm-commits
mailing list