[LLDB] [PECOFF] Fix error handling for executables that object::createBinary error out on

[mstorsjo]

Summary

llvm::object::createBinary returns an Expected<>, which requires not only checking the object for success, but also requires consuming the Error, if one was set.

Use LLDB_LOG_ERROR for this case, and change an existing similar log statement to use it as well, to make sure the Error is consumed even if the log channel is disabled.

[labath]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

175–179

There's an LLDB_LOG_ERROR macro, which makes sure to "handle" the error even if logging is disabled. It also handles all the .c_str() stuff *and* is able to prepend the name of the caller to the log message (if one requests that when enabling logging). So, overall, this could just be:
LLDB_LOG_ERROR(log, binary.takeError(), "Failed to create binary for file ({1}): {0}", file)

[mstorsjo]

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

175–179

Oh, awesome, that's much neater.

[mstorsjo]

Using LLDB_LOG_ERROR instead of formatting the error to a string before invoking logging macros.

lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp

 size_t ObjectFilePECOFF::GetModuleSpecifications(
     const lldb_private::FileSpec &file, lldb::DataBufferSP &data_sp,
     lldb::offset_t data_offset, lldb::offset_t file_offset,
     lldb::offset_t length, lldb_private::ModuleSpecList &specs) {
   const size_t initial_count = specs.GetSize();
   if (!data_sp || !ObjectFilePECOFF::MagicBytesMatch(data_sp))
     return initial_count;
 
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
+
   auto binary = llvm::object::createBinary(file.GetPath());
-  if (!binary)
+
+  if (!binary) {
+    LLDB_LOG_ERROR(log, binary.takeError(),
+                   "Failed to create binary for file ({1}): {0}", file);
     return initial_count;
+  }
 
   if (!binary->getBinary()->isCOFF() &&
       !binary->getBinary()->isCOFFImportFile())
     return initial_count;
 
   auto COFFObj =
     llvm::cast<llvm::object::COFFObjectFile>(binary->getBinary());
 
   ModuleSpec module_spec(file);

 bool ObjectFilePECOFF::CreateBinary() {
   if (m_owningbin)
     return true;
 
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
 
   auto binary = llvm::object::createBinary(m_file.GetPath());
   if (!binary) {
-    LLDB_LOGF(log,
-              "ObjectFilePECOFF::CreateBinary() - failed to create binary "
-              "for file (%s): %s",
-              m_file ? m_file.GetPath().c_str() : "<NULL>",
-              errorToErrorCode(binary.takeError()).message().c_str());
+    LLDB_LOG_ERROR(log, binary.takeError(),
+                   "Failed to create binary for file ({1}): {0}", m_file);
     return false;
   }
 
   // Make sure we only handle COFF format.
   if (!binary->getBinary()->isCOFF() &&
       !binary->getBinary()->isCOFFImportFile())
     return false;
 

lldb/test/Shell/ObjectFile/PECOFF/invalid-export-table.yaml

+## Test that errors in loading an exe doesn't crash lldb.
+## The ExportTable RelativeVirtualAddress is out of bounds here.
+
+# RUN: yaml2obj %s > %t.exe
+# RUN: %lldb %t.exe 2>&1 | FileCheck %s
+
+# CHECK: error: '{{.*}}' doesn't contain any {{.*}} platform architectures
+--- !COFF
+OptionalHeader:
+  AddressOfEntryPoint: 4096
+  ImageBase:       1073741824
+  SectionAlignment: 4096
+  FileAlignment:   512
+  MajorOperatingSystemVersion: 6
+  MinorOperatingSystemVersion: 0
+  MajorImageVersion: 0
+  MinorImageVersion: 0
+  MajorSubsystemVersion: 6
+  MinorSubsystemVersion: 0
+  Subsystem:       IMAGE_SUBSYSTEM_WINDOWS_CUI
+  DLLCharacteristics: [ IMAGE_DLL_CHARACTERISTICS_HIGH_ENTROPY_VA, IMAGE_DLL_CHARACTERISTICS_DYNAMIC_BASE, IMAGE_DLL_CHARACTERISTICS_NX_COMPAT, IMAGE_DLL_CHARACTERISTICS_TERMINAL_SERVER_AWARE ]
+  SizeOfStackReserve: 1048576
+  SizeOfStackCommit: 4096
+  SizeOfHeapReserve: 1048576
+  SizeOfHeapCommit: 4096
+  ExportTable:
+    RelativeVirtualAddress: 12345678
+    Size:            100
+  ImportTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  ResourceTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  ExceptionTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  CertificateTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  BaseRelocationTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  Debug:
+    RelativeVirtualAddress: 0
+    Size:            0
+  Architecture:
+    RelativeVirtualAddress: 0
+    Size:            0
+  GlobalPtr:
+    RelativeVirtualAddress: 0
+    Size:            0
+  TlsTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  LoadConfigTable:
+    RelativeVirtualAddress: 0
+    Size:            0
+  BoundImport:
+    RelativeVirtualAddress: 0
+    Size:            0
+  IAT:
+    RelativeVirtualAddress: 0
+    Size:            0
+  DelayImportDescriptor:
+    RelativeVirtualAddress: 0
+    Size:            0
+  ClrRuntimeHeader:
+    RelativeVirtualAddress: 0
+    Size:            0
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [ IMAGE_FILE_EXECUTABLE_IMAGE, IMAGE_FILE_LARGE_ADDRESS_AWARE ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    VirtualAddress:  4096
+    VirtualSize:     1
+    SectionData:     C3
+symbols:         []
+...