[Lldb-commits] [PATCH] D130942: [LLDB][DWARF] Set MSInheritanceAttr for record decl when it's using Microsoft compatible ABI.

[Zequan Wu via Phabricator via lldb-commits]

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