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

Tue Aug 15 02:06:10 PDT 2023

jhenderson added a comment.

In D144872#4584865 <https://reviews.llvm.org/D144872#4584865>, @DiggerLin wrote:

> In D144872#4579160 <https://reviews.llvm.org/D144872#4579160>, @jhenderson wrote:
>
>> I'll be honest, I'm not convinced the test coverage looks to be anywhere near comprehensive enough. However, I also haven't attempted to review it versus the code changes to check how much of the new code is actually covered. I'd suggest you consider using a code coverage tool to see how much coverage there is of your new code.
>
> In the test big-archive-xcoff-auxi-head-align.test , it test whether the XCOFF object data member which has auxiliary header  align correctly  based on the information of  auxiliary header. it is enough here.  In AIX OS since build bot use  CMake 3.22, it use llvm-ar instead of  AIX OS ar.  it will test the functionality too. (we have to add the -DCMAKE_AR=/usr/bin/ar when we compile, since llvm-ar do not align XCOFF object correctly for big archive in AIX os ). after the the patch commit, we will use the `llvm-ar` instead of  the AIX `ar`

For your code to land in LLVM, it needs to have appropriate levels of testing in LLVM (i.e. lit tests and gtest unit tests). Otherwise, an LLVM developer could easily make a change that breaks the existing behaviour in a subtle way. Relying on it being a system archiver on some cases will not provide the level of coverage you need IN LLVM. Bugs will creep in and will impact your system users, which is of no benefit to anyone.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872