[Lldb-commits] [PATCH] D73921: Assert that a subprogram should have a name when parsing DWARF

[Davide Italiano via Phabricator via lldb-commits]

davide added a comment.

In D73921#1868286 <https://reviews.llvm.org/D73921#1868286>, @JDevlieghere wrote:

> In D73921#1868228 <https://reviews.llvm.org/D73921#1868228>, @jingham wrote:
>
> > In D73921#1855961 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73921/new/

https://reviews.llvm.org/D73921