[Object] [COFF] Improve error messages

[mstorsjo]

Summary

This aids debugging when working with possibly broken files,
instead of just flat out erroring out without telling what's wrong.

Not sure if it's relevant with tests for this?

[rnk]

lgtm

If there are existing tests, it would be nice to check for additional context, but I don't think it makes sense to add coverage for every corner case here.

[mstorsjo]

Update a test that checks an error message.

[jhenderson]

Thanks for the improvement. Whilst I get what @rnk's saying about testing, I've learnt from past experience that if an error message is not tested, there's a non-trivial chance that the message is garbled in some way, e.g. due to incorrect format strings, or using wrong parameters. I'd personally prefer seeing a test for each case that has the additional new context (including, where possible, using the actual values passed in, rather than regexes), at least if there's already testing for the error path.

llvm/lib/Object/COFFObjectFile.cpp

488

Here and below, do you want the "0x" prefix?

490

No need for else after return.

517

No need for else after return.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test

107

For the filename parameter, I'd consider passing in the filename as a FileCheck variable, although I acknowledge it didn't before, so am not too fussed by this.

For the RVA parameter, if you don't want to hard-code in the value, I'd use a FileCheck numeric variable to ensure that the value is actually an expected number. A possible style is in the inline edit, but you may want a tighter restriction etc.

[mstorsjo]

In D120679#3350720, @jhenderson wrote:

Thanks for the improvement. Whilst I get what @rnk's saying about testing, I've learnt from past experience that if an error message is not tested, there's a non-trivial chance that the message is garbled in some way, e.g. due to incorrect format strings, or using wrong parameters. I'd personally prefer seeing a test for each case that has the additional new context (including, where possible, using the actual values passed in, rather than regexes), at least if there's already testing for the error path.

While I agree that it would be ideal to have tests for every single case, that’s way out of scope for what I meant to spend effort on here. If that’s required, I’ll just drop this patch - that’s not what I set out to do.

I’m not changing any error handling part, I’m just adding unique strings to all textless cases of object_error::parse_failed in the file, so that if such a message gets printed, it can be mapped back to the code unambiguously. And we happen to have one test that triggers one of the changed messages, so we actually do get the formatting tested.

Some of the errors might even be impossible to trigger, if it’s just an internal consistency check that is guarded by an outer condition too. I’m not planning on investigating if it’s possible to trigger them all, and how to (many of them might require handcrafting broken binaries), I’m just adding messages.

llvm/lib/Object/COFFObjectFile.cpp

488

Sure, that’s probably better.

490

Ok, sure.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test

107

Ok, I guess we can add the literal address, as it’s probably constant.

[jhenderson]

In D120679#3350849, @mstorsjo wrote:

While I agree that it would be ideal to have tests for every single case, that’s way out of scope for what I meant to spend effort on here. If that’s required, I’ll just drop this patch - that’s not what I set out to do.

I’m not changing any error handling part, I’m just adding unique strings to all textless cases of object_error::parse_failed in the file, so that if such a message gets printed, it can be mapped back to the code unambiguously. And we happen to have one test that triggers one of the changed messages, so we actually do get the formatting tested.

Some of the errors might even be impossible to trigger, if it’s just an internal consistency check that is guarded by an outer condition too. I’m not planning on investigating if it’s possible to trigger them all, and how to (many of them might require handcrafting broken binaries), I’m just adding messages.

Just to be clear, I'm not suggesting adding new tests where they don't exist, only enhancing existing testing to be a little stricter (if there is such testing). If such testing doesn't exist, then what you've done is fine.

llvm/test/tools/llvm-readobj/COFF/tls-directory.test

107

I'm mostly interested in getting rid of the naked wildcard regex, since it could hide dodgy content.

[mstorsjo]

Added an 0x prefix to the RVAs, removed the else statement after a return, checking explicitly for the literal RVA and file name in the test.

[jhenderson]

LGTM.

llvm/include/llvm/Object/COFF.h

public:
 
   Expected<StringRef> getSectionName(const coff_section *Sec) const;
   uint64_t getSectionSize(const coff_section *Sec) const;
   Error getSectionContents(const coff_section *Sec,
                            ArrayRef<uint8_t> &Res) const;
 
   uint64_t getImageBase() const;
   Error getVaPtr(uint64_t VA, uintptr_t &Res) const;
-  Error getRvaPtr(uint32_t Rva, uintptr_t &Res) const;
+  Error getRvaPtr(uint32_t Rva, uintptr_t &Res,
+                  const char *ErrorContext = nullptr) const;
 
   /// Given an RVA base and size, returns a valid array of bytes or an error
   /// code if the RVA and size is not contained completely within a valid
   /// section.
   Error getRvaAndSizeAsBytes(uint32_t RVA, uint32_t Size,
-                             ArrayRef<uint8_t> &Contents) const;
+                             ArrayRef<uint8_t> &Contents,
+                             const char *ErrorContext = nullptr) const;
 
   Error getHintName(uint32_t Rva, uint16_t &Hint,
                               StringRef &Name) const;
 
   /// Get PDB information out of a codeview debug directory entry.
   Error getDebugPDBInfo(const debug_directory *DebugDir,
                         const codeview::DebugInfo *&Info,
                         StringRef &PDBFileName) const;

llvm/lib/Object/COFFObjectFile.cpp

Error COFFObjectFile::initSymbolTablePtr() {
 
   // Treat table sizes < 4 as empty because contrary to the PECOFF spec, some
   // tools like cvtres write a size of 0 for an empty table instead of 4.
   if (StringTableSize < 4)
     StringTableSize = 4;
 
   // Check that the string table is null terminated if has any in it.
   if (StringTableSize > 4 && StringTable[StringTableSize - 1] != 0)
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed,
+                             "string table missing null terminator");
   return Error::success();
 }
 
 uint64_t COFFObjectFile::getImageBase() const {
   if (PE32Header)
     return PE32Header->ImageBase;
   else if (PE32PlusHeader)
     return PE32PlusHeader->ImageBase;
   // This actually comes up in practice.
   return 0;
 }
 
 // Returns the file offset for the given VA.
 Error COFFObjectFile::getVaPtr(uint64_t Addr, uintptr_t &Res) const {
   uint64_t ImageBase = getImageBase();
   uint64_t Rva = Addr - ImageBase;
   assert(Rva <= UINT32_MAX);
   return getRvaPtr((uint32_t)Rva, Res);
 }
 
 // Returns the file offset for the given RVA.
-Error COFFObjectFile::getRvaPtr(uint32_t Addr, uintptr_t &Res) const {
+Error COFFObjectFile::getRvaPtr(uint32_t Addr, uintptr_t &Res,
+                                const char *ErrorContext) const {
   for (const SectionRef &S : sections()) {
     const coff_section *Section = getCOFFSection(S);
     uint32_t SectionStart = Section->VirtualAddress;
     uint32_t SectionEnd = Section->VirtualAddress + Section->VirtualSize;
     if (SectionStart <= Addr && Addr < SectionEnd) {
       uint32_t Offset = Addr - SectionStart;
       Res = reinterpret_cast<uintptr_t>(base()) + Section->PointerToRawData +
             Offset;
       return Error::success();
     }
   }
-  return errorCodeToError(object_error::parse_failed);
+  if (ErrorContext)
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " for %s not found", Addr,
+                             ErrorContext);
+  else
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " not found", Addr);
 }
 
 Error COFFObjectFile::getRvaAndSizeAsBytes(uint32_t RVA, uint32_t Size,
-                                           ArrayRef<uint8_t> &Contents) const {
+                                           ArrayRef<uint8_t> &Contents,
+                                           const char *ErrorContext) const {
   for (const SectionRef &S : sections()) {
     const coff_section *Section = getCOFFSection(S);
     uint32_t SectionStart = Section->VirtualAddress;
     // Check if this RVA is within the section bounds. Be careful about integer
     // overflow.
     uint32_t OffsetIntoSection = RVA - SectionStart;
     if (SectionStart <= RVA && OffsetIntoSection < Section->VirtualSize &&
         Size <= Section->VirtualSize - OffsetIntoSection) {
       uintptr_t Begin = reinterpret_cast<uintptr_t>(base()) +
                         Section->PointerToRawData + OffsetIntoSection;
       Contents =
           ArrayRef<uint8_t>(reinterpret_cast<const uint8_t *>(Begin), Size);
       return Error::success();
     }
   }
-  return errorCodeToError(object_error::parse_failed);
+  if (ErrorContext)
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " for %s not found", RVA,
+                             ErrorContext);
+  else
+    return createStringError(object_error::parse_failed,
+                             "RVA %" PRIx32 " not found", RVA);
 }
 
 // Returns hint and name fields, assuming \p Rva is pointing to a Hint/Name
 // table entry.
 Error COFFObjectFile::getHintName(uint32_t Rva, uint16_t &Hint,
                                   StringRef &Name) const {
   uintptr_t IntPtr = 0;
   if (Error E = getRvaPtr(Rva, IntPtr))
     return E;
   const uint8_t *Ptr = reinterpret_cast<const uint8_t *>(IntPtr);
   Hint = *reinterpret_cast<const ulittle16_t *>(Ptr);
   Name = StringRef(reinterpret_cast<const char *>(Ptr + 2));
   return Error::success();
 }
 
 Error COFFObjectFile::getDebugPDBInfo(const debug_directory *DebugDir,
                                       const codeview::DebugInfo *&PDBInfo,
                                       StringRef &PDBFileName) const {
   ArrayRef<uint8_t> InfoBytes;
-  if (Error E = getRvaAndSizeAsBytes(
-          DebugDir->AddressOfRawData, DebugDir->SizeOfData, InfoBytes))
+  if (Error E =
+          getRvaAndSizeAsBytes(DebugDir->AddressOfRawData, DebugDir->SizeOfData,
+                               InfoBytes, "PDB info"))
     return E;
   if (InfoBytes.size() < sizeof(*PDBInfo) + 1)
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed, "PDB info too small");
   PDBInfo = reinterpret_cast<const codeview::DebugInfo *>(InfoBytes.data());
   InfoBytes = InfoBytes.drop_front(sizeof(*PDBInfo));
   PDBFileName = StringRef(reinterpret_cast<const char *>(InfoBytes.data()),
                           InfoBytes.size());
   // Truncate the name at the first null byte. Ignore any padding.
   PDBFileName = PDBFileName.split('\0').first;
   return Error::success();
 }
Error COFFObjectFile::initImportTablePtr() {
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
 
   uint32_t ImportTableRva = DataEntry->RelativeVirtualAddress;
 
   // Find the section that contains the RVA. This is needed because the RVA is
   // the import table's memory address which is different from its file offset.
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(ImportTableRva, IntPtr))
+  if (Error E = getRvaPtr(ImportTableRva, IntPtr, "import table"))
     return E;
   if (Error E = checkOffset(Data, IntPtr, DataEntry->Size))
     return E;
   ImportDirectory = reinterpret_cast<
       const coff_import_directory_table_entry *>(IntPtr);
   return Error::success();
 }
 
 // Initializes DelayImportDirectory and NumberOfDelayImportDirectory.
 Error COFFObjectFile::initDelayImportTablePtr() {
   const data_directory *DataEntry =
       getDataDirectory(COFF::DELAY_IMPORT_DESCRIPTOR);
   if (!DataEntry)
     return Error::success();
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
 
   uint32_t RVA = DataEntry->RelativeVirtualAddress;
   NumberOfDelayImportDirectory = DataEntry->Size /
       sizeof(delay_import_directory_table_entry) - 1;
 
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(RVA, IntPtr))
+  if (Error E = getRvaPtr(RVA, IntPtr, "delay import table"))
     return E;
   DelayImportDirectory = reinterpret_cast<
       const delay_import_directory_table_entry *>(IntPtr);
   return Error::success();
 }
 
 // Find the export table.
 Error COFFObjectFile::initExportTablePtr() {
   // First, we get the RVA of the export table. If the file lacks a pointer to
   // the export table, do nothing.
   const data_directory *DataEntry = getDataDirectory(COFF::EXPORT_TABLE);
   if (!DataEntry)
     return Error::success();
 
   // Do nothing if the pointer to export table is NULL.
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
 
   uint32_t ExportTableRva = DataEntry->RelativeVirtualAddress;
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(ExportTableRva, IntPtr))
+  if (Error E = getRvaPtr(ExportTableRva, IntPtr, "export table"))
     return E;
   ExportDirectory =
       reinterpret_cast<const export_directory_table_entry *>(IntPtr);
   return Error::success();
 }
 
 Error COFFObjectFile::initBaseRelocPtr() {
   const data_directory *DataEntry =
       getDataDirectory(COFF::BASE_RELOCATION_TABLE);
   if (!DataEntry)
     return Error::success();
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
 
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr))
+  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr,
+                          "base reloc table"))
     return E;
   BaseRelocHeader = reinterpret_cast<const coff_base_reloc_block_header *>(
       IntPtr);
   BaseRelocEnd = reinterpret_cast<coff_base_reloc_block_header *>(
       IntPtr + DataEntry->Size);
   // FIXME: Verify the section containing BaseRelocHeader has at least
   // DataEntry->Size bytes after DataEntry->RelativeVirtualAddress.
   return Error::success();
 }
 
 Error COFFObjectFile::initDebugDirectoryPtr() {
   // Get the RVA of the debug directory. Do nothing if it does not exist.
   const data_directory *DataEntry = getDataDirectory(COFF::DEBUG_DIRECTORY);
   if (!DataEntry)
     return Error::success();
 
   // Do nothing if the RVA is NULL.
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
 
   // Check that the size is a multiple of the entry size.
   if (DataEntry->Size % sizeof(debug_directory) != 0)
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed,
+                             "debug directory has uneven size");
 
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr))
+  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr,
+                          "debug directory"))
     return E;
   DebugDirectoryBegin = reinterpret_cast<const debug_directory *>(IntPtr);
   DebugDirectoryEnd = reinterpret_cast<const debug_directory *>(
       IntPtr + DataEntry->Size);
   // FIXME: Verify the section containing DebugDirectoryBegin has at least
   // DataEntry->Size bytes after DataEntry->RelativeVirtualAddress.
   return Error::success();
 }
Error COFFObjectFile::initTLSDirectoryPtr() {
   // Check that the size is correct.
   if (DataEntry->Size != DirSize)
     return createStringError(
         object_error::parse_failed,
         "TLS Directory size (%u) is not the expected size (%" PRIu64 ").",
         static_cast<uint32_t>(DataEntry->Size), DirSize);
 
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr))
+  if (Error E =
+          getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr, "TLS directory"))
     return E;
 
   if (is64())
     TLSDirectory64 = reinterpret_cast<const coff_tls_directory64 *>(IntPtr);
   else
     TLSDirectory32 = reinterpret_cast<const coff_tls_directory32 *>(IntPtr);
 
   return Error::success();
 }
 
 Error COFFObjectFile::initLoadConfigPtr() {
   // Get the RVA of the debug directory. Do nothing if it does not exist.
   const data_directory *DataEntry = getDataDirectory(COFF::LOAD_CONFIG_TABLE);
   if (!DataEntry)
     return Error::success();
 
   // Do nothing if the RVA is NULL.
   if (DataEntry->RelativeVirtualAddress == 0)
     return Error::success();
   uintptr_t IntPtr = 0;
-  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr))
+  if (Error E = getRvaPtr(DataEntry->RelativeVirtualAddress, IntPtr,
+                          "load config table"))
     return E;
 
   LoadConfig = (const void *)IntPtr;
   return Error::success();
 }
 
 Expected<std::unique_ptr<COFFObjectFile>>
 COFFObjectFile::create(MemoryBufferRef Object) {
Error COFFObjectFile::initialize() {
   if (checkSize(Data, EC, sizeof(dos_header) + sizeof(COFF::PEMagic))) {
     // PE/COFF, seek through MS-DOS compatibility stub and 4-byte
     // PE signature to find 'normal' COFF header.
     const auto *DH = reinterpret_cast<const dos_header *>(base());
     if (DH->Magic[0] == 'M' && DH->Magic[1] == 'Z') {
       CurPtr = DH->AddressOfNewExeHeader;
       // Check the PE magic bytes. ("PE\0\0")
       if (memcmp(base() + CurPtr, COFF::PEMagic, sizeof(COFF::PEMagic)) != 0) {
-        return errorCodeToError(object_error::parse_failed);
+        return createStringError(object_error::parse_failed,
+                                 "incorrect PE magic");
       }
       CurPtr += sizeof(COFF::PEMagic); // Skip the PE magic bytes.
       HasPEHeader = true;
     }
   }
 
   if (Error E = getObject(COFFHeader, Data, base() + CurPtr))
     return E;
if (Header->Magic == COFF::PE32Header::PE32) {
       DataDirAddr = base() + CurPtr + sizeof(pe32_header);
       DataDirSize = sizeof(data_directory) * PE32Header->NumberOfRvaAndSize;
     } else if (Header->Magic == COFF::PE32Header::PE32_PLUS) {
       PE32PlusHeader = reinterpret_cast<const pe32plus_header *>(Header);
       DataDirAddr = base() + CurPtr + sizeof(pe32plus_header);
       DataDirSize = sizeof(data_directory) * PE32PlusHeader->NumberOfRvaAndSize;
     } else {
       // It's neither PE32 nor PE32+.
-      return errorCodeToError(object_error::parse_failed);
+      return createStringError(object_error::parse_failed,
+                               "incorrect PE magic");
     }
     if (Error E = getObject(DataDirectory, Data, DataDirAddr, DataDirSize))
       return E;
   }
 
   if (COFFHeader)
     CurPtr += COFFHeader->SizeOfOptionalHeader;
 
if (Error E = initSymbolTablePtr()) {
       SymbolTable16 = nullptr;
       SymbolTable32 = nullptr;
       StringTable = nullptr;
       StringTableSize = 0;
     }
   } else {
     // We had better not have any symbols if we don't have a symbol table.
     if (getNumberOfSymbols() != 0) {
-      return errorCodeToError(object_error::parse_failed);
+      return createStringError(object_error::parse_failed,
+                               "symbol table missing");
     }
   }
 
   // Initialize the pointer to the beginning of the import table.
   if (Error E = initImportTablePtr())
     return E;
   if (Error E = initDelayImportTablePtr())
     return E;
Expected<const coff_section *> COFFObjectFile::getSection(int32_t Index) const {
   // Perhaps getting the section of a reserved section index should be an error,
   // but callers rely on this to return null.
   if (COFF::isReservedSectionNumber(Index))
     return (const coff_section *)nullptr;
   if (static_cast<uint32_t>(Index) <= getNumberOfSections()) {
     // We already verified the section table data, so no need to check again.
     return SectionTable + (Index - 1);
   }
-  return errorCodeToError(object_error::parse_failed);
+  return createStringError(object_error::parse_failed,
+                           "section index out of bounds");
 }
 
 Expected<StringRef> COFFObjectFile::getString(uint32_t Offset) const {
   if (StringTableSize <= 4)
     // Tried to get a string from an empty string table.
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed, "string table empty");
   if (Offset >= StringTableSize)
     return errorCodeToError(object_error::unexpected_eof);
   return StringRef(StringTable + Offset);
 }
 
 Expected<StringRef> COFFObjectFile::getSymbolName(COFFSymbolRef Symbol) const {
   return getSymbolName(Symbol.getGeneric());
 }

 
 iterator_range<imported_symbol_iterator>
 ImportDirectoryEntryRef::lookup_table_symbols() const {
   return make_range(lookup_table_begin(), lookup_table_end());
 }
 
 Error ImportDirectoryEntryRef::getName(StringRef &Result) const {
   uintptr_t IntPtr = 0;
-  if (Error E = OwningObject->getRvaPtr(ImportTable[Index].NameRVA, IntPtr))
+  if (Error E = OwningObject->getRvaPtr(ImportTable[Index].NameRVA, IntPtr,
+                                        "import directory name"))
     return E;
   Result = StringRef(reinterpret_cast<const char *>(IntPtr));
   return Error::success();
 }
 
 Error
 ImportDirectoryEntryRef::getImportLookupTableRVA(uint32_t  &Result) const {
   Result = ImportTable[Index].ImportLookupTableRVA;

 
 iterator_range<imported_symbol_iterator>
 DelayImportDirectoryEntryRef::imported_symbols() const {
   return make_range(imported_symbol_begin(), imported_symbol_end());
 }
 
 Error DelayImportDirectoryEntryRef::getName(StringRef &Result) const {
   uintptr_t IntPtr = 0;
-  if (Error E = OwningObject->getRvaPtr(Table[Index].Name, IntPtr))
+  if (Error E = OwningObject->getRvaPtr(Table[Index].Name, IntPtr,
+                                        "delay import directory name"))
     return E;
   Result = StringRef(reinterpret_cast<const char *>(IntPtr));
   return Error::success();
 }
 
 Error DelayImportDirectoryEntryRef::getDelayImportTable(
     const delay_import_directory_table_entry *&Result) const {
   Result = &Table[Index];
   return Error::success();
 }
 
 Error DelayImportDirectoryEntryRef::getImportAddress(int AddrIndex,
                                                      uint64_t &Result) const {
   uint32_t RVA = Table[Index].DelayImportAddressTable +
       AddrIndex * (OwningObject->is64() ? 8 : 4);
   uintptr_t IntPtr = 0;
-  if (Error E = OwningObject->getRvaPtr(RVA, IntPtr))
+  if (Error E = OwningObject->getRvaPtr(RVA, IntPtr, "import address"))
     return E;
   if (OwningObject->is64())
     Result = *reinterpret_cast<const ulittle64_t *>(IntPtr);
   else
     Result = *reinterpret_cast<const ulittle32_t *>(IntPtr);
   return Error::success();
 }
 
 bool ExportDirectoryEntryRef::
 operator==(const ExportDirectoryEntryRef &Other) const {
   return ExportTable == Other.ExportTable && Index == Other.Index;
 }
 
 void ExportDirectoryEntryRef::moveNext() {
   ++Index;
 }
 
 // Returns the name of the current export symbol. If the symbol is exported only
 // by ordinal, the empty string is set as a result.
 Error ExportDirectoryEntryRef::getDllName(StringRef &Result) const {
   uintptr_t IntPtr = 0;
-  if (Error E = OwningObject->getRvaPtr(ExportTable->NameRVA, IntPtr))
+  if (Error E =
+          OwningObject->getRvaPtr(ExportTable->NameRVA, IntPtr, "dll name"))
     return E;
   Result = StringRef(reinterpret_cast<const char *>(IntPtr));
   return Error::success();
 }
 
 // Returns the starting ordinal number.
 Error ExportDirectoryEntryRef::getOrdinalBase(uint32_t &Result) const {
   Result = ExportTable->OrdinalBase;
   return Error::success();
 }
 
 // Returns the export ordinal of the current export symbol.
 Error ExportDirectoryEntryRef::getOrdinal(uint32_t &Result) const {
   Result = ExportTable->OrdinalBase + Index;
   return Error::success();
 }
 
 // Returns the address of the current export symbol.
 Error ExportDirectoryEntryRef::getExportRVA(uint32_t &Result) const {
   uintptr_t IntPtr = 0;
-  if (Error EC =
-          OwningObject->getRvaPtr(ExportTable->ExportAddressTableRVA, IntPtr))
+  if (Error EC = OwningObject->getRvaPtr(ExportTable->ExportAddressTableRVA,
+                                         IntPtr, "export address"))
     return EC;
   const export_address_table_entry *entry =
       reinterpret_cast<const export_address_table_entry *>(IntPtr);
   Result = entry[Index].ExportRVA;
   return Error::success();
 }
 
 // Returns the name of the current export symbol. If the symbol is exported only
 // by ordinal, the empty string is set as a result.
 Error
 ExportDirectoryEntryRef::getSymbolName(StringRef &Result) const {
   uintptr_t IntPtr = 0;
-  if (Error EC =
-          OwningObject->getRvaPtr(ExportTable->OrdinalTableRVA, IntPtr))
+  if (Error EC = OwningObject->getRvaPtr(ExportTable->OrdinalTableRVA, IntPtr,
+                                         "export ordinal table"))
     return EC;
   const ulittle16_t *Start = reinterpret_cast<const ulittle16_t *>(IntPtr);
 
   uint32_t NumEntries = ExportTable->NumberOfNamePointers;
   int Offset = 0;
   for (const ulittle16_t *I = Start, *E = Start + NumEntries;
        I < E; ++I, ++Offset) {
     if (*I != Index)
       continue;
-    if (Error EC =
-            OwningObject->getRvaPtr(ExportTable->NamePointerRVA, IntPtr))
+    if (Error EC = OwningObject->getRvaPtr(ExportTable->NamePointerRVA, IntPtr,
+                                           "export table entry"))
       return EC;
     const ulittle32_t *NamePtr = reinterpret_cast<const ulittle32_t *>(IntPtr);
-    if (Error EC = OwningObject->getRvaPtr(NamePtr[Offset], IntPtr))
+    if (Error EC = OwningObject->getRvaPtr(NamePtr[Offset], IntPtr,
+                                           "export symbol name"))
       return EC;
     Result = StringRef(reinterpret_cast<const char *>(IntPtr));
     return Error::success();
   }
   Result = "";
   return Error::success();
 }
 
 Error ExportDirectoryEntryRef::isForwarder(bool &Result) const {
   const data_directory *DataEntry =
       OwningObject->getDataDirectory(COFF::EXPORT_TABLE);
   if (!DataEntry)
-    return errorCodeToError(object_error::parse_failed);
+    return createStringError(object_error::parse_failed,
+                             "export table missing");
   uint32_t RVA;
   if (auto EC = getExportRVA(RVA))
     return EC;
   uint32_t Begin = DataEntry->RelativeVirtualAddress;
   uint32_t End = DataEntry->RelativeVirtualAddress + DataEntry->Size;
   Result = (Begin <= RVA && RVA < End);
   return Error::success();
 }
 
 Error ExportDirectoryEntryRef::getForwardTo(StringRef &Result) const {
   uint32_t RVA;
   if (auto EC = getExportRVA(RVA))
     return EC;
   uintptr_t IntPtr = 0;
-  if (auto EC = OwningObject->getRvaPtr(RVA, IntPtr))
+  if (auto EC = OwningObject->getRvaPtr(RVA, IntPtr, "export forward target"))
     return EC;
   Result = StringRef(reinterpret_cast<const char *>(IntPtr));
   return Error::success();
 }
 
 bool ImportedSymbolRef::
 operator==(const ImportedSymbolRef &Other) const {
   return Entry32 == Other.Entry32 && Entry64 == Other.Entry64
if (Entry32[Index].isOrdinal())
       return Error::success();
     RVA = Entry32[Index].getHintNameRVA();
   } else {
     if (Entry64[Index].isOrdinal())
       return Error::success();
     RVA = Entry64[Index].getHintNameRVA();
   }
   uintptr_t IntPtr = 0;
-  if (Error EC = OwningObject->getRvaPtr(RVA, IntPtr))
+  if (Error EC = OwningObject->getRvaPtr(RVA, IntPtr, "import symbol name"))
     return EC;
   // +2 because the first two bytes is hint.
   Result = StringRef(reinterpret_cast<const char *>(IntPtr + 2));
   return Error::success();
 }
 
 Error ImportedSymbolRef::isOrdinal(bool &Result) const {
   if (Entry32)
Error ImportedSymbolRef::getOrdinal(uint16_t &Result) const {
   } else {
     if (Entry64[Index].isOrdinal()) {
       Result = Entry64[Index].getOrdinal();
       return Error::success();
     }
     RVA = Entry64[Index].getHintNameRVA();
   }
   uintptr_t IntPtr = 0;
-  if (Error EC = OwningObject->getRvaPtr(RVA, IntPtr))
+  if (Error EC = OwningObject->getRvaPtr(RVA, IntPtr, "import symbol ordinal"))
     return EC;
   Result = *reinterpret_cast<const ulittle16_t *>(IntPtr);
   return Error::success();
 }
 
 Expected<std::unique_ptr<COFFObjectFile>>
 ObjectFile::createCOFFObjectFile(MemoryBufferRef Object) {
   return COFFObjectFile::create(Object);

llvm/test/tools/llvm-readobj/COFF/tls-directory.test

 # X86-64-WRONG-SIZE-ERR: error: '{{.*}}': TLS Directory size (80) is not the expected size (40).
 
 
 ## This test has a correct TLS Directory size but the RVA is invalid.
 
 # RUN: yaml2obj %s --docnum=2 -o %t.bad-tls-rva.exe -DTLSRVA=999999 -DTLSSIZE=40
 # RUN: not llvm-readobj --coff-tls-directory %t.bad-tls-rva.exe 2>&1 | FileCheck %s --check-prefix BAD-TLS-RVA-ERR
 
-# BAD-TLS-RVA-ERR: error: '{{.*}}': Invalid data was encountered while parsing the file
+# BAD-TLS-RVA-ERR: error: '{{.*}}': RVA {{.*}} for TLS directory not found
 
 --- !COFF
 OptionalHeader:
   AddressOfEntryPoint: 0
   ImageBase:       0
   SectionAlignment: 4096
   FileAlignment:   512
   MajorOperatingSystemVersion: 0