This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NativePDB] Create inline function decls
ClosedPublic

Authored by zequanwu on Mar 17 2022, 3:27 PM.

Details

Summary

This creates inline functions decls in the TUs where the funcitons are inlined and local variable decls inside those functions.

Diff Detail

Event Timeline

zequanwu created this revision.Mar 17 2022, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 3:27 PM
zequanwu requested review of this revision.Mar 17 2022, 3:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 3:27 PM
zequanwu updated this revision to Diff 416621.Mar 18 2022, 3:34 PM

Fixing bugs and refactor.

zequanwu edited the summary of this revision. (Show Details)Mar 18 2022, 3:36 PM
zequanwu added reviewers: teemperor, labath.

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.

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.

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.

zequanwu updated this revision to Diff 417413.Mar 22 2022, 3:37 PM
zequanwu marked an inline comment as done.
  • 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.
zequanwu updated this revision to Diff 417414.Mar 22 2022, 3:39 PM

Use cast instead of dyn_cast.

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.

zequanwu updated this revision to Diff 417419.Mar 22 2022, 3:55 PM

Fix test case.

rnk added inline comments.Mar 22 2022, 4:04 PM
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);
zequanwu updated this revision to Diff 417432.Mar 22 2022, 4:37 PM
zequanwu marked 5 inline comments as done.

Address comments.

zequanwu added inline comments.Mar 22 2022, 4:46 PM
lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp
1221

Yes, there can be nested and then the StringIddRecord would contains all of that.
For example, function foo's ParentScope is the following.
0x1070 | LF_STRING_ID [size = 16] ID: <no type>, String: A::B

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.

zequanwu marked an inline comment as done.Mar 22 2022, 4:47 PM
rnk added inline comments.Mar 22 2022, 7:22 PM
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.

zequanwu updated this revision to Diff 417701.Mar 23 2022, 11:47 AM
zequanwu marked an inline comment as done.

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.

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.

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.

zequanwu updated this revision to Diff 418094.EditedMar 24 2022, 5:26 PM
  • change live debugging test to cpp file.
  • minor fix on previous change.

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.

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
zequanwu updated this revision to Diff 419521.Mar 31 2022, 11:14 AM
zequanwu marked 2 inline comments as done.

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.

labath accepted this revision.Apr 1 2022, 12:00 AM

Looks good thanks. I didn't really check the pdb code, but it looks like @rnk already reviewed that.

This revision is now accepted and ready to land.Apr 1 2022, 12:00 AM
This revision was automatically updated to reflect the committed changes.

Thanks for reviewing.