Assert that a subprogram should have a name when parsing DWARF

[shafik]

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.

[aprantl]

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]

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

832

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

[JDevlieghere]

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]

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]

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]

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]

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

[shafik]

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

[shafik]

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

[JDevlieghere]

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.

[jingham]

In D73921#1855961, @davide wrote:

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

In D73921#1868191, @JDevlieghere wrote:

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.

[JDevlieghere]

In D73921#1868228, @jingham wrote:

In D73921#1855961, @davide wrote:

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

In D73921#1868191, @JDevlieghere wrote:

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.

[davide]

In D73921#1868286, @JDevlieghere wrote:

In D73921#1868228, @jingham wrote:

In D73921#1855961, @davide wrote:

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

In D73921#1868191, @JDevlieghere wrote:

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]

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.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

 TypeSP DWARFASTParserClang::ParseSubroutine(const DWARFDIE &die,
                            ParsedDWARFTypeAttributes &attrs) {
   Log *log(LogChannelDWARF::GetLogIfAny(DWARF_LOG_TYPE_COMPLETION |
                                         DWARF_LOG_LOOKUPS));
 
   SymbolFileDWARF *dwarf = die.GetDWARF();
   const dw_tag_t tag = die.Tag();
 
+  if (tag == DW_TAG_subprogram && !attrs.name) {
+    LLDB_LOGF(log, "Parsing a Subprogram that has no name");
+    assert(false && "Subprograms require a name");
+  }
+
   bool is_variadic = false;
   bool is_static = false;
   bool has_template_params = false;
 
   unsigned type_quals = 0;
 
   std::string object_pointer_name;
   if (attrs.object_pointer) {