[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

836–837

849–854

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

2

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

2

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

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

 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!");
+  // TODO: Return the correct symbol type.
   return SymbolRef::ST_Other;
 }
 
 Expected<section_iterator>
 XCOFFObjectFile::getSymbolSection(DataRefImpl Symb) const {
   const int16_t SectNum = toSymbolRef(Symb).getSectionNumber();
 
   if (isReservedSectionNumber(SectNum))
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!");
+  // TODO: Return correct symbol flags.
   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;
 }

 Expected<XCOFFCsectAuxRef> XCOFFSymbolRef::getXCOFFCsectAuxRef() const {
   assert(isCsectSymbol() &&
          "Calling csect symbol interface with a non-csect symbol.");
 
   uint8_t NumberOfAuxEntries = getNumberOfAuxEntries();
 
   Expected<StringRef> NameOrErr = getName();
   if (auto Err = NameOrErr.takeError())
     return std::move(Err);
 
   if (!NumberOfAuxEntries) {
     return createStringError(object_error::parse_failed,
                              "csect symbol \"" + *NameOrErr +
                                  "\" contains no auxiliary entry");
   }
 
   if (!OwningObjectPtr->is64Bit()) {
     // In XCOFF32, the csect auxilliary entry is always the last auxiliary
     // entry for the symbol.
     uintptr_t AuxAddr = XCOFFObjectFile::getAdvancedSymbolEntryAddress(
         getEntryAddress(), NumberOfAuxEntries);
     return XCOFFCsectAuxRef(viewAs<XCOFFCsectAuxEnt32>(AuxAddr));
   }
 
   // XCOFF64 uses SymbolAuxType to identify the auxiliary entry type.
   // We need to iterate through all the auxiliary entries to find it.
   for (uint8_t Index = NumberOfAuxEntries; Index > 0; --Index) {
     uintptr_t AuxAddr = XCOFFObjectFile::getAdvancedSymbolEntryAddress(
         getEntryAddress(), Index);
     if (*OwningObjectPtr->getSymbolAuxType(AuxAddr) ==
         XCOFF::SymbolAuxType::AUX_CSECT) {
 #ifndef NDEBUG
       OwningObjectPtr->checkSymbolEntryPointer(AuxAddr);
 #endif
       return XCOFFCsectAuxRef(viewAs<XCOFFCsectAuxEnt64>(AuxAddr));

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

+# Test that the `llvm-objdump -l` can print correct line numbers.
+
+# 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