labath added a comment. I don't know much about PDBs, but overall, the patch seems fine to me.
In D121967#3393556 <https://reviews.llvm.org/D121967#3393556>, @zequanwu wrote: > I think adding a live debugging test case is necessary. I found several bugs > when working on this via live debugging the compiled `inline_sites.s` and > printing variables, and those bugs were not revealed by `image lookup`. Thanks for bringing that up. I am not against all live testing, I just want to avoid it where possible. And stack frames/local variables are one of the things that hard to test comprehensively without a live process (theoretically we could use a core file, but we don't have a good way to create those). I don't know what kinds of bugs you ran into, but (since you already have the assembly-based non-live test case) I'd consider writing the live test case in c++ (with judicious use of always_inline, noinline, etc. attributes), if possible. That way it could run on all windows architectures and not just x86. ================ Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1059 if (parent->isRecord()) { - clang::QualType parent_qt = llvm::cast<clang::TypeDecl>(parent) + clang::QualType parent_qt = llvm::dyn_cast<clang::TypeDecl>(parent) ->getTypeForDecl() ---------------- What's the reason for this change? The only difference between cast and dyn_cast is that the former asserts in case of a bad cast, while the latter returns a nullptr. But here you're dereferencing the returned value anyway, and an assertion failure produces a better error message than a segfault. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121967/new/ https://reviews.llvm.org/D121967 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
