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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:837
+        if (Error Err = SetNextSymFile(Buf, M->MemberName))
+          return std::move(Err);
+
----------------
jhenderson wrote:
> 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)
> 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.

I will add a new test scenario to cover it. 


> 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.
 
what is the purpose the test ? do you want to test whether the code go into the following block 
when `isAIXBigArchive(Kind)` of `if (NeedSymbols != SymtabWritingMode::NoSymtab || isAIXBigArchive(Kind))` is true, Or test the functionality of the following block(we already has test scenario for it)

```
{auto SetNextSymFile = [&NextSymFile,
    .....
    if ((M + 1) != NewMembers.end())
        if (Error Err = SetNextSymFile((M + 1)->Buf->getMemBufferRef(),
                                       (M + 1)->MemberName))
          return std::move(Err);
    }
```


if we want to check whether going into above block, I think the align functionality will not work without the code go into the above block. we already has test scenario to test the align of big archive.  I add the test scenario anyway.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872