Page MenuHomePhabricator

DWARFDebugLoc: Make parsing and error reporting more robust
AcceptedPublic

Authored by labath on Jun 20 2019, 3:01 AM.

Details

Summary

While examining this class for possible use in lldb, I noticed two
things:

  • it spits out parsing errors directly to stderr
  • the loclists parser can incorrectly return valid location lists when parsing malformed (truncated) data

I improve the stderr situation by making the parseOneLocationList
functions return Expected<T>s. The errors are still dumped to stderr by
their callers, so this is only a partial fix, but it is enough for my
use case, as I intend to parse the locations lists one by one.

I fix the behavior in the truncated scenario by making the parser
explictly check for success. This makes the code a bit clunky, as the
DataExtractor API makes it really hard to do that, but this is the best
implementation I came up with given the current API.

I also add tests for handling the error cases, as they currently have no
coverage.

Diff Detail

Event Timeline

labath created this revision.Jun 20 2019, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2019, 3:01 AM

Looks pretty good, and thanks especially for the error-case tests!
I'll give other folks a chance to chime in if they want to.

lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
101

This identical createError call occurs many times, maybe add a createLocListOverflowError() helper?

115

You could do SavedOffset = *Offset; here, and then add a SavedOffset == *Offset check to the next one. There's no harm to calling a get* function with an invalid offset.

218

Maybe put an llvm_unreachable here.

test/DebugInfo/X86/dwarfdump-debug-loc-error-cases.s
1

I was not aware of --defsym that looks incredibly useful!

In a test that generates multiple .o files I prefer to give each one a unique name, e.g. %t0.o and %t1.o etc. It can make it easier to debug a broken test.

labath updated this revision to Diff 205792.Jun 20 2019, 6:23 AM
labath marked 3 inline comments as done.
  • add createOverflowError helper
  • use unique file names in tests
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
115

The debug_loc function doesn't use the SavedOffset pattern, because it is always reading data in fixed-size chunks. I think it would be better to keep it that way, as this is slightly more readable.

test/DebugInfo/X86/dwarfdump-debug-loc-error-cases.s
1

I'm writing a bunch of tests in assembly these days, so I've learned a lot of interesting tricks there. :)

I'll update the tests to use distinct file names.

probinson accepted this revision.Jun 20 2019, 6:41 AM

LGTM but give the West Coast folks a chance to look at it.

This revision is now accepted and ready to land.Jun 20 2019, 6:41 AM
dblaikie added inline comments.Jun 20 2019, 12:47 PM
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
27

I guess "Ts &&... Vals" should be "const Ts &... Vals" since they're taken by const ref by createStringError anyway - no need for the fancy &&.

31

Should this be StringRef rather than const char*?

167–181

Looks to me like getULEB128 doesn't quite have the right error handling, if I'm reading it correctly:

unsigned shift = 0;
uint32_t offset = *offset_ptr;
uint8_t byte = 0;

while (isValidOffset(offset)) {
  byte = Data[offset++];
  result |= uint64_t(byte & 0x7f) << shift;
  shift += 7;
  if ((byte & 0x80) == 0)
    break;
}

*offset_ptr = offset;
return result;

I /imagine/ it shouldn't update offset_ptr if it breaks out of the loop via !isValidOffset, rather than via the break?

More broadly, I wonder if we should consider a more convenient way to do error handling here - since it's a bit unfortunate that you've had to split the logic for parsing these things across two switch statements - makes it a bit hard to follow what shape each LLE entry has, since it's spread out like this.

217–218

Given the loop condition is "while (true)" this unreachable seems a bit unnecessary (& the function has non-void return, so if there was a path that got through the loop I imagine the compiler would warn us about that?)

Or is this working around a compiler that warns here despite the lack of any path out of the loop?

probinson added inline comments.Jun 20 2019, 1:41 PM
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
217–218

I have had to add llvm_unreachable before in this kind of situation, IIRC, which is why I suggested it. Might not be necessary, if all 3 supported toolchains are smart enough nowadays.

labath updated this revision to Diff 205973.Jun 21 2019, 4:54 AM
labath marked 7 inline comments as done.
  • remove fancy references
  • remove llvm_unreachable
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
31

createStringError uses a printf format string. Taking a StringRef would mean I'd have to add .str().c_str() blurb.

167–181

Nice catch about the getULEB function. I'll create a separate patch for that.

Overall, I'm not really happy about how the error handling is implemented here. The DataExtractor functions seem to be really good at making sure you don't crash while using them, but they also make it incredibly hard to check the result for errors. We could change them to return an Optional<T> or something, but that would make using them a lot more verbose.

It sounds like me like it may be best to have some thing similar to what std::istream has. I.e., have an object which encapsulates three things:

  • the data to parse
  • current offset in that data
  • an error flag

This would mean one can still call GetXXX functions in sequence without additional error checking. However, at a suitable point in time (e.g., after parsing a single record/DIE/...), one can have a peek at the error flag to verify that the data he got is actually valid.

WDYT?

217–218

The debug_loc parser already uses this pattern without the terminating llvm_unreachable so I'd say we can assume the current compilers are fine with that..

Removing that llvm_unreachable is fine, in that case.
The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size.

The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size.

Hmm... Originally I was thinking of building something on top of DataExtractor. Putting the logic *into* the DataExtractor is an interesting idea. I kind of like it (it would solve the problem I had of how to capture the DataExtractor vs. DWARFDataExtractor relationship in the "on top" model), but there's also something that bothers me about that. I think it's the fact that this would make the DataExtractor class stateful, whereas previously it was completely stateless. That may not me all bad, but it would mean the transition has to be done more carefully (watch out for thread races, and other unintended effects of the error bit leaking out). However, it also feels weird to have the error flag be a part of DataExtractor state, while the offset isn't. So, e.g. if one extracts from the DataExtractor using two independent offsets simultaneously, the error state set by one extraction would impact the other. This would be most obvious with the strtab-style data extractors, which almost always get a bunch of completely independent queries.

One way to achieve this while keeping the DataExtractor stateless would be to pass the error flag as an additional argument to the extraction methods, just like the offset is now. But that would make things more verbose, which means one might still want to implement some kind of an abstraction on top of that to keep these things together...

Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors.

I have a couple more ideas to toss out there...

  • A DataExtractorBase class that returns Optional<whatever>, and then DataExtractor layers on top and converts None to zero, which preserves the non-statefulness as well as the current API. This adds some runtime overhead, not sure how much.
  • Or, a template DataExtractorBase that takes an error-handling class as a parameter (sort of like how STL containers take an allocator) and a DataExtractor specialization uses a no-op error-handling class. Should avoid runtime overhead at the cost of template cruft.

Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors.

Not really. The data extractor kind of does return errors, as we've seen in this patch, it's just that they're incredibly hard to check for. I was thinking of having the new class rely on the SavedOffset == Offset behavior, but only internally. In my prototype, I managed to tuck it all away into a single template function which takes a member function pointer argument :D. Unfortunately, that was ICE-ing on gcc :P, but that was because I was beeing too clever -- I'm sure it can be dumbed down a bit.

I have a couple more ideas to toss out there...

  • A DataExtractorBase class that returns Optional<whatever>, and then DataExtractor layers on top and converts None to zero, which preserves the non-statefulness as well as the current API. This adds some runtime overhead, not sure how much.

That would work. It wouldn't even have to be a separate class, if you just make sure the function names are somehow different. However, it would mean that one has to explicitly check the result of every get operation. Not the end of the world, but it would make the code using it more noisy.

  • Or, a template DataExtractorBase that takes an error-handling class as a parameter (sort of like how STL containers take an allocator) and a DataExtractor specialization uses a no-op error-handling class. Should avoid runtime overhead at the cost of template cruft.

I'm not exactly sure how you imagined that, but I'm sure it could be made to work, as templates can be made to do almost anything. :P I'm not sure if it would be simpler than having a wrapper class or not. I have a feeling it might end up looking fairly similar from the outside.

Pick whatever mechanism you like, we should debate it in that patch not here. :-)

Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine.

labath updated this revision to Diff 206205.Jun 24 2019, 6:07 AM

Leave a TODO in the code.

Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine.

How about this ? Theoretically I could also back out the SavedOffset changes. The main thing I was trying to fix is the stderr messages, this is just something I found while trying to write tests for the error handling code. I'm not too worried about the extra "zero" location lists being reported, as those are unlikely to be valid (but it would definitely be nice to fix them).

I also have a kind of a WIP patch for doing the error handling in a better way. I'm going to put that up separately so we can discuss it there.

PS: I'm going to have about two more patches here to make this stuff usable from lldb.

DataExtractor is a copy of the one from LLDB from a while back and changes have been made to adapt it to llvm. DataExtractor was designed so that you can have one of them (like for .debug_info or any other DWARF section) and use this same extractor from multiple threads. This is why it is currently stateless.

One solution to allowing for correct error handling would be to replace the current "uint32_t *offset_ptr" arguments to DataExtractor decoding functions with a "DataCursor &Pos" where DataCursor is something like:

class DataCursor {
  llvm::Expected<uint32_t> OffsetOrError;
};

Then all of the state like the offset and any error state. Or it could be two members, an offset and an error.

The main issues is to not decrease parsing performance by introducing error checking on each byte. The current DataExtractor will return zeroes when things fail to extract, which is kind of tuned for DWARF since zeros are not valid DW_TAG, DW_AT, DW_FORM and many other DWARF values. But it does allow for fast parsing. The idea was to quickly try and parse a bunch of data, and then make sure things are ok after doing some work (like parsing an entire DIE). So be careful with any changes to ensure DWARF parsing doesn't seriously regress.

lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
181

We should switch the LEB functions in DataExtractor over to use the ones from:

#include <llvm/Support/LEB128.h

and use the:

inline uint64_t decodeULEB128(const uint8_t *p, unsigned *n = nullptr,
                              const uint8_t *end = nullptr,
                              const char **error = nullptr);

inline int64_t decodeSLEB128(const uint8_t *p, unsigned *n = nullptr,
                             const uint8_t *end = nullptr,
                             const char **error = nullptr);

functions... They have all the error checking and are quite efficient. since DataExtractor had been converted from LLDB over into LLVM, the person that moved DataExtractor into LLVM hadn't realized these functions (might have been me) were there when the move happened.