Page MenuHomePhabricator

[DWARF] Fix split dwarf debug inlining in mix mode situation.
Needs ReviewPublic

Authored by ayermolo on May 3 2022, 6:09 PM.

Details

Summary

With ThinLTO enabled we can have a function in a CU that doesn't have split
dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in
constructInlinedScopeDIE, it's not present and we assert.

Not 100% sure this is a correct place to fix it. Open to suggestions.

Diff Detail

Unit TestsFailed

TimeTest
60,100 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libasan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxDynamicConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,080 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp
60,050 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=thread -Wall -m64 -msse4.2 -gline-tables-only -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -std=c++11 -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/../ -O1 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp && not /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/tsan/X86_64Config/Output/restore_stack.cpp.tmp 2>&1 | FileCheck /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/tsan/restore_stack.cpp

Event Timeline

ayermolo created this revision.May 3 2022, 6:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2022, 6:09 PM
ayermolo requested review of this revision.May 3 2022, 6:09 PM
Herald added a project: Restricted Project. · View Herald Transcript
ayermolo edited the summary of this revision. (Show Details)May 3 2022, 6:10 PM
ayermolo added a reviewer: dblaikie.

Also to add this also manifests if helper is build as monolithic.

@dblaikie Do you think you can review this, or someone else that you think would be better?

@dblaikie Do you think you can review this, or someone else that you think would be better?

This one's for me, for better and/or worse. I created this mess :)

@dblaikie Do you think you can review this, or someone else that you think would be better?

This one's for me, for better and/or worse. I created this mess :)

Thanks. I am not really sure if this is the right way to fix it. Personally seems hacky to me. I can explore other options if you have suggestions.

Yeah, mutating the DICompileUnit at this point seems a bad idea - it could end up inconsistent between things that happened before the mutation V after, etc.

Also the testing isn't suitable - LLVM tests can't depend on clang and the two IR files could be linked & the single IR result could be the test case? (it should just run llvm-mc - check other debug info tests). With the simplified/linked test, I can take a look and see if I can understand what's broken/how it might be suitable to fix it.

ayermolo updated this revision to Diff 427484.May 5 2022, 4:00 PM

simplified test

Yeah, mutating the DICompileUnit at this point seems a bad idea - it could end up inconsistent between things that happened before the mutation V after, etc.

Also the testing isn't suitable - LLVM tests can't depend on clang and the two IR files could be linked & the single IR result could be the test case? (it should just run llvm-mc - check other debug info tests). With the simplified/linked test, I can take a look and see if I can understand what's broken/how it might be suitable to fix it.

Ah thanks!

@dblaikie Maybe you can point me in a direction where the fix could be better implemented? I can see what I can come up with.

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

ayermolo added a comment.EditedMay 23 2022, 12:48 PM

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

So using this example.
When we are processing MachineFunction for main. We are processing SubProgram main, with CU in main.dwo. In constructAbstractSubprogramScopeDIE this is the SrcCU. It has debug inlining enabled.
What gets inlined in to it is Abstract Lexical Scope from helper.dwo that doesn't have debug inlining enabled.
Because of it we construct Abstract Subprogram Scope DIE only in SrcCU (Main DWO CU).

With my "fix" what happens is we end up creating CU for the helper dwo, Skeleton CU for it, and adding inline subprogram information to it for "func".
So when we construct subprogram scope die, when constructInlinedScopeDIE is invoked on scope for func (which comes from helper.cpp), we are able to find Inlined sub program and don't crash. Because we can find Origin DIE for inlined sub program.

Ideally I think we would like to have inlined sub program in main Skeleton CU. Otherwise it won't be complete. Although the way it is fixed now we end up with Skeleton CU for helper that is kind of broken (no DWO ID), and DWO CU.

Hopefully I am explaining it clearly. :)

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

So using this example.
When we are processing MachineFunction for main. We are processing SubProgram main, with CU in main.dwo. In constructAbstractSubprogramScopeDIE this is the SrcCU. It has debug inlining enabled.
What gets inlined in to it is Abstract Lexical Scope from helper.dwo that doesn't have debug inlining enabled.
Because of it we construct Abstract Subprogram Scope DIE only in SrcCU (Main DWO CU).

With my "fix" what happens is we end up creating CU for the helper dwo, Skeleton CU for it, and adding inline subprogram information to it for "func".
So when we construct subprogram scope die, when constructInlinedScopeDIE is invoked on scope for func (which comes from helper.cpp), we are able to find Inlined sub program and don't crash. Because we can find Origin DIE for inlined sub program.

Ideally I think we would like to have inlined sub program in main Skeleton CU. Otherwise it won't be complete.

I think that's probably where I'm suggesting differently: If someone didn't build SrcCU with split-dwarf-inlining, then even if that code gets inlined into MainCU that /does/ have split-dwarf-inlining, it would be OK/consistent /not/ to describe the inlined function from SrcCU. I think that'd be correctly respecting the level of detail the user asked for - no symbolizing for functions in (or from) SrcCU.

Could you implement that? By checking when we go to construct the inlined scope DIE if the subprogram comes from a CU without split-dwarf-inlining, and if so, skip creating the split dwarf inlined description?

With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.

So it seems there's probably some disconnect between the logic that adds abstract scope DIEs and the logic that looks them up? (one's using the source CU, one's using the destination CU?) - can you confirm that/which one's doing which?

& maybe them consistent would be good/suitable - I guess the right behavior is probably to treat the non-split-dwarf-inlining function as having no debug info for the purposes of the place it's inlined into?

So using this example.
When we are processing MachineFunction for main. We are processing SubProgram main, with CU in main.dwo. In constructAbstractSubprogramScopeDIE this is the SrcCU. It has debug inlining enabled.
What gets inlined in to it is Abstract Lexical Scope from helper.dwo that doesn't have debug inlining enabled.
Because of it we construct Abstract Subprogram Scope DIE only in SrcCU (Main DWO CU).

With my "fix" what happens is we end up creating CU for the helper dwo, Skeleton CU for it, and adding inline subprogram information to it for "func".
So when we construct subprogram scope die, when constructInlinedScopeDIE is invoked on scope for func (which comes from helper.cpp), we are able to find Inlined sub program and don't crash. Because we can find Origin DIE for inlined sub program.

Ideally I think we would like to have inlined sub program in main Skeleton CU. Otherwise it won't be complete.

I think that's probably where I'm suggesting differently: If someone didn't build SrcCU with split-dwarf-inlining, then even if that code gets inlined into MainCU that /does/ have split-dwarf-inlining, it would be OK/consistent /not/ to describe the inlined function from SrcCU. I think that'd be correctly respecting the level of detail the user asked for - no symbolizing for functions in (or from) SrcCU.

Could you implement that? By checking when we go to construct the inlined scope DIE if the subprogram comes from a CU without split-dwarf-inlining, and if so, skip creating the split dwarf inlined description?

Sure. Let me give it a shot next week.