[Lldb-commits] [PATCH] D121967: [LLDB][NativePDB] Create inline function decls

Tue Mar 22 02:44:42 PDT 2022

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