[PATCH] D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:837
+        if (Error Err = SetNextSymFile(Buf, M->MemberName))
+          return std::move(Err);
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > Test case needed.
> > > we just refactor the code here, not adding a new functionality here, I do not think we need a new test case here. 
> > Although I agree that the code to load a symbolic file is just a refactoring of existing code, this use of that code is a new call site, where an error needs checking in a test. If there's an existing test that hits this specific piece of code, then that's fine, but if there isn't, it needs a new test. Otherwise, there would be no test that shows that the error is properly handled in this case.
> > 
> > Same goes below.
> there is existing test for the code when you implement the https://reviews.llvm.org/D88288
> https://github.com/llvm/llvm-project/blob/main/llvm/test/Object/archive-malformed-object.test#L13
> https://github.com/llvm/llvm-project/blob/main/llvm/test/Object/archive-malformed-object.test#L19
Okay, thanks. I agree that this check on this line is covered by the existing test. However, `archive-malformed-object.test` doesn't cover the case where a second or later member is bad, I believe, which means that the check and report of the error at lines 841-843 in this current version of the patch aren't covered still.

Also, that test only covers it for the case where symbol tables are needed (i.e. when llvm-ar without the "S" option is used). Ideally, you would also have a test case that shows that when no symbols are requested (i.e. `llvm-ar S<more options> test.a xcoff.o`), an error is also reported for an invalid member of a big archive.

You could cover both missing cases sufficiently by adding the following two test cases somewhere:
```
llvm-ar rcS archive.a bad.o
llvm-ar rcS archive.a good.o bad.o
```
where `archive.a` is a big archive, `bad.o` is a file that will cause `getSymbolicFile` to fail, and `good.o` is a file that it will work with. (Obviously you'd rename these files in the real test)


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:56
+
+## The auxiliary header have neither the MaxAlignOfData nor MaxAlignOfText field.
+--- !XCOFF
----------------
It looks like you missed that it should be "has neither" not "have neither"


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:97
+## The auxiliary header has both MaxAlignOfData and MaxAlignOfText fields
+## but max(MaxAlignOfData,MaxAlignOfText) excess the page size(2^12).
+--- !XCOFF
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872