labath added a comment. Replying here as this is the relevant patch.
In D62073#1507063 <https://reviews.llvm.org/D62073#1507063>, @clayborg wrote: > Are type units still disabled with the kill switch we had in? Or is this on > top of the .debug_types patch? We have an internal .debug_types patch we have > been using on an older LLVM/Clang/LLDB and we had some major performance > issues regarding line tables so I am not sure where we would need to put > these fixes (in this patch or in the .debug_types specific patch. This patch > seems to enable parsing .debug_types already. The main issue we ran into were: > > - only search actual compile unit line tables for breakpoints as many > .debug_type units will point to existing line tables from real compile units > - don't add address ranges to type unit DWARFUnits from the line tables > - re-work how support files are parsed by sharing the line tables within a > SymbolFileDWARF. .debug_types has type units that reuse line tables from real > compile units (thousands of times) and we ended up seeing the same line table > being parsed over and over and over. Another way to fix this is to no vend a > lldb_private::CompileUnit for any units that aren't really compile units > (DW_UT_type and DW_UT_split_type). > > So if this patch is enabling all this, we will need to fix this patch to > avoid all of the above mentioned issues. Let me know your thoughts. Thanks for the heads up. This is a pretty good to-do list. I was aware of some of these things already (like the address ranges thing), but in the spirit of the incremental development policy <https://llvm.org/docs/DeveloperPolicy.html#incremental-development>, I deliberately tried to keep this patch small. I already have a patch for to stop adding address ranges for type units, but I didn't upload it yet, as I am still working on the test (that's the reason I side-stepped into dwarf5 a bit, because I believe it will make creating the test easier). Another thing, which this patch does not enable yet is the resolution of DW_AT_signatures, so with this patch alone you will be able to look up types, but not link them to variables. I also put that in a separate patch, because it was possible to do so. One more big feature, which is completely broken right now, is the combination of dwo+type units. For that I don't have a patch yet, and making that work is probably going to take a bit longer. For these reason I don't think it's reasonable to put all of these things into one big patch, and have a fully functional and optimised implementation from the start -- having things working 100% immediately is very unlikely and a large patch increases the likelyhood of breaking unrelated things. I think we should start with this patch. The address ranges patch and DW_AT_signature thingy, can come pretty much immediately after that. These are the only things (that I am aware of) which impact correctness, so we could have an implementation ready for general audience to try out as early as the end of this week. After that the road splits up a bit, and we can proceed in several directions (e.g. I'm hoping that DWO support can be mostly independent of the line table optimizations). I might even be able to get some help with that, so that we increase our velocity here, once the initial building blocks are in place. As for the "kill switch", I am open to keeping it in, but make it conditional on some setting or environment variable, until the implementation is more stable. However, I also don't think it's really necessary, as I believe we will have something sufficiently functional very quickly. (and even a slightly wonky experience is better than just refusing to debug a binary outright, imo) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62008/new/ https://reviews.llvm.org/D62008 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
