Page MenuHomePhabricator

Misc module/dwarf logging improvements
ClosedPublic

Authored by lemo on Aug 3 2018, 3:11 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

lemo created this revision.Aug 3 2018, 3:11 PM
labath added inline comments.Aug 4 2018, 4:52 AM
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.

LGTM after Pavel's inline comments are addressed.

lemo updated this revision to Diff 159368.Aug 6 2018, 12:48 PM

Incorporating feedback, thanks.

lemo marked 2 inline comments as done.Aug 6 2018, 12:50 PM
lemo added inline comments.
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 added inline comments.Aug 6 2018, 1:05 PM
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
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.

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.

lemo added inline comments.Aug 6 2018, 1:34 PM
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 added inline comments.Aug 6 2018, 2:21 PM
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 this revision to Diff 159432.Aug 6 2018, 4:45 PM
lemo marked an inline comment as done.

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 accepted this revision.Aug 7 2018, 2:00 AM
labath added inline comments.
source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
3413 ↗(On Diff #159368)

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

This revision is now accepted and ready to land.Aug 7 2018, 2:00 AM
This revision was automatically updated to reflect the committed changes.