[llvm-readobj][XCOFF] Add support for `--needed-libs` option.

[Esme]

Summary

This patch is trying to add support for llvm-readobj --needed-libs option under XCOFF.
For XCOFF, the needed libraries can be found from the Import File ID Name Table of the Loader Section.
Currently, I am using binary inputs in the test since yaml2obj does not yet support for writing the Loader Section and the import file table.

[jhenderson]

You'll need an XCOFF developer to review the object file code.

llvm/lib/Object/XCOFFObjectFile.cpp

830

This more closely represents the literal string you'd specify.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test

2

Add simple comment to the start of the test.

Also add test cases where there are no entries in the table (it is empty), and no table at all (assuming neither case is explicitly forbidden by the spec).

3

For both FileCheck invocations, I'd add --strict-whitespace --match-full-lines to ensure you are capturing the whole NeedLibraries output.

8–9

Use CHECK-NEXT.

Also, update your test inputs to have more than one entry.

12

a) The leading comma without any content looks wrong
b) If you craft your inputs differently, you could have the same output for both forms and avoid the need for two different checks.
c) Where are the spaces after the commas coming from in this output? The code doesn't seem to add them currently.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

501

Test case required.

507–518

Using a for loop seems like the right thing to do here. Also:

1. Don't use uint32_t for a count, use size_t.
2. Why are you omitting the LIBPATH value? It seems like you should have some method to dump this.
3. Index from 0 or people are going to think you have an off-by-one error.
4. No need to use comments to describe what the code clearly does.
5. Add a space after the comma?

[Esme]

Addressed comments.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test

2

According to the spec, no entries in the table is the same as no table at all, because a table consists only of entries.

12

a) Because there is no path name in some import ID entries. Should I not print the comma for an empty name? The output format references the output of the system objdump in AIX.

$ objdump -P loader 1.o
...
Import files:
  0: /home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib,,
  1: ,libc.a,shr.o

c) Spaces are automatically generated by ScopedPrinter when using startLine()<<.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

501

I will add cases to test wrong paths after the import file table can be customized in yaml2obj.

507–518

Thanks!

2. What I omitted here is /home/esme/workspace/build/bin/../lib/powerpc-ibm-aix7.2.0.0:/home/esme/workspace/build/bin/../xlmass/latest/lib/:/home/esme/workspace/build/bin/../lib/:/usr/lib:/lib\0\0\0, which contains a default LIBPATH name without file name or archive member name, and I think we don't need to dump it in needed libs? Btw, ELF doesn't print paths under --needed-libs either.

5. W.startLine() << will generate 2 spaces in the case.

void printIndent() {
  OS << Prefix;
  for (int i = 0; i < IndentLevel; ++i)
    OS << "  ";
}

[jhenderson]

Apologies, I haven't had a chance to get back to this, and I'm off work next week, so won't be able to until after that.

[Esme]

In D106643#2930596, @jhenderson wrote:

Apologies, I haven't had a chance to get back to this, and I'm off work next week, so won't be able to until after that.

Never mind! Thanks for your review.

[shchenz]

Some minor comments. Thanks for doing this.

llvm/include/llvm/Object/XCOFFObjectFile.h

110

32 bit loader section should have no l_rldoff field?

123

The padding seems not follow XCOFF doc. As I see, there should be 16 byte padding between OffsetToSymTbl and OffsetToRelEnt?

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test

8–9

Would it better to add a header for the contents, like:

PATH            BASE            MEMBER     
,            libc.a,            shr.o
,            libunwind.a,            libunwind.so.1
,            libc++abi.a,            libc++abi.so.1

And then, maybe we can use the alignment to tell which value is for which field. so we don't need the ,?

For me, it is a little strange with , as the start or end of an import file entry.

We may generate strings like:

,    libxxx.a,
,  libunwind.a,  libunwind.so.1

[Esme]

Address comments.

llvm/include/llvm/Object/XCOFFObjectFile.h

110

According to the spec, there is no l_symoff(OffsetToSymTbl) in 32-bit Loader section, but there is l_rldoff(OffsetToRelEnt).

[Esme]

Remove l_rldoff from LoaderSectionHeader32.

llvm/include/llvm/Object/XCOFFObjectFile.h

110

Ok, sorry, it seems the spec has some inconsistency on LoaderSectionHeader32, and you're right that 32-bit has no l_rldoff.

[jhenderson]

I like the new output format better. That being said, I think you need to be careful of really long filenames. I don't know what's the best approach here, but one option would be to parse all the names before printing the headers, and then adjust the column widths accordingly. Up to you though.

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test

11

If the PATH column is always empty, then there's no point in having it in the header. If it isn't, we need a test case showing that things can be printed in this field.

[Esme]

Address comment.

[jhenderson]

I think you need testing for long names in your test case, to show the column width adjustments.

llvm/tools/llvm-readobj/XCOFFDumper.cpp

497

The name here is somewhat generic, but the functionality is tied to the needed libraries implementation (i.e. it's to do with the base/member columns specifically). It probably makes more sense to just inline this function, since it's only caleld once.

[Esme]

In D106643#2951531, @jhenderson wrote:

I think you need testing for long names in your test case, to show the column width adjustments.

I think the test case is enough? Because the default width is 13, and libunwind.so.1's width is 14.

[Esme]

Remove the standalone function of getting max width.

[jhenderson]

In D106643#2951573, @Esme wrote:

In D106643#2951531, @jhenderson wrote:

I think you need testing for long names in your test case, to show the column width adjustments.

I think the test case is enough? Because the default width is 13, and libunwind.so.1's width is 14.

That's only the Member column. The Base column needs the same testing, as it is in fact the one that's more important. In fact, you could probably drop references to MemberWidth, thinking about it, since it has no impact on anything useful, as far as I understand it - having trailing spaces on the column is not useful. Thoughts?

[Esme]

Address comment.

[jhenderson]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

529–533

1. You don't want to justify/add trailing spaces on the last column - there's no point.
2. You can simplify this code.

[Esme]

Address comment and modify tests.

[jhenderson]

LGTM.

llvm/include/llvm/Object/XCOFFObjectFile.h

struct XCOFFSectionHeader64 : XCOFFSectionHeader<XCOFFSectionHeader64> {
   support::big64_t FileOffsetToRelocationInfo;
   support::big64_t FileOffsetToLineNumberInfo;
   support::ubig32_t NumberOfRelocations;
   support::ubig32_t NumberOfLineNumbers;
   support::big32_t Flags;
   char Padding[4];
 };
 
+struct LoaderSectionHeader32 {
+  support::ubig32_t Version;
+  support::ubig32_t NumberOfSymTabEnt;
+  support::ubig32_t NumberOfRelTabEnt;
+  support::ubig32_t LengthOfImpidStrTbl;
+  support::ubig32_t NumberOfImpid;
+  support::big32_t OffsetToImpid;
+  support::ubig32_t LengthOfStrTbl;
+  support::big32_t OffsetToStrTbl;
+};
+
+struct LoaderSectionHeader64 {
+  support::ubig32_t Version;
+  support::ubig32_t NumberOfSymTabEnt;
+  support::ubig32_t NumberOfRelTabEnt;
+  support::ubig32_t LengthOfImpidStrTbl;
+  support::ubig32_t NumberOfImpid;
+  support::ubig32_t LengthOfStrTbl;
+  support::big64_t OffsetToImpid;
+  support::big64_t OffsetToStrTbl;
+  support::big64_t OffsetToSymTbl;
+  char Padding[16];
+  support::big32_t OffsetToRelEnt;
+};
+
 struct XCOFFStringTable {
   uint32_t Size;
   const char *Data;
 };
 
 struct XCOFFCsectAuxEnt32 {
   support::ubig32_t SectionOrLength;
   support::ubig32_t ParameterHashIndex;
private:
 
   size_t getFileHeaderSize() const;
   size_t getSectionHeaderSize() const;
 
   const XCOFFSectionHeader32 *toSection32(DataRefImpl Ref) const;
   const XCOFFSectionHeader64 *toSection64(DataRefImpl Ref) const;
   uintptr_t getSectionHeaderTableAddress() const;
   uintptr_t getEndOfSymbolTableAddress() const;
+  Expected<uintptr_t> getLoaderSectionAddress() const;
 
   // This returns a pointer to the start of the storage for the name field of
   // the 32-bit or 64-bit SectionHeader struct. This string is *not* necessarily
   // null-terminated.
   const char *getSectionNameInternal(DataRefImpl Sec) const;
 
   static bool isReservedSectionNumber(int16_t SectionNumber);
 
public:
   // Relocation-related interfaces.
   template <typename T>
   Expected<uint32_t>
   getNumberOfRelocationEntries(const XCOFFSectionHeader<T> &Sec) const;
 
   template <typename Shdr, typename Reloc>
   Expected<ArrayRef<Reloc>> relocations(const Shdr &Sec) const;
 
+  // Loader section related interfaces.
+  Expected<StringRef> getImportFileTable() const;
+
   // This function returns string table entry.
   Expected<StringRef> getStringTableEntry(uint32_t Offset) const;
 
   // This function returns the string table.
   StringRef getStringTable() const;
 
   const XCOFF::SymbolAuxType *getSymbolAuxType(uintptr_t AuxEntryAddress) const;
 

llvm/lib/Object/XCOFFObjectFile.cpp

 }
 
 uint64_t XCOFFObjectFile::getSectionAlignment(DataRefImpl Sec) const {
   uint64_t Result = 0;
   llvm_unreachable("Not yet implemented!");
   return Result;
 }
 
+Expected<uintptr_t> XCOFFObjectFile::getLoaderSectionAddress() const {
+  uint64_t OffsetToLoaderSection = 0;
+  uint64_t SizeOfLoaderSection = 0;
+
+  if (is64Bit()) {
+    for (const auto &Sec64 : sections64())
+      if (Sec64.getSectionType() == XCOFF::STYP_LOADER) {
+        OffsetToLoaderSection = Sec64.FileOffsetToRawData;
+        SizeOfLoaderSection = Sec64.SectionSize;
+        break;
+      }
+  } else {
+    for (const auto &Sec32 : sections32())
+      if (Sec32.getSectionType() == XCOFF::STYP_LOADER) {
+        OffsetToLoaderSection = Sec32.FileOffsetToRawData;
+        SizeOfLoaderSection = Sec32.SectionSize;
+        break;
+      }
+  }
+
+  // No loader section is not an error.
+  if (!SizeOfLoaderSection)
+    return 0;
+
+  uintptr_t LoderSectionStart =
+      reinterpret_cast<uintptr_t>(base() + OffsetToLoaderSection);
+  if (Error E =
+          Binary::checkOffset(Data, LoderSectionStart, SizeOfLoaderSection))
+    return std::move(E);
+  return LoderSectionStart;
+}
+
 bool XCOFFObjectFile::isSectionCompressed(DataRefImpl Sec) const {
   return false;
 }
 
 bool XCOFFObjectFile::isSectionText(DataRefImpl Sec) const {
   return getSectionFlags(Sec) & XCOFF::STYP_TEXT;
 }
 
XCOFFObjectFile::parseStringTable(const XCOFFObjectFile *Obj, uint64_t Offset) {
 
   const char *StringTablePtr = StringTableOrErr.get();
   if (StringTablePtr[Size - 1] != '\0')
     return errorCodeToError(object_error::string_table_non_null_end);
 
   return XCOFFStringTable{Size, StringTablePtr};
 }
 
+// This function returns the import file table. Each entry in the import file
+// table consists of: "path_name\0base_name\0archive_member_name\0".
+Expected<StringRef> XCOFFObjectFile::getImportFileTable() const {
+  Expected<uintptr_t> LoaderSectionAddrOrError = getLoaderSectionAddress();
+  if (!LoaderSectionAddrOrError)
+    return LoaderSectionAddrOrError.takeError();
+
+  uintptr_t LoaderSectionAddr = LoaderSectionAddrOrError.get();
+  if (!LoaderSectionAddr)
+    return StringRef();
+
+  uint64_t OffsetToImportFileTable = 0;
+  uint64_t LengthOfImportFileTable = 0;
+  if (is64Bit()) {
+    const LoaderSectionHeader64 *LoaderSec64 =
+        viewAs<LoaderSectionHeader64>(LoaderSectionAddr);
+    OffsetToImportFileTable = LoaderSec64->OffsetToImpid;
+    LengthOfImportFileTable = LoaderSec64->LengthOfImpidStrTbl;
+  } else {
+    const LoaderSectionHeader32 *LoaderSec32 =
+        viewAs<LoaderSectionHeader32>(LoaderSectionAddr);
+    OffsetToImportFileTable = LoaderSec32->OffsetToImpid;
+    LengthOfImportFileTable = LoaderSec32->LengthOfImpidStrTbl;
+  }
+
+  auto ImportTableOrErr = getObject<char>(
+      Data,
+      reinterpret_cast<void *>(LoaderSectionAddr + OffsetToImportFileTable),
+      LengthOfImportFileTable);
+  if (Error E = ImportTableOrErr.takeError())
+    return std::move(E);
+
+  const char *ImportTablePtr = ImportTableOrErr.get();
+  if (ImportTablePtr[LengthOfImportFileTable - 1] != '\0')
+    return createStringError(
+        object_error::parse_failed,
+        "the import file table must end with a null terminator");
+
+  return StringRef(ImportTablePtr, LengthOfImportFileTable);
+}
+
 Expected<std::unique_ptr<XCOFFObjectFile>>
 XCOFFObjectFile::create(unsigned Type, MemoryBufferRef MBR) {
   // Can't use std::make_unique because of the private constructor.
   std::unique_ptr<XCOFFObjectFile> Obj;
   Obj.reset(new XCOFFObjectFile(Type, MBR));
 
   uint64_t CurOffset = 0;
   const auto *Base = Obj->base();

llvm/test/tools/llvm-readobj/XCOFF/needed-libs.test

+## In this test we check the --needed-libs option.
+
+# RUN: llvm-readobj --needed-libs %p/Inputs/needed-libs-32.o \
+# RUN:   %p/Inputs/needed-libs-64.o %p/Inputs/needed-libs-empty.o |\
+# RUN:   FileCheck %s --strict-whitespace --match-full-lines
+
+## Check 32-bit.
+
+#      CHECK:NeededLibraries [
+# CHECK-NEXT:  BASE             MEMBER
+# CHECK-NEXT:  libc.a           shr.o
+# CHECK-NEXT:  libpthreads.a    shr_xpg5.o
+# CHECK-NEXT:  libabcdefghijk.a 
+# CHECK-NEXT:]
+
+## Check 64-bit.
+
+#      CHECK:NeededLibraries [
+# CHECK-NEXT:  BASE               MEMBER
+# CHECK-NEXT:  libc.a             shr_64.o
+# CHECK-NEXT:  libpthreads.a      shr_xpg5_64.o
+# CHECK-NEXT:  libabcdefghijk64.a 
+# CHECK-NEXT:]
+
+## Check no lib.
+
+#      CHECK:NeededLibraries [
+# CHECK-NEXT:  BASE          MEMBER
+# CHECK-NEXT:]

llvm/tools/llvm-readobj/XCOFFDumper.cpp

 //===-- XCOFFDumper.cpp - XCOFF dumping utility -----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements an XCOFF specific dumper for llvm-readobj.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ObjDumper.h"
 #include "llvm-readobj.h"
 #include "llvm/Object/XCOFFObjectFile.h"
+#include "llvm/Support/FormattedStream.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 using namespace llvm;
 using namespace object;
 
 namespace {
 
 class XCOFFDumper : public ObjDumper {

 void XCOFFDumper::printUnwindInfo() {
   llvm_unreachable("Unimplemented functionality for XCOFFDumper");
 }
 
 void XCOFFDumper::printStackMap() const {
   llvm_unreachable("Unimplemented functionality for XCOFFDumper");
 }
 
 void XCOFFDumper::printNeededLibraries() {
-  llvm_unreachable("Unimplemented functionality for XCOFFDumper");
+  ListScope D(W, "NeededLibraries");
+  auto ImportFilesOrError = Obj.getImportFileTable();
+  if (!ImportFilesOrError) {
+    reportUniqueWarning(ImportFilesOrError.takeError());
+    return;
+  }
+
+  StringRef ImportFileTable = ImportFilesOrError.get();
+  const char *CurrentStr = ImportFileTable.data();
+  const char *TableEnd = ImportFileTable.end();
+  // Default column width for names is 13 even if no names are that long.
+  size_t BaseWidth = 13;
+
+  // Get the max width of BASE columns.
+  for (size_t StrIndex = 0; CurrentStr < TableEnd; ++StrIndex) {
+    size_t CurrentLen = strlen(CurrentStr);
+    CurrentStr += strlen(CurrentStr) + 1;
+    if (StrIndex % 3 == 1)
+      BaseWidth = std::max(BaseWidth, CurrentLen);
+  }
+
+  auto &OS = static_cast<formatted_raw_ostream &>(W.startLine());
+  // Each entry consists of 3 strings: the path_name, base_name and
+  // archive_member_name. The first entry is a default LIBPATH value and other
+  // entries have no path_name. We just dump the base_name and
+  // archive_member_name here.
+  OS << left_justify("BASE", BaseWidth)  << " MEMBER\n";
+  CurrentStr = ImportFileTable.data();
+  for (size_t StrIndex = 0; CurrentStr < TableEnd;
+       ++StrIndex, CurrentStr += strlen(CurrentStr) + 1) {
+    if (StrIndex >= 3 && StrIndex % 3 != 0) {
+      if (StrIndex % 3 == 1)
+        OS << "  " << left_justify(CurrentStr, BaseWidth) << " ";
+      else
+        OS << CurrentStr << "\n";
+    }
+  }
 }
 
 static const EnumEntry<XCOFF::SectionTypeFlags> SectionTypeFlagsNames[] = {
 #define ECase(X)                                                               \
   { #X, XCOFF::X }
     ECase(STYP_PAD),    ECase(STYP_DWARF), ECase(STYP_TEXT),
     ECase(STYP_DATA),   ECase(STYP_BSS),   ECase(STYP_EXCEPT),
     ECase(STYP_INFO),   ECase(STYP_TDATA), ECase(STYP_TBSS),