[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:526-527
+  // 'MaxAlignOfData' in the AuxiliaryHeader.
+  if (AuxHeaderSize < offsetof(AuxiliaryHeader, ModuleType))
+    return MinBigArchiveMemDataAlign;
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > This doesn't seem to be covered?
> > Are we need a test case to cover all the statement ?
> I added a test scenario for it anyway.
Yes, all statements should be covered by tests. Otherwise, you've got nothing that will show that this code is functioning correctly.


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


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:23
+## The content of t32_nomaxdata_text, t64_nomaxdata_text.o aligned at 2. 
+# RUN: %python -c 'f=open("t_aux.a","rb");f.seek(262);print(f.read(2));f.close()' | FileCheck --check-prefix=MAGIC32 %s
+# RUN: %python -c 'f=open("t_aux.a","rb");f.seek(528);print(f.read(2));f.close()' | FileCheck --check-prefix=MAGIC64 %s
----------------
Rather than repeating this python snippet over and over again, could you write a little python script at the end of this file, use split-file to split it into a .py file at runtime, and then execute the file with all the different input arguments?

The RUN line would end up something like:
```
# RUN: %python print-magic.py 262 | FileCheck --check-prefix=MAGIC32
```


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:36-37
+
+## Test that the content of an XCOFF object files, which have not an auxiliary
+## header, is aligned at 2 in a big archive.
+# RUN: env OBJECT_MODE=32_64 llvm-ar -q t3.a t32_1.o t64_1.o t32_2.o t64_2.o
----------------
Or:

"Test that the content of XCOFF object files, which don't have auxiliary headers, are aligned at 2 in a big archive."


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:54
+
+--- !XCOFF
+FileHeader:
----------------
This is a near-duplicate of the previous YAML doc. Could you just parameterise the section name, like you do with the `FLAG` input parameter?


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:61
+
+## The auxiliary header does not have both MaxAlignOfData and  MaxAlignOfText field
+--- !XCOFF
----------------



================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:73
+
+## The auxiliary header have both MaxAlignOfData and MaxAlignOfText field.
+--- !XCOFF
----------------



================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:101
+
+## The auxiliary header have both MaxAlignOfData and MaxAlignOfText field but excess the page size.
+--- !XCOFF
----------------
I'm not sure what "but excess the page size" means. Should it be "but they exceed the page size" or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872