[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 requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

I've added a few more nits. I've also gone through and highlighted a number of cases that I'm pretty confident don't have existing test coverage. I shouldn't have had to go through these bits myself and highlight all of this - you should have done this yourself, prior to the patch even going up for review. Requesting changes to clear the "Accepted" state.



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:526-527
+  // 'MaxAlignOfData' in the AuxiliaryHeader.
+  if (AuxHeaderSize < offsetof(AuxiliaryHeader, ModuleType))
+    return MinBigArchiveMemDataAlign;
+
----------------
This doesn't seem to be covered?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:531
+  // loadable, so align at the minimum value.
+  if (AuxHeader->SecNumOfLoader <= 0)
+    return MinBigArchiveMemDataAlign;
----------------
jhenderson wrote:
> This probably isn't covered?
`SecNumOfLoader` is unsigned, right? So it can never be less than 0 by definition...


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:531-532
+  // loadable, so align at the minimum value.
+  if (AuxHeader->SecNumOfLoader <= 0)
+    return MinBigArchiveMemDataAlign;
+
----------------
This probably isn't covered?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:540
+  uint16_t Log2OfAlign =
+      std::max(AuxHeader->MaxAlignOfText, AuxHeader->MaxAlignOfData);
+  return 1 << (Log2OfAlign > Log2OfAIXPageSize ? Log2OfMaxAlign : Log2OfAlign);
----------------
This needs test cases for the text alignment being bigger and the data alignment being bigger. I can't tell from the existing tests whether both cases are covered.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:541
+      std::max(AuxHeader->MaxAlignOfText, AuxHeader->MaxAlignOfData);
+  return 1 << (Log2OfAlign > Log2OfAIXPageSize ? Log2OfMaxAlign : Log2OfAlign);
+}
----------------
Are both parts of this ternary covered?


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:550
+  XCOFFObjectFile *XCOFFObj = dyn_cast_or_null<XCOFFObjectFile>(SymObj);
+
+  if (!XCOFFObj)
----------------
Nit: delete this blank line - the if below is strongly linked to the previous line.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:555
+  // If the desired alignment is > PAGESIZE, 32-bit members are aligned on a
+  // word boundary, while 64-bit members are aligned on a PAGESIZE boundary
+  return XCOFFObj->is64Bit()
----------------



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:837
+        if (Error Err = SetNextSymFile(Buf, M->MemberName))
+          return std::move(Err);
+
----------------
Test case needed.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:844
+                                       (M + 1)->MemberName))
+          return std::move(Err);
+    }
----------------
Test case needed.


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:5
+## header, is aligned in a big archive based on the MAX(maximum alignment
+## of .text, maximum alignment of .data)
+
----------------



================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:6
+## of .text, maximum alignment of .data)
+
+# RUN: rm -rf %t && mkdir %t
----------------
Delete this blank line - the comment above is associated with the following test case, but the blank line suggests it either doesn't, or that it applies to the whole test file.


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:8
+# RUN: rm -rf %t && mkdir %t
+# RUN: cp %p/../llvm-readobj/XCOFF/Inputs/needed-libs*.o %t
+# RUN: cd %t
----------------
Don't rely on inputs in another part of the test tree. It is quite possible that these files will move/be deleted etc, and people won't expect that to impact tests in another part of the testing tree. Instead, you should create tehse files using yaml2obj.

The other aspect of this is that I have no way of telling that the inputs you're using actually have the properties that you are trying to test for (without inspecting the binaries). Having the yaml2obj form of them would enable this.


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:12
+# RUN: env OBJECT_MODE=32_64 llvm-ar -q t2.a needed-libs-32.o needed-libs-64.o
+# RUN: %python -c 'f=open("t1.a","rb");f.seek(384);print(f.read(2));f.close()' | FileCheck -check-prefix=MAGIC64 %s
+# RUN: %python -c 'f=open("t1.a","rb");f.seek(7296);print(f.read(2));f.close()' | FileCheck -check-prefix=MAGIC32 %s
----------------
Nit: prefer `--check-prefix` over `-check-prefix` in new tests. Applies throughout.


================
Comment at: llvm/test/tools/llvm-ar/big-archive-xcoff-align.test:22
+# RUN: yaml2obj --docnum=2 -DFLAG=0x1F7 %s -o t64_2.o
+
+# RUN: env OBJECT_MODE=32_64 llvm-ar -q t3.a t32_1.o t64_1.o t32_2.o t64_2.o
----------------
This blank line to me means the comment above is not associated with the checks that follow below. Delete it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872