Page MenuHomePhabricator

[RFC][DwarfDebug][AsmPrinter] Support emitting function-local declaration for a lexical block
AcceptedPublic

Authored by krisb on Nov 12 2021, 1:21 AM.

Details

Summary

This is another attempt to make function-local declarations
(like static variables, structs/classes, and other) be correctly emitted
within a lexical (bracketed) block.

Some ideas were taken from previous attempts [0] and [1].

Limitations:

Since we skip lexical blocks if we couldn't find non-scope children for them,
we need a way to force the emission of blocks that have static locals or local
type declarations but don't have local variables. As a workaround, I
collect such scopes by collectLocalScopesWithDeclsFromCU() and
store them in a set. This set is used later to check whether we need to emit
a lexical block. It, however, lacks information about used records
and typedefs because there is no access to them from a CU.
In this case, we will put the entity to a first available parent scope
(with subprogram as a bound).

Fixes https://bugs.llvm.org/show_bug.cgi?id=19238.

clang's part is at D113743.

[0] https://reviews.llvm.org/D11180
[1] https://reviews.llvm.org/D15976
[2] https://bugs.llvm.org/show_bug.cgi?id=27579

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
krisb added inline comments.Nov 16 2021, 11:14 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

It's nice to know whether the given SP will have an abstract origin or not, but unfortunately, this way of obtaining such information isn't fully reliable. The problem is that having some DILocations with inlinedAt field doesn't guarantee that the inlined/abstract LexicalScope will be created (especially in optimized code). So, I'm not sure this is the right way to go. Maybe I should avoid doing this and collect all the scope DIEs instead to have more chances to emit accurate dwarf.

Also, thanks for including my tests from that diff!

It's me who should thank you for the tests :) By the way, do you plan to commit them separately?

ellis added inline comments.Nov 16 2021, 4:40 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

By the way, do you plan to commit them separately?

Let's just commit them here assuming they pass.

1190–1195

It's nice to know whether the given SP will have an abstract origin or not, but unfortunately, this way of obtaining such information isn't fully reliable. The problem is that having some DILocations with inlinedAt field doesn't guarantee that the inlined/abstract LexicalScope will be created (especially in optimized code). So, I'm not sure this is the right way to go. Maybe I should avoid doing this and collect all the scope DIEs instead to have more chances to emit accurate dwarf.

If I understand correctly, that means HasInlinedInstances() returns true for inlined SPs even if their DIEs will never be emitted (because they were optimized away for example). In that case we won't construct the scope anyway, so I think that's ok.

krisb updated this revision to Diff 387977.Nov 17 2021, 10:15 AM
krisb marked 5 inline comments as done.
krisb edited the summary of this revision. (Show Details)

Applied ellis's comments.
Stored all abstract scopes into a single map.
Stopped trying to guess which functions have inlined instances.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

Well, yet I decided to try a different approach. Instead of guessing which SPs have inlined instances and which have not in order to decide do we need to store the given DIE to the map, we simply can store all the DIEs (but those that are known to belong inlined instances). So later we can choose which tree to use: an abstract (if it exists) or a concrete one. It looks a bit better to me and we can use a single map for all abstract scopes and avoid duplcation. What do you think?

ellis added inline comments.Nov 17 2021, 11:01 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

I think any solution needs to be able to determine if an SP will have an abstract origin before processing any functions, i.e., this needs to be computed in beginModule(). Suppose a function f() has a concrete instance and is also inlined at the end of the module. It should have an abstract origin and all references should be made to the abstract origin DIE instead of the concrete DIE. That means getOrCreateSubprogramContextDIE() needs to know if it should get/create an abstract or concrete DIE even before the abstract origin is created.

I guess the import-inlined-declaration.ll test was not complete because it doesn't test if ns::foo() references the abstract origin if the concrete DIE exists. Could you update this test to handle that? Here is what I'm thinking, but I haven't actually tested this on your code yet.
https://godbolt.org/z/j1xdv3KWG

This would also be a bug when an imported entity tries to reference an SP before its abstract origin was created, but I don't have a concrete example of that.

Let's follow up on https://groups.google.com/g/llvm-dev/c/mlNUyj-Q5To if you have any more questions. Thanks!

krisb added inline comments.Nov 17 2021, 11:49 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

Suppose a function f() has a concrete instance and is also inlined at the end of the module. It should have an abstract origin and all references should be made to the abstract origin DIE instead of the concrete DIE.

This is why I finally decided to move the emission of imported entities (together with types and global variables) to DwarfDebug::endModule() instead of trying to handle these cases earlier. By endModule() we have all the local context emitted, so we don't need to force emitting abstract DIEs ahead of normal flow.

Could you update this test to handle that?

Sure, I will do. But I believe this case should be handled properly cause we never try to emit imported declarations before all the subprograms get processed.

ellis added inline comments.Nov 17 2021, 4:03 PM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1617
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1190–1195

This is why I finally decided to move the emission of imported entities (together with types and global variables) to DwarfDebug::endModule() instead of trying to handle these cases earlier. By endModule() we have all the local context emitted, so we don't need to force emitting abstract DIEs ahead of normal flow.

Ah ok makes sense!

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
625–628

Where are we creating DIEs without giving them a parent. In a previous attempt @dblaikie mentioned we should avoid even temporarily creating DIEs without a parent because of the getUnit() function.

krisb updated this revision to Diff 388823.Nov 22 2021, 2:04 AM
krisb marked an inline comment as done.

Rebase on D114350.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp
625–628

Right, this is a problem. I've opened another review request with refactoring for createScopeChildrenDIE() and createScopeChildrenDIE() that makes parent set right after creation of a DIE for local entity. Please, take a look if you have a chance.

krisb updated this revision to Diff 388825.Nov 22 2021, 2:12 AM

Minor fixups.

krisb updated this revision to Diff 390332.Nov 29 2021, 6:20 AM

Extracted https://reviews.llvm.org/D114705 (with more details on the change), rebased.

krisb marked an inline comment as done.Nov 29 2021, 6:21 AM
krisb edited the summary of this revision. (Show Details)Nov 29 2021, 6:23 AM

Generally on board with this direction - perhaps it doesn't make anything /worse/ (if the cases where types are not reachable through the CU's retainedTypes list get the same behavior they've always got - can you confirm that's the case? If that's the case, perhaps we can go forward with this, and separately/in addition, if you have time/interest/motivnation, you could work on adding local entities to the DISubprogram's retainedNodes list and search that to ensure those entities are put in the right scopes)

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1094–1097

This is fairly problematic to the strategy - generally we don't put types in the "retained types" list because the point is to lazily reference types/other content as much as possible (so if the middle end optimizes away a function and that's the only place where the type is referenced, then the type gets optimized away too).

Perhaps this points to some other direction (perhaps in addition) being needed? Perhaps local entities could go on the DISubprogram's retainedNodes attribute?

(this would address a test failure/FIXME you mentioned in one of the test cases - that a local type was missing from the intended scope)

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
675–676 ↗(On Diff #390332)

This change could probably be committed separately with a llvm-dwarfdump+assembly test case demonstrating this fix. Then the LLVM code generation changes that depend on this would come in the second patch. Not a huge deal, though.

@dblaikie thank you for looking at this!

if the cases where types are not reachable through the CU's retainedTypes list get the same behavior they've always got - can you confirm that's the case?

For local types that are not reachable through the CU's retainedTypes list there are two cases:

  • if type's direct parent scope was emitted (because it contains some other declarations), this type will be placed to this scope (as expected),
  • if type's direct parent scope was omitted (since there are no other decls in it), the type will go to a first available (emitted) parent scope (in the worst case - to the parent subprogram).

So, with this patch, local types will have the closest/most suitable parent from available. Before this patch, local types were placed directly to subprogram no matter which scope they really have (and actually, frontend never emits types with lexical block scope), so the old behavior is only kept as a fallback for the worst case.
For all non function-local types, this patch changes nothing.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1094–1097

Perhaps this points to some other direction (perhaps in addition) being needed? Perhaps local entities could go on the DISubprogram's retainedNodes attribute?

Unless I'm missing something retainedNodes semantic is to keep entities even if they can be optimized out (as of retainedTypes is for types, but only if we have corresponding compile option passed). So, I'm not sure we should use those fields to keep some local entities that are okay to be completely removed, if they unused/ get optimized away.
I was thinking about a kind of a new field for DISubprogram (and maybe for DILexicalBlock), something like localDecls which would keep references for static locals, local imported entities, and records/typedefs that belongs to this local scope (and stop referencing local entities from globals and imports fields of a compile unit), but have not come to a comprehensive solution yet.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
675–676 ↗(On Diff #390332)

Before this patch, AsmPrinter would crash on types scoped within a lexical block (cause DwarfCompileUnit would not able to create a context for such types), so I'm not sure how to reproduce the case. Do you have some ideas in mind?

dblaikie accepted this revision.Nov 30 2021, 12:00 PM

Sounds like this generally moves things forward, thanks for your patience!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1094–1097

Yep, complicated tradeoffs. By having the list we'd avoid miscoping these entities (because they're otherwise intractible to find - they could appear in any part of the DWARF) - but any attempt to keep them in a list risks them being preserved too much too.

keeping the list on the DILexicalBlock would be pretty good - that way if the lexical block became unreferenced (all instructions in it were optimized away) the lexical block could be removed and the list along with it - hmm, but there are still cases where the type could be alive despite the function being totally optimized away (eg: a trivial function that returns a stateless local type, like a lambda - so the local type escapes the scope, and the function gets inlined and optimized away - no scope, but the type is still out there in the rest of the DWARF) - in which case we'd still scope the type incorrectly...

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
675–676 ↗(On Diff #390332)

What you could do is using this patch, generate the assembly for a type scoped in a lexical block - but then commit that assembly test case ahead of the rest of the patch. It's still a good/general improvement to llvm-dwarfdump/libDebugInfoDWARF that applies even if LLVM never generates this DWARF (because llvm-dwarfdump is intended as a tool to investigate any DWARF, not just the DWARF that llvm generates).

This is true for most llvm-dwarfdump/libDebugInfoDWARF changes - they can come before the LLVM change that generates them, because they should/can be tested without any LLVM support at all, through assembly test cases .

This revision is now accepted and ready to land.Nov 30 2021, 12:00 PM
krisb marked 2 inline comments as done.Dec 1 2021, 11:02 AM
krisb added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
675–676 ↗(On Diff #390332)

Ah, got it! Thank you for all the details! I've opened a separate review for this change https://reviews.llvm.org/D114892. Please, take a look if you have a chance.

krisb updated this revision to Diff 391142.Dec 1 2021, 2:31 PM
krisb marked an inline comment as done.

Extracted https://reviews.llvm.org/D114892 + minor fixups for code style and comments.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1094–1097

Another problem is even if for every scope there would be a list of types declared within it wasn't guaranteed that these types would be placed in the correct scope. Lexical blocks DIEs are emitted based on LexicalScopes which may (and likely) not be created for optimized out scopes. Thus it still would be possible to misplace a type DIE. But as far as I know, this should not be a problem for debuggers (at least for those that I'm aware of).
We need a more tricky solution (or more changes in DWARF emission design) to make everything come together.

because they're otherwise intractible to find - they could appear in any part of the DWARF

I hope they will be at least within a parent subprogram. We may do the things in a more conservative way and in case there is no suitable parent scope DIE emitted for this type (or any other local declaration in the subject) place these entities directly into a subprogram DIE, so we will have 2 possible places for local decls:

  • direct scope,
  • subprogram scope if the direct scope is missed.

Concerning the case with lambdas, I've checked a few examples and found that clang produces scopeless DIs for such types, and this seems to be one more problem here.

This revision was landed with ongoing or failed builds.Dec 4 2021, 7:13 AM
This revision was automatically updated to reflect the committed changes.

I see this is already reverted (thanks for watching the bots after committing!), but fyi for the relabel: this also broke tests on macOS, e.g. http://45.33.8.238/mac/39897/step_11.txt

MaskRay added inline comments.
llvm/test/DebugInfo/Generic/lexical_block_static.ll
1

Somewhat strange the patch introduced new tests with both - and _ filename separators...

In most components of llvm, - is more popular though I can see directories where _ is more popular.

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.

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?

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?

If you think it'd be better to revert until the issue gets resolved then okay, let me know and I'll revert it. I can't say more without a reproducer.
But could you please share a way to reproduce the issue? Not small/simple is also okay.

llvm/test/DebugInfo/Generic/lexical_block_static.ll
1

This happened cause I borrowed some tests from ellis's patch, those ones with '-'.
Sorry, I somehow didn't notice this difference. I'll either fix it in an NFC or in the next recommit if it'll be decided it's better to revert this patch until LTO issues get resolved.

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?

If you think it'd be better to revert until the issue gets resolved then okay, let me know and I'll revert it. I can't say more without a reproducer.
But could you please share a way to reproduce the issue? Not small/simple is also okay.

Unfortunately due to ThinLTO it's a bit hard for me to extract (& not sure if there are bits I need to anonymize, etc - which is easier when reducing just a single file) and share at the moment :/

Some DIEs are coming through with a 0 offset, which prematurely terminates the pubnames/types list. I'm working on figuring out in what situation that happens.

Ah, seems to be related to "isDebugDirectivesOnly" - ie: we have debug info but only for middle end diagnostics, not to produce DWARF in the output...

hmm, wait, no, not that...

Hmm, think I'm honing in on it.

It looks like it's related to a unit that defines a type that's already defined by another unit - usually that first unit could be skipped entirely by https://github.com/llvm/llvm-project/blob/6683099a0d0a17fcde3576733e9c85e3b5f71de5/llvm/lib/CodeGen/AsmPrinter/DwarfFile.cpp#L47 - but since the namespace now gets created first, then the type is skipped...

Hm, no, maybe I'm chasing down a different manifestation caused by 78d15a112cbd545fbb6e1aa37c221ef5aeffb3f2 - perhaps still with this patch (D113741) as a root cause? I'm not sure yet.

Hmm, think I'm honing in on it.

It looks like it's related to a unit that defines a type that's already defined by another unit - usually that first unit could be skipped entirely by https://github.com/llvm/llvm-project/blob/6683099a0d0a17fcde3576733e9c85e3b5f71de5/llvm/lib/CodeGen/AsmPrinter/DwarfFile.cpp#L47 - but since the namespace now gets created first, then the type is skipped...

Correct link is actually: https://github.com/llvm/llvm-project/blob/6683099a0d0a17fcde3576733e9c85e3b5f71de5/llvm/lib/CodeGen/AsmPrinter/DwarfFile.cpp#L71

Hm, no, maybe I'm chasing down a different manifestation caused by 78d15a112cbd545fbb6e1aa37c221ef5aeffb3f2 - perhaps still with this patch (D113741) as a root cause? I'm not sure yet.

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.

krisb added a comment.EditedDec 22 2021, 6:44 AM

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.

(trying to guess the issue by keywords...)

Is the issue about a function-local import?

The only diff in imported entities handling I see is that we no longer skip emission of local imports under 'includeMinimalInlineScopes()' (which was the case before this patch, but it seems I missed this fact).
Can it be a hidden issue revealed by this mistake?

P.S. I'll fix 'includeMinimalInlineScopes()' for local import.

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.

(trying to guess the issue by keywords...)

Is the issue about a function-local import?

The only diff in imported entities handling I see is that we no longer skip emission of local imports under 'includeMinimalInlineScopes()' (which was the case before this patch, but it seems I missed this fact).
Can it be a hidden issue revealed by this mistake?

P.S. I'll fix 'includeMinimalInlineScopes()' for local import.

Yep, I'm certainly seeing some function-local imports in the test case I'm investigating, and that sounds related to be sure. Can't guarantee it's the precise issue, but I'll keep poking around at it & having a fix for the local import under minimal-inline-scopes would be good to have in any case.

dblaikie added inline comments.Dec 22 2021, 8:07 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
1039–1059

Adding back these two includeMinimalInlineScopes checks doesn't seem to address the failure I'm investigating, at least.

But they may still represent a bug (either an independent one, or as part of the issue I'm investigating).

OK, think I've got it now.

It requires splitDebugInlining=false in the source CU, and `true' in the destination CU. Needs a local using decl in the source CU and some function import from source to destination CU.

This example, compiled at -O1 does the trick:

; ModuleID = 'ab.bc'
source_filename = "llvm-link"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%"struct.ns::t1" = type { i8 }

; Function Attrs: mustprogress uwtable
define available_externally dso_local void @_Z2f1v() #0 !dbg !4 {
entry:
  %v1 = alloca %"struct.ns::t1", align 1
  call void @llvm.dbg.declare(metadata %"struct.ns::t1"* %v1, metadata !21, metadata !DIExpression()), !dbg !22
  call void @_Z3pinv(), !dbg !23
  ret void, !dbg !24
}

; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
declare void @llvm.dbg.declare(metadata, metadata, metadata) #1

declare dso_local void @_Z3pinv() #2

; Function Attrs: mustprogress norecurse uwtable
define dso_local i32 @main() #3 !dbg !25 {
entry:
  call void @_Z2f1v(), !dbg !29
  ret i32 0, !dbg !30
}

attributes #0 = { mustprogress uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
attributes #2 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #3 = { mustprogress norecurse uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

!llvm.dbg.cu = !{!0, !10}
!llvm.ident = !{!12, !12}
!llvm.module.flags = !{!13, !14, !15, !16, !17}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, imports: !2, splitDebugInlining: false, nameTableKind: GNU)
!1 = !DIFile(filename: "a.cpp", directory: "/usr/local/google/home/blaikie/dev/scratch")
!2 = !{!3}
!3 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !4, entity: !8, file: !1, line: 3)
!4 = distinct !DISubprogram(name: "f1", linkageName: "_Z2f1v", scope: !1, file: !1, line: 3, type: !5, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !7)
!5 = !DISubroutineType(types: !6)
!6 = !{null}
!7 = !{}
!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1", scope: !9, file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !7, identifier: "_ZTSN2ns2t1E")
!9 = !DINamespace(name: "ns", scope: null)
!10 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !11, producer: "clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: true, nameTableKind: GNU)
!11 = !DIFile(filename: "b.cpp", directory: "/usr/local/google/home/blaikie/dev/scratch")
!12 = !{!"clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)"}
!13 = !{i32 7, !"Dwarf Version", i32 4}
!14 = !{i32 2, !"Debug Info Version", i32 3}
!15 = !{i32 1, !"wchar_size", i32 4}
!16 = !{i32 7, !"uwtable", i32 1}
!17 = !{i32 7, !"frame-pointer", i32 2}
!18 = distinct !DISubprogram(name: "f2", linkageName: "_Z2f2v", scope: !1, file: !1, line: 4, type: !5, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !7)
!19 = !DILocation(line: 4, column: 13, scope: !18)
!20 = !DILocation(line: 4, column: 19, scope: !18)
!21 = !DILocalVariable(name: "v1", scope: !4, file: !1, line: 3, type: !8)
!22 = !DILocation(line: 3, column: 37, scope: !4)
!23 = !DILocation(line: 3, column: 41, scope: !4)
!24 = !DILocation(line: 3, column: 48, scope: !4)
!25 = distinct !DISubprogram(name: "main", scope: !11, file: !11, line: 2, type: !26, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !10, retainedNodes: !7)
!26 = !DISubroutineType(types: !27)
!27 = !{!28}
!28 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!29 = !DILocation(line: 3, column: 3, scope: !25)
!30 = !DILocation(line: 4, column: 1, scope: !25)

I'll look into reverting this (& dependent patches) after lunch to unblock the main branch - but happy to work with you/continue to work on this to figure out how to address the issue. It may be a latent bug of my own creation regarding testing the splitDwarfInilning property on the wrong CU or something.

oh, and in another example I found that this patch caused LLVM to produce two dwo CUs, which is incorrect (but unlike the other example, doesn't crash). (this is the same as the first example, but with splitDedugInlining enabled on both CUs)

; ModuleID = 'ab.bc'
source_filename = "llvm-link"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%"struct.ns::t1" = type { i8 }

; Function Attrs: mustprogress uwtable
define available_externally dso_local void @_Z2f1v() #0 !dbg !4 {
entry:
  %v1 = alloca %"struct.ns::t1", align 1
  call void @llvm.dbg.declare(metadata %"struct.ns::t1"* %v1, metadata !21, metadata !DIExpression()), !dbg !22
  call void @_Z3pinv(), !dbg !23
  ret void, !dbg !24
}

; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
declare void @llvm.dbg.declare(metadata, metadata, metadata) #1

declare dso_local void @_Z3pinv() #2

; Function Attrs: mustprogress norecurse uwtable
define dso_local i32 @main() #3 !dbg !25 {
entry:
  call void @_Z2f1v(), !dbg !29
  ret i32 0, !dbg !30
}

attributes #0 = { mustprogress uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { nofree nosync nounwind readnone speculatable willreturn }
attributes #2 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #3 = { mustprogress norecurse uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }

!llvm.dbg.cu = !{!0, !10}
!llvm.ident = !{!12, !12}
!llvm.module.flags = !{!13, !14, !15, !16, !17}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, imports: !2, splitDebugInlining: true, nameTableKind: GNU)
!1 = !DIFile(filename: "a.cpp", directory: "/usr/local/google/home/blaikie/dev/scratch")
!2 = !{!3}
!3 = !DIImportedEntity(tag: DW_TAG_imported_declaration, scope: !4, entity: !8, file: !1, line: 3)
!4 = distinct !DISubprogram(name: "f1", linkageName: "_Z2f1v", scope: !1, file: !1, line: 3, type: !5, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !7)
!5 = !DISubroutineType(types: !6)
!6 = !{null}
!7 = !{}
!8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1", scope: !9, file: !1, line: 1, size: 8, flags: DIFlagTypePassByValue, elements: !7, identifier: "_ZTSN2ns2t1E")
!9 = !DINamespace(name: "ns", scope: null)
!10 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !11, producer: "clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: true, nameTableKind: GNU)
!11 = !DIFile(filename: "b.cpp", directory: "/usr/local/google/home/blaikie/dev/scratch")
!12 = !{!"clang version 14.0.0 (git@github.com:llvm/llvm-project.git 75b622a7959479ccdb758ebe0973061b7b4fcacd)"}
!13 = !{i32 7, !"Dwarf Version", i32 4}
!14 = !{i32 2, !"Debug Info Version", i32 3}
!15 = !{i32 1, !"wchar_size", i32 4}
!16 = !{i32 7, !"uwtable", i32 1}
!17 = !{i32 7, !"frame-pointer", i32 2}
!18 = distinct !DISubprogram(name: "f2", linkageName: "_Z2f2v", scope: !1, file: !1, line: 4, type: !5, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !7)
!19 = !DILocation(line: 4, column: 13, scope: !18)
!20 = !DILocation(line: 4, column: 19, scope: !18)
!21 = !DILocalVariable(name: "v1", scope: !4, file: !1, line: 3, type: !8)
!22 = !DILocation(line: 3, column: 37, scope: !4)
!23 = !DILocation(line: 3, column: 41, scope: !4)
!24 = !DILocation(line: 3, column: 48, scope: !4)
!25 = distinct !DISubprogram(name: "main", scope: !11, file: !11, line: 2, type: !26, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !10, retainedNodes: !7)
!26 = !DISubroutineType(types: !27)
!27 = !{!28}
!28 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!29 = !DILocation(line: 3, column: 3, scope: !25)
!30 = !DILocation(line: 4, column: 1, scope: !25)
krisb added a comment.Dec 22 2021, 2:03 PM

@dblaikie, thank you for the reproducers!

I've tried to compile&examine them, but haven't found any issues. Likely, I did something wrong.
Could you, please, give more details on how to reproduce the issue(s)? I mean, exact compilation command line and/or crash details / wrong dwarf output? Thank you!

@dblaikie, thank you for the reproducers!

I've tried to compile&examine them, but haven't found any issues. Likely, I did something wrong.
Could you, please, give more details on how to reproduce the issue(s)? I mean, exact compilation command line and/or crash details / wrong dwarf output? Thank you!

Oh, right, sorry:

For the first one, you should observe a crash (in a +Asserts build) when running this command:

$ clang++-tot ab-crash.ll -gsplit-dwarf -c -O1

The backtrace looks something like this:

$ clang++-tot ab-crash.ll -gsplit-dwarf -c -O1 -S 
clang++-tot: /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:660: llvm::DIE *llvm::DwarfCompileUnit::constructInlinedScopeDIE(llvm::LexicalScope *): Assertion `OriginDIE && "Unable to find original DIE for an inlined subprogram."' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
0.      Program arguments: clang++-tot ab-crash.ll -gsplit-dwarf -c -O1 -S
1.      Code generation
2.      Running pass 'Function Pass Manager' on module 'ab-crash.ll'.
3.      Running pass 'X86 Assembly Printer' on function '@main'
 #0 0x0000000009e604ea llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:565:11
 #1 0x0000000009e6069b PrintStackTraceSignalHandler(void*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:632:1
 #2 0x0000000009e5ed63 llvm::sys::RunSignalHandlers() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Signals.cpp:97:5
 #3 0x0000000009e5fdfe llvm::sys::CleanupOnSignal(unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/Unix/Signals.inc:362:1
 #4 0x0000000009d6fa4e (anonymous namespace)::CrashRecoveryContextImpl::HandleCrash(int, unsigned long) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:0:7
 #5 0x0000000009d6fe03 CrashRecoverySignalHandler(int) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Support/CrashRecoveryContext.cpp:390:1
 #6 0x00007f60568c08e0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x138e0)
 #7 0x00007f6056389e71 raise ./signal/../sysdeps/unix/sysv/linux/raise.c:50:1
 #8 0x00007f6056373536 abort ./stdlib/abort.c:81:7
 #9 0x00007f605637341f get_sysdep_segment_value ./intl/loadmsgcat.c:509:8
#10 0x00007f605637341f _nl_load_domain ./intl/loadmsgcat.c:970:34
#11 0x00007f60563827f2 (/lib/x86_64-linux-gnu/libc.so.6+0x357f2)
#12 0x000000000b57ce6e llvm::DwarfCompileUnit::constructInlinedScopeDIE(llvm::LexicalScope*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:0:3
#13 0x000000000b57cb6c llvm::DwarfCompileUnit::constructScopeDIE(llvm::LexicalScope*, llvm::DIE&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:538:10
#14 0x000000000b57d3ae llvm::DwarfCompileUnit::createAndAddScopeChildren(llvm::LexicalScope*, llvm::DIE&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1072:7
#15 0x000000000b57efa1 llvm::DwarfCompileUnit::constructSubprogramScopeDIE(llvm::DISubprogram const*, llvm::LexicalScope*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:1023:14
#16 0x000000000b52bc0d llvm::DwarfDebug::endFunctionImpl(llvm::MachineFunction const*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2301:50
#17 0x000000000b505ab2 llvm::DebugHandlerBase::endFunction(llvm::MachineFunction const*) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp:0:5
#18 0x000000000b4e1f4c llvm::AsmPrinter::emitFunctionBody() /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1505:3
#19 0x0000000007ed7113 llvm::X86AsmPrinter::runOnMachineFunction(llvm::MachineFunction&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/Target/X86/X86AsmPrinter.cpp:82:3
#20 0x0000000008a96477 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:8
#21 0x0000000009165d5e llvm::FPPassManager::runOnFunction(llvm::Function&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1439:23
#22 0x000000000916abb2 llvm::FPPassManager::runOnModule(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1485:16
#23 0x0000000009166649 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1554:23
#24 0x00000000091661bd llvm::legacy::PassManagerImpl::run(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:542:16
#25 0x000000000916ae91 llvm::legacy::PassManager::run(llvm::Module&) /usr/local/google/home/blaikie/dev/llvm/src/llvm/lib/IR/LegacyPassManager.cpp:1681:3

But that's actually not the original situation I was investigating, so maybe it's another bug? (possibly same root cause, though)

Though the original issue I was investigating was from invalid .debug_gnu_pubnames (DIE offsets of 0, which prematurely terminates the .debug_gnu_pubnames list and produces a warning from llvm-dwarfdump), and by adding assert(Offset) in llvm::DIE::getOffset (initially I added it in the pubnames emission code, but generally it should be true that the offset of a DIE should not be queried until it's been computed, and no DIE has offset zero, because the offset is from the start of the CU contribution - so the headers come first and mean the DIE offset is always non-zero). It seems I still don't have a small reproduction for that exact issue, though.

The second test case provided above, that produces multiple DWO CUs can be reproduced as follows:

$ clang++-tot ab-multi-cu.ll -gsplit-dwarf -c -O1 && llvm-dwarfdump-tot ab-multi-cu.o
ab-multi-cu.o:  file format elf64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000053, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000057)

0x0000000b: DW_TAG_compile_unit
              DW_AT_stmt_list   (0x00000000)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_GNU_pubnames        (true)
              DW_AT_GNU_dwo_name        ("ab-multi-cu.dwo")
              DW_AT_GNU_dwo_id  (0xe9d1660e0395035d)
              DW_AT_low_pc      (0x0000000000000000)
              DW_AT_high_pc     (0x000000000000000d)
              DW_AT_GNU_addr_base       (0x00000000)

0x00000030:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000000000000)
                DW_AT_high_pc   (0x000000000000000d)
                DW_AT_name      ("main")

0x00000041:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin (0x000000000000007b "f1")
                  DW_AT_low_pc  (0x0000000000000004)
                  DW_AT_high_pc (0x0000000000000009)
                  DW_AT_call_file       ("/usr/local/google/home/blaikie/dev/scratch/b.cpp")
                  DW_AT_call_line       (3)
                  DW_AT_call_column     (0x03)

0x00000055:     NULL

0x00000056:   NULL
0x00000057: Compile Unit: length = 0x00000027, format = DWARF32, version = 0x0004, abbr_offset = 0x0000, addr_size = 0x08 (next unit at 0x00000082)

0x00000062: DW_TAG_compile_unit
              DW_AT_stmt_list   (0x0000004c)
              DW_AT_comp_dir    ("/usr/local/google/home/blaikie/dev/scratch")
              DW_AT_GNU_pubnames        (true)
              DW_AT_GNU_dwo_name        ("ab-multi-cu.dwo")
              DW_AT_GNU_dwo_id  (0xeb227ecffcd242c1)
              DW_AT_GNU_addr_base       (0x00000000)

0x0000007b:   DW_TAG_subprogram
                DW_AT_name      ("f1")
                DW_AT_inline    (DW_INL_inlined)

0x00000081:   NULL

In this case the second CU is OK (for the cross-CU inlining that's occurred), but it shouldn't have any DWO_* attributes, and the .dwo file shouldn't have a second CU in it (which it does after this patch) because Split DWARF doesn't have a good way to describe cross-CU references correctly once the CU is packaged in a dwp file. (this behavior is meant to be controlled by the SplitDwarfCrossCUReferences flag.

krisb added a comment.EditedDec 23 2021, 10:30 AM

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

About the issues.
First of all, thank you for all the details! I was able to reproduce both issues.
(1) The crash doesn't seem related to the patch and reverting it doesn't make things better. It seems there is an inconsistency between when/where an abstract origin gets emitted and when/where it's requested.
(2) The issue with wrong dwo attributes is indeed caused by the imported declaration that gets emitted where it shouldn't (at least comparing with the behavior before this patch).
Before this patch 'includeMinimalInlineScopes()' makes emission of imported entity skipped and the second CU assumed empty, so we did not emit dwo attributes for it.
But I'm not sure I fully understand how DwarfDebug handles split dwarf. Why do we need the 'includeMinimalInlineScopes()' check for correctness here?

krisb reopened this revision.Dec 23 2021, 10:42 AM
This revision is now accepted and ready to land.Dec 23 2021, 10:42 AM

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

About the issues.
First of all, thank you for all the details! I was able to reproduce both issues.
(1) The crash doesn't seem related to the patch and reverting it doesn't make things better. It seems there is an inconsistency between when/where an abstract origin gets emitted and when/where it's requested.

Hmm, bother - guess I failed to correctly reduce the issue. I'll try again.

(2) The issue with wrong dwo attributes is indeed caused by the imported declaration that gets emitted where it shouldn't (at least comparing with the behavior before this patch).
Before this patch 'includeMinimalInlineScopes()' makes emission of imported entity skipped and the second CU assumed empty, so we did not emit dwo attributes for it.
But I'm not sure I fully understand how DwarfDebug handles split dwarf. Why do we need the 'includeMinimalInlineScopes()' check for correctness here?

It gets a bit complicated because a DWO file can only contain a single CU, but the .o file can contain multiple CUs - so to preserve as much fidelity as possible, if "split dwarf inlining" is enabled (include inline stack frames in the CU in the .o file, so the program can be symbolized even without the dwo/dwp files) then the .o file will get multiple CUs with -g1-like debug info, and one of those CUs will also have the dwo attributes and be a skeleton for the .dwo CU (but the dwo file, instead of having multiple CUs to describe the cross-CU inlining, it'll degrade the debug info quality by moving the DIEs that should be in other CUs in the dwo into the single CU there).

Turned out we had two different patches causing the same assert (should've bisected them both to be sure - added some unnecessary confusion to this whole situation). One of my reduced test cases actually was for https://reviews.llvm.org/D115325#3209087 - working on another for this patch (D113741) now...

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

int main() {      // <====== function scope
  int x = 3;
  int num = 6;
  printf("%d\n", x);
  if (num) {   // <======= lexical block 1
    static int x = 45;
    printf("%d\n", x);
    if (x) {   // <======= lexical block 2
      int x = 145;
      printf("%d\n", x);
    }
  }
  return 0;
}

Without this patch:

<1><2a>: Abbrev Number: 2 (DW_TAG_subprogram)
   <2b>   DW_AT_low_pc      : 0x10010680
   <33>   DW_AT_high_pc     : 0xd4
   <37>   DW_AT_frame_base  : 1 byte block: 6f         (DW_OP_reg31 (r31))
   <39>   DW_AT_name        : (indirect string, offset: 0xcb): main
   <3f>   DW_AT_type        : <0x94>
   <43>   DW_AT_external    : 1
<2><43>: Abbrev Number: 3 (DW_TAG_variable)
   <44>   DW_AT_location    : 3 byte block: 91 f0 0    (DW_OP_fbreg: 112)
   <48>   DW_AT_name        : (indirect string, offset: 0xd4): x
   <4e>   DW_AT_type        : <0x94>
<2><52>: Abbrev Number: 3 (DW_TAG_variable)
   <53>   DW_AT_location    : 3 byte block: 91 ec 0    (DW_OP_fbreg: 108)
   <57>   DW_AT_name        : (indirect string, offset: 0xd0): num
   <5d>   DW_AT_type        : <0x94>
<2><61>: Abbrev Number: 4 (DW_TAG_lexical_block)     <<<<<<<<<<this should be for lexical block 2, so its DIE level should be 3
   <62>   DW_AT_low_pc      : 0x1001070c
   <6a>   DW_AT_high_pc     : 0x20
<3><6e>: Abbrev Number: 3 (DW_TAG_variable)             <<<<<<<<<this is for x=145 and nested in lexical block2, so its DIE level should be 4
   <6f>   DW_AT_location    : 3 byte block: 91 e8 0    (DW_OP_fbreg: 104)
   <73>   DW_AT_name        : (indirect string, offset: 0xd4): x
   <79>   DW_AT_type        : <0x94>
<3><7d>: Abbrev Number: 0
<2><7e>: Abbrev Number: 5 (DW_TAG_variable)              <<<<<<<<<<this is for static int x = 45, so there should be another lexical block DIE for lexical block 1, and its DIE level should be 2.
   <7f>   DW_AT_name        : (indirect string, offset: 0xd4): x
   <83>   DW_AT_type        : <0x94>
   <89>   DW_AT_location    : 9 byte block: 3 e8 a 3 10 0 0 0 0        (DW_OP_addr: 10030ae8)
<2><93>: Abbrev Number: 0
<1><94>: Abbrev Number: 6 (DW_TAG_base_type)
   <95>   DW_AT_name        : (indirect string, offset: 0x0): int
   <99>   DW_AT_encoding    : 5        (signed)
   <9a>   DW_AT_byte_size   : 4
<1><9b>: Abbrev Number: 0

And we get wrong result for static number x at line 7. In debugger, it is printed as 3.

With this patch:

0x0000002a:   DW_TAG_subprogram
                DW_AT_low_pc    (0x0000000010010680)
                DW_AT_high_pc   (0x0000000010010754)
                DW_AT_frame_base        (DW_OP_reg31 X31)
                DW_AT_name      ("main")
                DW_AT_type      (0x00000094 "int")
                DW_AT_external  (true)

0x00000043:     DW_TAG_variable
                  DW_AT_name    ("x")          <<<<<<<<<<<<we need a lexical block for this definition
                  DW_AT_type    (0x00000094 "int")
                  DW_AT_location        (DW_OP_addr 0x10030ae8)

0x00000058:     DW_TAG_variable
                  DW_AT_location        (DW_OP_fbreg +112)
                  DW_AT_name    ("x")
                  DW_AT_type    (0x00000094 "int")

0x00000067:     DW_TAG_variable
                  DW_AT_location        (DW_OP_fbreg +108)
                  DW_AT_name    ("num")
                  DW_AT_type    (0x00000094 "int")

0x00000076:     DW_TAG_lexical_block
                  DW_AT_low_pc  (0x000000001001070c)
                  DW_AT_high_pc (0x000000001001072c)

0x00000083:       DW_TAG_variable
                    DW_AT_location      (DW_OP_fbreg +104)
                    DW_AT_name  ("x")
                    DW_AT_type  (0x00000094 "int")

0x00000092:       NULL

Now we get wrong result for local variable x at line 3. In debugger, it is printed as 45. (The DIE for the local variable x is overridden by the static variable x in lexical block 1).

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?

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

krisb added a comment.Tue, Dec 28, 4:13 PM

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

This is the functional change itself, reverting patch is at https://reviews.llvm.org/D116225.

Without this patch:

I guess you meant without the revert, and actually _with_ this patch, right?
If so, you'd need a clang side change https://reviews.llvm.org/D113743 (which has been reverted as well) to see static locals placed correctly.
If to apply all the reverted patches (D114705, D113741, and D113743) I believe you should see a correct DWARF compiled for your example.

If to apply all the reverted patches (D114705, D113741, and D113743) I believe you should see a correct DWARF compiled for your example.

Yeah, verify with manually applying all the above patches, it works as expected. Looking forward to seeing these patches merged in again.