mstorsjo added inline comments.
================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754 + m_ast.getASTContext(), + clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance)); + } ---------------- rnk wrote: > mstorsjo wrote: > > rnk wrote: > > > I'm concerned that this isn't really the right fix. Changing the > > > inheritance model changes the size of the member pointer representation, > > > so the consequences of getting the wrong one could affect expression > > > evaluation results. Zequan suggested guessing the inheritance model from > > > the class definition, but we really ought to write down the inheritance > > > model in the DWARF when using the MS ABI. This probably needs its own > > > DWARF attribute. > > FWIW, there’s two use cases at play here: > > > > 1. The executable actually is using the itanium ABI, but is being > > (mis)interpreted with the MSVC ABI. When this happens, we currently crash, > > which is a terrible user experience. In this case, there’s probably no good > > solution (we can’t really get it right at this point) - but pretty much > > anything is better than crashing. Before D127048, we always interpreted > > everything with the MSVC C++ ABI; now we probably are getting it right in a > > majority of cases at least. > > > > 2. People intentionally using dwarf debug into with MSVC ABI. Not very > > common, but something that some people do use, as far as I know. Here we at > > least have a chance of getting right. > I see, I thought we already knew we were in use case 2 not 1. Having this as > a workaround to not crash seems fine, but we really ought to warn when LLDB > encounters these conditions: > 1. MS C++ ABI is in use > 2. DWARF is in use > 3. A C++ source file is in use > > Using DWARF for C code in the MS ABI model is fine, no need to warn in that > case. Some kind of warning would be great, yes. Is MS ABI C++ in dwarf essentially broken by definition (unless we’d extend our dwarf generation)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130942/new/ https://reviews.llvm.org/D130942 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
