[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL339161: Misc module/dwarf logging improvements (authored by 
lemo, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D50274?vs=159432=159551#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D50274

Files:
  lldb/trunk/lit/Modules/compressed-sections.yaml
  lldb/trunk/source/Core/Module.cpp
  lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
  lldb/trunk/source/Target/Process.cpp

Index: lldb/trunk/source/Core/Module.cpp
===
--- lldb/trunk/source/Core/Module.cpp
+++ lldb/trunk/source/Core/Module.cpp
@@ -162,9 +162,13 @@
   // 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());
Index: lldb/trunk/source/Target/Process.cpp
===
--- lldb/trunk/source/Target/Process.cpp
+++ lldb/trunk/source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // 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  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // 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) {
Index: lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -365,6 +365,12 @@
   // 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(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ lldb/trunk/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 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);
   }
 }
 
@@ -1494,7 +1494,7 @@
 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
@@ -3385,8 +3385,6 @@
   if (section->GetObjectFile() != this)
 return section->GetObjectFile()->ReadSectionData(section, section_data);
 
-  Log *log = lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_MODULES);
-
   size_t result = 

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added inline comments.
This revision is now accepted and ready to land.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

lemo wrote:
> labath wrote:
> > lemo wrote:
> > > labath wrote:
> > > > 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.
> > > 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)
> > The llvm manual 
> >  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("This is an error", 
> > inconvertibleErrorCode()))
> >   outs() << "I encountered an error but I am not handling it";
> > ```
> 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)
Cool, thanks. I'm sorry if my previous comments were a bit unclear.


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> lemo wrote:
> > labath wrote:
> > > 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.
> > 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)
> The llvm manual 
>  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("This is an error", 
> inconvertibleErrorCode()))
>   outs() << "I encountered an error but I am not handling it";
> ```
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)


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159432.
lemo marked an inline comment as done.
lemo added a comment.

Updated the LIT file too


https://reviews.llvm.org/D50274

Files:
  lit/Modules/compressed-sections.yaml
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // 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  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // 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) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // 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(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 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);
   }
 }
 
@@ -1494,7 +1494,7 @@
 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
@@ -3385,8 +3385,6 @@
   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;
@@ -3397,20 +3395,27 @@
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(Decompressor->getDecompressedSize(), 0);
-  if (auto Error = Decompressor->decompress(
+  if (auto error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),
size_t(buffer_sp->GetByteSize())})) {
-LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed",
-   section->GetName());
-return result;
+

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

lemo wrote:
> labath wrote:
> > 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.
> 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)
The llvm manual 
 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("This is an error", 
inconvertibleErrorCode()))
  outs() << "I encountered an error but I am not handling it";
```


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

labath wrote:
> 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.
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)


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

lemo wrote:
> labath wrote:
> > `lit/Modules/compressed-sections.yaml` test will need to be updated to 
> > account for the return 0.
> compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not 
> hit the 0-length path, am I wrong?
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.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString(),
+llvm::toString(Decompressor.takeError()).c_str());
+return 0;

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.


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo marked 2 inline comments as done.
lemo added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

labath wrote:
> `lit/Modules/compressed-sections.yaml` test will need to be updated to 
> account for the return 0.
compressed-sections.yaml seems to be gated by REQUIRES: zlib, so it would not 
hit the 0-length path, am I wrong?


https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo updated this revision to Diff 159368.
lemo added a comment.

Incorporating feedback, thanks.


https://reviews.llvm.org/D50274

Files:
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // 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  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // 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) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // 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(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 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);
   }
 }
 
@@ -1494,7 +1494,7 @@
 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
@@ -3385,8 +3385,6 @@
   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;
@@ -3397,20 +3395,25 @@
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());
+return 0;
   }
+
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(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(),
+

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

LGTM after Pavel's inline comments are addressed.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3400-3403
+GetModule()->ReportWarning(
+"Unable to initialize decompressor for section '%s'",
+section->GetName().GetCString());
+return 0;

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.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3407
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(buffer_sp->GetBytes()),

Same here.



Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413
+section->GetName().GetCString());
+return 0;
   }

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


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50274



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

[Leonard Mosescu via Phabricator via lldb-commits]

lemo created this revision.
lemo added reviewers: labath, clayborg.
lemo added a project: LLDB.
Herald added subscribers: JDevlieghere, arichardson, aprantl, emaste.
Herald added a reviewer: espindola.

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.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50274

Files:
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Process.cpp

Index: source/Target/Process.cpp
===
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -5842,7 +5842,7 @@
   // 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  : language_runtimes) {
@@ -6095,15 +6095,15 @@
   // 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) {
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -357,6 +357,12 @@
   // 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(module_spec);
   placeholder_module->CreateImageSection(module, GetTarget());
Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
===
--- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
+++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
@@ -1390,7 +1390,7 @@
 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);
   }
 }
 
@@ -1494,7 +1494,7 @@
 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
@@ -3397,19 +3397,20 @@
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'",
+section->GetName().GetCString());
+return 0;
   }
   auto buffer_sp =
   std::make_shared(Decompressor->getDecompressedSize(), 0);
   if (auto Error = Decompressor->decompress(
   {reinterpret_cast(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",
+section->GetName().GetCString());
+return 0;
   }