mstorsjo added inline comments.
================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754 + m_ast.getASTContext(), + clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance)); + } ---------------- mstorsjo wrote: > 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)? Ping @zequanwu - do you want to follow up on this one? 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
