[PATCH] D124865: [AIX] support read global symbol of big archive

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/Archive.cpp:1178
+    Err = malformedError(
+        "malformed AIX big archive: global symbol tables offset \"" +
+        RawOffset + "\" is not a number");
----------------
jhenderson wrote:
> 
Not addressed?


================
Comment at: llvm/lib/Object/Archive.cpp:1187
+    if (GlobSymOffset + sizeof(BigArMemHdrType) > BufferSize) {
+      Err = malformedError("Global symbol table out of file");
+      return;
----------------
I would probably include the offset value in this message, along with a couple of grammar tweaks: "global symbol table with offset 0x1234 is past the end of the file"


================
Comment at: llvm/lib/Object/Archive.cpp:1203
+    if (GlobSymOffset + sizeof(BigArMemHdrType) + Size > BufferSize) {
+      Err = malformedError("Global symbol table out of file");
+      return;
----------------
I'd do "out of file" -> "goes past the end of the file". It might be useful to have the offset and size in this error (e.g. "global symbol table at offset 0x1234 and size 0x4321 goes past the end of the file"). This will help developers spot clues as to why they're getting the error (e.g. the Size field is bad).


================
Comment at: llvm/lib/Object/Archive.cpp:1208
+    unsigned SymNum = getNumberOfSymbols();
+    unsigned SizeOfSymOffSets = 8 * (SymNum + 1);
+    uint64_t SymbolTableStringSize = Size - SizeOfSymOffSets;
----------------
This is a shorter name (plus the "S" in "Offsets" shouldn't be capitalized).


================
Comment at: llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test:1
+## Test malform of global symbal table of big archive.
+
----------------
You can do this test without needing additional big archive binaries. Instead, use python to truncate an existing big archive by an appropriate number of bytes.


================
Comment at: llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test:3
+
+
+#RUN: not llvm-ar t %p/Inputs/bigarchive-global-symbol-table-malform1.a 2>&1 | FileCheck -DFILE=%p/Inputs/bigarchive-global-symbol-table-malform1.a %s 
----------------
Nit: too many blank lines.


================
Comment at: llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test:4-5
+
+#RUN: not llvm-ar t %p/Inputs/bigarchive-global-symbol-table-malform1.a 2>&1 | FileCheck -DFILE=%p/Inputs/bigarchive-global-symbol-table-malform1.a %s 
+#RUN: not llvm-ar t %p/Inputs/bigarchive-global-symbol-table-malform2.a 2>&1 | FileCheck -DFILE=%p/Inputs/bigarchive-global-symbol-table-malform2.a %s
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124865