[XCOFF] Improve error message context.

[Esme]

Summary

This patch is trying to report error messages in more detail for XCOFF interfaces.

[qiucf]

Can I assume no tests will be affected if this is a refactor patch?

llvm/lib/Object/XCOFFObjectFile.cpp

46

Seems inline here not necessary.

[Higuoxing]

The code change generally looks good to me. But please make the error messages consistent, see my inline comments.

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

8

22

[jhenderson]

In D110320#3019605, @qiucf wrote:

Can I assume no tests will be affected if this is a refactor patch?

@Esme, the patch summary needs changing. "Refactor" implies there is no observable difference in behaviour. Perhaps "Improve error message context".

In D110320#3019656, @Higuoxing wrote:

The code change generally looks good to me. But please make the error messages consistent, see my inline comments.

See my inline comment re. error message styles. Unfortunately, some errors and error code messages are using the wrong style, so this can make things look inconsistent. Stretch goal for someone who has the time is to fix that!

llvm/lib/Object/XCOFFObjectFile.cpp

46

+1 inline is only necessary in header files.

842

(or "number of symbols")

924–927

": relocation data with ... goes past" or ": relocations with ... go past"

1000

Slightly surprised to see the name written as "ImportTable" here, but you don't use e.g. "StringTable". Is "ImportTable" how it is written in the XCOFF spec? What about string tables?

1007–1011

Add offset and size here too.

1046–1049

"section header table ... goes past" or "section headers ... go past"

1172–1175

Can there be more than one symbol with the same name? If so, you need the symbol index too.

Also "is not found" -> "has not been found"

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

Don't do this. Error messages should not have a leading capital letter as per the style guide.

Do consider prefixing this check with "error:", e.g. "error: the section index ..."

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

8

Ditto.

22

Ditto.

[Higuoxing]

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

Sorry for the incorrect comments. TIL.

[jhenderson]

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

It's okay! It would have been a good catch if there weren't a style guide rule :)

[Esme]

Address comments.

llvm/lib/Object/XCOFFObjectFile.cpp

1000

It's called Import File ID Name Table in the XCOFF spec. To make messages consistent with string table, I will use import file name table here, what do you think?

[jhenderson]

llvm/lib/Object/XCOFFObjectFile.cpp

841–843

Don't add "error:" to the error message text here. If the tool is using the correct LLVM functions for reporting errors, this will be added automatically. Otherwise, it will either have a good reason not to add it at the point where the error is actually printed (e.g. because the tool is matching the format of an existing tool, such as GNU binutils), or it needs fixing at that point. It definitely shouldn't be fixed at this low a level.

1000

Without knowing the content, it's a little tricky to suggest what makes sense. What is actually in the table?

1007–1011

Looks like a copy/paste error here?

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

Do consider prefixing this check with "error:", e.g. "error: the section index ..."

I was specifically referring to test checks in general, i.e. when checking warnings and errors, check for any "warning:"/"error:" that should be present.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

8

This is an example of where I'd expect there to be "error:" before the message. Don't update obj2yaml in this patch though if the prefix is missing.

22

See my other inline comments.

[Esme]

Address comments.

llvm/lib/Object/XCOFFObjectFile.cpp

841–843

Oh...I See. Sorry that I misunderstood your comments.

1000

The Import File ID Name Table is a part of the loader section as in the spec: https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__vra3i31ejbau

Loader Import File ID Name Table Definition:
The loader section import file ID name strings of a module provide a list of dependent modules that the system loader must load in order for the module to load successfully. However, this list does not contain the names of modules that the named modules depend on.

Each entry in the import file ID name table consists of:
Import file ID path name
Null delimiter (ASCII null character)
Import file ID base name
Null delimiter
Import file ID archive-file-member name
Null delimiter

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

So the format of warning: '[[FILE]]': symbol index 33 exceeds symbol count 0 should be fine? It seems that the warning/error messages for ELF, such as llvm/test/tools/llvm-readobj/ELF/symtab-shndx.test, follow this format as well.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

8

The prefix in the output does exist, but I missed checking it.

[jhenderson]

LGTM, aside from a possible new name as suggested inline.

Also, remove "trying" from the patch description - either the patch does something or it doesn't. There is no try.

llvm/lib/Object/XCOFFObjectFile.cpp

1000

Okay, how about simply "import file table"?

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

26

Yeah, exactly. Here, you have the prefix check.

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

8

Thanks. Looks like obj2yaml doesn't use the standard LLVM functions for reporting errors. Perhaps something to change in the future, if you fancy it, so that the output is simply "error: [[FILE]]: ...".

[teemperor]

The createError function introduces here breaks the LLVM_ENABLE_MODULES build:

/Users/teemperor/5llvm/llvm-project/llvm/lib/Object/XCOFFObjectFile.cpp:46:14: error: definition with same mangled name '_ZN4llvm6objectL11createErrorERKNS_5TwineE' as another definition
static Error createError(const Twine &Err) {
             ^
/Users/teemperor/5llvm/llvm-project/llvm/include/llvm/Object/ELF.h:84:21: note: previous definition is here
static inline Error createError(const Twine &Err) {
                    ^
1 error generated.

Could we maybe move both definitions into some utility header (both functions seem to do the same thing)?

[teemperor]

D111541 should address the issue.

llvm/lib/Object/XCOFFObjectFile.cpp

static Expected<const T *> getObject(MemoryBufferRef M, const void *Ptr,
   return reinterpret_cast<const T *>(Addr);
 }
 
 static uintptr_t getWithOffset(uintptr_t Base, ptrdiff_t Offset) {
   return reinterpret_cast<uintptr_t>(reinterpret_cast<const char *>(Base) +
                                      Offset);
 }
 
+static Error createError(const Twine &Err) {
+  return make_error<StringError>(Err, object_error::parse_failed);
+}
+
 template <typename T> static const T *viewAs(uintptr_t in) {
   return reinterpret_cast<const T *>(in);
 }
 
 static StringRef generateXCOFFFixedNameStringRef(const char *Name) {
   auto NulCharPtr =
       static_cast<const char *>(memchr(Name, '\0', XCOFF::NameSize));
   return NulCharPtr ? StringRef(Name, NulCharPtr - Name)
XCOFFObjectFile::getStringTableEntry(uint32_t Offset) const {
   // field; as a soft-error recovery mechanism, we treat such cases as having an
   // offset of 0.
   if (Offset < 4)
     return StringRef(nullptr, 0);
 
   if (StringTable.Data != nullptr && StringTable.Size > Offset)
     return (StringTable.Data + Offset);
 
-  return make_error<GenericBinaryError>("Bad offset for string table entry",
-                                        object_error::parse_failed);
+  return createError("entry with offset 0x" + Twine::utohexstr(Offset) +
+                     " in a string table with size 0x" +
+                     Twine::utohexstr(StringTable.Size) + " is invalid");
 }
 
 StringRef XCOFFObjectFile::getStringTable() const {
   // If the size is less than or equal to 4, then the string table contains no
   // string data.
   return StringRef(StringTable.Data,
                    StringTable.Size <= 4 ? 0 : StringTable.Size);
 }
if (is64Bit())
     OffsetToRaw = toSection64(Sec)->FileOffsetToRawData;
   else
     OffsetToRaw = toSection32(Sec)->FileOffsetToRawData;
 
   const uint8_t * ContentStart = base() + OffsetToRaw;
   uint64_t SectionSize = getSectionSize(Sec);
   if (Error E = Binary::checkOffset(
           Data, reinterpret_cast<uintptr_t>(ContentStart), SectionSize))
-    return std::move(E);
+    return createError(
+        toString(std::move(E)) + ": section data with offset 0x" +
+        Twine::utohexstr(OffsetToRaw) + " and size 0x" +
+        Twine::utohexstr(SectionSize) + " goes past the end of the file");
 
   return makeArrayRef(ContentStart,SectionSize);
 }
 
 uint64_t XCOFFObjectFile::getSectionAlignment(DataRefImpl Sec) const {
   uint64_t Result = 0;
   llvm_unreachable("Not yet implemented!");
   return Result;
Expected<uintptr_t> XCOFFObjectFile::getLoaderSectionAddress() const {
   // 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 createError(toString(std::move(E)) +
+                       ": loader section with offset 0x" +
+                       Twine::utohexstr(OffsetToLoaderSection) +
+                       " and size 0x" + Twine::utohexstr(SizeOfLoaderSection) +
+                       " goes past the end of the file");
+
   return LoderSectionStart;
 }
 
 bool XCOFFObjectFile::isSectionCompressed(DataRefImpl Sec) const {
   return false;
 }
 
 bool XCOFFObjectFile::isSectionText(DataRefImpl Sec) const {
return getAdvancedSymbolEntryAddress(
       reinterpret_cast<uintptr_t>(getPointerToSymbolTable()), Index);
 }
 
 Expected<StringRef>
 XCOFFObjectFile::getSymbolNameByIndex(uint32_t Index) const {
   const uint32_t NumberOfSymTableEntries = getNumberOfSymbolTableEntries();
 
   if (Index >= NumberOfSymTableEntries)
-    return errorCodeToError(object_error::invalid_symbol_index);
+    return createError("symbol index " + Twine(Index) +
+                       " exceeds symbol count " +
+                       Twine(NumberOfSymTableEntries));
 
   DataRefImpl SymDRI;
   SymDRI.p = getSymbolEntryAddressByIndex(Index);
   return getSymbolName(SymDRI);
 }
 
 uint16_t XCOFFObjectFile::getFlags() const {
   return is64Bit() ? fileHeader64()->Flags : fileHeader32()->Flags;
Expected<ArrayRef<Reloc>> XCOFFObjectFile::relocations(const Shdr &Sec) const {
 
   uint32_t NumRelocEntries = NumRelocEntriesOrErr.get();
   static_assert((sizeof(Reloc) == XCOFF::RelocationSerializationSize64 ||
                  sizeof(Reloc) == XCOFF::RelocationSerializationSize32),
                 "Relocation structure is incorrect");
   auto RelocationOrErr =
       getObject<Reloc>(Data, reinterpret_cast<void *>(RelocAddr),
                        NumRelocEntries * sizeof(Reloc));
-  if (Error E = RelocationOrErr.takeError())
-    return std::move(E);
+  if (!RelocationOrErr)
+    return createError(
+        toString(RelocationOrErr.takeError()) + ": relocations with offset 0x" +
+        Twine::utohexstr(Sec.FileOffsetToRelocationInfo) + " and size 0x" +
+        Twine::utohexstr(NumRelocEntries * sizeof(Reloc)) +
+        " go past the end of the file");
 
   const Reloc *StartReloc = RelocationOrErr.get();
 
   return ArrayRef<Reloc>(StartReloc, StartReloc + NumRelocEntries);
 }
 
 Expected<XCOFFStringTable>
 XCOFFObjectFile::parseStringTable(const XCOFFObjectFile *Obj, uint64_t Offset) {
XCOFFObjectFile::parseStringTable(const XCOFFObjectFile *Obj, uint64_t Offset) {
 
   // If the size is less then 4, then the string table is just a size and no
   // string data.
   if (Size <= 4)
     return XCOFFStringTable{4, nullptr};
 
   auto StringTableOrErr =
       getObject<char>(Obj->Data, Obj->base() + Offset, Size);
-  if (Error E = StringTableOrErr.takeError())
-    return std::move(E);
+  if (!StringTableOrErr)
+    return createError(toString(StringTableOrErr.takeError()) +
+                       ": string table with offset 0x" +
+                       Twine::utohexstr(Offset) + " and size 0x" +
+                       Twine::utohexstr(Size) +
+                       " goes past the end of the file");
 
   const char *StringTablePtr = StringTableOrErr.get();
   if (StringTablePtr[Size - 1] != '\0')
     return errorCodeToError(object_error::string_table_non_null_end);
 
   return XCOFFStringTable{Size, StringTablePtr};
 }
 
if (is64Bit()) {
     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);
+  if (!ImportTableOrErr)
+    return createError(
+        toString(ImportTableOrErr.takeError()) +
+        ": import file table with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
+        " and size 0x" + Twine::utohexstr(LengthOfImportFileTable) +
+        " goes past the end of the file");
 
   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 createError(
+        ": import file name table with offset 0x" +
+        Twine::utohexstr(LoaderSectionAddr + OffsetToImportFileTable) +
+        " and size 0x" + Twine::utohexstr(LengthOfImportFileTable) +
+        " 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;
XCOFFObjectFile::create(unsigned Type, MemoryBufferRef MBR) {
 
   CurOffset += Obj->getFileHeaderSize();
   // TODO FIXME we don't have support for an optional header yet, so just skip
   // past it.
   CurOffset += Obj->getOptionalHeaderSize();
 
   // Parse the section header table if it is present.
   if (Obj->getNumberOfSections()) {
-    auto SecHeadersOrErr = getObject<void>(Data, Base + CurOffset,
-                                           Obj->getNumberOfSections() *
-                                               Obj->getSectionHeaderSize());
-    if (Error E = SecHeadersOrErr.takeError())
-      return std::move(E);
+    uint64_t SectionHeadersSize =
+        Obj->getNumberOfSections() * Obj->getSectionHeaderSize();
+    auto SecHeadersOrErr =
+        getObject<void>(Data, Base + CurOffset, SectionHeadersSize);
+    if (!SecHeadersOrErr)
+      return createError(toString(SecHeadersOrErr.takeError()) +
+                         ": section headers with offset 0x" +
+                         Twine::utohexstr(CurOffset) + " and size 0x" +
+                         Twine::utohexstr(SectionHeadersSize) +
+                         " go past the end of the file");
+
     Obj->SectionHeaderTable = SecHeadersOrErr.get();
   }
 
   const uint32_t NumberOfSymbolTableEntries =
       Obj->getNumberOfSymbolTableEntries();
 
   // If there is no symbol table we are done parsing the memory buffer.
   if (NumberOfSymbolTableEntries == 0)
     return std::move(Obj);
 
   // Parse symbol table.
   CurOffset = Obj->is64Bit() ? Obj->getSymbolTableOffset64()
                              : Obj->getSymbolTableOffset32();
   const uint64_t SymbolTableSize =
       static_cast<uint64_t>(XCOFF::SymbolTableEntrySize) *
       NumberOfSymbolTableEntries;
   auto SymTableOrErr =
       getObject<void *>(Data, Base + CurOffset, SymbolTableSize);
-  if (Error E = SymTableOrErr.takeError())
-    return std::move(E);
+  if (!SymTableOrErr)
+    return createError(
+        toString(SymTableOrErr.takeError()) + ": symbol table with offset 0x" +
+        Twine::utohexstr(CurOffset) + " and size 0x" +
+        Twine::utohexstr(SymbolTableSize) + " goes past the end of the file");
+
   Obj->SymbolTblPtr = SymTableOrErr.get();
   CurOffset += SymbolTableSize;
 
   // Parse String table.
   Expected<XCOFFStringTable> StringTableOrErr =
       parseStringTable(Obj.get(), CurOffset);
   if (Error E = StringTableOrErr.takeError())
     return std::move(E);
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);
 
+  uint32_t SymbolIdx = OwningObjectPtr->getSymbolIndex(getEntryAddress());
   if (!NumberOfAuxEntries) {
-    return createStringError(object_error::parse_failed,
-                             "csect symbol \"" + *NameOrErr +
-                                 "\" contains no auxiliary entry");
+    return createError("csect symbol \"" + *NameOrErr + "\" with index " +
+                       Twine(SymbolIdx) + " 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));
     }
   }
 
-  return createStringError(
-      object_error::parse_failed,
-      "a csect auxiliary entry is not found for symbol \"" + *NameOrErr + "\"");
+  return createError(
+      "a csect auxiliary entry has not been found for symbol \"" + *NameOrErr +
+      "\" with index " + Twine(SymbolIdx));
 }
 
 Expected<StringRef> XCOFFSymbolRef::getName() const {
   // A storage class value with the high-order bit on indicates that the name is
   // a symbolic debugger stabstring.
   if (getStorageClass() & 0x80)
     return StringRef("Unimplemented Debug Name");
 
   if (Entry32) {

llvm/test/tools/llvm-objdump/XCOFF/section-headers.test

- Name:  .dwpbtyp
     Flags: [ STYP_PAD ]
 
 ## Truncate the file to end before the section header table ends.
 # RUN: yaml2obj %s --docnum=3 -o %t-truncate.o
 # RUN: %python -c "with open('%/t-truncate.o', 'r+b') as input: input.truncate(20)"
 # RUN: not llvm-objdump --section-headers %t-truncate.o 2>&1 \
 # RUN:  | FileCheck --check-prefix=ERROR %s
 
-# ERROR: The end of the file was unexpectedly encountered
+# ERROR: The end of the file was unexpectedly encountered: section headers with offset 0x14 and size 0x28 go past the end of the file
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x1DF
 Sections:
   - Name:  .text
     Size:  0x8
     Flags: [ STYP_TEXT ]

llvm/test/tools/llvm-readobj/XCOFF/relocations-invalid.test

 ## Check we report warnings when relocations are broken.
 
 # RUN: yaml2obj --docnum=1 %s -o %t1
 # RUN: llvm-readobj --relocs --expand-relocs %t1 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t1 --check-prefix=INVALID-REL
 
 # INVALID-REL:      Relocations [
-# INVALID-REL-NEXT:   warning: '[[FILE]]': The end of the file was unexpectedly encountered
+# INVALID-REL-NEXT:   warning: '[[FILE]]': The end of the file was unexpectedly encountered: relocations with offset 0x222 and size 0x0 go past the end of the file
 # INVALID-REL-NEXT: ]
 
 --- !XCOFF
 FileHeader:
   MagicNumber:   0x1DF
 Sections:
   - Name:        .text
     Flags:       [ STYP_TEXT ]
 ## Case 1: Incorrect offset to relocation entries.
     FileOffsetToRelocations: 0x222
 
 # RUN: yaml2obj --docnum=2 %s -o %t2
 # RUN: llvm-readobj --relocs --expand-relocs %t2 2>&1 | \
 # RUN:   FileCheck %s -DFILE=%t2 --check-prefix=INVALID-SYM
 
 # INVALID-SYM:      Relocations [
 # INVALID-SYM-NEXT:   Section (index: 1) .text {
-# INVALID-SYM-NEXT:     warning: '[[FILE]]': Invalid symbol index
+# INVALID-SYM-NEXT:     warning: '[[FILE]]': symbol index 33 exceeds symbol count 0
 # INVALID-SYM-NEXT:   }
 # INVALID-SYM-NEXT: ]
 
 --- !XCOFF
 FileHeader:
   MagicNumber:   0x1DF
 Sections:
   - Name:        .text
     Flags:       [ STYP_TEXT ]
     Relocations:
       - Address: 0x80
 ## Case 2: There is no symbol with index 0x21.
         Symbol:  0x21

llvm/test/tools/obj2yaml/XCOFF/invalid-section.yaml

 ## Check that obj2yaml reports a suitable error when it encounters
 ## invalid content in Section.
 
 ## Error1: failed to get section data.
 # RUN: yaml2obj %s --docnum=1 -o %t1
 # RUN: not obj2yaml %t1 2>&1 | FileCheck %s -DFILE=%t1 --check-prefix=ERROR1
 
-# ERROR1: The end of the file was unexpectedly encountered
+# ERROR1: The end of the file was unexpectedly encountered: section data with offset 0x70 and size 0x4 goes past the end of the file
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x01DF
 Sections:
   - SectionData:      '00007400'
     FileOffsetToData: 0x70
 
 ## Error2: failed to get relocations.
 # RUN: yaml2obj %s --docnum=2 -o %t2
 # RUN: not obj2yaml %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=ERROR2
 
-# ERROR2: The end of the file was unexpectedly encountered
+# ERROR2: The end of the file was unexpectedly encountered: relocations with offset 0x3c and size 0x1e go past the end of the file
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x01DF
 Sections:
   - NumberOfRelocations: 0x3
     Relocations:
       - Address: 0xE
         Symbol:  0x12
         Info:    0xF
         Type:    0x3

llvm/test/tools/obj2yaml/XCOFF/invalid-symbol.yaml

 ## Check that obj2yaml reports a suitable error when it encounters
 ## invalid content in Symbol.
 
 ## Error1: failed to get the section name for a symbol.
 # RUN: yaml2obj %s --docnum=1 -o %t1
-# RUN: not obj2yaml %t1 2>&1 | FileCheck %s --check-prefix=ERROR1
+# RUN: not obj2yaml %t1 2>&1 | FileCheck %s -DFILE=%t1 --check-prefix=ERROR1
 
-# ERROR1: Invalid section index
+# ERROR1: Error reading file: [[FILE]]: the section index (2) is invalid
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x01DF
 Sections:
   - Name: .text
 Symbols:
   - SectionIndex: 2
 
 ## Error2: failed to get the symbol name.
 # RUN: yaml2obj %s --docnum=2 -o %t2
-# RUN: not obj2yaml %t2 2>&1 | FileCheck %s --check-prefix=ERROR2
+# RUN: not obj2yaml %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=ERROR2
 
-# ERROR2: Invalid data was encountered while parsing the file
+# ERROR2: Error reading file: [[FILE]]: entry with offset 0x4 in a string table with size 0x4 is invalid
 
 --- !XCOFF
 FileHeader:
   MagicNumber: 0x01DF
 Symbols:
   - Name: nameInStrTbl
 StringTable:
   Length: 0

llvm/tools/obj2yaml/obj2yaml.h

 std::error_code coff2yaml(llvm::raw_ostream &Out,
                           const llvm::object::COFFObjectFile &Obj);
 llvm::Error elf2yaml(llvm::raw_ostream &Out,
                          const llvm::object::ObjectFile &Obj);
 llvm::Error macho2yaml(llvm::raw_ostream &Out,
                            const llvm::object::Binary &Obj);
 llvm::Error minidump2yaml(llvm::raw_ostream &Out,
                           const llvm::object::MinidumpFile &Obj);
-std::error_code xcoff2yaml(llvm::raw_ostream &Out,
-                           const llvm::object::XCOFFObjectFile &Obj);
+llvm::Error xcoff2yaml(llvm::raw_ostream &Out,
 std::error_code wasm2yaml(llvm::raw_ostream &Out,
                           const llvm::object::WasmObjectFile &Obj);
 llvm::Error archive2yaml(llvm::raw_ostream &Out, llvm::MemoryBufferRef Source);
 
 // Forward decls for dwarf2yaml
 namespace llvm {
 class DWARFContext;
 namespace DWARFYAML {

llvm/tools/obj2yaml/obj2yaml.cpp

 using namespace llvm;
 using namespace llvm::object;
 
 static Error dumpObject(const ObjectFile &Obj) {
   if (Obj.isCOFF())
     return errorCodeToError(coff2yaml(outs(), cast<COFFObjectFile>(Obj)));
 
   if (Obj.isXCOFF())
-    return errorCodeToError(xcoff2yaml(outs(), cast<XCOFFObjectFile>(Obj)));
+    return xcoff2yaml(outs(), cast<XCOFFObjectFile>(Obj));
 
   if (Obj.isELF())
     return elf2yaml(outs(), Obj);
 
   if (Obj.isWasm())
     return errorCodeToError(wasm2yaml(outs(), cast<WasmObjectFile>(Obj)));
 
   llvm_unreachable("unexpected object file format");

llvm/tools/obj2yaml/xcoff2yaml.cpp

for (const SymbolRef &S : Obj.symbols()) {
     Sym.StorageClass = SymbolEntRef.getStorageClass();
     Sym.NumberOfAuxEntries = SymbolEntRef.getNumberOfAuxEntries();
     Symbols.push_back(Sym);
   }
 
   return Error::success();
 }
 
-std::error_code xcoff2yaml(raw_ostream &Out,
-                           const object::XCOFFObjectFile &Obj) {
+Error xcoff2yaml(raw_ostream &Out, const object::XCOFFObjectFile &Obj) {
   XCOFFDumper Dumper(Obj);
 
   if (Error E = Dumper.dump())
-    return errorToErrorCode(std::move(E));
+    return E;
 
   yaml::Output Yout(Out);
   Yout << Dumper.getYAMLObj();
 
-  return std::error_code();
+  return Error::success();
 }

llvm/unittests/Object/XCOFFObjectFileTest.cpp

TEST(XCOFFObjectFileTest, XCOFFGetCsectAuxRef32) {
   Expected<XCOFFCsectAuxRef> CsectRefOrErr = SymRef.getXCOFFCsectAuxRef();
   ASSERT_THAT_EXPECTED(CsectRefOrErr, Succeeded());
 
   // Set csect symbol's auxiliary entry count to 0.
   XCOFF32Binary[113] = 0;
   Expected<XCOFFCsectAuxRef> ExpectErr = SymRef.getXCOFFCsectAuxRef();
   EXPECT_THAT_ERROR(
       ExpectErr.takeError(),
-      FailedWithMessage("csect symbol \".data\" contains no auxiliary entry"));
+      FailedWithMessage(
+          "csect symbol \".data\" with index 2 contains no auxiliary entry"));
 }
 
 TEST(XCOFFObjectFileTest, XCOFFGetCsectAuxRef64) {
   uint8_t XCOFF64Binary[] = {
       // File header.
       0x01, 0xf7, 0x00, 0x01, 0x5f, 0x59, 0x25, 0xeb, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x60, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x04,
 
TEST(XCOFFObjectFileTest, XCOFFGetCsectAuxRef64) {
   Expected<XCOFFCsectAuxRef> CsectRefOrErr = SymRef.getXCOFFCsectAuxRef();
   ASSERT_THAT_EXPECTED(CsectRefOrErr, Succeeded());
 
   // Inject incorrect auxiliary type value.
   XCOFF64Binary[167] = static_cast<uint8_t>(XCOFF::AUX_SYM);
   Expected<XCOFFCsectAuxRef> NotFoundErr = SymRef.getXCOFFCsectAuxRef();
   EXPECT_THAT_ERROR(
       NotFoundErr.takeError(),
-      FailedWithMessage(
-          "a csect auxiliary entry is not found for symbol \".data\""));
+      FailedWithMessage("a csect auxiliary entry has not been found for symbol "
+                        "\".data\" with index 2"));
 
   // Set csect symbol's auxiliary entry count to 0.
   XCOFF64Binary[149] = 0;
   Expected<XCOFFCsectAuxRef> ExpectErr = SymRef.getXCOFFCsectAuxRef();
   EXPECT_THAT_ERROR(
       ExpectErr.takeError(),
-      FailedWithMessage("csect symbol \".data\" contains no auxiliary entry"));
+      FailedWithMessage(
+          "csect symbol \".data\" with index 2 contains no auxiliary entry"));
 }
 
 TEST(XCOFFObjectFileTest, XCOFFTracebackTableErrorAtParameterType) {
   const uint8_t TBTableData[] = {0x00, 0x00, 0x22, 0x40, 0x80, 0x00, 0x01,
                                  0x05, 0x58, 0x00, 0x10, 0x00, 0x00, 0x00,
                                  0x00, 0x40, 0x00, 0x07, 0x61, 0x64, 0x64,
                                  0x5f, 0x61, 0x6c, 0x6c, 0x00, 0x00, 0x00};
   uint64_t Size = 28;