Page MenuHomePhabricator

Add error handling to the DataExtractor class
ClosedPublic

Authored by labath on Jun 24 2019, 6:29 AM.

Details

Summary

This is motivated by D63591, where we realized that there isn't a really
good way of telling whether a DataExtractor is reading actual data, or
is it just returning default values because it reached the end of the
buffer.

This patch resolves that by providing a new "Cursor" class. A Cursor
object encapsulates two things:

  • the current position/offset in the DataExtractor
  • an error object

Storing the error object inside the Cursor enables one to use the same
pattern as the std::{io}stream API, where one can blindly perform a
sequence of reads and only check for errors once at the end of the
operation. Similarly to the stream API, as soon as we encounter one
error, all of the subsequent operations are skipped (return default
values) too, even if the would suceed with clear error state. Unlike the
std::stream API (but in line with other llvm APIs), we force the error
state to be checked through usage of llvm::Error.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
probinson added inline comments.Jun 24 2019, 11:26 AM
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
181 ↗(On Diff #206206)

You've lost a range check on the size.

labath updated this revision to Diff 206430.Jun 25 2019, 6:40 AM
labath marked 10 inline comments as done.

Cursor implementation.

I'm kind of thinking a change to the cursor as Greg suggested might be better than wrapping the stream further - and probably using llvm::Error.

& yeah, I think using Error and saying "parsing failures are problems that must be handled" rather than allowing for accidentally ignoring failures,

Amounting to something like this:

Cursor C;
DataExtractor D = ...;
D.getU32(C);
D.getU64(C);
...
if (Error E = C.getError())
  ...;

That way early exits (if your record type has optional fields, etc) you can't accidentally forget your error checking (because if there's an Error inside the Cursor it'd have to be checked otherwise it'll assert that it's unchecked, etc)

Yeah, using a cursor API seems fine. I've updated the patch to use something like you desribed above. Let me know what you think.

But yeah, that's a huge amount of refactoring to add all the error handling into existing callers.

I think this thing can be done incrementally. The first step could be to add a Error *Err = nullptr argument to all existing DataExtractor functions. This way, anyone who wishes it, can already detect errors by passing an error object. Then, the cursor api becomes just syntactic sugar on top of that, and can be introduced gradually. Once all users have been ported, the non-cursor API can be removed (or more likely replaced by something returning Expected<T>, since the non-cursor api is still handier to use for one-off parses like those in debug_str and similar..

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp
60 ↗(On Diff #206206)

Indeed. Thanks for catching that. I'll have to remember to go through the comments on this patch and create regression tests once I'm done prototyping...

389 ↗(On Diff #206206)

Good catch.

This is a pattern that has been present a couple of times in this diff alone, so I've created a special DataExtractor function for safely reading a sequence of integers into a vector.

441 ↗(On Diff #206206)

Documenting them is easy. However, I would say that the more interesting question at this stage is "do they make the code more readable?". I would say "yes", because without this, correctly checking for EOF is nearly impossible. However, there may still be room for improvement here...

dblaikie added inline comments.Jun 25 2019, 4:31 PM
lib/Support/DataExtractor.cpp
28

I /think/ these should be early-returns if the Error is already set (assuming the goal is to support repeated calls to deserialization functions with the same Error and only check once at the end of the sequence of deserializations)

Also, you will need to be careful with how you check if the Error is already set - since you want to make sure you don't set the Error to the "checked" state (otherwise the caller won't be forced to check it, which is bad).

Hmm, perhaps you actually want to test that the Error is checked. If the error is unchecked, then return early & leave it to the caller to check.

The "unexpectedEndReached" can then just do "if (E) *E = ...;" without having to check the error state.

If the goal is to treat the Error as a straight out parameter without the repeated deserialization support (because something at a higher level will handle that in some way) - then maybe this should be ErrorAsOutParameter instead?

labath marked an inline comment as done.Jul 2 2019, 3:00 AM
labath added inline comments.
lib/Support/DataExtractor.cpp
28

The goal is "to support repeated calls to deserialization functions with the same Error and only check once at the end of the sequence of deserializations" like you've said.

I also think that it would be nice to have early returns here. The reason I am reluctant to do that is because this will add an additional branch to the hot success path, where we do not encounter an error. In reality, this will probably be two branches because we will also need to check the Err pointer, at least in the interim stage.

I hope that this would not matter in practice because once one reaches the end of the deserialization sequence and finds that the error object is set, the only thing he can reliably do is disregard all data he has read since the last time he checked for the error state.

Nonetheless having early returns here would definitely make the API more predictable. I'm going to try to get some numbers on the impact of returning early here...

labath added a comment.Jul 2 2019, 6:40 AM

Ok, I got some numbers now. I went for a micro-benchmark-like setup to show some kind of a worst case scenario. The test setup is as follows:

I took the largest single .o file I had around (Registry.cpp.o in clangDynamicASTMatchers library). I then linked it to remove any relocations (otherwise, most of the time is spend applying those). Then I modified llvm-dwarfdump to parse the debug_loc section without dumping anything (to avoid measuring the time spend in printfs). Both llvm-dwarfdump and Registry.cpp.o I was dumping were built with -O3 -g, with asserts disabled (no FDO, LTO or other fancy stuff). This resulted in about 4.5 megabytes of debug_loc for parsing in Registry.cpp.o. Then I used the linux perf command to run llvm-dwarfdump -debug-loc 1000 times and dump the stats.

The baseline stats are:

  27.285986      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.11% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,813      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 58,831,163      cycles:u                  #    2.156 GHz                      ( +-  0.07% )
    606,986      stalled-cycles-frontend:u #    1.03% frontend cycles idle     ( +-  3.76% )
  7,924,778      stalled-cycles-backend:u  #   13.47% backend cycles idle      ( +-  0.33% )
146,588,727      instructions:u            #    2.49  insn per cycle         
                                           #    0.05  stalled cycles per insn  ( +-  0.00% )
 29,545,620      branches:u                # 1082.813 M/sec                    ( +-  0.00% )
    222,276      branch-misses:u           #    0.75% of all branches          ( +-  0.15% )

0.027663381 seconds time elapsed                                          ( +-  0.11% )

The stats with this patch applied are:

  27.397390      task-clock:u (msec)       #    0.987 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,833      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 60,160,571      cycles:u                  #    2.196 GHz                      ( +-  0.07% )
    584,825      stalled-cycles-frontend:u #    0.97% frontend cycles idle     ( +-  3.37% )
 10,729,974      stalled-cycles-backend:u  #   17.84% backend cycles idle      ( +-  0.26% )
156,141,836      instructions:u            #    2.60  insn per cycle         
                                           #    0.07  stalled cycles per insn  ( +-  0.00% )
 31,599,940      branches:u                # 1153.392 M/sec                    ( +-  0.00% )
    221,247      branch-misses:u           #    0.70% of all branches          ( +-  0.06% )

0.027771865 seconds time elapsed                                          ( +-  0.10% )

The stats for a version of this patch which additionally checks for the error flag before attempting the parse (as discussed in the inline comment) are:

  27.808349      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,839      page-faults:u             #    0.102 M/sec                    ( +-  0.24% )
 62,887,388      cycles:u                  #    2.261 GHz                      ( +-  0.06% )
    575,264      stalled-cycles-frontend:u #    0.91% frontend cycles idle     ( +-  3.18% )
 14,757,888      stalled-cycles-backend:u  #   23.47% backend cycles idle      ( +-  0.23% )
167,562,307      instructions:u            #    2.66  insn per cycle         
                                           #    0.09  stalled cycles per insn  ( +-  0.00% )
 33,414,152      branches:u                # 1201.587 M/sec                    ( +-  0.00% )
    221,454      branch-misses:u           #    0.66% of all branches          ( +-  0.12% )

0.028201319 seconds time elapsed                                          ( +-  0.10% )

As can be seen, this patch increases the parsing time by about 0.4%. This is enough to be statistically significant in a benchmark like this, but probably not world-shattering (some slowdown is unavoidable with a change like this).

If we additionally enable the early returns we get an additional 1.5% slowdown (or 1.9% above baseline). Still not bad for a "micro-benchmark", but it does make one wonder, whether it is really worth it. My feeling would be that it isn't...

Ok, I got some numbers now. I went for a micro-benchmark-like setup to show some kind of a worst case scenario. The test setup is as follows:

I took the largest single .o file I had around (Registry.cpp.o in clangDynamicASTMatchers library). I then linked it to remove any relocations (otherwise, most of the time is spend applying those). Then I modified llvm-dwarfdump to parse the debug_loc section without dumping anything (to avoid measuring the time spend in printfs). Both llvm-dwarfdump and Registry.cpp.o I was dumping were built with -O3 -g, with asserts disabled (no FDO, LTO or other fancy stuff). This resulted in about 4.5 megabytes of debug_loc for parsing in Registry.cpp.o. Then I used the linux perf command to run llvm-dwarfdump -debug-loc 1000 times and dump the stats.

The baseline stats are:

  27.285986      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.11% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,813      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 58,831,163      cycles:u                  #    2.156 GHz                      ( +-  0.07% )
    606,986      stalled-cycles-frontend:u #    1.03% frontend cycles idle     ( +-  3.76% )
  7,924,778      stalled-cycles-backend:u  #   13.47% backend cycles idle      ( +-  0.33% )
146,588,727      instructions:u            #    2.49  insn per cycle         
                                           #    0.05  stalled cycles per insn  ( +-  0.00% )
 29,545,620      branches:u                # 1082.813 M/sec                    ( +-  0.00% )
    222,276      branch-misses:u           #    0.75% of all branches          ( +-  0.15% )
 
0.027663381 seconds time elapsed                                          ( +-  0.11% )

The stats with this patch applied are:

  27.397390      task-clock:u (msec)       #    0.987 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,833      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 60,160,571      cycles:u                  #    2.196 GHz                      ( +-  0.07% )
    584,825      stalled-cycles-frontend:u #    0.97% frontend cycles idle     ( +-  3.37% )
 10,729,974      stalled-cycles-backend:u  #   17.84% backend cycles idle      ( +-  0.26% )
156,141,836      instructions:u            #    2.60  insn per cycle         
                                           #    0.07  stalled cycles per insn  ( +-  0.00% )
 31,599,940      branches:u                # 1153.392 M/sec                    ( +-  0.00% )
    221,247      branch-misses:u           #    0.70% of all branches          ( +-  0.06% )
 
0.027771865 seconds time elapsed                                          ( +-  0.10% )

The stats for a version of this patch which additionally checks for the error flag before attempting the parse (as discussed in the inline comment) are:

  27.808349      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,839      page-faults:u             #    0.102 M/sec                    ( +-  0.24% )
 62,887,388      cycles:u                  #    2.261 GHz                      ( +-  0.06% )
    575,264      stalled-cycles-frontend:u #    0.91% frontend cycles idle     ( +-  3.18% )
 14,757,888      stalled-cycles-backend:u  #   23.47% backend cycles idle      ( +-  0.23% )
167,562,307      instructions:u            #    2.66  insn per cycle         
                                           #    0.09  stalled cycles per insn  ( +-  0.00% )
 33,414,152      branches:u                # 1201.587 M/sec                    ( +-  0.00% )
    221,454      branch-misses:u           #    0.66% of all branches          ( +-  0.12% )
 
0.028201319 seconds time elapsed                                          ( +-  0.10% )

As can be seen, this patch increases the parsing time by about 0.4%. This is enough to be statistically significant in a benchmark like this, but probably not world-shattering (some slowdown is unavoidable with a change like this).

If we additionally enable the early returns we get an additional 1.5% slowdown (or 1.9% above baseline). Still not bad for a "micro-benchmark", but it does make one wonder, whether it is really worth it. My feeling would be that it isn't...

Sorry I've lost some of the context here (my brain's like a sieve sometimes) - the first set of results come from this patch which, is this correct: implements a wrapper around DataExtractor that handles the offset comparison to determine validity? (if the offset isn't modified, the attempted parsing was invalid in some way)

And the second results are from adding a separate "if (already seen an error) return;" to these same wrapped function calls?

What if "if (already seen an error)" was added to the "isValidOffset" function (making all offsets invalid if there's already an error) - would that collapse the early return codepaths & perhaps make things simpler/faster?

labath updated this revision to Diff 210090.Jul 16 2019, 7:03 AM
  • update the patch to reflect our offline conversation with @dblaikie. We decided to go for the API which makes most sense (i.e. skip all reads as soon as one of them returns an error), at least until there is evidence that this makes a difference in practice (one can always prove that there is a slowdown here with a suitable micro-benchmark).
  • add tests for the new APIs
  • move the refactoring of other parsing classes into a separate patch (keeping this patch solely about DataExtractor).
labath retitled this revision from WIP: DataExtractor error handling to Add error handling to the DataExtractor class.Jul 16 2019, 7:08 AM
labath edited the summary of this revision. (Show Details)

I think this should be ready for a proper review now. To see the new API in action, please take a look at D64798.

Minor stuff.

include/llvm/Support/DataExtractor.h
169–174

Need to add Err to the doxygen.

243–244

Need to add Err to the doxygen.

280

it -> if

306–307

Need to add Err to the doxygen.

371–372

Need to add Err to the doxygen.

419–420

Need to add Err to the doxygen.

487–494

Need to add Err to the doxygen.

491

So eof() intentionally ignores the error state? (Just making sure the contract is understood.)

unittests/Support/DataExtractorTest.cpp
10

The Error is part of the DataExtractor interface, you should not need to #include this here.

266

Test for eof() in the error case? (attempt to read too far, but eof is still false, if that's the correct contract.)

labath marked 12 inline comments as done.Jul 17 2019, 1:52 AM
labath added inline comments.
include/llvm/Support/DataExtractor.h
491

Yes, that's analogous to std::iostream, where eof() tests the eofbit and operator bool checks the failbit (we don't have a badbit because there's no way to get a hard read error when reading from memory).

unittests/Support/DataExtractorTest.cpp
10

This is the testing support header which defines stuff that makes things like EXPECT_THAT_ERROR(..., Succeeded()) work.

labath updated this revision to Diff 210270.Jul 17 2019, 1:53 AM
labath marked an inline comment as done.
labath edited the summary of this revision. (Show Details)

Add doxygen comments and fix typo.

probinson accepted this revision.Jul 17 2019, 8:02 AM

I'm happy, but other people obviously have better eyesight than I do. Give Jonas and Blaikie a day to chime in, I think.

unittests/Support/DataExtractorTest.cpp
10

Doh! I'm due for an eye test.

This revision is now accepted and ready to land.Jul 17 2019, 8:02 AM
dblaikie added inline comments.Jul 17 2019, 11:01 AM
lib/Support/DataExtractor.cpp
18–21

Not sure this function adds value compared to writing the "createStringError" call directly in "unexpectedEndReached"?

28

The problem with this is that it clears the "unchecked" bit in the Error, which means code using DataExtractor would not get an assertion failure in Error about it being unchecked.

Might be worth having a gunit death test to demonstrate that failing to check the Error after parsing some fields does assert/crash.

34

Is the LLVM_UNLIKELY justified by performance data? (again, microbenchmarks could probably justify it in many parts of LLVM where it doesn't make a difference in practice - so I'd be inclined to leave these out for now)

I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)

I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)

That sounds like a good idea to me. It shouldn't complicate the DataExtractor implementation too much (it should be enough to make the getU & co. static functions templated also on the offset type), and since adopting the Cursor API will require changes to the code using the DataExtractor anyway, it will make it easier to check that the code is compatible with 64-bit offsets.

labath marked 3 inline comments as done.Jul 18 2019, 7:10 AM
labath added inline comments.
lib/Support/DataExtractor.cpp
18–21

Indeed. This was a relict from earlier versions of this patch..

28

I already have such tests (DataExtractorDeathTest, below). The reason this works is because ErrorAsOutParameter clears the "checked" flag when the function returns.

We could design a way to check this that is statically known to be safe, but I'm not sure if it's worth it for a function that is local to this file (the only method i can think of involves subclassing/wrapping ErrorAsOutParameter).

34

No, I don't have any performance data for this. I'll leave these out...

labath updated this revision to Diff 210558.Jul 18 2019, 7:12 AM
  • address feedback from @dblaikie
  • waiting to hear more opinions before updating the patch to use size_t
labath updated this revision to Diff 211720.Jul 25 2019, 5:12 AM

Use size_t in the Cursor versions of the API

labath requested review of this revision.Jul 25 2019, 5:15 AM

I finally got around to updating the patch for size_t. It ended up looking slightly uglier than I hoped for (like the need for a special isValidOffsetForDataOfSizeT), but OTOH I also was able to remove the default-null error arguments from the "legacy" APIs.

Overall I don't think this can be made much cleaner without porting existing users to size_t first. Please LMK what you think.

This probably needs to be rebased now that D64006 is in?

labath planned changes to this revision.Aug 9 2019, 1:01 AM

Yeah, I noticed that go by (pretty good stuff btw), but I didn't get around to updating this yet..

labath updated this revision to Diff 216597.Aug 22 2019, 6:23 AM

Rebase/reimplement the patch on top of the uint64_t changes

labath updated this revision to Diff 216600.Aug 22 2019, 6:25 AM
  • clang-format

All the API overloads that take a Cursor should have doxygen descriptions.
No other complaints.

dblaikie added inline comments.Aug 22 2019, 1:46 PM
include/llvm/Support/DataExtractor.h
54–76

I don't feel strongly, but figured I'd mention it - do you reckon this encapsulation is worthwhile compared to having a struct with the Offset and Error being public members? It doesn't look like there'd be a lot of misuse if the two members were public.

lib/Support/DataExtractor.cpp
31

You can skip the 0 here, if you like, "return T();" should do the right thing, if I recall correctly.

36

And here

38

But maybe it'd be simpler to move "T val = 0;" (or "T val = T();") earlier and "return val" in those places that have "return T(0);"?

unittests/Support/DataExtractorTest.cpp
225

This produces undefined behavior in the non-death case (the dtor will run twice - importantly it'll run on something that isn't an object of the right type (because that object's already been destroyed)

You could use an Optional<Cursor> to have control over the point of destruction (then "EXPECT_DEATH(opt.reset()..." to observe the unchecked Error).

@lhames - mind checking the testing of Error handling is following best practices?

labath updated this revision to Diff 216839.Aug 23 2019, 7:10 AM
labath marked 12 inline comments as done.
  • Add doxygen comments. I've tried to be more brief than the existing comments, as I have found them exessively verbose and repetitive (they're longer than the implementation!).
  • Fix the double destruction in the death tests
  • Remove the template argument from the vector version of the getU8 method -- there's a lot of confusion about whether bytes should be stored as chars or uint8_ts and this was an attempt to placate both. I now think this is not a good idea, and I will try to make the users consistent instead.
labath added inline comments.Aug 23 2019, 7:10 AM
include/llvm/Support/DataExtractor.h
54–76

No, I don't think the user could do much harm. I suppose one could somehow end up accidentally taking the address of the contained Offset member, use that for parsing but then later expect the regular cursor semantics to apply. But that probably not very likely to happen, At the end of the day, it just seems nicer to me to have accessor functions instead of the user twiddling with the fields of the struct directly, but that's highly subjective.

lib/Support/DataExtractor.cpp
31

Yeah, it will, except for the Uint24 pseudo-type, which does not have the default constructor. I could add the default constructor, but that does not matter now with the new code layout (I think val = 0 looks better than val = T())

unittests/Support/DataExtractorTest.cpp
225

Done. I've gone with a std::unique_ptr because it makes the construction of the cursor slightly nicer (missing an in_place_t for llvm::Optional).

labath updated this revision to Diff 216841.Aug 23 2019, 7:15 AM
  • make sure the tests actually compile after removing the getU8 template
dblaikie accepted this revision.Aug 23 2019, 4:39 PM

Looks good to me - if Lang has some ideas on how to tidy up the testing, that can be dealt with in follow-up commits.

This revision is now accepted and ready to land.Aug 23 2019, 4:39 PM
This revision was automatically updated to reflect the committed changes.