[AIX] support read global symbol of big archive

[jhenderson]

llvm/lib/Object/Archive.cpp

1111–1112

Once you start using braces for an if/else sequence, use them consistently throughout that sequence.

1191

1194–1196

You need to check that the offset falls within the file, and that the file is big enough to hold the header and table or you risk reading invalid memory.

1201

1205

Could you factor out 8 * (SymNum + 1) into a separate variable, please, rather than repeat it?

[DiggerLin]

address James's comment and a new global symbol table malformed test case , thanks

[jhenderson]

llvm/lib/Object/Archive.cpp

1191

Not addressed?

1200

I would probably include the offset value in this message, along with a couple of grammar tweaks: "global symbol table with offset 0x1234 is past the end of the file"

1216

I'd do "out of file" -> "goes past the end of the file". It might be useful to have the offset and size in this error (e.g. "global symbol table at offset 0x1234 and size 0x4321 goes past the end of the file"). This will help developers spot clues as to why they're getting the error (e.g. the Size field is bad).

1221

This is a shorter name (plus the "S" in "Offsets" shouldn't be capitalized).

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

1 ↗

(On Diff #427431)

You can do this test without needing additional big archive binaries. Instead, use python to truncate an existing big archive by an appropriate number of bytes.

3 ↗

(On Diff #427431)

Nit: too many blank lines.

4–5 ↗

(On Diff #427431)

[DiggerLin]

llvm/lib/Object/Archive.cpp

1221

thanks

[DiggerLin]

address Jame's comment. thanks for the comments

[jhenderson]

llvm/lib/Object/Archive.cpp

1191

STILL not addressed...

1203–1204

These will print in decimal, but offsets are usually printed in hex. Use utohexstr instead (you may need to explicitly add a 0x prefix too, but I don't know). Also "is" -> "goes" in the message, because the thing in question may start within range, but not finish. "is" implies that ot starts past the end of the file.

1220–1223

Same comments as above.

1221

Not addressed?

[MaskRay]

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

9 ↗

(On Diff #428429)

It may be useful to do rm -rf %t && cd %t at the top once you are using tricky lit feature like %/t.a... Then you can use regular filenames without the %t prefix.

[DiggerLin]

address comment

[jhenderson]

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

16 ↗

(On Diff #428967)

@MaskRay, I'm in two minds about this, so would like another opinion: would you prefer to have sizes in hex or decimal? On the one hand, decimal is more "natural" when speaking of sizes typically. On the other hand, it's much easier to do maths if the two things are in the same base, so my instinct would be to say that the sizes need to be hex too.

[DiggerLin]

llvm/lib/Object/Archive.cpp

1191

thanks.

1203–1204

thanks

1221

thanks

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

16 ↗

(On Diff #428967)

I can change to using both hex and decimal.
truncated or malformed archive (global symbol table header at offset 0x20e and size 114(0x72) goes past the end of file)
@jhenderson

9 ↗

(On Diff #428429)

thanks

[jhenderson]

llvm/lib/Object/Archive.cpp

1204–1205

I don't think we need the size to be printed in both hex and decimal. I think just hex is sufficient, having thought about it.

1224–1225

I don't think we need the size to be printed in both hex and decimal. I think just hex is sufficient, having thought about it.

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

16 ↗

(On Diff #428967)

As noted above, I think we should just have the size in one or other format, and I think I'd pick hex.

[jhenderson]

I feel like this code is missing testing for the non-error cases. In particular, I'd expect to see some sort of test that shows that the archive symbol table can be read successfully. I think you can do that using llvm-nm --print-armap. I'd be surprised if there aren't already equivalent test cases for other formats that do this, so you may be able to copy/modify those to cover big archives too.

llvm/lib/Object/Archive.cpp

1189–1192

Test case?

1214–1217

Test case?

[DiggerLin]

In D124865#3601036, @jhenderson wrote:

I feel like this code is missing testing for the non-error cases. In particular, I'd expect to see some sort of test that shows that the archive symbol table can be read successfully. I think you can do that using llvm-nm --print-armap. I'd be surprised if there aren't already equivalent test cases for other formats that do this, so you may be able to copy/modify those to cover big archives too.

There is already has "llvm-nm --print-armap" in the llvm/test/tools/llvm-ar/delete.test , the test case is disable in AIX OS, it enable in AIX in the patch by deleting the "# XFAIL: system-aix"

[jhenderson]

In D124865#3601518, @DiggerLin wrote:

In D124865#3601036, @jhenderson wrote:

I feel like this code is missing testing for the non-error cases. In particular, I'd expect to see some sort of test that shows that the archive symbol table can be read successfully. I think you can do that using llvm-nm --print-armap. I'd be surprised if there aren't already equivalent test cases for other formats that do this, so you may be able to copy/modify those to cover big archives too.

There is already has "llvm-nm --print-armap" in the llvm/test/tools/llvm-ar/delete.test , the test case is disable in AIX OS, it enable in AIX in the patch by deleting the "# XFAIL: system-aix"

delete.test is a test that shows that llvm-ar can delete symbols when its members are deleted. It's hardly comprehensive coverage of the archive symbol table code. However, there are many other symbol table tests both in the Object and tools/llvm-ar test directories. It's not clear to me why the changes you've made didn't cause a test to start passing/failing when it wasn't before, at least on AIX OS. You need to investigate, and ensure there is coverage of the new code you've added in a test dedicated to symbol table reading etc.

llvm/test/tools/llvm-ar/malform-global-symbol-table-bigarchive.test

1 ↗

(On Diff #438708)

Test-case name should be "malformed-..." (not "malform-...")

[DiggerLin]

delete.test is a test that shows that llvm-ar can delete symbols when its members are deleted. It's hardly comprehensive coverage of the archive symbol table code. However, there are many other symbol table tests both in the Object and tools/llvm-ar test directories. It's not clear to me why the changes you've made didn't cause a test to start passing/failing when it wasn't before, at least on AIX OS. You need to investigate, and ensure there is coverage of the new code you've added in a test dedicated to symbol table reading etc.

   
When we added "XFAIL" into a lot test cases in the patch https://reviews.llvm.org/D122746 "[AIX][XCOFF] print unsupported message for llvm-ar big archive write operation" , after implement the https://reviews.llvm.org/D123949 [AIX] "support write operation of big archive",  The "XFAIL" of some of the XFAIL test cases in https://reviews.llvm.org/D122746 are removed in patch D123949.  but there are still has some test cases still has "XFAIL" , after  investigated those test cases, I created several patches for the XFAIL test cases to remove remaining the "XFAIL" from test cases.

including:

D124017 [AIX] fixed llvm-ar can not read empty big archive correctly.
D124940 [AIX] llvm-link support big archive.
D124865.  [AIX] support read global symbol of big archive

There is source code as https://github.com/llvm/llvm-project/blob/main/llvm/tools/llvm-ar/llvm-ar.cpp#L916

Kind = !NewMembersP->empty() ? NewMembersP->front().detectKindFromObject()
                                   : object::Archive::getDefaultKindForHost();

in most of the test cases which has "llvm-nm --print-armap" , when create archive with new object file, the first file is no XCOFF object file. So they do not generate big archive in AIX OS. Only the llvm/test/tools/llvm-ar/delete.test , it has (first new member is not object file, the archive format will be decided by host OS)

/# RUN: llvm-ar rc %t.a %t1.txt %t-delete.o %t-keep.o %t2.txt

so it will generate the big archive based on the host. it effected by the patch.

[DiggerLin]

llvm/lib/Object/Archive.cpp

1189–1192

I do not think we can add a test case for the malformed in the test case.
since we do not support yaml2obj for big archive, we can not generate malformed big archive. there is https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-objdump/malformed-archives.test#L182
as in patch https://reviews.llvm.org/D111889#change-i2aT5QfHJFnJ

1214–1217

as above

[DiggerLin]

address James' comment

[jhenderson]

Thanks for the explanation on the topic of symbol table testing. It makes sense to me. I've added one inline suggestion for how to test the malformed inputs cases. I think it should be fairly straightforward, but let me know if it doesn't work - it's not the end of the world if we can't test it, but I think we should be able to. As a related aside, I think there is some support for an archive file format in yaml2obj, but I don't know how comprehensive it is. It might be worth investigating to see how simple it would be to extend it for big archives.

llvm/lib/Object/Archive.cpp

1189–1192

One option might be to use python to binary edit an archive created by llvm-ar. You could probably assume that the offset of the field is stable, so can just do something like:

1. use llvm-ar to create archive.a
2. read archive.a into byte array
3. modify byte at offset x
4. write byte array to disk as new file (bad.a)
5. run llvm-ar on bad.a

1214–1217

Same response.

[DiggerLin]

llvm/lib/Object/Archive.cpp

1189–1192

My suggestion is that we do not have a test case for the these code in the patch. @jhenderson

I will create a new patch to add test case for all malformedError test including the test case in https://reviews.llvm.org/D111889. the [AIX] Support of Big archive (read) ,if you suggestion work. and I think I need to write a python script and run the script.

1214–1217

as above

[jhenderson]

LGTM, with one request.

llvm/lib/Object/Archive.cpp

1189–1192

Thanks. That sounds reasonable. Please add a "TODO: add test case." comment to the bits of code that are missing test cases before landing this patch.

[MaskRay]

[AIX] support read global symbol of big archive

"Support reading global symbols in a big archive"

llvm/lib/Object/Archive.cpp

1220–1223

Hex offsets are common. If you choose hex for size, that seems fine (I'd do it, too).

llvm/test/tools/llvm-ar/malformed-global-symbol-table-bigarchive.test

13

Delete the otherwise empty #

llvm/include/llvm/Object/Archive.h

public:
   std::unique_ptr<AbstractArchiveMemberHeader>
   createArchiveMemberHeader(const char *RawHeaderPtr, uint64_t Size,
                             Error *Err) const;
 
 protected:
   uint64_t getArchiveMagicLen() const;
   void setFirstRegular(const Child &C);
 
-private:
   StringRef SymbolTable;
   StringRef StringTable;
 
+private:
   StringRef FirstRegularData;
   uint16_t FirstRegularStartOfFile = -1;
 
   unsigned Format : 3;
   unsigned IsThin : 1;
   mutable std::vector<std::unique_ptr<MemoryBuffer>> ThinBuffers;
 };
 

llvm/lib/Object/Archive.cpp

 
 StringRef Archive::Symbol::getName() const {
   return Parent->getSymbolTable().begin() + StringIndex;
 }
 
 Expected<Archive::Child> Archive::Symbol::getMember() const {
   const char *Buf = Parent->getSymbolTable().begin();
   const char *Offsets = Buf;
-  if (Parent->kind() == K_GNU64 || Parent->kind() == K_DARWIN64)
+  if (Parent->kind() == K_GNU64 || Parent->kind() == K_DARWIN64 ||
+      Parent->kind() == K_AIXBIG)
     Offsets += sizeof(uint64_t);
   else
     Offsets += sizeof(uint32_t);
   uint64_t Offset = 0;
   if (Parent->kind() == K_GNU) {
     Offset = read32be(Offsets + SymbolIndex * 4);
-  } else if (Parent->kind() == K_GNU64) {
+  } else if (Parent->kind() == K_GNU64 || Parent->kind() == K_AIXBIG) {
     Offset = read64be(Offsets + SymbolIndex * 8);
   } else if (Parent->kind() == K_BSD) {
     // The SymbolIndex is an index into the ranlib structs that start at
     // Offsets (the first uint32_t is the number of bytes of the ranlib
     // structs).  The ranlib structs are a pair of uint32_t's the first
     // being a string table offset and the second being the offset into
     // the archive of the member that defines the symbol.  Which is what
     // is needed here.
if (kind() == K_GNU) {
     ranlib_count = read64le(buf) / 16;
     const char *ranlibs = buf + 8;
     uint64_t ran_strx = 0;
     ran_strx = read64le(ranlibs);
     buf += sizeof(uint64_t) + (ranlib_count * (2 * (sizeof(uint64_t))));
     // Skip the byte count of the string table.
     buf += sizeof(uint64_t);
     buf += ran_strx;
+  } else if (kind() == K_AIXBIG) {
+    buf = getStringTable().begin();
   } else {
     uint32_t member_count = 0;
     uint32_t symbol_count = 0;
     member_count = read32le(buf);
     buf += 4 + (member_count * 4); // Skip offsets.
     symbol_count = read32le(buf);
     buf += 4 + (symbol_count * 2); // Skip indices.
   }
   uint32_t string_start_offset = buf - getSymbolTable().begin();
   return symbol_iterator(Symbol(this, 0, string_start_offset));
 }
 
 Archive::symbol_iterator Archive::symbol_end() const {
   return symbol_iterator(Symbol(this, getNumberOfSymbols(), 0));
 }
 
 uint32_t Archive::getNumberOfSymbols() const {
   if (!hasSymbolTable())
     return 0;
   const char *buf = getSymbolTable().begin();
   if (kind() == K_GNU)
     return read32be(buf);
-  if (kind() == K_GNU64)
+  if (kind() == K_GNU64 || kind() == K_AIXBIG)
     return read64be(buf);
   if (kind() == K_BSD)
     return read32le(buf) / 8;
   if (kind() == K_DARWIN64)
     return read64le(buf) / 16;
   uint32_t member_count = 0;
   member_count = read32le(buf);
   buf += 4 + (member_count * 4); // Skip offsets.
Err = malformedError("malformed AIX big archive: first member offset \"" +
                          RawOffset + "\" is not a number");
 
   RawOffset = getFieldRawString(ArFixLenHdr->LastChildOffset);
   if (RawOffset.getAsInteger(10, LastChildOffset))
     // TODO: Out-of-line.
     Err = malformedError("malformed AIX big archive: last member offset \"" +
                          RawOffset + "\" is not a number");
 
+  // Calculate the global symbol table.
+  uint64_t GlobSymOffset = 0;
+  RawOffset = getFieldRawString(ArFixLenHdr->GlobSymOffset);
+  if (RawOffset.getAsInteger(10, GlobSymOffset))
+    // TODO: add test case.
+    Err = malformedError(
+        "malformed AIX big archive: global symbol table offset \"" + RawOffset +
+        "\" is not a number");
+
+  if (Err)
+    return;
+
+  if (GlobSymOffset > 0) {
+    uint64_t BufferSize = Data.getBufferSize();
+    uint64_t GlobalSymTblContentOffset =
+        GlobSymOffset + sizeof(BigArMemHdrType);
+    if (GlobalSymTblContentOffset > BufferSize) {
+      Err = malformedError("global symbol table header at offset 0x" +
+                           Twine::utohexstr(GlobSymOffset) + " and size 0x" +
+                           Twine::utohexstr(sizeof(BigArMemHdrType)) +
+                           " goes past the end of file");
+      return;
+    }
+
+    const char *GlobSymTblLoc = Data.getBufferStart() + GlobSymOffset;
+    const BigArMemHdrType *GlobalSymHdr =
+        reinterpret_cast<const BigArMemHdrType *>(GlobSymTblLoc);
+    RawOffset = getFieldRawString(GlobalSymHdr->Size);
+    uint64_t Size;
+    if (RawOffset.getAsInteger(10, Size)) {
+      // TODO: add test case.
+      Err = malformedError(
+          "malformed AIX big archive: global symbol table size \"" + RawOffset +
+          "\" is not a number");
+      return;
+    }
+    if (GlobalSymTblContentOffset + Size > BufferSize) {
+      Err = malformedError("global symbol table content at offset 0x" +
+                           Twine::utohexstr(GlobalSymTblContentOffset) +
+                           " and size 0x" + Twine::utohexstr(Size) +
+                           " goes past the end of file");
+      return;
+    }
+    SymbolTable = StringRef(GlobSymTblLoc + sizeof(BigArMemHdrType), Size);
+    unsigned SymNum = getNumberOfSymbols();
+    unsigned SymOffsetsSize = 8 * (SymNum + 1);
+    uint64_t SymbolTableStringSize = Size - SymOffsetsSize;
+    StringTable =
+        StringRef(GlobSymTblLoc + sizeof(BigArMemHdrType) + SymOffsetsSize,
+                  SymbolTableStringSize);
+  }
+
   child_iterator I = child_begin(Err, false);
   if (Err)
     return;
   child_iterator E = child_end();
   if (I == E) {
     Err = Error::success();
     return;
   }
   setFirstRegular(*I);
   Err = Error::success();
 }

llvm/test/Object/archive-symtab.test

 # RUN: llvm-nm --print-armap %t.a | FileCheck %s
 # RUN: grep '/SYM64/' %t.a
 
 # RUN: rm -f %t.a
 # RUN: env SYM64_THRESHOLD=837 llvm-ar rcsU %t.a %t.elf-x86-64 %t2.elf-x86-64
 # RUN: llvm-nm --print-armap %t.a | FileCheck %s
 # RUN: not grep '/SYM64/' %t.a
 
+# RUN: rm -f %t.a
+# RUN: llvm-ar rcsU --format=bigarchive %t.a %t.elf-x86-64 %t2.elf-x86-64
+# RUN: llvm-nm --print-armap %t.a | FileCheck %s
+
 # CHECK: Archive map
 # CHECK-NEXT: main in {{.*}}.elf-x86-64
 # CHECK-NEXT: foo in {{.*}}2.elf-x86-64
 # CHECK-NEXT: main in {{.*}}2.elf-x86-64
 # CHECK-NOT: bar
 
 
 # RUN: rm -f %t.a

llvm/test/tools/llvm-ar/delete.test

-# XFAIL: system-aix
 ## Test the deletion of members and that symbols are removed from the symbol table.
 
 # RUN: yaml2obj %s -o %t-delete.o --docnum=1
 # RUN: yaml2obj %s -o %t-keep.o --docnum=2
 # RUN: touch %t1.txt
 # RUN: touch %t2.txt
 
 ## Add file:

llvm/test/tools/llvm-ar/malformed-global-symbol-table-bigarchive.test

+## Test malformed global symbal table of big archive.
+
+# RUN: rm -rf %t && mkdir %t && cd %t
+# RUN: yaml2obj %s -o t.o
+# RUN: llvm-ar q t.a t.o
+# RUN: cp t.a t2.a
+
+## Truncate the file to end before the global symbol table header ends.
+# RUN: %python -c "with open('t.a', 'r+b') as input: input.truncate(560)"
+## Truncate the file to end before the global symbol table ends.
+# RUN: %python -c "with open('t2.a', 'r+b') as input: input.truncate(656)"
+
+# RUN: not llvm-ar t t.a 2>&1 | FileCheck -DFILE=t.a %s
+# RUN: not llvm-ar t t2.a 2>&1 | FileCheck -DFILE=t2.a --check-prefixes=CHECK2 %s
+
+# CHECK:  error: unable to load '[[FILE]]': truncated or malformed archive (global symbol table header at offset 0x20e and size 0x72 goes past the end of file)
+# CHECK2: error: unable to load '[[FILE]]': truncated or malformed archive (global symbol table content at offset 0x280 and size 0x25 goes past the end of file)
+
+--- !XCOFF
+FileHeader:
+  MagicNumber:       0x1DF
+Sections:
+  - Name:            .data
+    Flags:           [ STYP_DATA ]
+Symbols:
+  - Name:            export_protected_var
+    Section:         .data
+    Type:            0x4000
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW