@dblaikie

In D144008#4185551, @dblaikie wrote:

@krisb - how's this handle cases of old IR being used in a newer compiler/linker?

@jmmartinez

In D144008#4184543, @jmmartinez wrote:

I've just finished reading and playing with the whole set of patches. Beside some minor nitpicks I've already posted everything else looks good to me.

I also tried compiling LLVM with the patches to see if there was any difference in size in clang's binary (w/debug info and static linking): 1454920 Kb before the patch vs 1454684 Kb after. I've got simiar results when trying with split bash.

I tested the code on some simple examples and it worked as expected.

Rebase.

In D144007#4185548, @dblaikie wrote:

I guess this one's a bit clearer than the type one about why changing the order is important for getting local entities correctly scoped - local static is a global variable for the debug info and IR, so we need to emit the scopes before we'll know where to put the globals, etc.

Apply comments and rebase.

Apply review comments and rebase.

Rebase.

In D144005#4185541, @dblaikie wrote:

Change seems OK - but might be nice to know a bit more about why this is helpful for correcting function-local entities?

Rebase.

@dblaikie thank you for looking at this! Hope this will be the final and successful iteration of the patchset.

Apply comments & rebase.

Rebase.

Hello @MaskRay, thanks for the review! And sorry for the delay with response.
The patch is useful in a probably rare case, indeed. It reproduces with a statically linked application that does not use exceptions itself, but hooks the personality routine because of some indirect dependencies that are removed by GC. Unfortunately, we cannot share our sample, but we will try to find something meaningful in the public domain.
Note that GNU ld produces smaller binaries because it analyzes the liveness of sections/pieces more thoroughly.

Friendly ping.

Apply review comments.

@jmmartinez thank you for looking at this!

Abandon in favor of https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544.

Abandon in favor of https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544.

Abandon in favor of https://discourse.llvm.org/t/rfc-dwarfdebug-fix-and-improve-handling-imported-entities-types-and-static-local-in-subprogram-and-lexical-block-scopes/68544.

@simon_tatham thank you for looking at this!

@peter.smith thank you for your comments!

Make test checks more readable.

@jmmartinez Just FYI, I'm going to resurrect/update the whole patchset related to D125693 in a week or two, and will appreciate any help in reviewing the patches.

Ping

Rebase on ToT

Let's give this another chance.

Keep original test case and add another one.

Rebase & ping

Fix lld test

In D137067#3896524, @dblaikie wrote:

Test case can be simplified a bit further:

Thank you!

Apply review comments.

Fix misprint.

• Rebase on top of 'main'.
• Reuse DIsubprogram's retainedNodes to track function-local types, imports and static locals.

Rebase

Rebase

Tracking function-local entities per-scope is good from optimization point (we may remove DILexicalBlock if there are no instruction left in it), but it makes handling abstract trees in AsmPrinter difficult.
We want to create an abstract scope if it is a parent scope for any of tracked nodes (this is true for local variables/labels as well as for other local entities). Since there are no easy ways to find which DILexicalBlocks belong to a particular subprogram, I'm not sure this approach is profitable.

In D125693#3653631, @dblaikie wrote:

In D125693#3648942, @krisb wrote:

In D125693#3644029, @dblaikie wrote:

In D125693#3641742, @krisb wrote:

@dblaikie, could you please take a look at this and/or D113741? Do you see any ways to proceed?

My concern with this direction is that anything that adds lists to the IR metadata makes it difficult for that data to be dropped when it becomes unused (the type graph, apart from the "retained types" on the CU, is structured so that if a variable, say, gets optimized away, or a function gets optimized away and its parameters along with it, the types get dropped too - similarly with function descriptions, they aren't in a list (they used to be) and are instead referenced from the llvm::Function ensuring that if the function is optimized away entirely, the debug info for that goes away too). Admittedly function-local things are handled somewhat differently, for instance there is a list on the DISubprogram of the local variables to ensure they are retained through optimizations so name lookup does the right things at function-scope. So /maybe/ it's OK to move in that direction here, but it might look more like that, add these other function-local things to the DISubprogram-scoped list (rename the list to generalize over more than just variables), rather than adding per-scope lists?

Initially, I made the dedicated per-scope list of local static vars/imports/types to make possible to remove unused entities if their parent scope gets optimized out. This doesn't fully resolve your concern about local types that might be emitted even if they are not used, but this makes the things a bit better. Moreover, such local entities as well as variables/labels are not going to be emitted to a final DWARF if their scope was optimized away. Currently, to emit any local entity we need its scope to have LexicalScope defined. If there are no DILocations in such a scope (so that it may be considered as optimized out), it will not have LexicalScope defined for it, thus no variables/labels/other local things will be emitted for it. So, if we are not going to change this design in a near future, it might be worse to consider switching local variables/labels to per-scope list as well.

What about merging variables/labels/other entities to a single list, I fully support this idea (it would require some additional checks in the backend, but would look more consistent).

Ah, fair point about implicitly dropping some debug info if it's in a dead scope. & yeah, maybe moving the existing optimized local variables list into a per-scope list might be a nice improvement regardless.

Well, this isn't as good as I thought. We are skipping optimized scopes and entities belong to them iff the scope is concrete (either inline or out-of-line). But we never skip abstract scopes; instead we create LexicalScope's just before constructing an abstract tree (see DwarfDebug::ensureAbstractEntityIsCreated()) if there are some retained nodes in them (currently, local variables or labels).

Rebase

In D125693#3644029, @dblaikie wrote:

In D125693#3641742, @krisb wrote:

@dblaikie, could you please take a look at this and/or D113741? Do you see any ways to proceed?

My concern with this direction is that anything that adds lists to the IR metadata makes it difficult for that data to be dropped when it becomes unused (the type graph, apart from the "retained types" on the CU, is structured so that if a variable, say, gets optimized away, or a function gets optimized away and its parameters along with it, the types get dropped too - similarly with function descriptions, they aren't in a list (they used to be) and are instead referenced from the llvm::Function ensuring that if the function is optimized away entirely, the debug info for that goes away too). Admittedly function-local things are handled somewhat differently, for instance there is a list on the DISubprogram of the local variables to ensure they are retained through optimizations so name lookup does the right things at function-scope. So /maybe/ it's OK to move in that direction here, but it might look more like that, add these other function-local things to the DISubprogram-scoped list (rename the list to generalize over more than just variables), rather than adding per-scope lists?

@dblaikie, could you please take a look at this and/or D113741? Do you see any ways to proceed?

Ping?

Ping

Make DwarfDebug::endModule() to skip function-local decls
CUs 'imports', 'globals' and 'enums' instead of asserting.

Ping

Rebase.

Rebased

In D125691#3580147, @aprantl wrote:

Do you need to add any special handling to CloneFunction.cpp or will it work correctly?

Tested the whole patchset with various combinations of

• opt levels (O0, O2, O3, ThinLTO, FullLTO),
• debug info options (-fdebug-types-section, -gline-tables-only, -fno-eliminate-unused-debug-types -gsplit-dwarf, -fsplit-dwarf-inlining),
• and some additional checks / asserts (like additional verification of abstract / concrete out-of-line tree to be sure local entities are not duplicated, do not go to the wrong tree)

on clang bootstrapping build. No issues so far (except PR55680, which caused not by this and related patches).

Ping

Applied review comments.

Add two tests from D113741.
Fixed crash in DwarfCompileUnit in case of PR55680 (the issue itself wasn't fixed), add a test.

Add tests:

• llvm/test/DebugInfo/Generic/split-dwarf-local-import-fulllto.ll (temporary)
• llvm/test/DebugInfo/Generic/split-dwarf-local-import3.ll