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

[ayermolo]

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.

[ayermolo]

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

[ayermolo]

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

[dblaikie]

In D124892#3494463, @ayermolo wrote:

@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 :)

[ayermolo]

In D124892#3494612, @dblaikie wrote:

In D124892#3494463, @ayermolo wrote:

@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.

[dblaikie]

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]

simplified test

[ayermolo]

In D124892#3494854, @dblaikie wrote:

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!

[ayermolo]

@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.

[dblaikie]

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]

In D124892#3529768, @dblaikie wrote:

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. :)

[dblaikie]

In D124892#3532246, @ayermolo wrote:

In D124892#3529768, @dblaikie wrote:

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?

[ayermolo]

In D124892#3538807, @dblaikie wrote:

In D124892#3532246, @ayermolo wrote:

In D124892#3529768, @dblaikie wrote:

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.