This creates inline functions decls in the TUs where the funcitons are inlined and local variable decls inside those functions.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
I don't know much about PDBs, but overall, the patch seems fine to me.
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.
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1059 | 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. |
- Split the inline_sites.s test into two. One for live testing on windows, another one for all platforms.
- Fix wrong variable address range when there are gaps.
I'd consider writing the live test case in c++ (with judicious use of always_inline, noinline, etc. attributes)
I think it's better to just add live test case on compiled inline_sites.s so the test result is not influenced by optimization change.
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1059 | Done. Probably modified accidentally. |
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1123 | Please add a comment about what the inlinee is, and what this lookup does. Basically, this lookup will find function declarations from previously inlined call sites. | |
1219 | We shouldn't use cantFail if it isn't implied by local invariants. You should check if the parent scope is in fact a string first, otherwise this code will crash on invalid PDBs. | |
1221 | Can these be nested? You may need to split these on :: and create or look up nested namespace decls. | |
1224 | This only requires func_ti as an input. Does it need to be part of the case? Should we compute param_count for methods too? | |
1229 | This is overwritten later, so I think you can remove this if block. | |
1240 | I don't know LLDB style, but LLVM style recommends using auto when casting so you do not need to repeat the casted type. This could be: const auto *func_type = llvm::dyn_cast<clang::FunctionProtoType>(func_qt); |
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1221 | Yes, there can be nested and then the StringIddRecord would contains all of that. | |
1224 | Removed. param_count is computed outside the switch but it's not useful for method decls. Method decls are created via TypeSystemClang::AddMethodToCXXRecordType which will add parameter decls. | |
1229 | Yeah, I forgot to remove this if block. It's not necessary anymore. |
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1124 | inline_site is unused from here on in. Really, this function gets or creates a FunctionDecl from an LF_FUNC_ID record. Inline sites happen to point to one of those. Instead of naming this GetOrCreateInlinedFunctionDecl, could you name this GetOrCreateFunctionDeclFromId and restructure it accordingly? I think that will be more useful in the future if and when we have to make decls from other ids. I believe S_*PROC32 records do not reference these records, so I don't think there is a way to share the codepaths. |
address comment.
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1124 | Split out GetOrCreateFunctionDeclFromId from GetOrCreateInlinedFunctionDecl to get or create function decls from function id. GetOrCreateFunctionDeclFromId needs both the ipi id and symbol id because we need to parse the parameter symbol records after the function symbol record. |
Well... if you write the test that way, then this is pretty much the only way to avoid random optimizer changes from braking the test.
But it has the opposite problem -- it's so strict that random changes to lldb (and its output) can break it. And since the test will only run on x86 windows, most lldb contributors (everyone except you, basically) will only notice it after it breaks.
This isn't how we usually write tests (although one could make a case doing more of it -- our coverage of debugging optimized code is fairly low). Can you say (in english) what are the properties that you are trying to check there? Maybe we can find a better way to do that...
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1219 | (FWIW, the amount of cantFails in the PDB code definitely makes me uncomfortable, but I don't know enough about PDBs or the PDB reader to say which of those are good invariants. But in general, we treat debug info as "user input" and try to avoid crashing on malformed data.) | |
lldb/test/Shell/SymbolFile/NativePDB/inline_sites_live.test | ||
2 ↗ | (On Diff #417701) | you also need REQUIRES: native && target-x86_64 |
Can you say (in english) what are the properties that you are trying to check there? Maybe we can find a better way to do that...
I'm trying to check for local variables values in inline functions by printing them (see inline comment on PdbAstBuilder::ParseBlockChildren). Since this test only runs on x64, I think I will change the live debugging test to a cpp file.
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp | ||
---|---|---|
1485 | This is only called when printing variables (or expression evaluation), transitively from https://github.com/llvm/llvm-project/blob/main/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp#L9448 . I saw an infinite recursion on this function when working on this patch because of the missing symbols.drop_front();. So, I want to add a live debugging test. |
Sorry about the delay. This dropped off my radar.
If the goal is checking for local variables, then I'd say the test is way too strict for that. In particular, moving through code (even unoptimized code) with step instructions is very sensitive to codegen changes. And the hardcoded line numbers everywhere will make life hard for whoever needs to update this tests.
Please see inline comments on how I'd approach a local variable test. I use the always_inline attribute, which allows you to create inline functions in unoptimized builds. This greatly reduces our exposure to codegen cleverness. If building with -O0, then the optnone attribute is not necessary, but I've left it there to demonstrate how that can be used to keep a variable live in optimized builds. (It also provides a good anchor to place a breakpoint). I am also using breakpoints for moving through the code. Breakpoints are more reliable as they're usually not affected by instruction reordering (mainly important for optimized code, but a good practice in any case). I set the breakpoint's via markers left in the source code, which allows me to avoid using any line numbers. I am deliberately not matching the entire block that lldb prints when the process stops, as that is not relevant for this test.
I wasn't sure what was the exact setup you wanted to check (one problem with overly strict tests), so I've created something simple but still fairly similar to your setup. It should be fairly easy to tweak the test to do check other things as well.
lldb/test/Shell/SymbolFile/NativePDB/inline_sites_live.cpp | ||
---|---|---|
7 | void __attribute__((optnone)) use(int) {} void __attribute__((always_inline)) bar(int param) { use(param); // BP_bar } void __attribute__((always_inline)) foo(int param) { int local = param+1; bar(local); use(param); // BP_foo use(local); } int main(int argc) { foo(argc); } | |
24 | br set -p BP_bar br set -p BP_foo run (CHECK: stop reason = breakpoint 1) (CHECK: frame #0: {{.*}}`main [inlined] bar) p param continue (CHECK: stop reason = breakpoint 2) (CHECK: frame #0: {{.*}}`main [inlined] foo) p param p local |
Update live debugging test to build with no optimization.
lldb/test/Shell/SymbolFile/NativePDB/inline_sites_live.cpp | ||
---|---|---|
7 | I moved the line // BP_foo to the next line, because there are some inaccuracy in the line table with inlined info. I'll fix it later. |
Looks good thanks. I didn't really check the pdb code, but it looks like @rnk already reviewed that.
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.