[DWARF] Rework debug line parsing to use llvm::Error and callbacks

[jhenderson]

Summary

The .debug_line parser previously reported errors by printing to stderr and return false. This is not particularly helpful for clients of the library code, as it prevents them from handling the errors in a manner based on the calling context. This change switches to using llvm::Error and callbacks to indicate what problems were detected during parsing, and has updated clients to handle the errors in a location-specific manner. In general, this means that they continue to do the same thing to external users. Below, I have outlined what the known behaviour changes are, relating to this change.

There are two levels of "errors" in the new error mechanism, to broadly distinguish between different fail states of the parser, since not every failure will prevent parsing of the unit, or of subsequent unit. Malformed table errors that prevent reading the remainder of the table (reported by returning them) and other minor issues representing problems with parsing that do not prevent attempting to continue reading the table (reported by calling a specified callback funciton). The only example of this currently is when the last sequence of a unit is unterminated. However, I think it would be good to change the handling of unrecognised opcodes to report as minor issues as well, rather than just printing to the stream if --verbose is used (this would be a subsequent change however).

I have substantially extended the DwarfGenerator to be able to handle custom-crafted .debug_line sections, allowing for comprehensive unit-testing of the parser code. For now, I am just adding unit tests to cover the basic error reporting, and positive cases, and do not currently intend to test every part of the parser, although the framework should be sufficient to do so at a later point.

Known behaviour changes:

• The dump function in DWARFContext now does not attempt to read subsequent tables when searching for a specific offset, if the unit length field of a table before the specified offset is a reserved value.
• getOrParseLineTable now returns a useful Error if an invalid offset is encountered, rather than simply a nullptr.
• The parse functions no longer use WithColor::warning directly to report errors, allowing LLD to call its own warning function.
• The existing parse error messages have been updated to not specifically include "warning" in their message, allowing consumers to determine what severity the problem is.
• If the line table version field appears to have a value less than 2, an informative error is returned, instead of just false.
• If the line table unit length field uses a reserved value, an informative error is returned, instead of just false.
• Dumping of .debug_line.dwo sections is now implemented the same as regular .debug_line sections.
• Verbose dumping of .debug_line[.dwo] sections now prints the prologue, if there is a prologue error, just like non-verbose dumping.

As a helper for the generator code, I have re-added emitInt64 to the AsmPrinter code. This previously existed, but was removed way back in r100296, presumably because it was dead at the time.

This change also requires a change to LLD. I have posted D44562 for this, since I don't know how to reliably create a review for both repositories simultaneously.

I'm conscious that this is quite a large change, so if anybody has any suggestions on how to usefully break it up, please let me know.

[jhenderson]

Use bool conversion initialiser instead of ternary.

[JDevlieghere]

Hi James, thanks for taking the time to come up with an alternative. I very much prefer this approach over the previous differential!

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

34

Let's move this comment to isFatal. I think the name of the variable is sufficiently self-explanatory for the constructor.

36

How about switching the order and giving the bool a default value (false?)

47

It won't matter here (yet) but padding-wise it's (generally) better to have the smaller types last.

[JDevlieghere]

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

59

How do you feel about the following idea: what if we use this function as the callback and have it take *and* return an error. If the function handles the error (i.e. print it), this returns success, and the caller continues. If the error is fatal and/or prevents further parsing, we defer the problem and we have the parse method return the underlying error. Would that fit this use case?

65

I think something like warn would be sufficiently clear.

278

I'm not convinced on the minor/major nomenclature. I think we could two things here:

• Rename this to something like warnCallback to make it clear that this is what the callback is for.
• Make this a callback that takes an error *and* returns an error, allowing the callback to decide whether to handle it and how. Probably I'm over-engineering it here, but conceptually it would be nice to have the client decide whether the issue is "minor" or not. For example, one client could decide to just print the input error as a warning (and then return success(), while another could simply forward the error. This does add complexity to the parse method, as we then would need to check whether the callback returned success or not, and return the error in the latter case.

FWIW I don't think we need the second option here, but from an interface-design point of view it could be interesting.

[jhenderson]

Thanks for the comments @JDevlieghere. I'll get an updated diff posted later, maybe Monday.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

59

It's possible I don't fully understand what you're saying, but I'm not sure that would work. This would need to be a different callback to the warn callback, since the behaviour in the latter is to continue parsing the table, which is different to what is needed for other problems. We could perhaps have the callback take a message, and a severity, which could allow it to print a message or raise an error as appropriate, but I'm not sure I see any benefit in that over what is in D44382.

65

It's a namespacing thing - I don't want this function confused with, say, LLD's "warn" function, which may not do the same thing. I guess an argument could be made for adding a "warn" function the Support library, or similar, though I'm not sure about that. Maybe I could make it a static method of DWARFDebugLine?

278

I really don't think it's the place of the callback to decide whether or not parsing a LineTable can continue or not. The parser knows the current state. It should decide.

I deliberately chose the naming scheme to make it clear that this doesn't cover other kinds of issues found in parsing (i.e. the ones for which an Error are returned). However, I guess from the point of view of the parser, there are issues preventing it completing (which could be considered errors), and other issues that don't prevent reading (which could be considered warnings).

[jhenderson]

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

36

I tried doing this, but there was little benefit, since this is always created via createError, which is a function with variadic templates, making it impossible (to my knowledge) to use a default argument in createError. I could still change it, but IsFatal will still be specified explicitly every time it is called.

[JDevlieghere]

I hope the code snippet below helps clarify my original suggestion. Like I said earlier, this might be taking things a step to far and I'm not convinced whether the flexibility outweighs the added complexity.

// Library implementation A print a warning and is happy to have the parser
// continue after the warning.
Error parseCallback(Error ParseError)
{
  if (ParseError) {
    WithColor(errs(), HighlightColor::Warning).get() << "warning: ";
    errs() << Message;
  }
  return Error::succes();
}

// Library implementation B is more pedantic and wants parsing to abort after a
// warning.
Error parseCallback(Error ParseError)
{
  return ParseError;
}

Error DWARFDebugLine::LineTable::parse(...)
{
  ... if (!State.Sequence.Empty)
  {
    // The parser checks whether the 'minor issue' has been dealt with by the
    // callback. If not, it aborts parsing and returns the error.
    if (Error Err = parseCallback(createError("last sequence in debug line table is not terminated!")))
      return Err;
  }
  ...
}

If we go this route we could have the same interface for handleDebugLineParseErrors and communicate that we aborted parsing because of a fatal error.

Error llvm::handleDebugLineParseErrors(Error ParseErrors) {
  bool FatalError = false;
  handleAllErrors(std::move(ParseErrors), ...);
  if (FatalError) 
    return make_error<StringError>("Parsing of this line table was aborted because fatal error encountered");
  return Error:succes();
}

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

36

Alright, I'll leave the decision up to you. My reasoning was that it doesn't harm and might make things easier in the future if we ever want to construct this error without the createError helper.

65

Makes sense, the static method sounds good to me.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

42

Should a fatal error not be displayed as an error instead of a warning?

[jhenderson]

I was in the middle of updating this diff when you made your latest comment! Thanks for the explanation. I figured out what you meant subsequent to my previous comments (see the inline comment which I'd written before I saw your explanation). If you're okay with it, I'd prefer to keep the callback as it currently is in this diff (apart from anything else, it keeps the LLD-side changes simpler). If we get a concrete use-case later, maybe then is the time to make the change?

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

36

That's a fair point. I'll make that change.

278

I realised that I completely misunderstood your second bullet-point here. From my understanding, you were suggesting that the callback would be called for "minor" issues, and it would then return Error (success for the default behaviour of just printing), i.e. the signature of the callback is std::function<Error(StringRef)>. I certainly agree that is kind of interesting, but I'm not sure what the use-case of it is currently. The obvious case is for wanting to treat warnings as errors, but the only current user of that in this situation to my knowledge is LLD, which has a function that does all the work for us (see the changes in D44562 and the warn function), so I suggest that this change is saved for when there is a concrete use-case.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

42

The intention of this function is to provide a "default" handler for all current users of the code. Prior to my change, all problems detected were reported as warnings to the end user (or simply not printed at all), and whatever it was doing would simply stop, and not cause an error. I'd prefer to keep it that way for now (i.e. in this change). I think we could make a case for changing to emitting errors, not warnings, but I'm not sure this change is necessarily the right place to have that discussion, since it is a more fundamental change in behaviour in tools like llvm-dwarfdump.

[jhenderson]

Addressed review comments:

• Renamed and moved warnForMinorIssue as discussed.
• Made IsFatal a default argument.
• Moved fatal error comment.
• Reordered members of DebugLineError.

[JDevlieghere]

In D44560#1041532, @jhenderson wrote:

I was in the middle of updating this diff when you made your latest comment! Thanks for the explanation. I figured out what you meant subsequent to my previous comments (see the inline comment which I'd written before I saw your explanation). If you're okay with it, I'd prefer to keep the callback as it currently is in this diff (apart from anything else, it keeps the LLD-side changes simpler). If we get a concrete use-case later, maybe then is the time to make the change?

Yup, totally fine. Now we have some kind of record in case this issue ever comes up.

Also, great work on extending the dwarf generator, it looks very useful!

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

301

s/MinorIssueCallback/WarnCallback/

lib/DebugInfo/DWARF/DWARFContext.cpp

436–437

Not sure if this much better, but this way you don't need to "check" the error twice?

if (Error Err = LineTable.Prologue.parse(LineData, &Offset, *this, U)) 
  if (handleDebugLineParseErrors(std::move(Err)))
    break;
else if (!DumpOffset || OldOffset == *DumpOffset))
  LineTable.dump(OS, DumpOpts);

444

Also looks like verbose dumping of the line tables is missing for DWO. I filed PR36800.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

42

Alright, I'm okay with keeping it a warning for now. We can always revisit this in the future.

[jhenderson]

Addressed comments - renamed missed function declaration parameter and reworked logic to no longer "check" an Error twice (done in two locations).

Thanks for the help so far.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

301

Oops, well spotted!

lib/DebugInfo/DWARF/DWARFContext.cpp

436–437

Thanks I prefer that (the boolean usage looked ugly). I've made the same change above for .debug_line.

[• espindola]

One thing that I am probably missing is why we need this much.

On the lld side we only read this section when there is an error, and if reading it fails we just want to print the extra info to the user.

My guess is that something like lldb would be similar, read the line table and print a message if it failed.

For both cases a simple Expected<> should be sufficient, no?

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

292–294

Why reformat this?

[• rafael]

David Blaikie via llvm-commits <[e-mail]> writes:

[jhenderson]

There's been quite a bit of discussion on the mailing list between @espindola and @dblaikie over the callback, and the consensus there seems to be that the callback should return an Error.

To clarify a few things (most of these came up with the discussion on the topic with @JDevlieghere earlier, but are summarised here for clarity):

• The callback is only called for recoverable problems (currently only a missing end sequence, but I think there is scope to extend it to e.g. unrecognised opcodes). By changing this to return an Error, we make the parser have to handle unhandled Errors in this context.
• At the moment, there are no users for stopping after the first recoverable error is found. Every user at the moment I think would benefit from getting all the information about minor issues rather than stopping, so I'd prefer not to extend this interface at this point (though I could be persuaded otherwise).
• In general, the table is in a (reasonably) valid state after a recoverable issue and the rest of the table can be parsed, but not after other issues. As such I'd be opposed to the default suggested by @espindola of issuing an error and stopping in this situation - this would be a change in behaviour from the existing behaviour, and I don't think brings any great benefit. The default should be to print a warning and continue.
• There are 3 different "bad" states of parsing currently: the recoverable issues discussed above, unrecoverable issues for parsing this line table (e.g. an invalid prologue length), but which do not prevent further reading of later tables (the unit length is valid, so the caller can use this to skip to the next table), and unrecoverable issues for which the unit length is invalid, preventing knowing where to skip to to continue reading. The latter is what I have described as "Fatal" errors.
• We need between 1 and 3 callbacks for these different types, if we want to handle everything via a callback (which would allow us to drop the custom error class). I could see the signatures being as follows:
  • For a 1-callback model: std::function<Error(StringRef, Severity)>, where "Severity" is an enum representing "Recoverable/UnrecoverableButValidUnitLength/Fatal". It would be up to the callback code itself to set some external state if the caller needs to distinguish between the different failure modes. This would be my preference if people are opposed to a new error type.
  • For a 2-callback model, the first callback would be simply std::function<Error(StringRef)> and could be used for recoverable issues. The other would take a boolean as well, indicating fatal or not, which would be used by the callback to set some state to decide whether to attempt to parse the next table or not.
  • For a 3-callback model, each would have the same signature ( std::function<Error(StringRef)>), and the different callbacks could all be the same function, or could be different, if the caller cared about different states. Again, external state would need to be set if we cared about the difference between fatal and non-fatal issues.

@dblaikie said this on the mailing list in response to @espindola's comment "Having which errors are fatal be a property of the particular error type is odd."

Agreed - I'd rather not introduce the complexity of semantically meaningful Error types, if it's reasonable to do so.

The callback would only be used for recoverable errors, though, yes? Allowing the user to differentiate the two cases ("recoverable things come through the callback, non-recoverable things are errors seen as a result that didn't pass through/come from the callback"). Seems fair to me.

I think you may have missed the difference between errors which allow parsing later tables and errors which prevent parsing any later tables. If the unrecoverable errors do not go through a callback, we need the extra information of whether the length field is valid or not available to the caller somehow. I suppose we could add an extra method to the LineTable class instead to query whether the length is valid or not instead of the custom error type, but there's a risk here that users will miss this function and still try to use the length field.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

292–294

The second line is over 80 characters. I did this whilst I was touching the functions immediately below, but can revert it if it's bothersome.

[• espindola]

The callback would only be used for recoverable errors, though, yes? Allowing the user to differentiate the two cases ("recoverable things come through the callback, non-recoverable things are errors seen as a result that didn't pass through/come from the callback"). Seems fair to me.

I think you may have missed the difference between errors which allow parsing later tables and errors which prevent parsing any later tables. If the unrecoverable errors do not go through a callback, we need the extra information of whether the length field is valid or not available to the caller somehow. I suppose we could add an extra method to the LineTable class instead to query whether the length is valid or not instead of the custom error type, but there's a risk here that users will miss this function and still try to use the length field.

I don't think we missed it. The callback is used when there is any way that the parsing can continue. The callback should be passed information about the issue. What should not happen is for the code is the library to have the notion of an Error severity.

We have discussed two use cases. In dwarfdump we want to parse as much as we can and print whatever issues are found to stderr.

In lld we would be happy to stop as soon as any issue is found, but as you point out that is not required and we can also just print every issue to stderr.

Neither of these suggests the need for a severity. In fact, given the above it seems the callback can return void, take a StringRef and the parsing code produces an Error only when it cannot recover. What use case do you have in mind that needs more than this?

Cheers,
Rafael

[jhenderson]

In D44560#1043915, @espindola wrote:

I don't think we missed it. The callback is used when there is any way that the parsing can continue. The callback should be passed information about the issue. What should not happen is for the code is the library to have the notion of an Error severity.

We have discussed two use cases. In dwarfdump we want to parse as much as we can and print whatever issues are found to stderr.

In lld we would be happy to stop as soon as any issue is found, but as you point out that is not required and we can also just print every issue to stderr.

Neither of these suggests the need for a severity. In fact, given the above it seems the callback can return void, take a StringRef and the parsing code produces an Error only when it cannot recover. What use case do you have in mind that needs more than this?

I think the phrase "parsing can continue" may be a bit overloaded in the current context: the parser only deals with a single table at a time. In some situations, it sees issues that don't preventing parsing of the rest of that table, where in the existing behaviour it just prints to stderr and continues. Other errors cause the parser to return false, and to print a warning.

Some callers, such as llvm-dwarfdump, loop over and parse all the tables, rather than a specific one at a given offset. It is this additional behaviour that needs supporting. Hopefully the following explanation should clear that up a bit more:

In llvm-dwarfdump, the current behaviour, prior to my change, is to stop as soon as any line table is found that cannot be fully parsed, or to continue parsing that line table after printing a warning, if a minor issue is found that does not make the line table unreadable. Preventing further parsing is achieved by the parser resetting the offset value and the caller checking if the offset has been reset. I think we all agree that this is not ideal, because there is the potential to be able to parse later tables after the broken one. However, in some situations, this is not possible, because the unit length is broken in some crazy way (in this diff, only possible if illegally using a reserved value, although I plan to add more checks later), so we need to be able to distinguish between this in different ways. The parser could indicate this in one of four ways that I can think of:

1. As with the existing behaviour, reset the length field, and then have the caller check this. The callback would be used in the same manner as the current diff (i.e. for errors that don't prevent continuation of parsing the current table). Other problems would be returned as Errrors (there would be no need for additional information in this case).
2. Add a method to LineTable::Prologue that states whether the unit length is valid or not somehow. The caller would then be responsible for checking this, if it might want to continue onto the next table (as in the dwarfdump case), but can also ignore it and stop. The callback would be used in the same manner as above.
3. As in the current version of this diff, provide information in the Error returned to the caller, which the caller uses, if it wishes to. Again, the callback would be used as above.
4. Pass additional information into the callback, making the callback signature Error(bool IsUnitLengthValid, StringRef Msg) or similar. The parser should always bail out with the returned Error, even for Error::success(), if an issue is found preventing further parsing of that table, but when it knows that it can carry on, it can check against Error::success().

In the first three cases, whether or not the callback should be changed to return an Error (potentially Error::success()), instead of void, depends on whether there is a user for it. I can certainly imagine there being one, but there currently isn't.

[jhenderson]

Lots of small changes:

• Rename some functions, tests and parameters in the unit tests, to remove reference to Minor/Major, reflecting the changein the code.
• Added checking of the Prologue fields to the valid table case.
• Added a missing argument to one of the test functions, causing it to test the wrong thing.
• Added a TODO to extend the valid-table testing to verify the body of the program. I chose not to do this at this time, as it requires fairly comprehensive testing, and the behaviour isn't changing.
• Reduced the number of test cases used for the paramterised tests, based on comments on llvm-dev.

[jhenderson]

Currently trying to rebase this change following rLLD328284. In that change, LLD was changed to call getLineTableForUnit, instead of getOrParseLineTable. This function is effectively another level up the call stack, which, if I wanted to follow the current pattern, would result in lots of callers having to consume the Expected return value, as well as pass in a callback for the warning. I was discussing this with a colleague, and one suggestion he had was to instead register separate callbacks for warnings and errors in the DWARFContext. The parser would then simply call the appropriate callback. Potentially, other parts of the DWARF library could use the same callbacks ultimately. Tools would register their callback with their instance of the DWARFContext. Thus, for LLD, it would be to always call the LLD warn function (or whatever we choose to replace it - see the discussion on D44562), whilst other consumers would continue to use the default callbacks (currently handleDebugLineParseError and DWARFDebugLine::warn). This raises the question of how to handle the fatal error case (i.e. where a valid unit length cannot be read). I think the options here are the same as before:

1. As with the existing behaviour, reset the length field, and then have the caller check this.
2. Add a method to LineTable::Prologue that states whether the unit length is valid or not somehow. The caller would then be responsible for checking this, if it might want to continue onto the next table (as in the dwarfdump case), but can also ignore it and stop.
3. As in the current version of this diff, provide information in the Error returned to the caller, which the caller uses, if it wishes to.
4. Pass additional information into the callback, making the callback signature Error(bool IsUnitLengthValid, StringRef Msg) or similar.

I'd appreciate some feedback from people on which approach seems the best, both in terms of registering the callback(s), and in handling errors that prevent further section parsing (as opposed to further table parsing).

[JDevlieghere]

In D44560#1046829, @jhenderson wrote:

Currently trying to rebase this change following rLLD328284. In that change, LLD was changed to call getLineTableForUnit, instead of getOrParseLineTable. This function is effectively another level up the call stack, which, if I wanted to follow the current pattern, would result in lots of callers having to consume the Expected return value, as well as pass in a callback for the warning. I was discussing this with a colleague, and one suggestion he had was to instead register separate callbacks for warnings and errors in the DWARFContext. The parser would then simply call the appropriate callback.

Do you mean registering some kind of error handler in the dwarf context? I guess that sounds reasonable if it’s not too intrusive. We could provide a default implementation that simply prints “warning” and “error” so that the behavior remains unchanged initially. If we decide this is the way to go it should definitely go into a separate diff.

Potentially, other parts of the DWARF library could use the same callbacks ultimately. Tools would register their callback with their instance of the DWARFContext. Thus, for LLD, it would be to always call the LLD warn function (or whatever we choose to replace it - see the discussion on D44562), whilst other consumers would continue to use the default callbacks (currently handleDebugLineParseError and DWARFDebugLine::warn). This raises the question of how to handle the fatal error case (i.e. where a valid unit length cannot be read). I think the options here are the same as before:

1. As with the existing behaviour, reset the length field, and then have the caller check this.
2. Add a method to LineTable::Prologue that states whether the unit length is valid or not somehow. The caller would then be responsible for checking this, if it might want to continue onto the next table (as in the dwarfdump case), but can also ignore it and stop.
3. As in the current version of this diff, provide information in the Error returned to the caller, which the caller uses, if it wishes to.
4. Pass additional information into the callback, making the callback signature Error(bool IsUnitLengthValid, StringRef Msg) or similar.

As the error handling for line tables is significantly different,, I see no harm in having a dedicated signature for the callback in the error handler. Based on the layering I expect we don’t have access to the co text in the parser? If anything I’d prefer not to clutter the interface too much, and directly use the context’s error handler from the parser, if at all possible.

I'd appreciate some feedback from people on which approach seems the best, both in terms of registering the callback(s), and in handling errors that prevent further section parsing (as opposed to further table parsing).

I’m currently on vacation but I’ll be able to give some more in-depth feedback early next week.

[jhenderson]

In D44560#1049021, @JDevlieghere wrote:

Do you mean registering some kind of error handler in the dwarf context? I guess that sounds reasonable if it’s not too intrusive. We could provide a default implementation that simply prints “warning” and “error” so that the behavior remains unchanged initially. If we decide this is the way to go it should definitely go into a separate diff.

Yes, that's what I mean. Not sure about the default implementation, since at least for debug line, we never "error" in the sense of printing an error and returning exit code 1 on termination, but it should be easy to do either way.

As the error handling for line tables is significantly different,, I see no harm in having a dedicated signature for the callback in the error handler. Based on the layering I expect we don’t have access to the co text in the parser? If anything I’d prefer not to clutter the interface too much, and directly use the context’s error handler from the parser, if at all possible.

Do you mean "context" in the parser (as opposed to "co text")? Because, we do actually have it, looking at the parse signatures, so we could call it directly, if we wanted (and I see no reason not to).

I'd appreciate some feedback from people on which approach seems the best, both in terms of registering the callback(s), and in handling errors that prevent further section parsing (as opposed to further table parsing).

I’m currently on vacation but I’ll be able to give some more in-depth feedback early next week.

No problem. I'm going to try experimenting with the context-based error handler a little in the meantime, and will upload another diff to present it, if I have enough time by the end of the week. FYI, I'm away on annual leave myself from Friday until Euro LLVM, so won't be reading emails until I'm back in the office (will you be around at the conference?).

[jhenderson]

So it turns out that the DWARFContext already has an error handler, which has a Halt/Continue policy (similar to what we need for debug_line)! I'm still investigating usability, but it looks like we might not need an entirely new mechanism...

[jhenderson]

In D44560#1049039, @jhenderson wrote:

So it turns out that the DWARFContext already has an error handler, which has a Halt/Continue policy (similar to what we need for debug_line)! I'm still investigating usability, but it looks like we might not need an entirely new mechanism...

I'm less enthusiastic about this now, although I think the principle of using a single error handling for DWARFContext is still the right one overall. The Halt/Continue policy is what the error handler returns, which the code then has to check. I'm not sure I see a benefit for this over either return Error, or not returning anything (and indicating success/failure in another manner), at least for debug line parsing.

[jhenderson]

Rebased to account for rLLD328284 and rL328235, plus some minor tweaks:

• Convert the fprintf added in rL328235 into a call to createError and add a unit test to match.
• Added a new signature for getLineTableForUnit that takes a callback and returns an Expected, to allow LLD to provide its own error mechanism. Existing users of the function will use the existing signature, which follows the default policy of printing Errors returned by getOrParseLineTable as warnings.
• Renamed set of function and parameters to use singular when referring to Error, since it's never expected that there will be more than one Error returned.
• Remove an extraneous FIXME comment that had snuck in previously.
• Update a comment that suggested that we might expect errors. Errors are always unexpected under normal behaviour.
• Move responsibility of appending '\n' to the error/warning message to the logging functions, for consistency with LLD behaviour.

I haven't attempted in this version to implement a new error handler mechanism for the whole DWARFContext, as I didn't have a clear idea of how to tie this in with the debug line parser (and how it needs to indicate that the line table does not have a valid length). This can be done either in a later update to this diff, or a new diff.

[jhenderson]

I will be away on annual leave for two weeks, up to the Euro LLVM conference. I've asked @andrewng to keep an eye on this and answer any questions he is able to, as he has been reviewing this internally. I will respond to any developments on here after I am back.

[dblaikie]

I'd probably guess that the best way to expose this is to use a higher
level construct than a raw offset exposed to the user of the API - some
kind of iterator should be exposed & then if parsing can't continue, it
doesn't continue. Rather than exposing the raw offset to the user & having
them have to handle passing it back in, or detecting when not to do so.

[• espindola]

A quick request: please git-clang-format.

[• espindola]

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

1941

Don't repeat names in comment.
It should start with a lowercase letter.

I will update the surrounding code.

[• espindola]

include/llvm/DebugInfo/DWARF/DWARFContext.h

262–269

Please document what this version does when there is an error/warning.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

44

I still think this is a design error.

Being fatal or not is something for the caller to decide. Looking at the code I think that the issue that makes this look necessary is that we just need two error types.

Imagine if a filesystem API didn't distinguish "no such file" and "permission denied" :-)

I have uploaded a somewhat hackish modified version to https://reviews.llvm.org/D45074.

The idea is to return a DebugLineLengthError when &Offset was not updated correctly and a StringError when it was.

Please update this patch to use something along those lines.

[JDevlieghere]

I agree with @espindola, it makes more sense to have to have a separate error.

In D44560#1049025, @jhenderson wrote:

No problem. I'm going to try experimenting with the context-based error handler a little in the meantime, and will upload another diff to present it, if I have enough time by the end of the week. FYI, I'm away on annual leave myself from Friday until Euro LLVM, so won't be reading emails until I'm back in the office (will you be around at the conference?).

Yes, I look forward to meeting you!

[dblaikie]

I'm still a bit undecided about new Error types here - I think exposing an
iterator (or iterator-like thing) rather than special changes to the offset
or an error kind the user checks to see whether to iterate might be better?

[JDevlieghere]

In D44560#1055708, @dblaikie wrote:

I'm still a bit undecided about new Error types here - I think exposing an
iterator (or iterator-like thing) rather than special changes to the offset
or an error kind the user checks to see whether to iterate might be better?

I agree, but (unless I misunderstand) that's on another level, right? We'd still need to communicate from the parser whether we can parse the next LT, i.e. if there's going to be a next iterator?

[dblaikie]

I was picturing the parser being the thing that exposes the iterator - so
it would be an internal detail & wouldn't really warrant an extra Error
type - but I haven't thought about it too hard & maybe that doesn't make
sense?

[JDevlieghere]

In D44560#1055714, @dblaikie wrote:

I was picturing the parser being the thing that exposes the iterator - so
it would be an internal detail & wouldn't really warrant an extra Error
type - but I haven't thought about it too hard & maybe that doesn't make
sense?

The way the code is currently structured, Parse initializes the object on which you call it. What you say makes a lot of sense to have in DWARFContext, or alternatively in an abstraction between the two. Still, with the iterator you don't know what the reason is you don't have a line table, which I think was the motivation for this patch (to expose this to LLD).

[dblaikie]

iterator or iterator-like thing. One option would be a callback with Error
when the iterator is retrieved - and that's called back for any error (&
the user wouldn't need to differentiate the different kinds of error - the
iterator would stop when it couldn't continue). Or an Iterator-like but
not-actually-iterator API that exposes the Error during iteration (but
that'd probably look a bit awkward & be something like pair<Error,
Optional<T>> or, yes, as suggested - a separate Error type to communicate
"fail and cotninue" separately from "fail and stop"... )

*ponders* Yeah, sort of feel like the callback would be pretty suitable - a
plain/real iterator that only gives valid line tables (or sufficiently
valid to have some interesting content for dumping, etc?) & anything
invalid triggers the Error callback, etc.

I know I've discussed this sort of API (iteration with an Error callback)
with Lang before - I think he used that in libObject in some places
already? Maybe? Might be worth chatting with him a bit?

[jhenderson]

Thanks for the comments everybody. As the two error types is not far off what I have at the moment, I will try implementing that first. I will then take a look at returning an iterator and providing an error-handling callback (and probably post it in a separate review). Similar to some of the earlier ideas I actually believe this would need to be two callbacks - one for errors and one for warnings. Signatures would be void(Error) (for errors) and void(StringRef) (for warnings). Default behaviour would be to simply print the message. Encountering a "fatal" error would cause the internal indicator for the iterator to be set to the end of the section, in addition to feeding an Error to the callback.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

44

Thanks for the idea. I think this is a good thought.

[jhenderson]

Address review comments + make other minor changes:

• Rebase
• Use lower-case 'e' for emitInt64
• Update comments for emitInt64 and new getLineTable overload.
• clang-format
• Use updated WithColor methods for default printing of warnings.
• Rename custom error class and use this for reporting "fatal" errors, and StringError otherwise.

[• espindola]

LGTM.

Since this is mainly about debug info and I not an expert in the area, please get one more LGTM before committing.

[probinson]

I'm not convinced that all of these errors/warnings are really parse-stoppers, but this patch is about the reporting mechanics and debating the merit of individual cases should happen separately.
@dblaikie or @JDevlieghere should do a final sign-off as they were the other main reviewers.

[dblaikie]

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

30

I thought at some point the review reached concensus that a separate error type was not needed - how'd it come back around again?

I think I'd expect that API users wouldn't need to know the difference - they'd get told errors and get given as many line tables as could be parsed correctly, regardless of what kind of errors they were. (in the implementation, errors that result in inability to parse more things, would stop producing more things)

Also, the mention in the patch description that this would result in inability to parse a line table contribution at a known offset seems problematic - if you have a section with some junk, then a line table, and there's a debug_info section that refers to the line table by the correct offset, the presence of junk coming before that line table doesn't seem like it should break dumping, right? But I suppose that's just "nice to have" maybe & not worth contorting the code too much to support.

[jhenderson]

Sorry for not responding, I started drafting some follow-up comments regarding iterator-like implementations, but got caught up with other work, and never finished them off.

@dblaikie wrote:

I thought at some point the review reached concensus that a separate error type was not needed - how'd it come back around again?

I got the impression that there wasn't consensus, so I was going to explore both approaches, but time has meant that I've only managed the second error type version, as presented by @espindola. In the current version, the majority of problems are now reported using StringError. If the user cares about the ability to continue to iterate, they should specifically handle DebugLineLengthError.

I think I'd expect that API users wouldn't need to know the difference - they'd get told errors and get given as many line tables as could be parsed correctly, regardless of what kind of errors they were. (in the implementation, errors that result in inability to parse more things, would stop producing more things)

It's worth noting that at the moment, only one client requires the ability to iterate over all tables in sequence (DWARFContext, in the context of dumping them all). All others just ask for the line table at a given offset, so we don't necessarily need to optimise the iterate-over-all tables API, though I agree that it would be nice to improve it to be more iterator-like.

Also, the mention in the patch description that this would result in inability to parse a line table contribution at a known offset seems problematic - if you have a section with some junk, then a line table, and there's a debug_info section that refers to the line table by the correct offset, the presence of junk coming before that line table doesn't seem like it should break dumping, right? But I suppose that's just "nice to have" maybe & not worth contorting the code too much to support.

I think you're right, although I don't think my change necessarily makes this worse - prior to my change, llvm-dwarfdump tries to read each table prologue in turn, and uses the length from the prologue to skip to the next one until it finds the correct offset. The length field could be broken in all sorts of ways, including being a reserved value. If a reserved value is hit, then the length is still used to jump to the next location, even though that is likely bogus (it's also likely outside of the section, though not necessarily).

In my implementation, if the length field is a reserved value, we stop. The rest of the time it's the same behaviour as the old behaviour. I'll update the summary to make this clearer.

A separate change could be made to simply jump directly to the desired offset rather than iterating over each table in turn, when a specific offset is specified. This would sort out this problem, and also allow further testing of the unit length field (e.g. to see if it falls within the section boundaries).

[probinson]

In D44560#1083788, @jhenderson wrote:

A separate change could be made to simply jump directly to the desired offset rather than iterating over each table in turn, when a specific offset is specified. This would sort out this problem, and also allow further testing of the unit length field (e.g. to see if it falls within the section boundaries).

Hmm one reason to iterate through the section is to gain confidence that the specified offset is actually the start of a line-table header. But I guess if we pick up an offset from a compile-unit, we should really assume it points to a reasonable place (maybe not in verify mode, but normally).

There has been a bit of grumbling on the dwarf-discuss list recently about "whiny consumers" and I think it's a valid point; we should be "tolerant in what we receive" or however the old Internet-RFC put it.

[jhenderson]

Sorry for the delay - I've been working on other things, and doing this in gaps over the past week or two. I've found what I think is a good iterator-like solution (not actually an iterator), that allows removal of the new error type. Please let me know your thoughts. Key changes are noted below.

• Added a new class SectionParser that is for incrementally parsing the line tables in a .debug_line section. Users provide callbacks that are used for reporting errors.
• Instead of a new Error type, added a method on the Prologue to determine if the length is valid. I felt that this was more appropriate, since the only client who iterates over everything uses the SectionParser.
• Modified the DWARFContext dumping of .debug_line and .debug_line.dwo to use this via a common lambda (note: this adds some additional functionality to .debug_line.dwo dumping, specifically supporting verbose dumping.
• Changed the debug line table parse function to print the prologue in verbose mode even if there is an error. This fixes a weird inconsistency between verbose and non-verbose, where non-verbose dumping would print the prologue and verbose wouldn't, if there were problems with the prologue.

[dblaikie]

Seems good - thanks for your patience.

unittests/DebugInfo/DWARF/DwarfGenerator.cpp

472–474

Usually LLVM code omits braces from single line blocks.

501–502

make_unique?

[jhenderson]

unittests/DebugInfo/DWARF/DwarfGenerator.cpp

472–474

Of course. My bad.

501–502

Ah, I didn't realise there was an llvm::make_unique. Thanks for pointing it out! I'll tweak the function above it too to match.

[jhenderson]

Fix some Linux build errors by renaming a variable (Generator -> Gen), explicitly specifying an empty variadic macro argument for INSTANTIATE_TEST_CASE_P, and reordering constructors/class members. Also made suggested changes by @dblaikie: use make_unique, and remove unnecessary braces.

[JDevlieghere]

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

299 ↗

(On Diff #146108)

Why not have both callbacks take an Error? I consider one a recoverable error and the other a non-recoverable error. It would be nice if both use the same "warn" callback by default?

[jhenderson]

llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

299 ↗

(On Diff #146108)

That's a good suggestion, thanks. I'll create a new review shortly with that change.

include/llvm/CodeGen/AsmPrinter.h

public:
   void emitInt8(int Value) const;
 
   /// Emit a short directive and value.
   void emitInt16(int Value) const;
 
   /// Emit a long directive and value.
   void emitInt32(int Value) const;
 
+  /// Emit a long long directive and value.
+  void emitInt64(uint64_t Value) const;
+
   /// Emit something like ".long Hi-Lo" where the size in bytes of the directive
   /// is specified by Size and Hi/Lo specify the labels.  This implicitly uses
   /// .set if it is available.
   void EmitLabelDifference(const MCSymbol *Hi, const MCSymbol *Lo,
                            unsigned Size) const;
 
   /// Emit something like ".uleb128 Hi-Lo".
   void EmitLabelDifferenceAsULEB128(const MCSymbol *Hi,

include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
 
   /// Get a reference to the parsed accelerator table object.
   const AppleAcceleratorTable &getAppleNamespaces();
 
   /// Get a reference to the parsed accelerator table object.
   const AppleAcceleratorTable &getAppleObjC();
 
   /// Get a pointer to a parsed line table corresponding to a compile unit.
-  const DWARFDebugLine::LineTable *getLineTableForUnit(DWARFUnit *cu);
+  /// Report any parsing issues as warnings on stderr.
+  const DWARFDebugLine::LineTable *getLineTableForUnit(DWARFUnit *U);
+
+  /// Get a pointer to a parsed line table corresponding to a compile unit.
+  /// Report any parsing warnings using the callback.
+  Expected<const DWARFDebugLine::LineTable *>
+  getLineTableForUnit(DWARFUnit *U,
+                      std::function<void(StringRef)> WarnCallback);
 
   DataExtractor getStringExtractor() const {
     return DataExtractor(DObj->getStringSection(), false, 0);
   }
   DataExtractor getLineStringExtractor() const {
     return DataExtractor(DObj->getLineStringSection(), false, 0);
   }
 

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

 #include <string>
 #include <vector>
 
 namespace llvm {
 
 class DWARFUnit;
 class raw_ostream;
 
+class DebugLineLengthError : public ErrorInfo<DebugLineLengthError> {
+public:
+  static char ID;
+
+  DebugLineLengthError(const Twine &Msg) : Msg(Msg.str()) {}
+
+  void log(raw_ostream &OS) const override;
+  std::error_code convertToErrorCode() const override {
+    return inconvertibleErrorCode();
+  }
+
+private:
+  std::string Msg;
+};
+
+/// Report any issues detected during parsing of a debug line table.
+///
+/// \param ParseError The Errors returned by a call to LineTable::parse() or
+/// Prologue::parse().
+///
+/// \returns true if the prologue length value is a valid length.
+bool handleDebugLineParseError(Error ParseError);
+
 class DWARFDebugLine {
 public:
   struct FileNameEntry {
     FileNameEntry() = default;
 
     DWARFFormValue Name;
     uint64_t DirIdx = 0;
     uint64_t ModTime = 0;
     uint64_t Length = 0;
     MD5::MD5Result Checksum;
     DWARFFormValue Source;
   };
 
   /// Tracks which optional content types are present in a DWARF file name
   /// entry format.
   struct ContentTypeTracker {
     ContentTypeTracker() = default;
 
     /// Whether filename entries provide a modification timestamp.
     bool HasModTime = false;
     /// Whether filename entries provide a file size.
struct Prologue {
     }
 
     int32_t getMaxLineIncrementForSpecialOpcode() const {
       return LineBase + (int8_t)LineRange - 1;
     }
 
     void clear();
     void dump(raw_ostream &OS, DIDumpOptions DumpOptions) const;
-    bool parse(const DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
+    Error parse(const DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
                 const DWARFContext &Ctx, const DWARFUnit *U = nullptr);
   };
 
   /// Standard .debug_line state machine structure.
   struct Row {
     explicit Row(bool DefaultIsStmt = false);
 
     /// Called after a row is appended to the matrix.
     void postAppend();
struct Sequence {
 
     bool isValid() const {
       return !Empty && (LowPC < HighPC) && (FirstRowIndex < LastRowIndex);
     }
 
     bool containsPC(uint64_t PC) const { return (LowPC <= PC && PC < HighPC); }
   };
 
+  /// Helper function for DWARFDebugLine parse functions, to report issues that
+  /// don't prevent parsing the remainder of the table, as warnings.
+  ///
+  /// \param Message The message to report.
+  static void warn(StringRef Message);
+
   struct LineTable {
     LineTable();
 
     /// Represents an invalid row
     const uint32_t UnknownRowIndex = UINT32_MAX;
 
     void appendRow(const DWARFDebugLine::Row &R) { Rows.push_back(R); }
 
struct LineTable {
     bool getFileLineInfoForAddress(uint64_t Address, const char *CompDir,
                                    DILineInfoSpecifier::FileLineInfoKind Kind,
                                    DILineInfo &Result) const;
 
     void dump(raw_ostream &OS, DIDumpOptions DumpOptions) const;
     void clear();
 
     /// Parse prologue and all rows.
-    bool parse(DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
+    Error parse(DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
                 const DWARFContext &Ctx, const DWARFUnit *U,
+                std::function<void(StringRef)> WarnCallback = warn,
                 raw_ostream *OS = nullptr);
 
     using RowVector = std::vector<Row>;
     using RowIter = RowVector::const_iterator;
     using SequenceVector = std::vector<Sequence>;
     using SequenceIter = SequenceVector::const_iterator;
 
     struct Prologue Prologue;
     RowVector Rows;
     SequenceVector Sequences;
 
   private:
     uint32_t findRowInSeq(const DWARFDebugLine::Sequence &Seq,
                           uint64_t Address) const;
-    Optional<StringRef> getSourceByIndex(uint64_t FileIndex,
-                                         DILineInfoSpecifier::FileLineInfoKind Kind) const;
+    Optional<StringRef>
+    getSourceByIndex(uint64_t FileIndex,
   };
 
   const LineTable *getLineTable(uint32_t Offset) const;
-  const LineTable *getOrParseLineTable(DWARFDataExtractor &DebugLineData,
-                                       uint32_t Offset, const DWARFContext &C,
-                                       const DWARFUnit *U);
+  Expected<const LineTable *>
+  getOrParseLineTable(DWARFDataExtractor &DebugLineData, uint32_t Offset,
+                      const DWARFContext &Ctx, const DWARFUnit *U,
+                      std::function<void(StringRef)> WarnCallback = warn);
 
 private:
   struct ParsingState {
     ParsingState(struct LineTable *LT);
 
     void resetRowAndSequence();
     void appendRowToMatrix(uint32_t Offset);
 

lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitInt16(int Value) const {
   OutStreamer->EmitIntValue(Value, 2);
 }
 
 /// Emit a long directive and value.
 void AsmPrinter::emitInt32(int Value) const {
   OutStreamer->EmitIntValue(Value, 4);
 }
 
+/// Emit a long long directive and value.
+void AsmPrinter::emitInt64(uint64_t Value) const {
+  OutStreamer->EmitIntValue(Value, 8);
+}
+
 /// Emit something like ".long Hi-Lo" where the size in bytes of the directive
 /// is specified by Size and Hi/Lo specify the labels. This implicitly uses
 /// .set if it avoids relocations.
 void AsmPrinter::EmitLabelDifference(const MCSymbol *Hi, const MCSymbol *Lo,
                                      unsigned Size) const {
   OutStreamer->emitAbsoluteSymbolDiff(Hi, Lo, Size);
 }
 

lib/DebugInfo/DWARF/DWARFContext.cpp

while (Offset < LineData.getData().size()) {
       auto It = LineToUnit.find(Offset);
       if (It != LineToUnit.end())
         U = It->second;
       LineData.setAddressSize(U ? U->getAddressByteSize() : 0);
       DWARFDebugLine::LineTable LineTable;
       if (DumpOffset && Offset != *DumpOffset) {
         // Find the size of this part of the line table section and skip it.
         unsigned OldOffset = Offset;
-        LineTable.Prologue.parse(LineData, &Offset, *this, U);
+        Error Err = LineTable.Prologue.parse(LineData, &Offset, *this, U);
+        if (!handleDebugLineParseError(std::move(Err)))
+          break;
         Offset = OldOffset + LineTable.Prologue.TotalLength +
                  LineTable.Prologue.sizeofTotalLength();
         continue;
       }
       // Verbose dumping is done during parsing and not on the intermediate
       // representation.
       OS << "debug_line[" << format("0x%8.8x", Offset) << "]\n";
       unsigned OldOffset = Offset;
       if (DumpOpts.Verbose) {
-        LineTable.parse(LineData, &Offset, *this, U, &OS);
+        Error Err = LineTable.parse(LineData, &Offset, *this, U,
+                                    DWARFDebugLine::warn, &OS);
+        if (!handleDebugLineParseError(std::move(Err)))
+          break;
       } else {
-        LineTable.parse(LineData, &Offset, *this, U);
+        if (Error Err = LineTable.parse(LineData, &Offset, *this, U)) {
+          if (!handleDebugLineParseError(std::move(Err)))
+            break;
+        } else
           LineTable.dump(OS, DIDumpOptions());
       }
-      // Check for unparseable prologue, to avoid infinite loops.
-      if (OldOffset == Offset)
-        break;
+      // If we get here, we can assume that the unit length field is valid, so
+      // use that to move Offset to past the end of the table, in case it wasn't
+      // adjusted automatically by the parser.
+      Offset = OldOffset + LineTable.Prologue.TotalLength +
+               LineTable.Prologue.sizeofTotalLength();
     }
   }
 
   if (shouldDump(ExplicitDWO, ".debug_line.dwo", DIDT_ID_DebugLine,
                  DObj->getLineDWOSection().Data)) {
     LineToUnitMap LineToUnit =
         buildLineToUnitMap(dwo_compile_units(), dwo_type_unit_sections());
     unsigned Offset = 0;
     DWARFDataExtractor LineData(*DObj, DObj->getLineDWOSection(),
                                 isLittleEndian(), 0);
     while (Offset < LineData.getData().size()) {
       DWARFUnit *U = nullptr;
       auto It = LineToUnit.find(Offset);
       if (It != LineToUnit.end())
         U = It->second;
       DWARFDebugLine::LineTable LineTable;
       unsigned OldOffset = Offset;
-      if (!LineTable.Prologue.parse(LineData, &Offset, *this, U))
+      if (Error Err = LineTable.Prologue.parse(LineData, &Offset, *this, U)) {
+        if (!handleDebugLineParseError(std::move(Err)))
           break;
-      if (!DumpOffset || OldOffset == *DumpOffset)
+      } else if (!DumpOffset || OldOffset == *DumpOffset)
         LineTable.dump(OS, DumpOpts);
+      Offset = OldOffset + LineTable.Prologue.TotalLength +
+               LineTable.Prologue.sizeofTotalLength();
     }
   }
 
   if (shouldDump(Explicit, ".debug_cu_index", DIDT_ID_DebugCUIndex,
                  DObj->getCUIndexSection())) {
     getCUIndex().dump(OS);
   }
 
   if (shouldDump(Explicit, ".debug_tu_index", DIDT_ID_DebugTUIndex,
return getAccelTable(AppleNamespaces, *DObj,
                        DObj->getStringSection(), isLittleEndian());
 }
 
 const AppleAcceleratorTable &DWARFContext::getAppleObjC() {
   return getAccelTable(AppleObjC, *DObj, DObj->getAppleObjCSection(),
                        DObj->getStringSection(), isLittleEndian());
 }
 
-const DWARFLineTable *
+const DWARFDebugLine::LineTable *
 DWARFContext::getLineTableForUnit(DWARFUnit *U) {
+  Expected<const DWARFDebugLine::LineTable *> ExpectedLineTable =
+      getLineTableForUnit(U, DWARFDebugLine::warn);
+  if (!ExpectedLineTable) {
+    handleDebugLineParseError(ExpectedLineTable.takeError());
+    return nullptr;
+  }
+  return *ExpectedLineTable;
+}
+
+Expected<const DWARFDebugLine::LineTable *>
+DWARFContext::getLineTableForUnit(DWARFUnit *U,
+                                  std::function<void(StringRef)> WarnCallback) {
   if (!Line)
     Line.reset(new DWARFDebugLine);
 
   auto UnitDIE = U->getUnitDIE();
   if (!UnitDIE)
     return nullptr;
 
   auto Offset = toSectionOffset(UnitDIE.find(DW_AT_stmt_list));
   if (!Offset)
     return nullptr; // No line table for this compile unit.
 
   uint32_t stmtOffset = *Offset + U->getLineTableOffset();
   // See if the line table is cached.
   if (const DWARFLineTable *lt = Line->getLineTable(stmtOffset))
     return lt;
 
   // Make sure the offset is good before we try to parse.
   if (stmtOffset >= U->getLineSection().Data.size())
     return nullptr;
 
   // We have to parse it first.
   DWARFDataExtractor lineData(*DObj, U->getLineSection(), isLittleEndian(),
                               U->getAddressByteSize());
-  return Line->getOrParseLineTable(lineData, stmtOffset, *this, U);
+
+  return Line->getOrParseLineTable(lineData, stmtOffset, *this, U,
+                                   WarnCallback);
 }
 
 void DWARFContext::parseCompileUnits() {
   CUs.parse(*this, DObj->getInfoSection());
 }
 
 void DWARFContext::parseTypeUnits() {
   if (!TUs.empty())

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 #include <cstdio>
 #include <utility>
 
 using namespace llvm;
 using namespace dwarf;
 
 using FileLineInfoKind = DILineInfoSpecifier::FileLineInfoKind;
 
+char DebugLineLengthError::ID = 0;
+
+void DebugLineLengthError::log(raw_ostream &OS) const { OS << Msg; }
+
+bool llvm::handleDebugLineParseError(Error ParseError) {
+  bool CanContinue = true;
+  auto LogError = [](ErrorInfoBase &Info) {
+    raw_ostream &Stream = WithColor::warning();
+    Info.log(Stream);
+    Stream << "\n";
+  };
+
+  handleAllErrors(std::move(ParseError),
+                  [&](DebugLineLengthError &Info) {
+                    LogError(Info);
+                    CanContinue = false;
+                  },
+                  [&](ErrorInfoBase &Info) { LogError(Info); });
+
+  return CanContinue;
+}
 namespace {
 
 struct ContentDescriptor {
   dwarf::LineNumberEntryFormat Type;
   dwarf::Form Form;
 };
 
 using ContentDescriptors = SmallVector<ContentDescriptor, 4>;
for (auto Descriptor : FileDescriptors) {
         break;
       }
     }
     FileNames.push_back(FileEntry);
   }
   return true;
 }
 
-bool DWARFDebugLine::Prologue::parse(const DWARFDataExtractor &DebugLineData,
+template <typename... Ts>
+static std::string formatErrorString(char const *Fmt, const Ts &... Vals) {
+  std::string Buffer;
+  raw_string_ostream Stream(Buffer);
+  Stream << format(Fmt, Vals...);
+  return Stream.str();
+}
+
+template <typename... Ts>
+static Error createError(char const *Fmt, const Ts &... Vals) {
+  return make_error<StringError>(formatErrorString(Fmt, Vals...),
+                                 inconvertibleErrorCode());
+}
+
+template <typename... Ts>
+static Error createLengthError(char const *Fmt, const Ts &... Vals) {
+  return make_error<DebugLineLengthError>(formatErrorString(Fmt, Vals...));
+}
+
+Error DWARFDebugLine::Prologue::parse(const DWARFDataExtractor &DebugLineData,
                                       uint32_t *OffsetPtr,
                                       const DWARFContext &Ctx,
                                       const DWARFUnit *U) {
   const uint64_t PrologueOffset = *OffsetPtr;
 
   clear();
   TotalLength = DebugLineData.getU32(OffsetPtr);
   if (TotalLength == UINT32_MAX) {
     FormParams.Format = dwarf::DWARF64;
     TotalLength = DebugLineData.getU64(OffsetPtr);
   } else if (TotalLength >= 0xffffff00) {
-    return false;
+    return createLengthError(
+        "parsing line table prologue at offset 0x%8.8" PRIx64
+        " unsupported reserved unit length found of value 0x%8.8" PRIx64,
+        PrologueOffset, TotalLength);
   }
   FormParams.Version = DebugLineData.getU16(OffsetPtr);
   if (getVersion() < 2)
-    return false;
+    return createError("parsing line table prologue at offset 0x%8.8" PRIx64
+                       " found unsupported version 0x%2.2" PRIx16,
+                       PrologueOffset, getVersion());
 
   if (getVersion() >= 5) {
     FormParams.AddrSize = DebugLineData.getU8(OffsetPtr);
     assert((DebugLineData.getAddressSize() == 0 ||
             DebugLineData.getAddressSize() == getAddressSize()) &&
            "Line table header and data extractor disagree");
     SegSelectorSize = DebugLineData.getU8(OffsetPtr);
   }
for (uint32_t I = 1; I < OpcodeBase; ++I) {
     uint8_t OpLen = DebugLineData.getU8(OffsetPtr);
     StandardOpcodeLengths.push_back(OpLen);
   }
 
   if (getVersion() >= 5) {
     if (!parseV5DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
                               FormParams, Ctx, U, ContentTypes,
                               IncludeDirectories, FileNames)) {
-      WithColor::warning() << format(
+      return createError(
           "parsing line table prologue at 0x%8.8" PRIx64
           " found an invalid directory or file table description at"
-          " 0x%8.8" PRIx64 "\n",
+          " 0x%8.8" PRIx64,
           PrologueOffset, (uint64_t)*OffsetPtr);
-      return false;
     }
   } else
     parseV2DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
                          ContentTypes, IncludeDirectories, FileNames);
 
-  if (*OffsetPtr != EndPrologueOffset) {
-    WithColor::warning() << format(
-        "parsing line table prologue at 0x%8.8" PRIx64
-        " should have ended at 0x%8.8" PRIx64 " but it ended at 0x%8.8" PRIx64
-        "\n",
+  if (*OffsetPtr != EndPrologueOffset)
+    return createError("parsing line table prologue at 0x%8.8" PRIx64
+                       " should have ended at 0x%8.8" PRIx64
+                       " but it ended at 0x%8.8" PRIx64,
                        PrologueOffset, EndPrologueOffset, (uint64_t)*OffsetPtr);
-    return false;
-  }
-  return true;
+  return Error::success();
 }
 
 DWARFDebugLine::Row::Row(bool DefaultIsStmt) { reset(DefaultIsStmt); }
 
 void DWARFDebugLine::Row::postAppend() {
   BasicBlock = false;
   PrologueEnd = false;
   EpilogueBegin = false;

 const DWARFDebugLine::LineTable *
 DWARFDebugLine::getLineTable(uint32_t Offset) const {
   LineTableConstIter Pos = LineTableMap.find(Offset);
   if (Pos != LineTableMap.end())
     return &Pos->second;
   return nullptr;
 }
 
-const DWARFDebugLine::LineTable *
-DWARFDebugLine::getOrParseLineTable(DWARFDataExtractor &DebugLineData,
-                                    uint32_t Offset, const DWARFContext &Ctx,
-                                    const DWARFUnit *U) {
+Expected<const DWARFDebugLine::LineTable *> DWARFDebugLine::getOrParseLineTable(
+    DWARFDataExtractor &DebugLineData, uint32_t Offset, const DWARFContext &Ctx,
+    const DWARFUnit *U, std::function<void(StringRef)> WarnCallback) {
   if (!DebugLineData.isValidOffset(Offset))
-    return nullptr;
+    return createLengthError("offset 0x%8.8" PRIx64
+                             " is not a valid debug line section offset",
+                             Offset);
 
   std::pair<LineTableIter, bool> Pos =
       LineTableMap.insert(LineTableMapTy::value_type(Offset, LineTable()));
   LineTable *LT = &Pos.first->second;
   if (Pos.second) {
-    if (!LT->parse(DebugLineData, &Offset, Ctx, U))
-      return nullptr;
+    if (Error Err = LT->parse(DebugLineData, &Offset, Ctx, U, WarnCallback))
+      return std::move(Err);
+    return LT;
   }
   return LT;
 }
 
-bool DWARFDebugLine::LineTable::parse(DWARFDataExtractor &DebugLineData,
-                                      uint32_t *OffsetPtr,
-                                      const DWARFContext &Ctx,
-                                      const DWARFUnit *U, raw_ostream *OS) {
+void DWARFDebugLine::warn(StringRef Message) {
+  WithColor::warning() << Message;
+}
+
+Error DWARFDebugLine::LineTable::parse(
+    DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
+    const DWARFContext &Ctx, const DWARFUnit *U,
+    std::function<void(StringRef)> WarnCallback, raw_ostream *OS) {
   const uint32_t DebugLineOffset = *OffsetPtr;
 
   clear();
 
-  if (!Prologue.parse(DebugLineData, OffsetPtr, Ctx, U)) {
-    // Restore our offset and return false to indicate failure!
-    *OffsetPtr = DebugLineOffset;
-    return false;
-  }
+  if (Error PrologueErr = Prologue.parse(DebugLineData, OffsetPtr, Ctx, U))
+    return PrologueErr;
 
   if (OS) {
     // The presence of OS signals verbose dumping.
     DIDumpOptions DumpOptions;
     DumpOptions.Verbose = true;
     Prologue.dump(*OS, DumpOptions);
   }
 
if (Opcode == 0) {
         // that affect the address register add a delta to it. This instruction
         // stores a relocatable value into it instead.
         //
         // Make sure the extractor knows the address size.  If not, infer it
         // from the size of the operand.
         if (DebugLineData.getAddressSize() == 0)
           DebugLineData.setAddressSize(Len - 1);
         else if (DebugLineData.getAddressSize() != Len - 1) {
-          WithColor::warning()
-              << format("mismatching address size at offset 0x%8.8" PRIx32
-                        " expected 0x%2.2" PRIx8 " found 0x%2.2" PRIx64 "\n",
-                        ExtOffset, DebugLineData.getAddressSize(), Len - 1);
-          // Skip the rest of the line-number program.
-          *OffsetPtr = EndOffset;
-          return false;
+          return createError("mismatching address size at offset 0x%8.8" PRIx32
+                             " expected 0x%2.2" PRIx8 " found 0x%2.2" PRIx64,
+                             ExtOffset, DebugLineData.getAddressSize(),
+                             Len - 1);
         }
         State.Row.Address = DebugLineData.getRelocatedAddress(OffsetPtr);
         if (OS)
           *OS << format(" (0x%16.16" PRIx64 ")", State.Row.Address);
         break;
 
       case DW_LNE_define_file:
         // Takes 4 arguments. The first is a null terminated string containing
if (Opcode == 0) {
               << format(" length %" PRIx64, Len);
         // Len doesn't include the zero opcode byte or the length itself, but
         // it does include the sub_opcode, so we have to adjust for that.
         (*OffsetPtr) += Len - 1;
         break;
       }
       // Make sure the stated and parsed lengths are the same.
       // Otherwise we have an unparseable line-number program.
-      if (*OffsetPtr - ExtOffset != Len) {
-        WithColor::warning()
-            << format("unexpected line op length at offset 0x%8.8" PRIx32
-                      " expected 0x%2.2" PRIx64 " found 0x%2.2" PRIx32 "\n",
+      if (*OffsetPtr - ExtOffset != Len)
+        return createError("unexpected line op length at offset 0x%8.8" PRIx32
+                           " expected 0x%2.2" PRIx64 " found 0x%2.2" PRIx32,
                            ExtOffset, Len, *OffsetPtr - ExtOffset);
-        // Skip the rest of the line-number program.
-        *OffsetPtr = EndOffset;
-        return false;
-      }
     } else if (Opcode < Prologue.OpcodeBase) {
       if (OS)
         *OS << LNStandardString(Opcode);
       switch (Opcode) {
       // Standard Opcodes
       case DW_LNS_copy:
         // Takes no arguments. Append a row to the matrix using the
         // current values of the state-machine registers. Then set
if (Opcode == 0) {
       // Reset discriminator to 0.
       State.Row.Discriminator = 0;
     }
     if(OS)
       *OS << "\n";
   }
 
   if (!State.Sequence.Empty)
-    WithColor::warning() << "last sequence in debug line table is not"
-                            "terminated!\n";
+    WarnCallback("last sequence in debug line table is not terminated!");
 
   // Sort all sequences so that address lookup will work faster.
   if (!Sequences.empty()) {
     llvm::sort(Sequences.begin(), Sequences.end(), Sequence::orderByLowPC);
     // Note: actually, instruction address ranges of sequences should not
     // overlap (in shared objects and executables). If they do, the address
     // lookup would still work, though, but result would be ambiguous.
     // We don't report warning in this case. For example,
     // sometimes .so compiled from multiple object files contains a few
     // rudimentary sequences for address ranges [0x0, 0xsomething).
   }
 
-  return EndOffset;
+  return Error::success();
 }
 
 uint32_t
 DWARFDebugLine::LineTable::findRowInSeq(const DWARFDebugLine::Sequence &Seq,
                                         uint64_t Address) const {
   if (!Seq.containsPC(Address))
     return UnknownRowIndex;
   // Search for instruction address in the rows describing the sequence.

test/DebugInfo/X86/dwarfdump-bogus-LNE.s

LT2_header_end:
         # Real opcode with incorrect length.
         .byte   0
         .byte   2               # Wrong length, should be 1.
         .byte   1               # DW_LNE_end_sequence
 LT2_end:
 
 # ERR:      warning: unexpected line op length at offset 0x0000005e
 # ERR-SAME: expected 0x02 found 0x01
-
-# The above parsing errors still let us move to the next unit.
-# If the prologue is bogus, we need to bail out because we can't
-# even find the next unit.
-
-# DWARF v4 line-table header #3.
-LT3_start:
-        .long   LT3_end-LT3_version   # Length of Unit (DWARF-32 format)
-LT3_version:
-        .short  4               # DWARF version number
-        .long   LT3_header_end-LT3_params   # Length of Prologue
-LT3_params:
-        .byte   1               # Minimum Instruction Length
-        .byte   1               # Maximum Operations per Instruction
-        .byte   1               # Default is_stmt
-        .byte   -5              # Line Base
-        .byte   14              # Line Range
-        .byte   13              # Opcode Base
-        .byte   0               # Standard Opcode Lengths
-        .byte   1
-        .byte   1
-        .byte   1
-        .byte   1
-        .byte   0
-        .byte   0
-        .byte   0
-        .byte   1
-        .byte   0
-        .byte   0
-        .byte   1
-        # No directories.
-        .byte   0
-        # No files.
-        .byte   0
-        # Extra junk at the end of the prologue, so the length isn't right.
-        .long   0
-LT3_header_end:
-        # Real opcode and operand.
-        .byte   0
-        .byte   9
-        .byte   2               # DW_LNE_set_address
-        .quad   .text
-        # Real opcode with incorrect length.
-        .byte   0
-        .byte   2               # Wrong length, should be 1.
-        .byte   1               # DW_LNE_end_sequence
-LT3_end:
-
-# We should have bailed out above, so never see this in the dump.
-# DWARF v4 line-table header #4.
-LT4_start:
-        .long   LT4_end-LT4_version   # Length of Unit (DWARF-32 format)
-LT4_version:
-        .short  4               # DWARF version number
-        .long   LT4_header_end-LT4_params   # Length of Prologue
-LT4_params:
-        .byte   1               # Minimum Instruction Length
-        .byte   1               # Maximum Operations per Instruction
-        .byte   1               # Default is_stmt
-        .byte   -5              # Line Base
-        .byte   14              # Line Range
-        .byte   13              # Opcode Base
-        .byte   0               # Standard Opcode Lengths
-        .byte   1
-        .byte   1
-        .byte   1
-        .byte   1
-        .byte   0
-        .byte   0
-        .byte   0
-        .byte   1
-        .byte   0
-        .byte   0
-        .byte   1
-        # No directories.
-        .byte   0
-        # No files.
-        .byte   0
-LT4_header_end:
-        # Real opcode and operand.
-        .byte   0
-        .byte   9
-        .byte   2               # DW_LNE_set_address
-        .quad   .text
-        # Real opcode with correct length.
-        .byte   0
-        .byte   1
-        .byte   1               # DW_LNE_end_sequence
-LT4_end:
-
-# Look for the dump of unit 3, and don't want unit 4.
-# CHECK:     Line table prologue:
-# CHECK-NOT: Line table prologue:
-
-# And look for the error message.
-# ERR:      warning: parsing line table prologue at 0x0000005f should have
-# ERR-SAME: ended at 0x00000081 but it ended at 0x0000007d

test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s

+.section .debug_line,"",@progbits
+# Leading good section
+.long   .Lunit1_end - .Lunit1_start # Length of Unit (DWARF-32 format)
+.Lunit1_start:
+.short  4               # DWARF version number
+.long   .Lprologue1_end-.Lprologue1_start # Length of Prologue
+.Lprologue1_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue1_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0x0badbeef
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunit1_end:
+
+# version 0
+.long   .Lunit_v0_end - .Lunit_v0_start # unit length
+.Lunit_v0_start:
+.short  0               # version
+.Lunit_v0_end:
+
+# version 1
+.long   .Lunit_v1_end - .Lunit_v1_start # unit length
+.Lunit_v1_start:
+.short  1               # version
+.Lunit_v1_end:
+
+# version 5 malformed line/include table
+.long   .Lunit_v5_end - .Lunit_v5_start # unit length
+.Lunit_v5_start:
+.short  5               # version
+.byte   8               # address size
+.byte   8               # segment selector
+.long   .Lprologue_v5_end-.Lprologue_v5_start # Length of Prologue
+.Lprologue_v5_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.byte   0               # directory table (invalid)
+.Lprologue_v5_end:
+.Lunit_v5_end:
+
+# Short prologue
+.long   .Lunit_short_prologue_end - .Lunit_short_prologue_start # unit length
+.Lunit_short_prologue_start:
+.short  4               # version
+.long   .Lprologue_short_prologue_end-.Lprologue_short_prologue_start - 2 # Length of Prologue
+.Lprologue_short_prologue_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue_short_prologue_end:
+.Lunit_short_prologue_end:
+
+# Over-long prologue
+.long   .Lunit_long_prologue_end - .Lunit_long_prologue_start # unit length
+.Lunit_long_prologue_start:
+.short  4               # version
+.long   .Lprologue_long_prologue_end-.Lprologue_long_prologue_start + 1 # Length of Prologue
+.Lprologue_long_prologue_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue_long_prologue_end:
+.Lunit_long_prologue_end:
+
+# Over-long extended opcode
+.long   .Lunit_long_opcode_end - .Lunit_long_opcode_start # unit length
+.Lunit_long_opcode_start:
+.short  4               # version
+.long   .Lprologue_long_opcode_end-.Lprologue_long_opcode_start # Length of Prologue
+.Lprologue_long_opcode_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue_long_opcode_end:
+.byte   0, 9, 2        # DW_LNE_set_address
+.quad   0xabbadaba
+.byte   0, 2, 1         # DW_LNE_end_sequence (wrong length)
+.byte   0, 9, 2        # DW_LNE_set_address
+.quad   0xbabb1e45
+.byte   0, 1, 1         # DW_LNE_end_sequence (wrong length)
+.Lunit_long_opcode_end:
+
+# No end of sequence
+.long   .Lunit_no_eos_end - .Lunit_no_eos_start # unit length
+.Lunit_no_eos_start:
+.short  4               # version
+.long   .Lprologue_no_eos_end-.Lprologue_no_eos_start # Length of Prologue
+.Lprologue_no_eos_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue_no_eos_end:
+.byte   0, 9, 2        # DW_LNE_set_address
+.quad   0xdeadfade
+.byte   1              # DW_LNS_copy
+.Lunit_no_eos_end:
+
+# Trailing good section
+.long   .Lunit_good_end - .Lunit_good_start # Length of Unit (DWARF-32 format)
+.Lunit_good_start:
+.short  4               # DWARF version number
+.long   .Lprologue_good_end-.Lprologue_good_start # Length of Prologue
+.Lprologue_good_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue_good_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0xcafebabe
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunit_good_end:

test/tools/llvm-dwarfdump/X86/Inputs/debug_line_reserved_length.s

+.section .debug_line,"",@progbits
+# Leading good section
+.long   .Lunit1_end - .Lunit1_start # Length of Unit (DWARF-32 format)
+.Lunit1_start:
+.short  4               # DWARF version number
+.long   .Lprologue1_end-.Lprologue1_start # Length of Prologue
+.Lprologue1_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue1_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0x0badbeef
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunit1_end:
+
+# Malformed section
+.long   0xfffffffe      # reserved unit length
+
+# Trailing good section
+.long   .Lunit3_end - .Lunit3_start # Length of Unit (DWARF-32 format)
+.Lunit3_start:
+.short  4               # DWARF version number
+.long   .Lprologue3_end-.Lprologue3_start # Length of Prologue
+.Lprologue3_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+.asciz "dir1"           # Include table
+.asciz "dir2"
+.byte   0
+.asciz "file1"          # File table
+.byte   0, 0, 0
+.asciz "file2"
+.byte   1, 0, 0
+.byte   0
+.Lprologue3_end:
+.byte   0, 9, 2         # DW_LNE_set_address
+.quad   0xcafebabe
+.byte   0, 1, 1         # DW_LNE_end_sequence
+.Lunit3_end:

test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

+# Test the different error cases in the debug line parsing and how they prevent
+# or don't prevent further dumping of section contents.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %S/Inputs/debug_line_reserved_length.s -filetype=obj -o %t-reserved.o
+# RUN: llvm-dwarfdump -debug-line %t-reserved.o 2> %t-reserved.err | FileCheck %s --check-prefixes=FIRST,FATAL
+# RUN: FileCheck %s --input-file=%t-reserved.err --check-prefix=RESERVED
+# RUN: llvm-dwarfdump -debug-line %t-reserved.o -verbose 2> %t-reserved-verbose.err | FileCheck %s --check-prefixes=FIRST,FATAL
+# RUN: FileCheck %s --input-file=%t-reserved-verbose.err --check-prefix=RESERVED
+
+# We should still produce warnings for malformed tables after the specified unit.
+# RUN: llvm-dwarfdump -debug-line=0 %t-reserved.o 2> %t-reserved-off-first.err | FileCheck %s --check-prefixes=FIRST,NOLATER
+# RUN: FileCheck %s --input-file=%t-reserved-off-first.err --check-prefix=RESERVED
+
+# Stop looking for the specified unit, if a fatally-bad prologue is detected.
+# RUN: llvm-dwarfdump -debug-line=0x4b %t-reserved.o 2> %t-reserved-off-last.err | FileCheck %s --check-prefixes=NOFIRST,NOLATER
+# RUN: FileCheck %s --input-file=%t-reserved-off-last.err --check-prefix=RESERVED
+
+# RUN: llvm-mc -triple x86_64-pc-linux %S/Inputs/debug_line_malformed.s -filetype=obj -o %t-malformed.o
+# RUN: llvm-dwarfdump -debug-line %t-malformed.o 2> %t-malformed.err | FileCheck %s --check-prefixes=FIRST,NONFATAL,TERSE
+# RUN: FileCheck %s --input-file=%t-malformed.err --check-prefixes=ALL,OTHER
+# RUN: llvm-dwarfdump -debug-line %t-malformed.o -verbose 2> %t-malformed-verbose.err | FileCheck %s --check-prefixes=FIRST,NONFATAL,VERBOSE
+# RUN: FileCheck %s --input-file=%t-malformed-verbose.err --check-prefixes=ALL,OTHER
+
+# RUN: llvm-dwarfdump -debug-line=0 %t-malformed.o 2> %t-malformed-off-first.err | FileCheck %s --check-prefixes=FIRST,NOLATER
+# RUN: FileCheck %s --input-file=%t-malformed-off-first.err --check-prefix=ALL
+
+# Don't stop looking for the later unit if non-fatal issues are found.
+# RUN: llvm-dwarfdump -debug-line=0x183 %t-malformed.o 2> %t-malformed-off-last.err | FileCheck %s --check-prefixes=LASTONLY
+# RUN: FileCheck %s --input-file=%t-malformed-off-last.err --check-prefix=ALL
+
+# FIRST: debug_line[0x00000000]
+# FIRST: 0x000000000badbeef {{.*}} end_sequence
+# NOFIRST-NOT: debug_line[0x00000000]
+# NOFIRST-NOT: 0x000000000badbeef {{.*}} end_sequence
+# NOLATER-NOT: debug_line[{{.*}}]
+# NOLATER-NOT: end_sequence
+
+# For fatal issues, the malformed table should not be dumped, nor should the following table.
+# FATAL: debug_line[0x00000048]
+# FATAL-NOT: Line table prologue
+# FATAL-NOT: debug_line
+
+# For non-fatal issues, the malformed table should not be dumped, but any subsequent ones should be.
+# NONFATAL: debug_line[0x00000048]
+# NONFATAL-NOT: Line table prologue
+# NONFATAL: debug_line[0x0000004e]
+# NONFATAL-NOT: Line table prologue
+# NONFATAL: debug_line[0x00000054]
+# NONFATAL-NOT: Line table prologue
+# NONFATAL: debug_line[0x00000073]
+# NONFATAL-NOT: Line table prologue
+# NONFATAL: debug_line[0x000000ad]
+# NONFATAL-NOT: Line table prologue
+# NONFATAL: debug_line[0x000000e7]
+# TERSE-NOT: Line table prologue
+# Verbose dumping prints the line table prologue and any valid operations up to the point causing
+# the problem.
+# VERBOSE-NEXT: Line table prologue
+# VERBOSE: 0x00000000abbadaba {{.*}} end_sequence
+# VERBOSE-NOT: DW_LNE_set_address
+
+# For minor issues, we can dump the table.
+# NONFATAL: debug_line[0x0000013d]
+# NONFATAL-NEXT: Line table prologue
+# NONFATAL-NOT: debug_line[{{.*}}]
+# NONFATAL: 0x00000000deadfade {{.*}}
+# NONFATAL: debug_line[0x00000183]
+# NONFATAL-NOT: debug_line[{{.*}}]
+# NONFATAL: 0x00000000cafebabe {{.*}} end_sequence
+# NONFATAL-NOT: debug_line[{{.*}}]
+
+# LASTONLY-NOT: debug_line[{{.*}}]
+# LASTONLY: debug_line[0x00000183]
+# LASTONLY: 0x00000000cafebabe {{.*}} end_sequence
+
+# RESERVED: warning: parsing line table prologue at offset 0x00000048 unsupported reserved unit length found of value 0xfffffffe
+
+# ALL-NOT:  warning:
+# ALL:      warning: parsing line table prologue at offset 0x00000048 found unsupported version 0x00
+# ALL-NEXT: warning: parsing line table prologue at offset 0x0000004e found unsupported version 0x01
+# ALL-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
+# FIXME - The latter offset in the next line should be 0xad. The filename parsing code does not notice a missing terminating byte.
+# ALL-NEXT: warning: parsing line table prologue at 0x00000073 should have ended at 0x000000ab but it ended at 0x000000ac
+# ALL-NEXT: warning: parsing line table prologue at 0x000000ad should have ended at 0x000000e8 but it ended at 0x000000e7
+# OTHER-NEXT: warning: unexpected line op length at offset 0x0000012e expected 0x02 found 0x01
+# OTHER-NEXT: warning: last sequence in debug line table is not terminated!
+# ALL-NOT:  warning:

tools/dsymutil/DwarfLinker.cpp

if (auto *OutputDIE = Unit.getOutputUnitDIE())
     patchStmtList(*OutputDIE, DIEInteger(Streamer->getLineSectionSize()));
 
   // Parse the original line info for the unit.
   DWARFDebugLine::LineTable LineTable;
   uint32_t StmtOffset = *StmtList;
   DWARFDataExtractor LineExtractor(
       OrigDwarf.getDWARFObj(), OrigDwarf.getDWARFObj().getLineSection(),
       OrigDwarf.isLittleEndian(), Unit.getOrigUnit().getAddressByteSize());
-  LineTable.parse(LineExtractor, &StmtOffset, OrigDwarf, &Unit.getOrigUnit());
+
+  Error Err = LineTable.parse(LineExtractor, &StmtOffset, OrigDwarf,
+                              &Unit.getOrigUnit());
+  handleDebugLineParseError(std::move(Err));
 
   // This vector is the output line table.
   std::vector<DWARFDebugLine::Row> NewRows;
   NewRows.reserve(LineTable.Rows.size());
 
   // Current sequence of rows being extracted, before being inserted
   // in NewRows.
   std::vector<DWARFDebugLine::Row> Seq;

unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugLine.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 using namespace dwarf;
+using namespace dwarfgen;
 using namespace object;
 using namespace utils;
+using namespace testing;
 
 namespace {
+struct CommonFixture {
+  CommonFixture() : LineData("", true, 0){};
 
-struct DebugLineGenerator {
-  bool init() {
+  bool setupGenerator(uint16_t Version = 4) {
     Triple T = getHostTripleForAddrSize(8);
     if (!isConfigurationSupported(T))
       return false;
-    auto ExpectedGenerator = dwarfgen::Generator::create(T, 4);
+    auto ExpectedGenerator = Generator::create(T, Version);
     if (ExpectedGenerator)
       Generator.reset(ExpectedGenerator->release());
     return true;
   }
 
+  void generate() {
+    Context = createContext();
+    assert(Context != nullptr && "test state is not valid");
+    const DWARFObject &Obj = Context->getDWARFObj();
+    LineData = DWARFDataExtractor(Obj, Obj.getLineSection(), true, 8);
+  }
+
   std::unique_ptr<DWARFContext> createContext() {
     if (!Generator)
       return nullptr;
     StringRef FileBytes = Generator->generate();
     MemoryBufferRef FileBuffer(FileBytes, "dwarf");
     auto Obj = object::ObjectFile::createObjectFile(FileBuffer);
     if (Obj)
       return DWARFContext::create(**Obj);
     return nullptr;
   }
 
-  std::unique_ptr<dwarfgen::Generator> Generator;
+  void recordIssue(StringRef Message) { IssueMessage = Message; }
+
+  template <typename ErrorType>
+  void checkGetOrParseLineTableEmitsError(StringRef ExpectedMsg,
+                                          uint64_t Offset = 0) {
+    auto ExpectedLineTable = Line.getOrParseLineTable(
+        LineData, Offset, *Context, nullptr,
+        std::bind(&CommonFixture::recordIssue, this, std::placeholders::_1));
+    EXPECT_FALSE(ExpectedLineTable);
+    EXPECT_TRUE(IssueMessage.empty());
+
+    Error Err = ExpectedLineTable.takeError();
+    ASSERT_TRUE(Err.operator bool());
+    Error Remaining =
+        handleErrors(std::move(Err), [&](const ErrorType &Actual) {
+          // Use .str(), because googletest doesn't visualise a StringRef
+          // properly.
+          EXPECT_EQ(Actual.message(), ExpectedMsg.str());
+        });
+    EXPECT_FALSE(Remaining);
+  }
+
+  std::unique_ptr<Generator> Generator;
+  std::unique_ptr<DWARFContext> Context;
+  DWARFDataExtractor LineData;
+  DWARFDebugLine Line;
+  std::string IssueMessage;
+};
+
+// Fixtures must derive from "Test", but parameterised fixtures from
+// "TestWithParam". It does not seem possible to inherit from both, so we share
+// the common state in a separate class, inherited by the two fixture classes.
+struct DebugLineBasicFixture : public Test, public CommonFixture {};
+
+struct DebugLineParameterisedFixture
+    : public TestWithParam<std::pair<uint16_t, DwarfFormat>>,
+      public CommonFixture {
+  void SetUp() { std::tie(Version, Format) = GetParam(); }
+
+  uint16_t Version;
+  DwarfFormat Format;
 };
 
-TEST(DWARFDebugLine, GetLineTableAtInvalidOffset) {
-  DebugLineGenerator LineGen;
-  if (!LineGen.init())
+void checkDefaultPrologue(uint16_t Version, DwarfFormat Format,
+                          DWARFDebugLine::Prologue Prologue,
+                          uint64_t BodyLength) {
+  // Check version specific fields and values.
+  uint64_t UnitLength;
+  uint64_t PrologueLength;
+  switch (Version) {
+  case 4:
+    PrologueLength = 36;
+    UnitLength = PrologueLength + 2;
+    EXPECT_EQ(Prologue.MaxOpsPerInst, 1);
+    break;
+  case 2:
+  case 3:
+    PrologueLength = 35;
+    UnitLength = PrologueLength + 2;
+    break;
+  case 5:
+    PrologueLength = 39;
+    UnitLength = PrologueLength + 4;
+    EXPECT_EQ(Prologue.getAddressSize(), 8);
+    EXPECT_EQ(Prologue.SegSelectorSize, 0);
+    break;
+  default:
+    llvm_unreachable("unsupported DWARF version");
+  }
+  UnitLength += BodyLength + (Format == DWARF32 ? 4 : 8);
+
+  EXPECT_EQ(Prologue.TotalLength, UnitLength);
+  EXPECT_EQ(Prologue.PrologueLength, PrologueLength);
+  EXPECT_EQ(Prologue.MinInstLength, 1);
+  EXPECT_EQ(Prologue.DefaultIsStmt, 1);
+  EXPECT_EQ(Prologue.LineBase, -5);
+  EXPECT_EQ(Prologue.LineRange, 14);
+  EXPECT_EQ(Prologue.OpcodeBase, 13);
+  std::vector<uint8_t> ExpectedLengths = {0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1};
+  EXPECT_EQ(Prologue.StandardOpcodeLengths, ExpectedLengths);
+  ASSERT_EQ(Prologue.IncludeDirectories.size(), 1);
+  ASSERT_EQ(Prologue.IncludeDirectories[0].getForm(), DW_FORM_string);
+  EXPECT_STREQ(*Prologue.IncludeDirectories[0].getAsCString(), "a dir");
+  ASSERT_EQ(Prologue.FileNames.size(), 1);
+  ASSERT_EQ(Prologue.FileNames[0].Name.getForm(), DW_FORM_string);
+  EXPECT_STREQ(*Prologue.FileNames[0].Name.getAsCString(), "a file");
+}
+
+TEST_F(DebugLineBasicFixture, GetOrParseLineTableAtInvalidOffset) {
+  if (!setupGenerator())
     return;
+  generate();
 
-  DWARFDebugLine Line;
-  std::unique_ptr<DWARFContext> Context = LineGen.createContext();
-  ASSERT_TRUE(Context != nullptr);
-  const DWARFObject &Obj = Context->getDWARFObj();
-  DWARFDataExtractor LineData(Obj, Obj.getLineSection(), true, 8);
+  checkGetOrParseLineTableEmitsError<DebugLineLengthError>(
+      "offset 0x00000000 is not a valid debug line section offset", 0);
+  // Repeat to show that an error is reported each time.
+  checkGetOrParseLineTableEmitsError<DebugLineLengthError>(
+      "offset 0x00000000 is not a valid debug line section offset", 0);
+  // Show that an error is reported for later offsets too.
+  checkGetOrParseLineTableEmitsError<DebugLineLengthError>(
+      "offset 0x00000001 is not a valid debug line section offset", 1);
+}
+
+TEST_F(DebugLineBasicFixture, GetOrParseLineTableAtInvalidOffsetAfterData) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  LT.setCustomPrologue({{0, LineTable::Byte}});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<DebugLineLengthError>(
+      "offset 0x00000001 is not a valid debug line section offset", 1);
+}
+
+TEST_P(DebugLineParameterisedFixture, GetOrParseLineTableValidTable) {
+  if (!setupGenerator(Version))
+    return;
+
+  SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
+               (Format == DWARF64 ? "DWARF64" : "DWARF32"));
+
+  LineTable &LT = Generator->addLineTable(Format);
+  LT.addExtendedOpcode(9, DW_LNE_set_address, {{0xadd4e55, LineTable::Quad}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  LineTable &LT2 = Generator->addLineTable(Format);
+  LT2.addExtendedOpcode(9, DW_LNE_set_address, {{0x11223344, LineTable::Quad}});
+  LT2.addStandardOpcode(DW_LNS_copy, {});
+  LT2.addByte(0xbb);
+  LT2.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  LT2.addExtendedOpcode(9, DW_LNE_set_address, {{0x55667788, LineTable::Quad}});
+  LT2.addStandardOpcode(DW_LNS_copy, {});
+  LT2.addByte(0xcc);
+  LT2.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  auto RecordIssue =
+      std::bind(&CommonFixture::recordIssue, this, std::placeholders::_1);
+
+  auto ExpectedLineTable =
+      Line.getOrParseLineTable(LineData, 0, *Context, nullptr, RecordIssue);
+  ASSERT_TRUE(ExpectedLineTable.operator bool());
+  EXPECT_TRUE(IssueMessage.empty());
+  const DWARFDebugLine::LineTable *Expected = *ExpectedLineTable;
+  checkDefaultPrologue(Version, Format, Expected->Prologue, 16);
+  EXPECT_EQ(Expected->Sequences.size(), 1);
+
+  uint64_t SecondOffset =
+      Expected->Prologue.sizeofTotalLength() + Expected->Prologue.TotalLength;
+  IssueMessage.clear();
+  auto ExpectedLineTable2 = Line.getOrParseLineTable(
+      LineData, SecondOffset, *Context, nullptr, RecordIssue);
+  ASSERT_TRUE(ExpectedLineTable2.operator bool());
+  EXPECT_TRUE(IssueMessage.empty());
+  const DWARFDebugLine::LineTable *Expected2 = *ExpectedLineTable2;
+  checkDefaultPrologue(Version, Format, Expected2->Prologue, 32);
+  EXPECT_EQ(Expected2->Sequences.size(), 2);
+
+  EXPECT_NE(Expected, Expected2);
+
+  // Check that if the same offset is requested, the exact same pointer is
+  // returned.
+  IssueMessage.clear();
+  auto ExpectedLineTable3 =
+      Line.getOrParseLineTable(LineData, 0, *Context, nullptr, RecordIssue);
+  ASSERT_TRUE(ExpectedLineTable3.operator bool());
+  EXPECT_TRUE(IssueMessage.empty());
+  EXPECT_EQ(Expected, *ExpectedLineTable3);
+
+  IssueMessage.clear();
+  auto ExpectedLineTable4 = Line.getOrParseLineTable(
+      LineData, SecondOffset, *Context, nullptr, RecordIssue);
+  ASSERT_TRUE(ExpectedLineTable4.operator bool());
+  EXPECT_TRUE(IssueMessage.empty());
+  EXPECT_EQ(Expected2, *ExpectedLineTable4);
+
+  // TODO: Add tests that show that the body of the programs have been read
+  // correctly.
+}
+
+TEST_F(DebugLineBasicFixture, LengthErrorForReservedLength) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  LT.setCustomPrologue({{0xffffff00, LineTable::Long}});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<DebugLineLengthError>(
+      "parsing line table prologue at offset 0x00000000 unsupported reserved "
+      "unit length found of value 0xffffff00");
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForLowVersion) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  LT.setCustomPrologue(
+      {{LineTable::Half, LineTable::Long}, {1, LineTable::Half}});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<StringError>("parsing line table prologue "
+                                                  "at offset 0x00000000 found "
+                                                  "unsupported version 0x01");
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForInvalidV5IncludeDirTable) {
+  if (!setupGenerator(5))
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  LT.setCustomPrologue({
+      {19, LineTable::Long}, // unit length
+      {5, LineTable::Half},  // version
+      {8, LineTable::Byte},  // addr size
+      {0, LineTable::Byte},  // segment selector size
+      {11, LineTable::Long}, // prologue length
+      {1, LineTable::Byte},  // min instruction length
+      {1, LineTable::Byte},  // max ops per instruction
+      {1, LineTable::Byte},  // default is_stmt
+      {0, LineTable::Byte},  // line base
+      {14, LineTable::Byte}, // line range
+      {2, LineTable::Byte},  // opcode base (small to reduce the amount of
+                             // setup required).
+      {0, LineTable::Byte},  // standard opcode lengths
+      {0, LineTable::Byte},  // directory entry format count (should not be
+                             // zero).
+      {0, LineTable::ULEB},  // directories count
+      {0, LineTable::Byte},  // file name entry format count
+      {0, LineTable::ULEB}   // file name entry count
+  });
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<StringError>(
+      "parsing line table prologue at 0x00000000 found an invalid directory or "
+      "file table description at 0x00000014");
+}
+
+TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
+  if (!setupGenerator(Version))
+    return;
+
+  SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
+               (Format == DWARF64 ? "DWARF64" : "DWARF32"));
+
+  LineTable &LT = Generator->addLineTable(Format);
+  DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
+  ++Prologue.PrologueLength;
+  LT.setPrologue(Prologue);
+
+  generate();
+
+  uint64_t ExpectedEnd =
+      Prologue.TotalLength + 1 + Prologue.sizeofTotalLength();
+  checkGetOrParseLineTableEmitsError<StringError>(
+      (Twine("parsing line table prologue at 0x00000000 should have ended at "
+             "0x000000") +
+       Twine::utohexstr(ExpectedEnd) + " but it ended at 0x000000" +
+       Twine::utohexstr(ExpectedEnd - 1))
+          .str());
+}
+
+TEST_P(DebugLineParameterisedFixture, ErrorForTooShortPrologueLength) {
+  if (!setupGenerator(Version))
+    return;
+
+  SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
+               (Format == DWARF64 ? "DWARF64" : "DWARF32"));
+
+  LineTable &LT = Generator->addLineTable(Format);
+  DWARFDebugLine::Prologue Prologue = LT.createBasicPrologue();
+  // FIXME: Ideally, we'd test for 1 less than expected, but the code does not
+  // currently fail if missing only the terminator of a v2-4 file table.
+  if (Version < 5)
+    Prologue.PrologueLength -= 2;
+  else
+    Prologue.PrologueLength -= 1;
+  LT.setPrologue(Prologue);
+
+  generate();
+
+  uint64_t ExpectedEnd =
+      Prologue.TotalLength - 1 + Prologue.sizeofTotalLength();
+  if (Version < 5)
+    --ExpectedEnd;
+  checkGetOrParseLineTableEmitsError<StringError>(
+      (Twine("parsing line table prologue at 0x00000000 should have ended at "
+             "0x000000") +
+       Twine::utohexstr(ExpectedEnd) + " but it ended at 0x000000" +
+       Twine::utohexstr(ExpectedEnd + 1))
+          .str());
+}
+
+INSTANTIATE_TEST_CASE_P(
+    LineTableTestParams, DebugLineParameterisedFixture,
+    Values(std::make_pair(
+               2, DWARF32), // Test lower-bound of v2-3 fields and DWARF32.
+           std::make_pair(3, DWARF32), // Test upper-bound of v2-3 fields.
+           std::make_pair(4, DWARF64), // Test v4 fields and DWARF64.
+           std::make_pair(5, DWARF32), std::make_pair(5, DWARF64)));
+
+TEST_F(DebugLineBasicFixture, ErrorForInvalidExtendedOpcodeLength) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  // The Length should be 1 for an end sequence opcode.
+  LT.addExtendedOpcode(2, DW_LNE_end_sequence, {});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<StringError>("unexpected line op length "
+                                                  "at offset 0x00000030 "
+                                                  "expected 0x02 found 0x01");
+}
+
+TEST_F(DebugLineBasicFixture, ErrorForMismatchedAddressSize) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  // The line data extractor expects size 8 (Quad) addresses.
+  LT.addExtendedOpcode(5, DW_LNE_set_address, {{0x11223344, LineTable::Long}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+
+  generate();
+
+  checkGetOrParseLineTableEmitsError<StringError>(
+      "mismatching address size at offset 0x00000030 expected 0x08 found 0x04");
+}
+
+TEST_F(DebugLineBasicFixture, CallbackUsedForUnterminatedSequence) {
+  if (!setupGenerator())
+    return;
+
+  LineTable &LT = Generator->addLineTable();
+  LT.addExtendedOpcode(9, DW_LNE_set_address,
+                       {{0x1122334455667788, LineTable::Quad}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  LT.addByte(0xaa);
+  LT.addExtendedOpcode(1, DW_LNE_end_sequence, {});
+  LT.addExtendedOpcode(9, DW_LNE_set_address,
+                       {{0x99aabbccddeeff00, LineTable::Quad}});
+  LT.addStandardOpcode(DW_LNS_copy, {});
+  LT.addByte(0xbb);
+  LT.addByte(0xcc);
+
+  generate();
 
-  EXPECT_EQ(Line.getOrParseLineTable(LineData, 0, *Context, nullptr), nullptr);
+  auto ExpectedLineTable = Line.getOrParseLineTable(
+      LineData, 0, *Context, nullptr,
+      std::bind(&CommonFixture::recordIssue, this, std::placeholders::_1));
+  EXPECT_EQ(IssueMessage,
+            "last sequence in debug line table is not terminated!");
+  ASSERT_TRUE(ExpectedLineTable.operator bool());
+  EXPECT_EQ((*ExpectedLineTable)->Rows.size(), 6);
+  // The unterminated sequence is not added to the sequence list.
+  EXPECT_EQ((*ExpectedLineTable)->Sequences.size(), 1);
 }
 
 } // end anonymous namespace

unittests/DebugInfo/DWARF/DwarfGenerator.h

public:
   uint64_t getOffset() const { return DU.getDebugSectionOffset(); }
   uint64_t getLength() const { return DU.getLength(); }
   uint16_t getVersion() const { return DU.getDwarfVersion(); }
   uint16_t getAddressSize() const { return DU.getAddressSize(); }
   void setOffset(uint64_t Offset) { DU.setDebugSectionOffset(Offset); }
   void setLength(uint64_t Length) { DU.setLength(Length); }
 };
 
+/// A DWARF line unit-like class used to generate DWARF line units.
+///
+/// Instances of this class are created by instances of the Generator class.
+class LineTable {
+public:
+  enum ValueLength { Byte = 1, Half = 2, Long = 4, Quad = 8, ULEB, SLEB };
+
+  struct ValueAndLength {
+    uint64_t Value;
+    ValueLength Length;
+  };
+
+  LineTable(Generator &DG, uint16_t Version, dwarf::DwarfFormat Format,
+            uint8_t AddrSize, uint8_t SegSize = 0)
+      : DG(DG), Version(Version), Format(Format), AddrSize(AddrSize),
+        SegSize(SegSize) {
+    assert(Version >= 2 && Version <= 5 && "unsupported version");
+  }
+
+  // Create a Prologue suitable to pass to setPrologue, with a single file and
+  // include directory entry.
+  DWARFDebugLine::Prologue createBasicPrologue() const;
+
+  // Set or replace the current prologue with the specified prologue. If no
+  // prologue is set, a default one will be used when generating.
+  void setPrologue(DWARFDebugLine::Prologue NewPrologue);
+  // Used to write an arbitrary payload instead of the standard prologue. This
+  // is useful if you wish to test handling of corrupt .debug_line sections.
+  void setCustomPrologue(ArrayRef<ValueAndLength> NewPrologue);
+
+  // Add a byte to the program, with the given value. This can be used to
+  // specify a special opcode, or to add arbitrary contents to the section.
+  void addByte(uint8_t Value);
+  // Add a standard opcode to the program. The opcode and operands do not have
+  // to be valid.
+  void addStandardOpcode(uint8_t Opcode, ArrayRef<ValueAndLength> Operands);
+  // Add an extended opcode to the program with the specified length, opcode,
+  // and operands. These values do not have to be valid.
+  void addExtendedOpcode(uint64_t Length, uint8_t Opcode,
+                         ArrayRef<ValueAndLength> Operands);
+
+  // Write the contents of the LineUnit to the current section in the generator.
+  void generate(MCContext &MC, AsmPrinter &Asm) const;
+
+private:
+  void writeData(ArrayRef<ValueAndLength> Data, AsmPrinter &Asm) const;
+  MCSymbol *writeDefaultPrologue(AsmPrinter &Asm) const;
+  void writePrologue(AsmPrinter &Asm) const;
+
+  void writeProloguePayload(const DWARFDebugLine::Prologue &Prologue,
+                            AsmPrinter &Asm) const;
+
+  Generator &DG;
+  llvm::Optional<DWARFDebugLine::Prologue> Prologue;
+  std::vector<ValueAndLength> CustomPrologue;
+  std::vector<ValueAndLength> Contents;
+
+  // The Version field is used for determining how to write the Prologue, if a
+  // non-custom prologue is used. The version value actually written, will be
+  // that specified in the Prologue, if a custom prologue has been passed in.
+  // Otherwise, it will be this value.
+  uint16_t Version;
+
+  uint8_t AddrSize;
+  uint8_t SegSize;
+  dwarf::DwarfFormat Format;
+};
+
 /// A DWARF generator.
 ///
 /// Generate DWARF for unit tests by creating any instance of this class and
 /// calling Generator::addCompileUnit(), and then getting the dwarfgen::DIE from
 /// the returned compile unit and adding attributes and children to each DIE.
 class Generator {
   std::unique_ptr<MCRegisterInfo> MRI;
   std::unique_ptr<MCAsmInfo> MAI;
   std::unique_ptr<MCObjectFileInfo> MOFI;
   std::unique_ptr<MCContext> MC;
   MCAsmBackend *MAB; // Owned by MCStreamer
   std::unique_ptr<MCInstrInfo> MII;
   std::unique_ptr<MCSubtargetInfo> MSTI;
   MCCodeEmitter *MCE; // Owned by MCStreamer
   MCStreamer *MS;     // Owned by AsmPrinter
   std::unique_ptr<TargetMachine> TM;
   std::unique_ptr<AsmPrinter> Asm;
   BumpPtrAllocator Allocator;
   std::unique_ptr<DwarfStringPool> StringPool; // Entries owned by Allocator.
   std::vector<std::unique_ptr<CompileUnit>> CompileUnits;
+  std::vector<std::unique_ptr<LineTable>> LineTables;
   DIEAbbrevSet Abbreviations;
 
   SmallString<4096> FileBytes;
   /// The stream we use to generate the DWARF into as an ELF file.
   std::unique_ptr<raw_svector_ostream> Stream;
   /// The DWARF version to generate.
   uint16_t Version;
 
public:
   /// Generate all DWARF sections and return a memory buffer that
   /// contains an ELF file that contains the DWARF.
   StringRef generate();
 
   /// Add a compile unit to be generated.
   ///
   /// \returns a dwarfgen::CompileUnit that can be used to retrieve the compile
   /// unit dwarfgen::DIE that can be used to add attributes and add child DIE
-  /// objedts to.
+  /// objects to.
   dwarfgen::CompileUnit &addCompileUnit();
 
+  /// Add a line table unit to be generated.
+  /// \param Format the DWARF format to use (DWARF32 or DWARF64).
+  ///
+  /// \returns a dwarfgen::LineTable that can be used to customise the contents
+  /// of the line table.
+  LineTable &
+  addLineTable(dwarf::DwarfFormat DwarfFormat = dwarf::DwarfFormat::DWARF32);
+
   BumpPtrAllocator &getAllocator() { return Allocator; }
   AsmPrinter *getAsmPrinter() const { return Asm.get(); }
   MCContext *getMCContext() const { return MC.get(); }
   DIEAbbrevSet &getAbbrevSet() { return Abbreviations; }
   DwarfStringPool &getStringPool() { return *StringPool; }
 
   /// Save the generated DWARF file to disk.
   ///

unittests/DebugInfo/DWARF/DwarfGenerator.cpp

return dwarfgen::DIE(CU,
                        &Die->addChild(llvm::DIE::get(DG.getAllocator(), Tag)));
 }
 
 dwarfgen::DIE dwarfgen::CompileUnit::getUnitDIE() {
   return dwarfgen::DIE(this, &DU.getUnitDie());
 }
 
 //===----------------------------------------------------------------------===//
+/// dwarfgen::LineTable implementation.
+//===----------------------------------------------------------------------===//
+DWARFDebugLine::Prologue dwarfgen::LineTable::createBasicPrologue() const {
+  DWARFDebugLine::Prologue P;
+  switch (Version) {
+  case 2:
+  case 3:
+    P.TotalLength = 41;
+    P.PrologueLength = 35;
+    break;
+  case 4:
+    P.TotalLength = 42;
+    P.PrologueLength = 36;
+    break;
+  case 5:
+    P.TotalLength = 47;
+    P.PrologueLength = 39;
+    P.FormParams.AddrSize = AddrSize;
+    break;
+  default:
+    llvm_unreachable("unsupported version");
+  }
+  if (Format == DWARF64) {
+    P.TotalLength += 4;
+    P.FormParams.Format = DWARF64;
+  }
+  P.FormParams.Version = Version;
+  P.MinInstLength = 1;
+  P.MaxOpsPerInst = 1;
+  P.DefaultIsStmt = 1;
+  P.LineBase = -5;
+  P.LineRange = 14;
+  P.OpcodeBase = 13;
+  P.StandardOpcodeLengths = {0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1};
+  P.IncludeDirectories.push_back(DWARFFormValue(DW_FORM_string));
+  P.IncludeDirectories.back().setPValue("a dir");
+  P.FileNames.push_back(DWARFDebugLine::FileNameEntry());
+  P.FileNames.back().Name.setPValue("a file");
+  P.FileNames.back().Name.setForm(DW_FORM_string);
+  return P;
+}
+
+void dwarfgen::LineTable::setPrologue(DWARFDebugLine::Prologue NewPrologue) {
+  Prologue = NewPrologue;
+  CustomPrologue.clear();
+}
+
+void dwarfgen::LineTable::setCustomPrologue(
+    ArrayRef<ValueAndLength> NewPrologue) {
+  Prologue.reset();
+  CustomPrologue = NewPrologue;
+}
+
+void dwarfgen::LineTable::addByte(uint8_t Value) {
+  Contents.push_back({Value, Byte});
+}
+
+void dwarfgen::LineTable::addStandardOpcode(uint8_t Opcode,
+                                            ArrayRef<ValueAndLength> Operands) {
+  Contents.push_back({Opcode, Byte});
+  Contents.insert(Contents.end(), Operands.begin(), Operands.end());
+}
+
+void dwarfgen::LineTable::addExtendedOpcode(uint64_t Length, uint8_t Opcode,
+                                            ArrayRef<ValueAndLength> Operands) {
+  Contents.push_back({0, Byte});
+  Contents.push_back({Length, ULEB});
+  Contents.push_back({Opcode, Byte});
+  Contents.insert(Contents.end(), Operands.begin(), Operands.end());
+}
+
+void dwarfgen::LineTable::generate(MCContext &MC, AsmPrinter &Asm) const {
+  MC.setDwarfVersion(Version);
+
+  MCSymbol *EndSymbol = nullptr;
+  if (!CustomPrologue.empty()) {
+    writeData(CustomPrologue, Asm);
+  } else if (!Prologue) {
+    EndSymbol = writeDefaultPrologue(Asm);
+  } else {
+    writePrologue(Asm);
+  }
+
+  writeData(Contents, Asm);
+  if (EndSymbol != nullptr)
+    Asm.OutStreamer->EmitLabel(EndSymbol);
+}
+
+void dwarfgen::LineTable::writeData(ArrayRef<ValueAndLength> Data,
+                                    AsmPrinter &Asm) const {
+  for (auto Entry : Data) {
+    switch (Entry.Length) {
+    case Byte:
+    case Half:
+    case Long:
+    case Quad:
+      Asm.OutStreamer->EmitIntValue(Entry.Value, Entry.Length);
+      break;
+    case ULEB:
+      Asm.EmitULEB128(Entry.Value);
+      break;
+    case SLEB:
+      Asm.EmitSLEB128(Entry.Value);
+      break;
+    default:
+      llvm_unreachable("unsupported ValueAndLength Length value");
+    }
+  }
+}
+
+MCSymbol *dwarfgen::LineTable::writeDefaultPrologue(AsmPrinter &Asm) const {
+  MCSymbol *UnitStart = Asm.createTempSymbol("line_unit_start");
+  MCSymbol *UnitEnd = Asm.createTempSymbol("line_unit_end");
+  if (Format == DwarfFormat::DWARF64) {
+    Asm.emitInt32(0xffffffff);
+    Asm.EmitLabelDifference(UnitEnd, UnitStart, 8);
+  } else {
+    Asm.EmitLabelDifference(UnitEnd, UnitStart, 4);
+  }
+  Asm.OutStreamer->EmitLabel(UnitStart);
+  Asm.emitInt16(Version);
+  if (Version == 5) {
+    Asm.emitInt8(AddrSize);
+    Asm.emitInt8(SegSize);
+  }
+
+  MCSymbol *PrologueStart = Asm.createTempSymbol("line_prologue_start");
+  MCSymbol *PrologueEnd = Asm.createTempSymbol("line_prologue_end");
+  Asm.EmitLabelDifference(PrologueEnd, PrologueStart,
+                          Format == DwarfFormat::DWARF64 ? 8 : 4);
+  Asm.OutStreamer->EmitLabel(PrologueStart);
+
+  DWARFDebugLine::Prologue DefaultPrologue = createBasicPrologue();
+  writeProloguePayload(DefaultPrologue, Asm);
+  Asm.OutStreamer->EmitLabel(PrologueEnd);
+  return UnitEnd;
+}
+
+void dwarfgen::LineTable::writePrologue(AsmPrinter &Asm) const {
+  if (Format == DwarfFormat::DWARF64) {
+    Asm.emitInt32(0xffffffff);
+    Asm.emitInt64(Prologue->TotalLength);
+  } else {
+    Asm.emitInt32(Prologue->TotalLength);
+  }
+  Asm.emitInt16(Prologue->getVersion());
+  if (Version == 5) {
+    Asm.emitInt8(Prologue->getAddressSize());
+    Asm.emitInt8(Prologue->SegSelectorSize);
+  }
+  if (Format == DwarfFormat::DWARF64)
+    Asm.emitInt64(Prologue->PrologueLength);
+  else
+    Asm.emitInt32(Prologue->PrologueLength);
+
+  writeProloguePayload(*Prologue, Asm);
+}
+
+static void writeCString(StringRef Str, AsmPrinter &Asm) {
+  Asm.OutStreamer->EmitBytes(Str);
+  Asm.emitInt8(0);
+}
+
+static void writeV2IncludeAndFileTable(const DWARFDebugLine::Prologue &Prologue,
+                                       AsmPrinter &Asm) {
+  for (auto Include : Prologue.IncludeDirectories) {
+    assert(Include.getAsCString() && "expected a string form for include dir");
+    writeCString(*Include.getAsCString(), Asm);
+  }
+  Asm.emitInt8(0);
+
+  for (auto File : Prologue.FileNames) {
+    assert(File.Name.getAsCString() && "expected a string form for file name");
+    writeCString(*File.Name.getAsCString(), Asm);
+    Asm.EmitULEB128(File.DirIdx);
+    Asm.EmitULEB128(File.ModTime);
+    Asm.EmitULEB128(File.Length);
+  }
+  Asm.emitInt8(0);
+}
+
+static void writeV5IncludeAndFileTable(const DWARFDebugLine::Prologue &Prologue,
+                                       AsmPrinter &Asm) {
+  Asm.emitInt8(1); // directory_entry_format_count.
+  // TODO: Add support for other content descriptions - we currently only
+  // support a single DW_LNCT_path/DW_FORM_string.
+  Asm.EmitULEB128(DW_LNCT_path);
+  Asm.EmitULEB128(DW_FORM_string);
+  Asm.EmitULEB128(Prologue.IncludeDirectories.size());
+  for (auto Include : Prologue.IncludeDirectories) {
+    assert(Include.getAsCString() && "expected a string form for include dir");
+    writeCString(*Include.getAsCString(), Asm);
+  }
+
+  Asm.emitInt8(1); // file_name_entry_format_count.
+  Asm.EmitULEB128(DW_LNCT_path);
+  Asm.EmitULEB128(DW_FORM_string);
+  Asm.EmitULEB128(Prologue.FileNames.size());
+  for (auto File : Prologue.FileNames) {
+    assert(File.Name.getAsCString() && "expected a string form for file name");
+    writeCString(*File.Name.getAsCString(), Asm);
+  }
+}
+
+void dwarfgen::LineTable::writeProloguePayload(
+    const DWARFDebugLine::Prologue &Prologue, AsmPrinter &Asm) const {
+  Asm.emitInt8(Prologue.MinInstLength);
+  if (Version >= 4)
+    Asm.emitInt8(Prologue.MaxOpsPerInst);
+  Asm.emitInt8(Prologue.DefaultIsStmt);
+  Asm.emitInt8(Prologue.LineBase);
+  Asm.emitInt8(Prologue.LineRange);
+  Asm.emitInt8(Prologue.OpcodeBase);
+  for (auto Length : Prologue.StandardOpcodeLengths) {
+    Asm.emitInt8(Length);
+  }
+
+  if (Version < 5)
+    writeV2IncludeAndFileTable(Prologue, Asm);
+  else
+    writeV5IncludeAndFileTable(Prologue, Asm);
+}
+
+//===----------------------------------------------------------------------===//
 /// dwarfgen::Generator implementation.
 //===----------------------------------------------------------------------===//
 
 dwarfgen::Generator::Generator()
     : MAB(nullptr), MCE(nullptr), MS(nullptr), StringPool(nullptr),
       Abbreviations(Allocator) {}
 dwarfgen::Generator::~Generator() = default;
 
for (auto &CU : CompileUnits) {
     } else {
       Asm->emitInt8(dwarf::DW_UT_compile);
       Asm->emitInt8(CU->getAddressSize());
       Asm->emitInt32(0);
     }
     Asm->emitDwarfDIE(*CU->getUnitDIE().Die);
   }
 
+  MS->SwitchSection(MOFI->getDwarfLineSection());
+  for (auto &LT : LineTables) {
+    LT->generate(*MC, *Asm);
+  }
+
   MS->Finish();
   if (FileBytes.empty())
     return StringRef();
   return StringRef(FileBytes.data(), FileBytes.size());
 }
 
 bool dwarfgen::Generator::saveFile(StringRef Path) {
   if (FileBytes.empty())
     return false;
   std::error_code EC;
   raw_fd_ostream Strm(Path, EC, sys::fs::F_None);
   if (EC)
     return false;
   Strm.write(FileBytes.data(), FileBytes.size());
   Strm.close();
   return true;
 }
 
 dwarfgen::CompileUnit &dwarfgen::Generator::addCompileUnit() {
   CompileUnits.push_back(std::unique_ptr<CompileUnit>(
       new CompileUnit(*this, Version, Asm->getPointerSize())));
   return *CompileUnits.back();
 }
+
+dwarfgen::LineTable &dwarfgen::Generator::addLineTable(DwarfFormat Format) {
+  LineTables.push_back(std::unique_ptr<LineTable>(
+      new LineTable(*this, Version, Format, Asm->getPointerSize())));
+  return *LineTables.back();
+}