Add error handling to the DataExtractor class

[labath]

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.

[labath]

Besides prototyping the "DataStream" class, this also converts debug_loc and accel table (the two pieces of code I'm familiar with) parsers to use it in order to demonstrate how would this look in action.

I have also highlighted some of the open questions in inline comments.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

409–413

This shows that if reaching the end of the stream is the only kind of error one may encounter, then he doesn't need an Expected<T> return type, as the error flag is implicitly returned through the stream.

That may be viewed as a good thing, or as a bad one... :)

Since these are just private methods, I would say that's a good thing here.

include/llvm/Support/DataExtractor.h

434

This isn't consistent with iostreams (which detect eof only after one attempts to read past it), but it seems to me that this makes using the class much simpler.

442

maybe readUXX would be better instead of getUXX?

445

As can be seen here, checking whether the offset is updated (which is currently the only way of checking for errors) is a very tricky thing, since some functions can also "succeed" while not updating the offset.

470–474

To be consistent with iostreams we should stop attempting to read data once we encounter the first error. Doing that would add another branch to the code. Not doing that opens up the possibility for some weird behavior, where we first try to read a uint64_t and fail, but then try to read a uint8_t and succeed because the stream happened to have one more byte left. This could be mitigated by setting Offset to UINT32_MAX on failure.

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

72

This code had a bug where it did not check the value of the NumAtoms field. So it could end up returning millions of empty/invalid Atoms while still claiming the accelerator table is valid.

441–444

This check wasn't fully correct, because we could still cross the EntriesBase boundary after reading the two ULEB fields.

485–488

Some nice way of slicing the DataExtractor/DataStream would also be handy. That would also be useful when parsing DIEs from a DWARF unit (to make sure we don't cross into the neighbouring unit).

[JDevlieghere]

I like the overall direction of this!

include/llvm/Support/DataExtractor.h

425

We'll definitely need to add some comment here to motivate its existence, probably contrasting it with the statelessness of the DataExtractor. I saw this patch first and I wondered "why not add this to the DataExtractor directly?", before catching up on the other patch.

442

+1

466

Should we store an llvm::Error here instead? I guess it doesn't matter much if there's only one error we can detect, but OTOH it'd be nice if we're going to convert them anyway, and all these errors are consistent.

[labath]

include/llvm/Support/DataExtractor.h

466

If we enlist the help of DataExtractor class, then there may be more kinds of errors that we can detect (and I'm thinking we should enlist it, because checking for errors via these offsets is tricky and probably slower than if the DataExtractor set a flag directly). The one error kind that comes to mind is "invalid uleb128" -- right now the uleb functions will happily read a 1 megabyte uleb, if all the bytes have bit7 set.

Storing an Error object as a member variable is a bit tricky due to the checked flag, noncopyability, etc. We could definitely at least have a member function that returns an Error object. But I think the first question we need to answer is what kind of error semantics do we want to have here. Should it be something strictly optional (as it is right now), or should it be something that blows up if you forget to check for errors (like llvm::Error does)...

I don't really have an opinion here, as I can see a case for both things..

[dblaikie]

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)

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

[probinson]

The new API doesn't let you eliminate *all* checks. :-)
Also it introduces some new dependencies as noted inline.

include/llvm/Support/DataExtractor.h

425

Also that this depends on DataExtractor "get" methods returning zero for past-the-end calls.

428

assert(Offset <= DE.size()); to guard against construction with a bogus Offset?

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

60

NumAtoms still isn't validated before being used, and could consume all available memory before you get to the error check.

389

You're now resizing the string without validating the new size.

441

This depends on extractAttributeEncoding()returning a sentinel if you run off the end of the stream. That depends on sentinalAttrEnc() using the same values that getULEB128() returns when it runs off the end. These dependencies need to be documented.

492

Again the requirements on sentinels need to be documented (not here, but where those functions are defined).

lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

148

Might want a static_assert for that?

[probinson]

lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

181

You've lost a range check on the size.

[labath]

Cursor implementation.

[labath]

In D63713#1556126, @dblaikie wrote:

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

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

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

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]

lib/Support/DataExtractor.cpp

37 ↗

(On Diff #206430)

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]

lib/Support/DataExtractor.cpp

37 ↗

(On Diff #206430)

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]

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...

[dblaikie]

In D63713#1566605, @labath wrote:

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]

• 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]

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

[probinson]

Minor stuff.

include/llvm/Support/DataExtractor.h

130

Need to add Err to the doxygen.

192

Need to add Err to the doxygen.

228

it -> if

235

Need to add Err to the doxygen.

293

Need to add Err to the doxygen.

334

Need to add Err to the doxygen.

395

Need to add Err to the doxygen.

399

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

unittests/Support/DataExtractorTest.cpp

10 ↗

(On Diff #210090)

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

266 ↗

(On Diff #210090)

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

[labath]

include/llvm/Support/DataExtractor.h

399

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 ↗

(On Diff #210090)

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

[labath]

Add doxygen comments and fix typo.

[probinson]

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 ↗

(On Diff #210090)

Doh! I'm due for an eye test.

[dblaikie]

lib/Support/DataExtractor.cpp

17–20 ↗

(On Diff #210270)

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

27 ↗

(On Diff #210270)

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.

37 ↗

(On Diff #210270)

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)

[probinson]

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.)

[labath]

In D63713#1591371, @probinson wrote:

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]

lib/Support/DataExtractor.cpp

17–20 ↗

(On Diff #210270)

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

27 ↗

(On Diff #210270)

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).

37 ↗

(On Diff #210270)

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

[labath]

• address feedback from @dblaikie
• waiting to hear more opinions before updating the patch to use size_t

[labath]

Use size_t in the Cursor versions of the API

[labath]

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.

[probinson]

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

[labath]

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

[labath]

Rebase/reimplement the patch on top of the uint64_t changes

[labath]

• clang-format

[probinson]

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

[dblaikie]

include/llvm/Support/DataExtractor.h

53–75

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 ↗

(On Diff #216600)

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

36 ↗

(On Diff #216600)

And here

38 ↗

(On Diff #216600)

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 ↗

(On Diff #216600)

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).

[dblaikie]

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

[labath]

• 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]

include/llvm/Support/DataExtractor.h

53–75

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 ↗

(On Diff #216600)

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 ↗

(On Diff #216600)

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]

• make sure the tests actually compile after removing the getU8 template

[dblaikie]

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.

include/llvm/DebugInfo/DWARF/DWARFAcceleratorTable.h

public:
   class NameIndex;
   class NameIterator;
   class ValueIterator;
 
   /// DWARF v5 Name Index header.
   struct Header : public HeaderPOD {
     SmallString<8> AugmentationString;
 
-    Error extract(const DWARFDataExtractor &AS, uint32_t *Offset);
+    void extract(DWARFDataStream &DS);
     void dump(ScopedPrinter &W) const;
   };
 
   /// Index attribute and its encoding.
   struct AttributeEncoding {
     dwarf::Index Index;
     dwarf::Form Form;
 
class NameIndex {
     void dumpLocalTUs(ScopedPrinter &W) const;
     void dumpForeignTUs(ScopedPrinter &W) const;
     void dumpAbbreviations(ScopedPrinter &W) const;
     bool dumpEntry(ScopedPrinter &W, uint32_t *Offset) const;
     void dumpName(ScopedPrinter &W, const NameTableEntry &NTE,
                   Optional<uint32_t> Hash) const;
     void dumpBucket(ScopedPrinter &W, uint32_t Bucket) const;
 
-    Expected<AttributeEncoding> extractAttributeEncoding(uint32_t *Offset);
+    AttributeEncoding extractAttributeEncoding(DataStream &DS);
 
-    Expected<std::vector<AttributeEncoding>>
-    extractAttributeEncodings(uint32_t *Offset);
+    std::vector<AttributeEncoding> extractAttributeEncodings(DataStream &DS);
 
-    Expected<Abbrev> extractAbbrev(uint32_t *Offset);
+    Abbrev extractAbbrev(DataStream &DS);
 
   public:
     NameIndex(const DWARFDebugNames &Section, uint32_t Base)
         : Section(Section), Base(Base) {}
 
     /// Reads offset of compilation unit CU. CU is 0-based.
     uint32_t getCUOffset(uint32_t CU) const;
     uint32_t getCUCount() const { return Hdr.CompUnitCount; }

include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

public:
   /// For these values, \p AbsPosOffset is used to fix them, which should
   /// reflect the absolute address of this pointer.
   Optional<uint64_t> getEncodedPointer(uint32_t *Offset, uint8_t Encoding,
                                        uint64_t AbsPosOffset = 0) const;
 
   size_t size() const { return Section == nullptr ? 0 : Section->Data.size(); }
 };
 
+class DWARFDataStream : public DataStream {
+public:
+  DWARFDataStream(const DWARFDataExtractor &DE, uint32_t Offset)
+      : DataStream(DE, Offset) {}
+
+  const DWARFDataExtractor &getExtractor() const {
+    return static_cast<const DWARFDataExtractor &>(DataStream::getExtractor());
+  }
+
+  uint64_t getRelocatedAddress(uint64_t *SecIx = nullptr) {
+    uint32_t SavedOffset = Offset;
+    uint64_t Result = getExtractor().getRelocatedAddress(&Offset, SecIx);
+    if (SavedOffset == Offset)
+      Error = true;
+    return Result;
+  }
+};
+
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARFDATAEXTRACTOR_H

include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h

public:
 
   /// Parse the debug_loc section accessible via the 'data' parameter using the
   /// address size also given in 'data' to interpret the address ranges.
   void parse(const DWARFDataExtractor &data);
 
   /// Return the location list at the given offset or nullptr.
   LocationList const *getLocationListAtOffset(uint64_t Offset) const;
 
-  static Expected<LocationList> parseOneLocationList(DWARFDataExtractor Data,
-                                                     uint32_t *Offset);
+  static Expected<LocationList> parseOneLocationList(DWARFDataStream &DS);
 };
 
 class DWARFDebugLoclists {
 public:
   struct Entry {
     uint8_t Kind;
     uint64_t Value0;
     uint64_t Value1;

 public:
   void parse(DataExtractor data, unsigned Version);
   void dump(raw_ostream &OS, uint64_t BaseAddr, const MCRegisterInfo *RegInfo,
             Optional<uint64_t> Offset) const;
 
   /// Return the location list at the given offset or nullptr.
   LocationList const *getLocationListAtOffset(uint64_t Offset) const;
 
-  static Expected<LocationList>
-  parseOneLocationList(DataExtractor Data, unsigned *Offset, unsigned Version);
+  static Expected<LocationList> parseOneLocationList(DataStream &DS,
+                                                     unsigned Version);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFDEBUGLOC_H

include/llvm/Support/DataExtractor.h

public:
   /// Construct with a buffer that is owned by the caller.
   ///
   /// This constructor allows us to use data that is owned by the
   /// caller. The data must stay around as long as this object is
   /// valid.
   DataExtractor(StringRef Data, bool IsLittleEndian, uint8_t AddressSize)
     : Data(Data), IsLittleEndian(IsLittleEndian), AddressSize(AddressSize) {}
 
   /// Get the data pointed to by this extractor.
   StringRef getData() const { return Data; }
   /// Get the endianness for this extractor.
   bool isLittleEndian() const { return IsLittleEndian; }
   /// Get the address size for this extractor.
   uint8_t getAddressSize() const { return AddressSize; }
   /// Set the address size for this extractor.
   void setAddressSize(uint8_t Size) { AddressSize = Size; }
 
   /// Extract a C string from \a *offset_ptr.
   ///
   /// Returns a pointer to a C String from the data at the offset
   /// pointed to by \a offset_ptr. A variable length NULL terminated C
   /// string will be extracted and the \a offset_ptr will be
   /// updated with the offset of the byte that follows the NULL
   /// terminator byte.
   ///
   /// @param[in,out] offset_ptr
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     A pointer to the C string value in the data. If the offset
   ///     pointed to by \a offset_ptr is out of bounds, or if the
   ///     offset plus the length of the C string is out of bounds,
   ///     NULL will be returned.
   const char *getCStr(uint32_t *offset_ptr) const;
 
public:
   ///     unmodified.
   ///
   /// @param[in] byte_size
   ///     The size in byte of the integer to extract.
   ///
   /// @return
   ///     The unsigned integer value that was extracted, or zero on
   ///     failure.
   uint64_t getUnsigned(uint32_t *offset_ptr, uint32_t byte_size) const;
 
   /// Extract an signed integer of size \a byte_size from \a *offset_ptr.
   ///
   /// Extract a single signed integer value (sign extending if required)
   /// and update the offset pointed to by \a offset_ptr. The size of
   /// the extracted integer is specified by the \a byte_size argument.
   /// \a byte_size should have a value greater than or equal to one
   /// and less than or equal to eight since the return value is 64
public:
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     The extracted uint8_t value.
   uint8_t getU8(uint32_t *offset_ptr) const;
 
   /// Extract \a count uint8_t values from \a *offset_ptr.
   ///
   /// Extract \a count uint8_t values from the binary data at the
   /// offset pointed to by \a offset_ptr, and advance the offset on
   /// success. The extracted values are copied into \a dst.
   ///
   /// @param[in,out] offset_ptr
public:
   /// Extract a uint16_t value from \a *offset_ptr.
   ///
   /// Extract a single uint16_t from the binary data at the offset
   /// pointed to by \a offset_ptr, and update the offset on success.
   ///
   /// @param[in,out] offset_ptr
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     The extracted uint16_t value.
   //------------------------------------------------------------------
   uint16_t getU16(uint32_t *offset_ptr) const;
 
   /// Extract \a count uint16_t values from \a *offset_ptr.
   ///
   /// Extract \a count uint16_t values from the binary data at the
   /// offset pointed to by \a offset_ptr, and advance the offset on
   /// success. The extracted values are copied into \a dst.
   ///
   /// @param[in,out] offset_ptr
public:
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     The extracted uint32_t value.
   uint32_t getU32(uint32_t *offset_ptr) const;
 
   /// Extract \a count uint32_t values from \a *offset_ptr.
   ///
   /// Extract \a count uint32_t values from the binary data at the
   /// offset pointed to by \a offset_ptr, and advance the offset on
   /// success. The extracted values are copied into \a dst.
   ///
   /// @param[in,out] offset_ptr
public:
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     The extracted uint64_t value.
   uint64_t getU64(uint32_t *offset_ptr) const;
 
   /// Extract \a count uint64_t values from \a *offset_ptr.
   ///
   /// Extract \a count uint64_t values from the binary data at the
   /// offset pointed to by \a offset_ptr, and advance the offset on
   /// success. The extracted values are copied into \a dst.
   ///
   /// @param[in,out] offset_ptr
public:
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
   /// @return
   ///     The extracted unsigned integer value.
   uint64_t getULEB128(uint32_t *offset_ptr) const;
 
   /// Test the validity of \a offset.
   ///
   /// @return
   ///     \b true if \a offset is a valid offset into the data in this
   ///     object, \b false otherwise.
   bool isValidOffset(uint32_t offset) const { return Data.size() > offset; }
 
   /// Test the availability of \a length bytes of data from \a offset.
   ///
   /// @return
   ///     \b true if \a offset is a valid offset and there are \a
public:
   ///     \b true if \a offset is a valid offset and there are enough
   ///     bytes for a pointer available at that offset, \b false
   ///     otherwise.
   bool isValidOffsetForAddress(uint32_t offset) const {
     return isValidOffsetForDataOfSize(offset, AddressSize);
   }
 };
 
+class DataStream {
+public:
+  DataStream(const DataExtractor &DE, uint32_t Offset = 0)
+      : DE(DE), Offset(Offset), Error(false) {}
+  DataStream(const DataStream &) = delete;
+  DataStream &operator=(const DataStream &) = delete;
+
+  uint32_t tell() const { return Offset; }
+  explicit operator bool() const { return !Error; }
+  bool eof() const { return Offset == DE.getData().size(); }
+  void skip(uint32_t Value) {
+    if (DE.isValidOffsetForDataOfSize(Offset, Value))
+      Offset += Value;
+    else
+      Error = true;
+  }
+
+  uint8_t getU8() { return wrap<uint8_t>(&DataExtractor::getU8); }
+  uint8_t *getU8(uint8_t *dst, uint32_t count) {
+    // If count == 0, Offset is validly not updated.
+    return count ? wrap<uint8_t *>(&DataExtractor::getU8, dst, count) : dst;
+  }
+  uint16_t getU16() { return wrap<uint16_t>(&DataExtractor::getU16); }
+  uint32_t getU32() { return wrap<uint32_t>(&DataExtractor::getU32); }
+  uint64_t getULEB128() { return wrap(&DataExtractor::getULEB128); }
+  uint64_t getAddress() { return wrap(&DataExtractor::getAddress); }
+  StringRef getBytes(uint32_t Size) {
+    if (!DE.isValidOffsetForDataOfSize(Offset, Size)) {
+      Error = true;
+      return StringRef();
+    }
+    StringRef Bytes = DE.getData().substr(Offset, Size);
+    Offset += Size;
+    return Bytes;
+  }
+
+  const DataExtractor &getExtractor() const { return DE; }
+
+protected:
+  const DataExtractor &DE;
+  uint32_t Offset;
+  bool Error;
+
+  template <typename Ret, typename... Ts>
+  Ret wrap(Ret (DataExtractor::*Fun)(uint32_t *, Ts...) const, Ts... Vals) {
+    uint32_t SavedOffset = Offset;
+    Ret Result = (DE.*Fun)(&Offset, Vals...);
+    if (SavedOffset == Offset)
+      Error = true;
+    return Result;
+  }
+};
+
 } // namespace llvm
 
 #endif

lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp

 }
 } // namespace
 
 static Atom formatAtom(unsigned Atom) { return {Atom}; }
 
 DWARFAcceleratorTable::~DWARFAcceleratorTable() = default;
 
 Error AppleAcceleratorTable::extract() {
-  uint32_t Offset = 0;
+  DataStream DS(AccelSection);
 
-  // Check that we can at least read the header.
-  if (!AccelSection.isValidOffset(offsetof(Header, HeaderDataLength) + 4))
-    return createStringError(errc::illegal_byte_sequence,
-                             "Section too small: cannot read header.");
+  Hdr.Magic = DS.getU32();
+  Hdr.Version = DS.getU16();
+  Hdr.HashFunction = DS.getU16();
+  Hdr.BucketCount = DS.getU32();
+  Hdr.HashCount = DS.getU32();
+  Hdr.HeaderDataLength = DS.getU32();
 
-  Hdr.Magic = AccelSection.getU32(&Offset);
-  Hdr.Version = AccelSection.getU16(&Offset);
-  Hdr.HashFunction = AccelSection.getU16(&Offset);
-  Hdr.BucketCount = AccelSection.getU32(&Offset);
-  Hdr.HashCount = AccelSection.getU32(&Offset);
-  Hdr.HeaderDataLength = AccelSection.getU32(&Offset);
+  HdrData.DIEOffsetBase = DS.getU32();
+  uint32_t NumAtoms = DS.getU32();
+
+  for (unsigned i = 0; i < NumAtoms; ++i) {
+    uint16_t AtomType = DS.getU16();
+    auto AtomForm = static_cast<dwarf::Form>(DS.getU16());
+    HdrData.Atoms.push_back(std::make_pair(AtomType, AtomForm));
+  }
+
+  if (!DS)
+    return createStringError(errc::illegal_byte_sequence, "Section too small");
 
   // Check that we can read all the hashes and offsets from the
   // section (see SourceLevelDebugging.rst for the structure of the index).
   // We need to substract one because we're checking for an *offset* which is
   // equal to the size for an empty table and hence pointer after the section.
   if (!AccelSection.isValidOffset(sizeof(Hdr) + Hdr.HeaderDataLength +
                                   Hdr.BucketCount * 4 + Hdr.HashCount * 8 - 1))
     return createStringError(
         errc::illegal_byte_sequence,
         "Section too small: cannot read buckets and hashes.");
 
-  HdrData.DIEOffsetBase = AccelSection.getU32(&Offset);
-  uint32_t NumAtoms = AccelSection.getU32(&Offset);
-
-  for (unsigned i = 0; i < NumAtoms; ++i) {
-    uint16_t AtomType = AccelSection.getU16(&Offset);
-    auto AtomForm = static_cast<dwarf::Form>(AccelSection.getU16(&Offset));
-    HdrData.Atoms.push_back(std::make_pair(AtomType, AtomForm));
-  }
-
   IsValid = true;
   return Error::success();
 }
 
 uint32_t AppleAcceleratorTable::getNumBuckets() { return Hdr.BucketCount; }
 uint32_t AppleAcceleratorTable::getNumHashes() { return Hdr.HashCount; }
 uint32_t AppleAcceleratorTable::getSizeHdr() { return sizeof(Hdr); }
 uint32_t AppleAcceleratorTable::getHeaderDataLength() {
void DWARFDebugNames::Header::dump(ScopedPrinter &W) const {
   W.printNumber("Local TU count", LocalTypeUnitCount);
   W.printNumber("Foreign TU count", ForeignTypeUnitCount);
   W.printNumber("Bucket count", BucketCount);
   W.printNumber("Name count", NameCount);
   W.printHex("Abbreviations table size", AbbrevTableSize);
   W.startLine() << "Augmentation: '" << AugmentationString << "'\n";
 }
 
-Error DWARFDebugNames::Header::extract(const DWARFDataExtractor &AS,
-                                             uint32_t *Offset) {
-  // Check that we can read the fixed-size part.
-  if (!AS.isValidOffset(*Offset + sizeof(HeaderPOD) - 1))
-    return createStringError(errc::illegal_byte_sequence,
-                             "Section too small: cannot read header.");
-
-  UnitLength = AS.getU32(Offset);
-  Version = AS.getU16(Offset);
-  Padding = AS.getU16(Offset);
-  CompUnitCount = AS.getU32(Offset);
-  LocalTypeUnitCount = AS.getU32(Offset);
-  ForeignTypeUnitCount = AS.getU32(Offset);
-  BucketCount = AS.getU32(Offset);
-  NameCount = AS.getU32(Offset);
-  AbbrevTableSize = AS.getU32(Offset);
-  AugmentationStringSize = alignTo(AS.getU32(Offset), 4);
+void DWARFDebugNames::Header::extract(DWARFDataStream &DS) {
+  UnitLength = DS.getU32();
+  Version = DS.getU16();
+  Padding = DS.getU16();
+  CompUnitCount = DS.getU32();
+  LocalTypeUnitCount = DS.getU32();
+  ForeignTypeUnitCount = DS.getU32();
+  BucketCount = DS.getU32();
+  NameCount = DS.getU32();
+  AbbrevTableSize = DS.getU32();
 
-  if (!AS.isValidOffsetForDataOfSize(*Offset, AugmentationStringSize))
-    return createStringError(
-        errc::illegal_byte_sequence,
-        "Section too small: cannot read header augmentation.");
+  AugmentationStringSize = alignTo(DS.getU32(), 4);
   AugmentationString.resize(AugmentationStringSize);
-  AS.getU8(Offset, reinterpret_cast<uint8_t *>(AugmentationString.data()),
+  DS.getU8(reinterpret_cast<uint8_t *>(AugmentationString.data()),
            AugmentationStringSize);
-  return Error::success();
 }
 
 void DWARFDebugNames::Abbrev::dump(ScopedPrinter &W) const {
   DictScope AbbrevScope(W, ("Abbreviation 0x" + Twine::utohexstr(Code)).str());
   W.startLine() << formatv("Tag: {0}\n", Tag);
 
   for (const auto &Attr : Attributes)
     W.startLine() << formatv("{0}: {1}\n", Attr.Index, Attr.Form);

 DWARFDebugNames::Abbrev DWARFDebugNames::AbbrevMapInfo::getEmptyKey() {
   return sentinelAbbrev();
 }
 
 DWARFDebugNames::Abbrev DWARFDebugNames::AbbrevMapInfo::getTombstoneKey() {
   return DWARFDebugNames::Abbrev(~0, dwarf::Tag(0), {});
 }
 
-Expected<DWARFDebugNames::AttributeEncoding>
-DWARFDebugNames::NameIndex::extractAttributeEncoding(uint32_t *Offset) {
-  if (*Offset >= EntriesBase) {
-    return createStringError(errc::illegal_byte_sequence,
-                             "Incorrectly terminated abbreviation table.");
-  }
-
-  uint32_t Index = Section.AccelSection.getULEB128(Offset);
-  uint32_t Form = Section.AccelSection.getULEB128(Offset);
+DWARFDebugNames::AttributeEncoding
+DWARFDebugNames::NameIndex::extractAttributeEncoding(DataStream &DS) {
+  uint32_t Index = DS.getULEB128();
+  uint32_t Form = DS.getULEB128();
   return AttributeEncoding(dwarf::Index(Index), dwarf::Form(Form));
 }
 
-Expected<std::vector<DWARFDebugNames::AttributeEncoding>>
-DWARFDebugNames::NameIndex::extractAttributeEncodings(uint32_t *Offset) {
+std::vector<DWARFDebugNames::AttributeEncoding>
+DWARFDebugNames::NameIndex::extractAttributeEncodings(DataStream &DS) {
   std::vector<AttributeEncoding> Result;
   for (;;) {
-    auto AttrEncOr = extractAttributeEncoding(Offset);
-    if (!AttrEncOr)
-      return AttrEncOr.takeError();
-    if (isSentinel(*AttrEncOr))
-      return std::move(Result);
+    AttributeEncoding AttrEnc = extractAttributeEncoding(DS);
+    if (isSentinel(AttrEnc))
+      return Result;
 
-    Result.emplace_back(*AttrEncOr);
+    Result.emplace_back(AttrEnc);
   }
 }
 
-Expected<DWARFDebugNames::Abbrev>
-DWARFDebugNames::NameIndex::extractAbbrev(uint32_t *Offset) {
-  if (*Offset >= EntriesBase) {
-    return createStringError(errc::illegal_byte_sequence,
-                             "Incorrectly terminated abbreviation table.");
-  }
-
-  uint32_t Code = Section.AccelSection.getULEB128(Offset);
+DWARFDebugNames::Abbrev
+DWARFDebugNames::NameIndex::extractAbbrev(DataStream &DS) {
+  uint32_t Code = DS.getULEB128();
   if (Code == 0)
     return sentinelAbbrev();
 
-  uint32_t Tag = Section.AccelSection.getULEB128(Offset);
-  auto AttrEncOr = extractAttributeEncodings(Offset);
-  if (!AttrEncOr)
-    return AttrEncOr.takeError();
-  return Abbrev(Code, dwarf::Tag(Tag), std::move(*AttrEncOr));
+  uint32_t Tag = DS.getULEB128();
+  std::vector<AttributeEncoding> AttrEnc = extractAttributeEncodings(DS);
+  return Abbrev(Code, dwarf::Tag(Tag), std::move(AttrEnc));
 }
 
 Error DWARFDebugNames::NameIndex::extract() {
-  const DWARFDataExtractor &AS = Section.AccelSection;
-  uint32_t Offset = Base;
-  if (Error E = Hdr.extract(AS, &Offset))
-    return E;
+  DWARFDataStream DS(Section.AccelSection, Base);
+  Hdr.extract(DS);
 
-  CUsBase = Offset;
-  Offset += Hdr.CompUnitCount * 4;
-  Offset += Hdr.LocalTypeUnitCount * 4;
-  Offset += Hdr.ForeignTypeUnitCount * 8;
-  BucketsBase = Offset;
-  Offset += Hdr.BucketCount * 4;
-  HashesBase = Offset;
+  CUsBase = DS.tell();
+  DS.skip(Hdr.CompUnitCount * 4);
+  DS.skip(Hdr.LocalTypeUnitCount * 4);
+  DS.skip(Hdr.ForeignTypeUnitCount * 8);
+  BucketsBase = DS.tell();
+  DS.skip(Hdr.BucketCount * 4);
+  HashesBase = DS.tell();
   if (Hdr.BucketCount > 0)
-    Offset += Hdr.NameCount * 4;
-  StringOffsetsBase = Offset;
-  Offset += Hdr.NameCount * 4;
-  EntryOffsetsBase = Offset;
-  Offset += Hdr.NameCount * 4;
-
-  if (!AS.isValidOffsetForDataOfSize(Offset, Hdr.AbbrevTableSize))
+    DS.skip(Hdr.NameCount * 4);
+  StringOffsetsBase = DS.tell();
+  DS.skip(Hdr.NameCount * 4);
+  EntryOffsetsBase = DS.tell();
+  DS.skip(Hdr.NameCount * 4);
+
+  if (!DS)
+    return createStringError(errc::illegal_byte_sequence, "Section too small.");
+
+  uint32_t AbbrevBase = DS.tell();
+  if (!DS.getExtractor().isValidOffsetForDataOfSize(AbbrevBase,
+                                                    Hdr.AbbrevTableSize))
     return createStringError(errc::illegal_byte_sequence,
                              "Section too small: cannot read abbreviations.");
 
-  EntriesBase = Offset + Hdr.AbbrevTableSize;
+  EntriesBase = DS.tell() + Hdr.AbbrevTableSize;
 
+  DataExtractor AbbrevExtractor(
+      DS.getExtractor().getData().substr(AbbrevBase, Hdr.AbbrevTableSize),
+      DS.getExtractor().isLittleEndian(), DS.getExtractor().getAddressSize());
+  DataStream AbbrevStream(AbbrevExtractor, 0);
   for (;;) {
-    auto AbbrevOr = extractAbbrev(&Offset);
-    if (!AbbrevOr)
-      return AbbrevOr.takeError();
-    if (isSentinel(*AbbrevOr))
-      return Error::success();
+    Abbrev Abbr = extractAbbrev(AbbrevStream);
+    if (isSentinel(Abbr))
+      break;
 
-    if (!Abbrevs.insert(std::move(*AbbrevOr)).second)
+    if (!Abbrevs.insert(std::move(Abbr)).second)
       return createStringError(errc::invalid_argument,
                                "Duplicate abbreviation code.");
   }
+  if (!AbbrevStream)
+    return createStringError(errc::illegal_byte_sequence,
+                             "Incorrectly terminated abbreviation table.");
+  return Error::success();
 }
 
 DWARFDebugNames::Entry::Entry(const NameIndex &NameIdx, const Abbrev &Abbr)
     : NameIdx(&NameIdx), Abbr(&Abbr) {
   // This merely creates form values. It is up to the caller
   // (NameIndex::getEntry) to populate them.
   Values.reserve(Abbr.Attributes.size());
   for (const auto &Attr : Abbr.Attributes)

lib/DebugInfo/DWARF/DWARFDebugLoc.cpp

void DWARFDebugLoc::dump(raw_ostream &OS, const MCRegisterInfo *MRI,
   }
 
   for (const LocationList &L : Locations) {
     DumpLocationList(L);
   }
 }
 
 Expected<DWARFDebugLoc::LocationList>
-DWARFDebugLoc::parseOneLocationList(DWARFDataExtractor Data, unsigned *Offset) {
+DWARFDebugLoc::parseOneLocationList(DWARFDataStream &DS) {
   LocationList LL;
-  LL.Offset = *Offset;
+  LL.Offset = DS.tell();
 
   // 2.6.2 Location Lists
   // A location list entry consists of:
   while (true) {
     Entry E;
-    if (!Data.isValidOffsetForDataOfSize(*Offset, 2 * Data.getAddressSize()))
-      return createOverflowError("debug_loc");
 
     // 1. A beginning address offset. ...
-    E.Begin = Data.getRelocatedAddress(Offset);
+    E.Begin = DS.getRelocatedAddress();
 
     // 2. An ending address offset. ...
-    E.End = Data.getRelocatedAddress(Offset);
+    E.End = DS.getRelocatedAddress();
 
+    if (!DS)
+      return createOverflowError("debug_loc");
     // The end of any given location list is marked by an end of list entry,
     // which consists of a 0 for the beginning address offset and a 0 for the
     // ending address offset.
     if (E.Begin == 0 && E.End == 0)
       return LL;
 
-    if (!Data.isValidOffsetForDataOfSize(*Offset, 2))
-      return createOverflowError("debug_loc");
-
-    unsigned Bytes = Data.getU16(Offset);
-    if (!Data.isValidOffsetForDataOfSize(*Offset, Bytes))
-      return createOverflowError("debug_loc");
-
+    unsigned Bytes = DS.getU16();
     // A single location description describing the location of the object...
-    StringRef str = Data.getData().substr(*Offset, Bytes);
-    *Offset += Bytes;
+    StringRef str = DS.getBytes(Bytes);
     E.Loc.reserve(str.size());
     llvm::copy(str, std::back_inserter(E.Loc));
     LL.Entries.push_back(std::move(E));
   }
 }
 
 void DWARFDebugLoc::parse(const DWARFDataExtractor &data) {
   IsLittleEndian = data.isLittleEndian();
   AddressSize = data.getAddressSize();
 
-  uint32_t Offset = 0;
-  while (data.isValidOffset(Offset + data.getAddressSize() - 1)) {
-    if (auto LL = parseOneLocationList(data, &Offset))
+  DWARFDataStream DS(data, 0);
+  while (!DS.eof()) {
+    if (auto LL = parseOneLocationList(DS))
       Locations.push_back(std::move(*LL));
     else {
       logAllUnhandledErrors(LL.takeError(), WithColor::error());
       break;
     }
   }
-  if (data.isValidOffset(Offset))
-    WithColor::error() << "failed to consume entire .debug_loc section\n";
 }
 
 Expected<DWARFDebugLoclists::LocationList>
-DWARFDebugLoclists::parseOneLocationList(DataExtractor Data, unsigned *Offset,
-                                         unsigned Version) {
+DWARFDebugLoclists::parseOneLocationList(DataStream &DS, unsigned Version) {
   LocationList LL;
-  LL.Offset = *Offset;
-
-  while (true) {
-    if (!Data.isValidOffset(*Offset))
-      return createOverflowError("debug_loclists");
+  LL.Offset = DS.tell();
 
+  // dwarf::DW_LLE_end_of_list_entry is 0 and indicates the end of the list.
+  while (auto Kind = static_cast<dwarf::LocationListEntry>(DS.getU8())) {
     Entry E;
-    E.Kind = static_cast<dwarf::LocationListEntry>(Data.getU8(Offset));
-    if (E.Kind == dwarf::DW_LLE_end_of_list)
-      return LL;
-
-    // Verify that the reads were sucessful by checking the offset field before
-    // and after the read. This relies on DataExtractor not updating the offset
-    // on failure.
-    // TODO: Simplify this code once we have a better way of handling
-    // DataExtractor errors.
-    unsigned SavedOffset = *Offset;
-    switch (E.Kind) {
-    case dwarf::DW_LLE_startx_length:
-    case dwarf::DW_LLE_offset_pair:
-      E.Value0 = Data.getULEB128(Offset);
-      break;
-    case dwarf::DW_LLE_start_length:
-    case dwarf::DW_LLE_base_address:
-      E.Value0 = Data.getAddress(Offset);
-      break;
-    default:
-      return createError("LLE of kind %x not implemented", (int)E.Kind);
-    }
-    if (SavedOffset == *Offset)
-      return createOverflowError("debug_loclists");
-
-    SavedOffset = *Offset;
-    switch (E.Kind) {
+    E.Kind = Kind;
+    switch (Kind) {
     case dwarf::DW_LLE_startx_length:
+      E.Value0 = DS.getULEB128();
       // Pre-DWARF 5 has different interpretation of the length field. We have
       // to support both pre- and standartized styles for the compatibility.
       if (Version < 5)
-        E.Value1 = Data.getU32(Offset);
+        E.Value1 = DS.getU32();
       else
-        E.Value1 = Data.getULEB128(Offset);
+        E.Value1 = DS.getULEB128();
       break;
     case dwarf::DW_LLE_start_length:
+      E.Value0 = DS.getAddress();
+      E.Value1 = DS.getULEB128();
+      break;
     case dwarf::DW_LLE_offset_pair:
-      E.Value1 = Data.getULEB128(Offset);
+      E.Value0 = DS.getULEB128();
+      E.Value1 = DS.getULEB128();
       break;
     case dwarf::DW_LLE_base_address:
+      E.Value0 = DS.getAddress();
       break;
+    default:
+      return createError("LLE of kind %x not implemented", (int)E.Kind);
     }
-    if (E.Kind != dwarf::DW_LLE_base_address) {
-      if (SavedOffset == *Offset)
-        return createOverflowError("debug_loclists");
-
-      SavedOffset = *Offset;
-      unsigned Bytes =
-          Version >= 5 ? Data.getULEB128(Offset) : Data.getU16(Offset);
-      if (SavedOffset == *Offset ||
-          !Data.isValidOffsetForDataOfSize(*Offset, Bytes))
-        return createOverflowError("debug_loclists");
 
+    if (Kind != dwarf::DW_LLE_base_address) {
+      unsigned Bytes = Version >= 5 ? DS.getULEB128() : DS.getU16();
       // A single location description describing the location of the object...
-      StringRef str = Data.getData().substr(*Offset, Bytes);
-      *Offset += Bytes;
+      StringRef str = DS.getBytes(Bytes);
       E.Loc.resize(str.size());
       llvm::copy(str, E.Loc.begin());
     }
 
     LL.Entries.push_back(std::move(E));
   }
+  if (!DS)
+    return createOverflowError("debug_loclists");
+  return LL;
 }
 
 void DWARFDebugLoclists::parse(DataExtractor data, unsigned Version) {
   IsLittleEndian = data.isLittleEndian();
   AddressSize = data.getAddressSize();
 
-  uint32_t Offset = 0;
-  while (data.isValidOffset(Offset)) {
-    if (auto LL = parseOneLocationList(data, &Offset, Version))
+  DataStream DS(data);
+  while (!DS.eof()) {
+    if (auto LL = parseOneLocationList(DS, Version))
       Locations.push_back(std::move(*LL));
     else {
       logAllUnhandledErrors(LL.takeError(), WithColor::error());
       return;
     }
   }
 }
 

lib/DebugInfo/DWARF/DWARFDie.cpp

static void dumpLocation(raw_ostream &OS, DWARFFormValue &FormValue,
 
   FormValue.dump(OS, DumpOpts);
   if (FormValue.isFormClass(DWARFFormValue::FC_SectionOffset)) {
     uint32_t Offset = *FormValue.getAsSectionOffset();
     if (!U->isDWOUnit() && !U->getLocSection()->Data.empty()) {
       DWARFDebugLoc DebugLoc;
       DWARFDataExtractor Data(Obj, *U->getLocSection(), Ctx.isLittleEndian(),
                               Obj.getAddressSize());
-      if (auto LL = DebugLoc.parseOneLocationList(Data, &Offset)) {
+      DWARFDataStream DS(Data, Offset);
+      if (auto LL = DebugLoc.parseOneLocationList(DS)) {
         uint64_t BaseAddr = 0;
         if (Optional<object::SectionedAddress> BA = U->getBaseAddress())
           BaseAddr = BA->Address;
         LL->dump(OS, Ctx.isLittleEndian(), Obj.getAddressSize(), MRI, U,
                  BaseAddr, Indent);
       } else
         logAllUnhandledErrors(LL.takeError(), OS,
                               "error extracting location list:");
       return;
     }
 
     bool UseLocLists = !U->isDWOUnit();
     StringRef LoclistsSectionData =
         UseLocLists ? Obj.getLoclistsSection().Data : U->getLocSectionData();
 
     if (!LoclistsSectionData.empty()) {
       DataExtractor Data(LoclistsSectionData, Ctx.isLittleEndian(),
                          Obj.getAddressSize());
 
       // Old-style location list were used in DWARF v4 (.debug_loc.dwo section).
       // Modern locations list (.debug_loclists) are used starting from v5.
       // Ideally we should take the version from the .debug_loclists section
       // header, but using CU's version for simplicity.
+      DataStream DS(Data, Offset);
       auto LL = DWARFDebugLoclists::parseOneLocationList(
-          Data, &Offset, UseLocLists ? U->getVersion() : 4);
+          DS, UseLocLists ? U->getVersion() : 4);
 
       uint64_t BaseAddr = 0;
       if (Optional<object::SectionedAddress> BA = U->getBaseAddress())
         BaseAddr = BA->Address;
 
       if (LL)
         LL->dump(OS, BaseAddr, Ctx.isLittleEndian(), Obj.getAddressSize(), MRI,
                  U, Indent);

test/tools/llvm-dwarfdump/X86/debug-names-verify-short1.s

 # RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj | \
 # RUN:   not llvm-dwarfdump -verify - | FileCheck %s
 
-# CHECK: Section too small: cannot read header.
+# CHECK: Section too small.
 
   .section  .debug_str,"MS",@progbits,1
 .Lstring_producer:
   .asciz  "Hand-written dwarf"
 
   .section  .debug_abbrev,"",@progbits
 .Lsection_abbrev:
   .byte 1                       # Abbreviation Code

test/tools/llvm-dwarfdump/X86/debug-names-verify-short2.s

 # RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj | \
 # RUN:   not llvm-dwarfdump -verify - | FileCheck %s
 
-# CHECK: Section too small: cannot read header augmentation.
+# CHECK: Section too small.
 
   .section  .debug_str,"MS",@progbits,1
 .Lstring_producer:
   .asciz  "Hand-written dwarf"
 
   .section  .debug_abbrev,"",@progbits
 .Lsection_abbrev:
   .byte 1                       # Abbreviation Code