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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
