Page MenuHomePhabricator

Assert that a subprogram should have a name when parsing DWARF
AbandonedPublic

Authored by shafik on Feb 3 2020, 1:19 PM.

Details

Summary

This is just an enforcement of the DWARF requirement that a DW_TAG_subprogram should have a DW_AT_name.

This came up when updating how we generating some debug info and one of the possible change caused several LLDB tests to fail. This was ultimately due to subprograms being generated without names but the immediate symptom did not point to that.

Diff Detail

Event Timeline

shafik created this revision.Feb 3 2020, 1:19 PM
aprantl added inline comments.Feb 3 2020, 2:46 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
832

The message should be:

"DWARF validation error: DW_TAG_subprogram without DW_AT_name"

Can you double-check that we don't already have an error reporting mechanism for malformed debug info?

833

"Subprograms require a name" raises more questions than it answers:

  • does that mean that LLDB will crash when this happens?
  • since there is an assertion it definitely means that this code path is untested ...

If LLDB doesn't crash, then perhaps say something like:

"this is a bug in the producer"

In any case you need to be prepared for the possibility that somebody will find a compiler out in the wild that produces this kind of DWARF and will ask you to remove the assertion again. So it's probably better to leave this out.

aprantl added inline comments.Feb 3 2020, 2:52 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
832

Looks like other places use GetObjectFile()->GetModule()->ReportError() for this.

JDevlieghere added inline comments.Feb 3 2020, 2:54 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
833

Using an assertion for invalid input goes against the assertion manifesto on https://lldb.llvm.org/resources/contributing.html

davide added a subscriber: davide.EditedFeb 3 2020, 3:06 PM

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

davide added a comment.Feb 3 2020, 3:07 PM

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

labath added inline comments.Feb 3 2020, 5:21 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
833

Using an assertion for invalid input goes against the assertion manifesto on https://lldb.llvm.org/resources/contributing.html

+100

If you want to surface this somehow you can use the Module::ReportError function.

shafik planned changes to this revision.Feb 4 2020, 8:34 AM

Everyone has brought up great feedback, let me go back and revise this.

shafik updated this revision to Diff 243678.Feb 10 2020, 2:38 PM
shafik marked 5 inline comments as done.

Updated approach based on comments and added test for the new approach.

I will try to look into dwarfdump --verify separately.

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

I think you misunderstood my suggestion. I'm not saying that we should limit this to dsymutil. I'm saying that dsymutil has a mode where it verifies the dSYM it just created. It's entirely optional and you have to pass --verify to enable it. I suggest we have something similar in LLDB, where we have a pedantic mode that, when enabled, checks all the DWARF it reads with the DWARF verifier.

As discussed offline with Shafik, I prefer this over the current approach for a few reasons:

  • It would make this behavior opt-in. Verifying the DWARF can be expensive and not every user has control over the debug info it reads. It should be possible to silence these warnings if they don't change LLDB's behavior.
  • It would provide much better coverage than some ad-hoc checks. Currently, not getting these kind of errors form LLDB doesn't tell you much. We may or may not have a check for a particular kind of invalid DWARF, so to be sure you'd still have to run it through dwarfdump -verify.
  • It would mean we only have to maintain a single DWARF verifier, which is already tested extensively.
  • It fits with our long-term goal of moving to LLVM's DWARF parser.

DWARFASTParserClang looks to me the wrong layer to fix this. Why can't this be caught in the generic DWARF Parser?
I also believe that it's better if dwarfdump -verify crashes on this, rather than lldb.

Many

Given that this error is non-actionable, I don't see any value in diagnosing this LLDB. It is important to have this in dwarfdump, which does not detect this right now.

It might be interesting to have LLDB run in a sort of "pedantic" mode which verifies all the DWARF it consumes with the dwarf verifier in LLVM. We have something similar in dsymutil which runs the verifier over the generated dSYM.

Note that many OS X developers never debug a dSYM build of their project. They debug with .o files, then make a dSYM when they do their release builds. And they probably don't look at the output of dsymutil amidst all the noise of a build. So if we only do this in dsymutil, we are greatly narrowing the range of folks who might see & report this error to us.

I think you misunderstood my suggestion. I'm not saying that we should limit this to dsymutil. I'm saying that dsymutil has a mode where it verifies the dSYM it just created. It's entirely optional and you have to pass --verify to enable it. I suggest we have something similar in LLDB, where we have a pedantic mode that, when enabled, checks all the DWARF it reads with the DWARF verifier.

As discussed offline with Shafik, I prefer this over the current approach for a few reasons:

  • It would make this behavior opt-in. Verifying the DWARF can be expensive and not every user has control over the debug info it reads. It should be possible to silence these warnings if they don't change LLDB's behavior.
  • It would provide much better coverage than some ad-hoc checks. Currently, not getting these kind of errors form LLDB doesn't tell you much. We may or may not have a check for a particular kind of invalid DWARF, so to be sure you'd still have to run it through dwarfdump -verify.
  • It would mean we only have to maintain a single DWARF verifier, which is already tested extensively.
  • It fits with our long-term goal of moving to LLVM's DWARF parser.

I second this motion.
Realistically what this patch is currently doing is diagnosing a very narrow problem emitting a somewhat obscure diagnostic for users. People who use debuggers aren't necessarily asked to understand DWARF.
If we really want to move towards a verification mode, the plan suggested above is much more reasonable than having piecemeal diagnostic sprinkled over the parser.

shafik abandoned this revision.Feb 13 2020, 3:49 PM

I am going to abandon this change b/c the consensus seems to be that we want a different solution and I don't know how much work would require ATM but I may revisit another time,

I will note that we do currently have a lot of these and they have in the past be very helpful solving problems but clearly the bar is higher now for new ones.