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

Fri Sep 1 00:32:26 PDT 2023

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:
> > > > 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.
> 
> what is the purpose the test ?

Unlike the last few tests I've requested, which test very specific code paths, this is more of a high-level-thinking test (often called a "black box test"), i.e. one where we don't know what the details of the code are. Specifically, the aim of the test is to show that "For Big Archive, if an input object can't be loaded, we correctly report an error." Such high-level tests are useful in addition to the low-level coverage tests, because they are less likely to be invalidated by subsequent changes to the code (for example, if somebody changed this code path to be big archive only, because they decide to handle symbol tables differently, some of the test coverage provided by the existing test would no longer cover this area of code).

High-level tests can often provide low-level coverage at the same time, and similarly low-level coverage tests can cover high-level topics at the same time (e.g. the other test you've recently modified covers "For archives that require a symbol table, if an input object can't be loaded, we correctly report an error"). A consequence of this dual-purpose is that sometimes there is overlap in terms of raw coverage that tests provide, as you've seen here.

================
Comment at: llvm/test/Object/archive-malformed-object.test:14
+## Malformed bitcode object is the last file member of big archive.
+# RUN: yaml2obj xcoff.yaml -o xcoff.o
+# RUN: rm -rf bad.a
----------------
I don't think this test should use an XCOFF object. XCOFF is not a well-known format, so using it purely to provide a "good" first object will make it look like it has been chosen very deliberately, which confuses the purpose of the test. Better would be to have another bitcode object that isn't malformed as the first object. You can then have `bad.bc` and `good.bc`, which more clearly indicates what is important.

================
Comment at: llvm/test/Object/archive-malformed-object.test:16
+# RUN: rm -rf bad.a
+# RUN: not llvm-ar rcS bad.a xcoff.o input.bc 2>&1 | FileCheck %s --check-prefix=ERR1
+
----------------
I'm pretty sure you don't want the `S` here? Compare this command to the one above. If you are using a non-big archive format (which will be the default on many people's systems), then the symbols are only loaded if `S` is not present. If the symbols aren't loaded, the bitcode file won't result in an error (I think).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872