[llvm-objdump][XCOFF][AIX] Enable the -l (--line-numbers) option.

[Esme]

Summary

Added support for dumping line number information in llvm-objdump.

[Esme]

Updated test cases.

[MaskRay]

The llvm-objdump test looks good to me. Instead of # REQUIRES: powerpc-registered-target, you can add a lit.local.cfg with if not 'PowerPC' in config.root.targets:\n config.unsupported = True

[Esme]

Addressed Mask's comment, i.e added the lit.local.cfg.

[Higuoxing]

Only minor suggestions, otherwise LGTM. But please give somebody else a chance to review this change :)

llvm/lib/Object/XCOFFObjectFile.cpp

858–859

871–876

I would suggest putting the code block

if (!getNumberOfAuxEntries())
    return false;

in the front to avoid potential useless computation.

[Esme]

Thanks for your review @Higuoxing.

[jhenderson]

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test

1

Add a simple comment to the start of this file describing what it is testing.

[Esme]

In D101272#2728191, @jhenderson wrote:

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

[jhenderson]

In D101272#2747181, @Esme wrote:

In D101272#2728191, @jhenderson wrote:

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

For the parts that aren't required and can't be exercised at all, it seems like you should be able to just leave them out? For those parts that are required, you could add gtest unit testing to cover them. See llvm/unittest/Object for some examples.

[Esme]

In D101272#2747236, @jhenderson wrote:

In D101272#2747181, @Esme wrote:

In D101272#2728191, @jhenderson wrote:

There seem to be quite a few new code paths added in this patch, and I'm not convinced they are all tested? Please add the necessary testing.

It appears that the --line-numbers option can't cover these paths.
I would be very grateful if you have any suggestions for adding tests, or find any examples I can follow. Thanks!

For the parts that aren't required and can't be exercised at all, it seems like you should be able to just leave them out? For those parts that are required, you could add gtest unit testing to cover them. See llvm/unittest/Object for some examples.

The return values of these two hooks do not affect the result of line number. I removed these paths in the patch and will post another patch to implement them.

[jhenderson]

One nit remaining, otherwise looks good.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test

1

Ping this coment.

[Esme]

Addressed comment.

[jhenderson]

LGTM, once my last comment has been addressed.

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test

2

As per other comments, use ## for the marker. Also, no need to mention llvm-objdump (since we're in the llvm-objdump test directory).

llvm/include/llvm/Object/XCOFFObjectFile.h

public:
   bool isRelocatableObject() const override;
 
   // Below here is the non-inherited interface.
   bool is64Bit() const;
 
   const void *getPointerToSymbolTable() const { return SymbolTblPtr; }
 
   Expected<StringRef> getSymbolSectionName(XCOFFSymbolRef Ref) const;
-
+  unsigned getSymbolSectionID(SymbolRef Sym) const;
   XCOFFSymbolRef toSymbolRef(DataRefImpl Ref) const;
 
   // File header related interfaces.
   uint16_t getMagic() const;
   uint16_t getNumberOfSections() const;
   int32_t getTimeStamp() const;
 
   // Symbol table offset and entry count are handled differently between
#undef GETVALUE
 
   uintptr_t getEntryAddress() const {
     return Entry32 ? reinterpret_cast<uintptr_t>(Entry32)
                    : reinterpret_cast<uintptr_t>(Entry64);
   }
 
   Expected<StringRef> getName() const;
   bool isFunction() const;
+  bool isExternal() const;
+  bool isWeakExternal() const;
+  bool isUndefined() const;
+  bool isCommon() const;
+  bool isFileRecord() const;
+  bool isDebug() const;
+  bool isSectionDefinition() const;
+  bool isHidden() const;
+  bool isExported() const;
   bool isCsectSymbol() const;
   Expected<XCOFFCsectAuxRef> getXCOFFCsectAuxRef() const;
 
 private:
   const XCOFFObjectFile *OwningObjectPtr;
   const XCOFFSymbolEntry32 *Entry32 = nullptr;
   const XCOFFSymbolEntry64 *Entry64 = nullptr;
 };

llvm/lib/Object/SymbolSize.cpp

 //===- SymbolSize.cpp -----------------------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Object/SymbolSize.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/Wasm.h"
+#include "llvm/Object/XCOFFObjectFile.h"
 
 using namespace llvm;
 using namespace object;
 
 // Orders increasingly by (SectionID, Address).
 int llvm::object::compareAddress(const SymEntry *A, const SymEntry *B) {
   if (A->SectionID != B->SectionID)
     return A->SectionID < B->SectionID ? -1 : 1;
   if (A->Address != B->Address)
     return A->Address < B->Address ? -1 : 1;
   return 0;
 }
 
 static unsigned getSectionID(const ObjectFile &O, SectionRef Sec) {
   if (auto *M = dyn_cast<MachOObjectFile>(&O))
     return M->getSectionID(Sec);
   if (isa<WasmObjectFile>(&O))
     return Sec.getIndex();
-
+  if (isa<XCOFFObjectFile>(&O))
+    return Sec.getIndex();
   return cast<COFFObjectFile>(O).getSectionID(Sec);
 }
 
 static unsigned getSymbolSectionID(const ObjectFile &O, SymbolRef Sym) {
   if (auto *M = dyn_cast<MachOObjectFile>(&O))
     return M->getSymbolSectionID(Sym);
   if (const auto *M = dyn_cast<WasmObjectFile>(&O))
     return M->getSymbolSectionId(Sym);
+  if (const auto *M = dyn_cast<XCOFFObjectFile>(&O))
+    return M->getSymbolSectionID(Sym);
   return cast<COFFObjectFile>(O).getSymbolSectionID(Sym);
 }
 
 std::vector<std::pair<SymbolRef, uint64_t>>
 llvm::object::computeSymbolSizes(const ObjectFile &O) {
   std::vector<std::pair<SymbolRef, uint64_t>> Ret;
 
   if (const auto *E = dyn_cast<ELFObjectFileBase>(&O)) {

llvm/lib/Object/XCOFFObjectFile.cpp

 namespace llvm {
 
 using namespace XCOFF;
 
 namespace object {
 
 static const uint8_t FunctionSym = 0x20;
 static const uint16_t NoRelMask = 0x0001;
+static const uint32_t SymVisMask = 0xF000;
 static const size_t SymbolAuxTypeOffset = 17;
 
 // Checks that [Ptr, Ptr + Size) bytes fall inside the memory buffer
 // 'M'. Returns a pointer to the underlying object on success.
 template <typename T>
 static Expected<const T *> getObject(MemoryBufferRef M, const void *Ptr,
                                      const uint64_t Size = sizeof(T)) {
   uintptr_t Addr = reinterpret_cast<uintptr_t>(Ptr);

 uint64_t XCOFFObjectFile::getCommonSymbolSizeImpl(DataRefImpl Symb) const {
   uint64_t Result = 0;
   llvm_unreachable("Not yet implemented!");
   return Result;
 }
 
 Expected<SymbolRef::Type>
 XCOFFObjectFile::getSymbolType(DataRefImpl Symb) const {
-  llvm_unreachable("Not yet implemented!");
+  XCOFFSymbolRef XCOFFSymRef(Symb, this);
+  if (XCOFFSymRef.isUndefined())
+    return SymbolRef::ST_Unknown;
+  if (XCOFFSymRef.isCommon())
+    return SymbolRef::ST_Data;
+  if (XCOFFSymRef.isDebug() || XCOFFSymRef.isSectionDefinition())
+    return SymbolRef::ST_Debug;
+  if (XCOFFSymRef.isFileRecord())
+    return SymbolRef::ST_File;
+  if (XCOFFSymRef.isFunction())
+    return SymbolRef::ST_Function;
+
+  int16_t SecNum = XCOFFSymRef.getSectionNumber();
+  if (isReservedSectionNumber(SecNum))
+    return SymbolRef::ST_Data;
+
   return SymbolRef::ST_Other;
 }
 
 Expected<section_iterator>
 XCOFFObjectFile::getSymbolSection(DataRefImpl Symb) const {
   const int16_t SectNum = toSymbolRef(Symb).getSectionNumber();
 
   if (isReservedSectionNumber(SectNum))
void XCOFFObjectFile::getRelocationTypeName(
   if (is64Bit())
     report_fatal_error("64-bit support not implemented yet");
   const XCOFFRelocation32 *Reloc = viewAs<XCOFFRelocation32>(Rel.p);
   StringRef Res = XCOFF::getRelocationTypeString(Reloc->Type);
   Result.append(Res.begin(), Res.end());
 }
 
 Expected<uint32_t> XCOFFObjectFile::getSymbolFlags(DataRefImpl Symb) const {
-  uint32_t Result = 0;
-  llvm_unreachable("Not yet implemented!");
+  uint32_t Result = SymbolRef::SF_None;
+  XCOFFSymbolRef XCOFFSymRef(Symb, this);
+  if (XCOFFSymRef.isExternal() || XCOFFSymRef.isWeakExternal())
+    Result |= SymbolRef::SF_Global;
+  if (XCOFFSymRef.isWeakExternal() && XCOFFSymRef.getNumberOfAuxEntries())
+    Result |= SymbolRef::SF_Weak;
+  if (XCOFFSymRef.isUndefined())
+    Result |= SymbolRef::SF_Undefined;
+  if (XCOFFSymRef.isFileRecord() || XCOFFSymRef.isSectionDefinition())
+    Result |= SymbolRef::SF_FormatSpecific;
+  if (XCOFFSymRef.getSectionNumber() == XCOFF::N_ABS)
+    Result |= SymbolRef::SF_Absolute;
+  if (XCOFFSymRef.isCommon())
+    Result |= SymbolRef::SF_Common;
+  if (XCOFFSymRef.isHidden())
+    Result |= SymbolRef::SF_Hidden;
+  if (XCOFFSymRef.isExported())
+    Result |= SymbolRef::SF_Exported;
   return Result;
 }
 
 basic_symbol_iterator XCOFFObjectFile::symbol_begin() const {
   DataRefImpl SymDRI;
   SymDRI.p = reinterpret_cast<uintptr_t>(SymbolTblPtr);
   return basic_symbol_iterator(SymbolRef(SymDRI, this));
 }
default:
     Expected<DataRefImpl> SecRef = getSectionByNum(SectionNum);
     if (SecRef)
       return generateXCOFFFixedNameStringRef(
           getSectionNameInternal(SecRef.get()));
     return SecRef.takeError();
   }
 }
 
+unsigned XCOFFObjectFile::getSymbolSectionID(SymbolRef Sym) const {
+  XCOFFSymbolRef XCOFFSymRef(Sym.getRawDataRefImpl(), this);
+  return XCOFFSymRef.getSectionNumber();
+}
+
 bool XCOFFObjectFile::isReservedSectionNumber(int16_t SectionNumber) {
   return (SectionNumber <= 0 && SectionNumber >= -2);
 }
 
 uint16_t XCOFFObjectFile::getNumberOfSections() const {
   return is64Bit() ? fileHeader64()->NumberOfSections
                    : fileHeader32()->NumberOfSections;
 }
bool XCOFFSymbolRef::isFunction() const {
   if (!SI) {
     consumeError(SI.takeError());
     return false;
   }
 
   return (OwningObjectPtr->getSectionFlags(SI.get()) & XCOFF::STYP_TEXT);
 }
 
+bool XCOFFSymbolRef::isExternal() const {
+  return getStorageClass() == XCOFF::C_EXT;
+}
+
+bool XCOFFSymbolRef::isWeakExternal() const {
+  return getStorageClass() == XCOFF::C_WEAKEXT;
+}
+
+bool XCOFFSymbolRef::isUndefined() const {
+  return ((isExternal() && getSectionNumber() == XCOFF::N_UNDEF &&
+       getSymbolType() == 0) || isWeakExternal());
+}
+
+bool XCOFFSymbolRef::isCommon() const {
+  return (getStorageClass() == XCOFF::C_EXT &&
+          getSectionNumber() == XCOFF::N_UNDEF && getSymbolType() != 0);
+}
+
+bool XCOFFSymbolRef::isFileRecord() const {
+  return getStorageClass() == XCOFF::C_FILE;
+}
+
+bool XCOFFSymbolRef::isDebug() const {
+  return getSectionNumber() == XCOFF::N_DEBUG;
+}
+
+bool XCOFFSymbolRef::isSectionDefinition() const {
+  if (!getNumberOfAuxEntries())
+    return false;
+  bool isAppdomainGlobal =
+      getStorageClass() == XCOFF::C_EXT && getSectionNumber() == XCOFF::N_ABS;
+  bool isOrdinarySection = getStorageClass() == XCOFF::C_STAT;
+  return isAppdomainGlobal || isOrdinarySection;
+}
+
+bool XCOFFSymbolRef::isHidden() const {
+  return (getSymbolType() & SymVisMask) == XCOFF::SYM_V_HIDDEN;
+}
+
+bool XCOFFSymbolRef::isExported() const {
+  return (getSymbolType() & SymVisMask) == XCOFF::SYM_V_EXPORTED;
+}
+
 bool XCOFFSymbolRef::isCsectSymbol() const {
   XCOFF::StorageClass SC = getStorageClass();
   return (SC == XCOFF::C_EXT || SC == XCOFF::C_WEAKEXT ||
           SC == XCOFF::C_HIDEXT);
 }
 
 Expected<XCOFFCsectAuxRef> XCOFFSymbolRef::getXCOFFCsectAuxRef() const {
   assert(isCsectSymbol() &&

llvm/test/tools/llvm-objdump/XCOFF/disassemble-all.test

-# REQUIRES: powerpc-registered-target
-
 # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck %s
 
 # RUN: llvm-objdump -D -r %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --check-prefixes=CHECK,WITH-R %s
 
 ## xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 ## compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c

llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test

-# REQUIRES: powerpc-registered-target
-
 # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --check-prefixes=COMMON,PLAIN %s
 
 # RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --check-prefixes=COMMON,DESC %s
 
 # RUN: llvm-objdump -D -r --symbol-description %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --check-prefixes=COMMON,DESC,RELOC %s

llvm/test/tools/llvm-objdump/XCOFF/lit.local.cfg

+if not 'PowerPC' in config.root.targets:
+    config.unsupported = True

llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test

+# RUN: llvm-objdump -d -l %p/Inputs/basic32.o | FileCheck %s --check-prefix=LINES32
+# RUN: llvm-objdump -d -l %p/Inputs/basic64.o | FileCheck %s --check-prefix=LINES64
+
+## basic32.o Compiled with IBM XL C/C++ for AIX, V16.1.1.0
+## Compiler command: xlc basic.c -c -qdbgfmt=dwarf -g -q32 -o basic32.o
+##
+## basic64.o Compiled with IBM XL C/C++ for AIX, V16.1.1.0
+## Compiler command: xlc basic.c -c -qdbgfmt=dwarf -g -q64 -o basic64.o
+##
+## basic.c:
+## int main(void)
+## {
+##   return 0;
+## }
+
+# LINES32:       Inputs/basic32.o:  file format aixcoff-rs6000
+# LINES32:       Disassembly of section .text:
+# LINES32:       00000000 <.text>:
+# LINES32:       ; .main():
+# LINES32-NEXT:  ; /basic.c:1
+# LINES32-NEXT:         0: 38 60 00 00    li 3, 0
+# LINES32-NEXT:  ; /basic.c:3
+# LINES32-NEXT:         4: 48 00 00 04    b 0x8
+# LINES32-NEXT:  ; /basic.c:4
+# LINES32-NEXT:         8: 4e 80 00 20    blr
+# LINES32-NEXT:         c: 00 00 00 00    <unknown>
+# LINES32-NEXT:        10: 00 00 20 40    <unknown>
+# LINES32-NEXT:        14: 00 00 00 01    <unknown>
+# LINES32-NEXT:        18: 00 00 00 0c    <unknown>
+# LINES32-NEXT:        1c: 00 04 6d 61    <unknown>
+# LINES32-NEXT:        20: 69 6e 00 00    xori 14, 11, 0
+
+# LINES64:       Inputs/basic64.o:  file format aix5coff64-rs6000
+# LINES64:       Disassembly of section .text:
+# LINES64:       0000000000000000 <.main>:
+# LINES64:       ; .main():
+# LINES64-NEXT:  ; /basic.c:1
+# LINES64-NEXT:         0: 38 60 00 00    li 3, 0
+# LINES64-NEXT:  ; /basic.c:3
+# LINES64-NEXT:         4: 48 00 00 04    b 0x8
+# LINES64-NEXT:  ; /basic.c:4
+# LINES64-NEXT:         8: 4e 80 00 20    blr
+# LINES64-NEXT:         c: 00 00 00 00    <unknown>
+# LINES64-NEXT:        10: 00 00 20 40    <unknown>
+# LINES64-NEXT:        14: 00 00 00 01    <unknown>
+# LINES64-NEXT:        18: 00 00 00 0c    <unknown>
+# LINES64-NEXT:        1c: 00 04 6d 61    <unknown>
+# LINES64-NEXT:        20: 69 6e 00 00    xori 14, 11, 0

llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test

-# REQUIRES: powerpc-registered-target
-
 # RUN: llvm-objdump -r %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --match-full-lines --strict-whitespace %s
 
 # RUN: llvm-objdump -r --symbol-description %p/Inputs/xcoff-section-headers.o | \
 # RUN:   FileCheck --match-full-lines --strict-whitespace --check-prefix=DESCP %s
 
 ## xcoff-section-headers.o Compiled with IBM XL C/C++ for AIX, V16.1.0
 ## compiler command: xlc -qtls -o xcoff-section-headers.o -c test.c