zequanwu added inline comments.
================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1754 + m_ast.getASTContext(), + clang::MSInheritanceAttr::Spelling::Keyword_unspecified_inheritance)); + } ---------------- mstorsjo wrote: > 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? Hi, I don't think I have time to follow up on this recently. 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
