Page MenuHomePhabricator

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

Authored by jhenderson on Mar 16 2018, 6:44 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JDevlieghere added inline comments.Mar 16 2018, 7:35 AM
include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
59 ↗(On Diff #138689)

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 ↗(On Diff #138689)

I think something like warn would be sufficiently clear.

286 ↗(On Diff #138689)

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.

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

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
59 ↗(On Diff #138689)

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 ↗(On Diff #138689)

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?

286 ↗(On Diff #138689)

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 added inline comments.Mar 16 2018, 10:17 AM
include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
36 ↗(On Diff #138689)

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.

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 ↗(On Diff #138689)

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 ↗(On Diff #138689)

Makes sense, the static method sounds good to me.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
42 ↗(On Diff #138689)

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

jhenderson marked 5 inline comments as done.Mar 19 2018, 4:45 AM

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 ↗(On Diff #138689)

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

286 ↗(On Diff #138689)

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 ↗(On Diff #138689)

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 marked an inline comment as done.

Addressed review comments:

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

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
310 ↗(On Diff #138895)

s/MinorIssueCallback/WarnCallback/

lib/DebugInfo/DWARF/DWARFContext.cpp
437 ↗(On Diff #138895)

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);
445 ↗(On Diff #138895)

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

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
42 ↗(On Diff #138689)

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

jhenderson marked 2 inline comments as done.

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
310 ↗(On Diff #138895)

Oops, well spotted!

lib/DebugInfo/DWARF/DWARFContext.cpp
437 ↗(On Diff #138895)

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

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
301 ↗(On Diff #138957)

Why reformat this?

David Blaikie via llvm-commits <llvm-commits@lists.llvm.org> writes:

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
301 ↗(On Diff #138957)

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.

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

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.

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.

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 added a comment.EditedMar 27 2018, 3:13 AM

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.

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

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

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 edited the summary of this revision. (Show Details)

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.

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.

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.

A quick request: please git-clang-format.

espindola added inline comments.Mar 29 2018, 3:47 PM
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
1921 ↗(On Diff #140064)

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

I will update the surrounding code.

espindola added inline comments.Mar 29 2018, 5:45 PM
include/llvm/DebugInfo/DWARF/DWARFContext.h
262 ↗(On Diff #140064)

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

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
44 ↗(On Diff #140064)

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.

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

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!

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'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?

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?

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

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?

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 ↗(On Diff #140064)

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

jhenderson marked 3 inline comments as done.
jhenderson edited the summary of this revision. (Show Details)
jhenderson removed a subscriber: rafael.

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 accepted this revision.Apr 26 2018, 2:54 PM

LGTM.

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

This revision is now accepted and ready to land.Apr 26 2018, 2:54 PM

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 added inline comments.Apr 30 2018, 11:37 AM
include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
30 ↗(On Diff #143531)

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.

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

jhenderson edited the summary of this revision. (Show Details)May 1 2018, 1:44 AM
jhenderson edited the summary of this revision. (Show Details)May 1 2018, 1:49 AM

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 updated this revision to Diff 145929.May 9 2018, 8:38 AM
jhenderson edited the summary of this revision. (Show Details)

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 accepted this revision.May 9 2018, 11:42 AM

Seems good - thanks for your patience.

unittests/DebugInfo/DWARF/DwarfGenerator.cpp
472–474 ↗(On Diff #145929)

Usually LLVM code omits braces from single line blocks.

501–502 ↗(On Diff #145929)

make_unique?

jhenderson added inline comments.May 10 2018, 3:27 AM
unittests/DebugInfo/DWARF/DwarfGenerator.cpp
472–474 ↗(On Diff #145929)

Of course. My bad.

501–502 ↗(On Diff #145929)

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 marked 2 inline comments as done.

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.

jhenderson edited the summary of this revision. (Show Details)May 10 2018, 3:41 AM
This revision was automatically updated to reflect the committed changes.
JDevlieghere added inline comments.May 14 2018, 4:29 AM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
299

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 added inline comments.May 14 2018, 5:57 AM
llvm/trunk/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
299

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