Misc module/dwarf logging improvements

[lemo]

Summary

This change improves the logging for the lldb.module category to note a few interesting cases:

1. Local object file found, but specs not matching
2. Local object file not found, using a placeholder module

The logging for failing to load compressed dwarf symbols is also improved.

[labath]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3400–3403 ↗

(On Diff #159101)

You have to consume the Decompressor.takeError() object to fulfill the llvm::Error contract. Easiest way to do that is to actually print it out.

3407 ↗

(On Diff #159101)

Same here.

3413 ↗

(On Diff #159101)

lit/Modules/compressed-sections.yaml test will need to be updated to account for the return 0.

[clayborg]

LGTM after Pavel's inline comments are addressed.

[lemo]

Incorporating feedback, thanks.

[lemo]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159101)

compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not hit the 0-length path, am I wrong?

[labath]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159368)

This needs to be std::move(Error). If you built with asserts enabled and hit this line, you would crash because you did not consume the Error object.

3413 ↗

(On Diff #159101)

The .bogus section in that test deliberately contains malformed data, so you should still hit the first error case.

However, looking at the lldb-test implementation, it just ignores the ReadSectionData return value, and relies on the size in the DataExtractor instead (one day I'll declare war on functions of this type), which is probably why you hadn't noticed this.

I guess in this case, it would be nice to reset the data extractor before returning 0 and possibly teaching lldb-test to report discrepancies in the returned sizes.

[lemo]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159368)

Can you please elaborate? std::move is just a cast (and the result of Error::takeValue() is already an rvalue - the error object has been already moved into a temporary Error instance)

[labath]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159368)

The llvm manual http://llvm.org/docs/ProgrammersManual.html#recoverable-errors says

All Error instances, whether success or failure, must be either checked or moved from (via std::move or a return) before they are destructed. Accidentally discarding an unchecked error will cause a program abort at the point where the unchecked value’s destructor is run, making it easy to identify and fix violations of this rule.
...
Success values are considered checked once they have been tested (by invoking the boolean conversion operator).
...
Failure values are considered checked once a handler for the error type has been activated.

The error object created on line 3407 (in the if-declaration) is neither moved from nor has it's handler invoked. You only invoke it's bool operator, which is not enough for it to be considered "checked" if it is in the "failure" state. This means it will assert once it's destructor is executed. By writing llvm::toString(std::move(Error)), you will "move" from the object, thereby clearing it. (It also makes sense to print out the error that you have just checked instead of some error from a previous step.)

Try this pattern out on a toy program to see what happens:

if (Error E = make_error<StringError>("This is an error", inconvertibleErrorCode()))
  outs() << "I encountered an error but I am not handling it";

[lemo]

Updated the LIT file too

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159368)

Thanks. I see, I was looking at the previous block.

By writing llvm::toString(std::move(Error)), you will "move" from the object, thereby clearing it.

It's a nice contract, although the "move" part was not the problem nor the solution in itself (I took a close look at the Error class, it doesn't matter how much you move it, someone has to eventually call handleErrors() on it. Conveniently, llvm::toString() does it)

[labath]

source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

3413 ↗

(On Diff #159368)

Cool, thanks. I'm sorry if my previous comments were a bit unclear.

lldb/trunk/lit/Modules/compressed-sections.yaml

 # CHECK-NEXT: File size: 28
 # CHECK-NEXT: Data:
 # CHECK-NEXT: 20304050 60708090
 
 # CHECK: Name: .bogus
 # CHECK-NEXT: Type: regular
 # CHECK-NEXT: VM size: 0
 # CHECK-NEXT: File size: 8
-# CHECK-NEXT: Data:
-# CHECK-NEXT: DEADBEEF BAADF00D
+# CHECK-NEXT: Data: ()

lldb/trunk/source/Core/Module.cpp

Module::Module(const ModuleSpec &module_spec)
   // Now make sure that one of the module specifications matches what we just
   // extract. We might have a module specification that specifies a file
   // "/usr/lib/dyld" with UUID XXX, but we might have a local version of
   // "/usr/lib/dyld" that has
   // UUID YYY and we don't want those to match. If they don't match, just don't
   // fill any ivars in so we don't accidentally grab the wrong file later since
   // they don't match...
   ModuleSpec matching_module_spec;
-  if (modules_specs.FindMatchingModuleSpec(module_spec, matching_module_spec) ==
-      0)
+  if (!modules_specs.FindMatchingModuleSpec(module_spec,
+                                            matching_module_spec)) {
+    if (log) {
+      log->Printf("Found local object file but the specs didn't match");
+    }
     return;
+  }
 
   if (module_spec.GetFileSpec())
     m_mod_time = FileSystem::GetModificationTime(module_spec.GetFileSpec());
   else if (matching_module_spec.GetFileSpec())
     m_mod_time =
         FileSystem::GetModificationTime(matching_module_spec.GetFileSpec());
 
   // Copy the architecture from the actual spec if we got one back, else use

lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp

else if ((note.n_type == LLDB_NT_GNU_ABI_TAG) &&
             arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
             break;
           }
         }
         if (arch_spec.IsMIPS() &&
             arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS)
           // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some
           // cases (e.g. compile with -nostdlib) Hence set OS to Linux
-          arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); 
+          arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux);
       }
     }
 
     // Calculate the offset of the next note just in case "offset" has been
     // used to poke at the contents of the note data
     offset = note_offset + note.GetByteSize();
   }
 
size_t ObjectFileELF::GetSectionHeaderInfo(SectionHeaderColl &section_headers,
   // Only initialize the arch_spec to okay defaults if they're not already set.
   // We'll refine this with note data as we parse the notes.
   if (arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS) {
     llvm::Triple::OSType ostype;
     llvm::Triple::OSType spec_ostype;
     const uint32_t sub_type = subTypeFromElfHeader(header);
     arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type,
                               header.e_ident[EI_OSABI]);
-    
     // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is
     // determined based on EI_OSABI flag and the info extracted from ELF notes
     // (see RefineModuleDetailsFromNote). However in some cases that still
     // might be not enough: for example a shared library might not have any
     // notes at all and have EI_OSABI flag set to System V, as result the OS
     // will be set to UnknownOS.
     GetOsFromOSABI(header.e_ident[EI_OSABI], ostype);
     spec_ostype = arch_spec.GetTriple().getOS();

 }
 
 size_t ObjectFileELF::ReadSectionData(Section *section,
                                       DataExtractor &section_data) {
   // If some other objectfile owns this data, pass this to them.
   if (section->GetObjectFile() != this)
     return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = ObjectFile::ReadSectionData(section, section_data);
   if (result == 0 || !section->Test(SHF_COMPRESSED))
     return result;
 
   auto Decompressor = llvm::object::Decompressor::create(
       section->GetName().GetStringRef(),
       {reinterpret_cast<const char *>(section_data.GetDataStart()),
        size_t(section_data.GetByteSize())},
       GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8);
   if (!Decompressor) {
-    LLDB_LOG_ERROR(log, Decompressor.takeError(),
-                   "Unable to initialize decompressor for section {0}",
-                   section->GetName());
-    return result;
+    GetModule()->ReportWarning(
+        "Unable to initialize decompressor for section '%s': %s",
+        section->GetName().GetCString(),
+        llvm::toString(Decompressor.takeError()).c_str());
+    section_data.Clear();
+    return 0;
   }
+
   auto buffer_sp =
       std::make_shared<DataBufferHeap>(Decompressor->getDecompressedSize(), 0);
-  if (auto Error = Decompressor->decompress(
+  if (auto error = Decompressor->decompress(
           {reinterpret_cast<char *>(buffer_sp->GetBytes()),
            size_t(buffer_sp->GetByteSize())})) {
-    LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-                   section->GetName());
-    return result;
+    GetModule()->ReportWarning(
+        "Decompression of section '%s' failed: %s",
+        section->GetName().GetCString(),
+        llvm::toString(std::move(error)).c_str());
+    section_data.Clear();
+    return 0;
   }
+
   section_data.SetData(buffer_sp);
   return buffer_sp->GetByteSize();
 }
 
 bool ObjectFileELF::AnySegmentHasPhysicalAddress() {
   size_t header_count = ParseProgramHeaders();
   for (size_t i = 1; i <= header_count; ++i) {
     auto header = GetProgramHeaderByIndex(i);

lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp

for (auto module : filtered_modules) {
     if (!module_sp || error.Fail()) {
       // We failed to locate a matching local object file. Fortunately, the
       // minidump format encodes enough information about each module's memory
       // range to allow us to create placeholder modules.
       //
       // This enables most LLDB functionality involving address-to-module
       // translations (ex. identifing the module for a stack frame PC) and
       // modules/sections commands (ex. target modules list, ...)
+      if (log) {
+        log->Printf("Unable to locate the matching object file, creating a "
+                    "placeholder module for: %s",
+                    name.getValue().c_str());
+      }
+
       auto placeholder_module =
           std::make_shared<PlaceholderModule>(module_spec);
       placeholder_module->CreateImageSection(module, GetTarget());
       module_sp = placeholder_module;
       GetTarget().GetImages().Append(module_sp);
     }
 
     if (log) {

lldb/trunk/source/Target/Process.cpp

for (auto pos = m_instrumentation_runtimes.begin();
     InstrumentationRuntimeSP runtime = pos->second;
     runtime->ModulesDidLoad(module_list);
   }
 
   // Let any language runtimes we have already created know about the modules
   // that loaded.
 
   // Iterate over a copy of this language runtime list in case the language
-  // runtime ModulesDidLoad somehow causes the language riuntime to be
+  // runtime ModulesDidLoad somehow causes the language runtime to be
   // unloaded.
   LanguageRuntimeCollection language_runtimes(m_language_runtimes);
   for (const auto &pair : language_runtimes) {
     // We must check language_runtime_sp to make sure it is not nullptr as we
     // might cache the fact that we didn't have a language runtime for a
     // language.
     LanguageRuntimeSP language_runtime_sp = pair.second;
     if (language_runtime_sp)
supported_type_names.ForEach(
         const_type_names.insert(ConstString(type_name->GetValue()));
         LLDB_LOG(log, "- {0}", type_name->GetValue());
         return true;
       });
 
   // For each StructuredDataPlugin, if the plugin handles any of the types in
   // the supported_type_names, map that type name to that plugin. Stop when
   // we've consumed all the type names.
-  // FIXME: should we return an error if there are type names nobody 
+  // FIXME: should we return an error if there are type names nobody
   // supports?
   for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) {
     auto create_instance =
            PluginManager::GetStructuredDataPluginCreateCallbackAtIndex(
                plugin_index);
     if (!create_instance)
       break;
-      
     // Create the plugin.
     StructuredDataPluginSP plugin_sp = (*create_instance)(*this);
     if (!plugin_sp) {
       // This plugin doesn't think it can work with the process. Move on to the
       // next.
       continue;
     }