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

[Martin Storsjö via Phabricator via lldb-commits]

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:
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130942/new/

https://reviews.llvm.org/D130942