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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the explanation on the topic of symbol table testing. It makes sense to me. I've added one inline suggestion for how to test the malformed inputs cases. I think it should be fairly straightforward, but let me know if it doesn't work - it's not the end of the world if we can't test it, but I think we should be able to. As a related aside, I think there is some support for an archive file format in yaml2obj, but I don't know how comprehensive it is. It might be worth investigating to see how simple it would be to extend it for big archives.



================
Comment at: llvm/lib/Object/Archive.cpp:1189-1192
+  if (RawOffset.getAsInteger(10, GlobSymOffset))
+    Err = malformedError(
+        "malformed AIX big archive: global symbol table offset \"" + RawOffset +
+        "\" is not a number");
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Test case?
> I do not think we can add a test case for the malformed in the test case. 
> since we do not support yaml2obj  for big archive, we can not generate malformed big archive.  there is https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-objdump/malformed-archives.test#L182 
> as in patch https://reviews.llvm.org/D111889#change-i2aT5QfHJFnJ
> 
One option might be to use python to binary edit an archive created by llvm-ar. You could probably assume that the offset of the field is stable, so can just do something like:
# use llvm-ar to create archive.a
# read archive.a into byte array
# modify byte at offset x
# write byte array to disk as new file (bad.a)
# run llvm-ar on bad.a


================
Comment at: llvm/lib/Object/Archive.cpp:1214-1217
+    if (RawOffset.getAsInteger(10, Size)) {
+      Err = malformedError(
+          "malformed AIX big archive: global symbol table size \"" + RawOffset +
+          "\" is not a number");
----------------
DiggerLin wrote:
> jhenderson wrote:
> > Test case?
> as above
Same response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124865