In D113741#3538443, @dblaikie wrote:

OK, so if I want to reproduce the issue for you, I need to apply these three patches? D114705 D113741 D113743

You, likely, not need D113743 (it'd been reverted just after being committed, so it seems it couldn't be the guilty one).

In D113741#3532850, @dblaikie wrote:

In D113741#3516038, @krisb wrote:

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

Ah, OK - so that 5-part sequence is an alternative to this one?

Mostly. The alternative implementation reuses large part of work of this patch.
Basically, this first implementation has (had) 3 patches in sequence:

• D114705 (moving handling of globals, types and imports from beginModule() to endModule() in DwarfDebug.cpp)
• D113741 (this one, backend's part)
• D113743 (clang's part)

Thank you, @asavonic for your comments!

Fix comment.

Split on two: D125694 and D125695.

There is an alternative implementation that relies on addition field of DISubprogram/DILexicalScope that tracks static locals, local imports and types D125693.

Mark this and D125691 as WIP since I'm still testing the approach on various combinations of debug info and optimization options (O0, O3, thinlto, split-dwarf, split-dwarf-inlining, gline-tables-only, etc.).
Except of the testing purpose itself I'm also trying to catch the issue reported by David here https://reviews.llvm.org/D113741#3437182 for previous implementation (while I hope this approach wouldn't trigger the issue, it'd be good to catch and fix it anyway).

Abandon for now.
Issue on gitlab: https://github.com/llvm/llvm-project/issues/54475

Rebase on ToT.

In D113741#3437182, @dblaikie wrote:

I tried applying this internally & running the same build that tripped over something last time this was committed & I seem to be tripping over the same thing:

(.debug_gnu_pubnames): name lookup table at offset 0x38cd has a terminator at offset 0x38db before the expected end at 0x3a3d

As well as a bunch of unresolved symbols during linking... :/

I'll have to work on reducing a test case for this.

In D113741#3414966, @dblaikie wrote:

In D113741#3348795, @krisb wrote:

Statement 1. The problem with the example you provided above was because this patch made subprograms and their local "global" entities emitted in different CUs in the case of split DWARF. This is a bug, for sure.

Is that bug now addressed in the proposed patch & with tests to demonstrate it's fixed?

Friendly ping :)

In D120494#3343880, @tra wrote:

What is the problem the patch is intended to solve? To allow better vectorization of param-space loads/stores ?

Ping

Ping

@tra thank you for looking at this!

In D113741#3343849, @dblaikie wrote:

In D113741#3343683, @krisb wrote:

In D113741#3343496, @dblaikie wrote:

I'm not immediately following why a list of CUs is needed/why we'd create the variable in more than one CU - could you explain that?

Well, this is because we may have a parent subprogram emitted in more than one CU (in the case of split dwarf), and we do not know in which one.
Here are the situations I see (hope I got them right):
(1) If split dwarf disabled, we have a single list of subprogram's abstract origins and can reference them between CUs. So, we'll emit a concrete definition and its abstract origin (any existing) in the same CU. In this case, we have only one CU.
(2) If we have split dwarf enabled, and shareAcrossDWOCUs() is false (it's false by default), every CU will maintain its own abstract origin's list. In this case, we cannot have references between CUs, and if we have cross-CU inlining, we'll create:

• concrete def -> in the CU the subprogram is defined,
• abstract origin -> in every CU the subprogram is inlined. For all local entities we should follow the same logic.

Do you have a test case for this (sorry, I realize it might be one of the test cases in the patch - I haven't looked/would appreciate some pointers)? My recollection is that without shareAcrossDWOCUs we should probably be only emitting one CU if using Split DWARF - Split DWARF doesn't really support multiple CUs in a single .dwo file (& we only support emitting a single .dwo for a given .o file) - but I guess maybe we do in full LTO mode, even if we don't do it in ThinLTO mode (ThinLTO's the only thing I've been especially concerned with/the only mode we use at Google)?

Apply David's comment and add 'REQUIRES: object-emission' to tests.

Fix a test.

Add SM_70 requirement for 'match' builtins.

In D113741#3343849, @dblaikie wrote:

In D113741#3343683, @krisb wrote:

In D113741#3343496, @dblaikie wrote:

All good.

I'm not immediately following why a list of CUs is needed/why we'd create the variable in more than one CU - could you explain that?

Well, this is because we may have a parent subprogram emitted in more than one CU (in the case of split dwarf), and we do not know in which one.
Here are the situations I see (hope I got them right):
(1) If split dwarf disabled, we have a single list of subprogram's abstract origins and can reference them between CUs. So, we'll emit a concrete definition and its abstract origin (any existing) in the same CU. In this case, we have only one CU.
(2) If we have split dwarf enabled, and shareAcrossDWOCUs() is false (it's false by default), every CU will maintain its own abstract origin's list. In this case, we cannot have references between CUs, and if we have cross-CU inlining, we'll create:

• concrete def -> in the CU the subprogram is defined,
• abstract origin -> in every CU the subprogram is inlined. For all local entities we should follow the same logic.

Do you have a test case for this (sorry, I realize it might be one of the test cases in the patch - I haven't looked/would appreciate some pointers)? My recollection is that without shareAcrossDWOCUs we should probably be only emitting one CU if using Split DWARF - Split DWARF doesn't really support multiple CUs in a single .dwo file (& we only support emitting a single .dwo for a given .o file) - but I guess maybe we do in full LTO mode, even if we don't do it in ThinLTO mode (ThinLTO's the only thing I've been especially concerned with/the only mode we use at Google)?

Attempt 2.

In D113743#3343481, @dblaikie wrote:

In D113743#3177437, @krisb wrote:

In D113743#3175301, @dblaikie wrote:

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

Thank you for the suggestion!
I implemented a small workaround that makes lldb behaves like if the types have subprogram scope (see https://reviews.llvm.org/D115277), not sure I'll be able to do something better any time soon.
So, if D115277 isn't acceptable, and nobody volunteers to implement this properly, the only way to proceed is to turn this functionality off for lldb.

Thanks for having a go at a fix/workaround here.

I think we'll still need to hear from Apple folks (@aprantl @JDevlieghere @shafik ) - I'm not sure what their backwards compatibility (new clang-produced binaries being debugged with an old lldb) requirements are. If they need some backcompat there, then I guess they/we/you would still need to disable this DWARF fidelity improvement when tuning for lldb. Hopefully that can be avoided, but I don't know.

In D113741#3343496, @dblaikie wrote:

In D113741#3343268, @krisb wrote:

@dblaikie I've updated the patch with a fix for split dwarf issue (the test is included). Could I ask you to check the full build on your side?

The issue was in wrong assumptions about in which CUs we need to create those local entities. Basically, they [CUs] should match the ones where the parent subprogram was emitted.
It's not an easy thing to find the particular list of suited CUs while we are in endModule() (especially, because split-dwarf and split-dwarf inlining introduce a tricky logic around), so I chose to collect this information during subprograms emission.
The solution still looks a bit hacky, but I hope this is a step forward.

What about the second issue, unfortunately (or fortunately? :)), I'm still able to reproduce the crash on ToT of main, so I'm not sure this patch may cause the issue. I'll appreciate if could check it once again.

Thank you and sorry for the long delay.

All good.

I'm not immediately following why a list of CUs is needed/why we'd create the variable in more than one CU - could you explain that?

Minor fix.

Rebase on top of 'main'.

@dblaikie I've updated the patch with a fix for split dwarf issue (the test is included). Could I ask you to check the full build on your side?

• Rebase on ToT & minor fixes.
• Fix the issue when an local imported entity caused emission of multiple CUs in a dwo when split dwarf is enabled.
• Add a test for the aforementioned issue.

Rebase & align checks with new order in the following tests:

Ping

In D116225#3279861, @ellis wrote:

Hi @krisb. Are their plans to give this fix another go? Do we need to first wait for D113741 to land again?

In D113741#3211123, @shchenz wrote:

In D113741#3210838, @dblaikie wrote:

In D113741#3210175, @shchenz wrote:

To continue with a more accurate fix, one example for your reference. (The behavior is changed with/without this reversion):

Sorry, I'm not following this comment ^ (& the rest) - could you rephrase if there's a question/concern you have I can help clarify/describe/work with you on?

I guess there will be a follow-up patch for the function-local declaration for lexical block? This patch seems like a reversion of a previous functionality change?
In our internal test, we also found an issue related to variables in the lexical block, so I pasted it here for reference : )

In D116225#3208579, @dblaikie wrote:

Thanks for giving this a go. Apparently there were some lingering failures from that precursor patch ( 0ac75e82fff93a80ca401d3db3541e8d1d9098f9 ) you're reverting here as well, so that's good. I'm happy to try applying/testing the next reapplication attempt once you/we've worked out the kinks.

Do you think this is ready to commit? I do see some auto-test failures, but if I'm reading them right, they're that generic failure about the go bindings I see all the time (at least locally - I guess I don't have some go pieces installed) and probably not related to this patch?

@dblaikie added another attempt to revert this as a review: D116225. Will commit once pre-commit tests + local lldb tests passed.

@dblaikie, thank you for the reproducers!

In D113741#3205306, @dblaikie wrote:

Alrighty, got an A/B comparison with/without this patch (but still not reduced to share) - seems we end up creating a second split unit for one of the imported units under ThinLTO, which is incorrect (because Split DWARF only really supports one CU per split unit - so having two in a single dwo file is broken - at least if you're going to package the file with dwp, which we do).

Looking into it further/trying to get a reduced example.

In D113741#3190810, @dblaikie wrote:

In D113741#3190544, @dblaikie wrote:

FWIW this also caused some ThinLTO build failures (linker errors specifically) along the lines of:

(.debug_gnu_pubnames): name lookup table at offset 0x9a5 has a terminator at offset 0x9b3 before the expected end at 0x9f2

I don't have a simple/small reproduction at the moment. But would be good to work together/reach out to me if it's not easily discoverable and figure out this issue before recommitting.

Oh, I misread - that this issue was seen on the recommit & hasn't ben reverted yet.

I'll try to reduce a test case, but might be worth reverting in the interim regardless?

@rtrieu, thank you for looking at this! Do you have any comments about the rest of the patch? Do you think it makes sense?

In D115277#3178248, @jingham wrote:

I worry this makes the case where a function has two definitions of the same type in one function a more confusing error than the "can't find type" error you would get without this patch. Is this of sufficient urgency that we can't take the time to fix it correctly?

In D113743#3175301, @dblaikie wrote:

Not super surprising that lldb might not be able to deal with DWARF in this shape - is LLDB support important to you/something you plan to work on for this DWARF? Otherwise we might need to opt-out of this functionality when tuning for LLDB, for instance. Unless LLDB-invested folks are interested in doing the integration work (& even then, maybe disabling it for LLDB tuning until that's fixed). @aprantl @JDevlieghere

In D113743#3173981, @JDevlieghere wrote:

Hey Kristina, this broke TestSetData.py on GreenDragon: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/39089/

Since the bot has been red for several hours I went ahead and reverted your change in 4cb79294e8df8c91ae15264d1014361815d34a53.

Ping

Simplified diagnostic condition in TemplateDeclInstantiator::InstantiateTypedefNameDecl().

In D114892#3166220, @jhenderson wrote:

The patch is more about testing generic DebugInfo behaviour, rather than anything specific to llvm-dwarfdump. I wonder if the test would make more sense there? No strong feelings on that though.