Page MenuHomePhabricator

[RFC][clang][DebugInfo] Allow function-local statics and types to be scoped within a lexical block
AcceptedPublic

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

Details

Summary

This is almost a reincarnation of https://reviews.llvm.org/D15977 originally
implemented by Amjad Aboud. It was discussed on llvm-dev [0], committed
with its backend counterpart [1], but finally reverted [2].

This patch makes clang to emit debug info for function-local static variables,
records (classes, structs and unions) and typdefs correctly scoped if
those function-local entites defined within a lexical (bracketed) block.

Before this patch, clangs emits all those entities directly scoped in
DISubprogram no matter where they were really defined, causing
debug info loss (reported several times in [3], [4], [5]).

Backend support is at D113741.

[0] https://lists.llvm.org/pipermail/llvm-dev/2015-November/092551.html
[1] https://reviews.llvm.org/rG30e7a8f694a19553f64b3a3a5de81ce317b9ec2f
[2] https://reviews.llvm.org/rGdc4531e552af6c880a69d226d3666756198fbdc8
[3] https://bugs.llvm.org/show_bug.cgi?id=19238
[4] https://bugs.llvm.org/show_bug.cgi?id=23164
[5] https://bugs.llvm.org/show_bug.cgi?id=44695

Diff Detail

Event Timeline

krisb requested review of this revision.Nov 12 2021, 1:23 AM
krisb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2021, 1:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This looks reasonable to me. Thanks!

clang/lib/CodeGen/CGDebugInfo.cpp
234

maybe use LexicalBlockMap.insert({&D, LexicalBlockStack.back()}); to make it clear that we are not going to overwrite an entry?

krisb updated this revision to Diff 387956.Nov 17 2021, 8:41 AM
krisb marked an inline comment as done.

Applied the comment & silence clang-format.

krisb added a comment.Nov 17 2021, 8:42 AM

@aprantl thank you for taking a look at this!

dblaikie accepted this revision.Dec 2 2021, 10:34 AM

Guessing Adrian probably meant to sign off on this, judging by his phrasing but maybe forgot to hit the "accept" button.

This revision is now accepted and ready to land.Dec 2 2021, 10:34 AM

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.

krisb added a comment.Dec 6 2021, 9:59 AM

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.

Thank you for taking care of this!
I'm looking at the issue, but it's been taking more time than I expected.

This doesn't seem like a flaw of the patch, but likely is a lack of support of records/typedefs scoped within a bracketed block from lldb side.
I see lldb couldn't handle cases like

int foo(int a) {
  {
    typedef int Int;
    Int local = a;
    return local;
  }
}

which produces the same error as for TestSetData.py:

Process 2487354 stopped
* thread #1, name = 'a.out', stop reason = step over
    frame #0: 0x000000000040111d a.out`foo(a=1) at test_lldb.cpp:5:12
   2   	  {
   3   	    typedef int Int;
   4   	    Int local = a;
-> 5   	    return local;
   6   	  }
   7   	}
   8   	
(lldb) p local
error: expression failed to parse:
error: <lldb wrapper prefix>:45:31: no member named 'local' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::local;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'local'
local
^

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.

Thank you for taking care of this!
I'm looking at the issue, but it's been taking more time than I expected.

This doesn't seem like a flaw of the patch, but likely is a lack of support of records/typedefs scoped within a bracketed block from lldb side.
I see lldb couldn't handle cases like

int foo(int a) {
  {
    typedef int Int;
    Int local = a;
    return local;
  }
}

which produces the same error as for TestSetData.py:

Process 2487354 stopped
* thread #1, name = 'a.out', stop reason = step over
    frame #0: 0x000000000040111d a.out`foo(a=1) at test_lldb.cpp:5:12
   2   	  {
   3   	    typedef int Int;
   4   	    Int local = a;
-> 5   	    return local;
   6   	  }
   7   	}
   8   	
(lldb) p local
error: expression failed to parse:
error: <lldb wrapper prefix>:45:31: no member named 'local' in namespace '$__lldb_local_vars'
    using $__lldb_local_vars::local;
          ~~~~~~~~~~~~~~~~~~~~^
error: <user expression 0>:1:1: use of undeclared identifier 'local'
local
^

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

krisb added a comment.Dec 7 2021, 12:54 PM

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.

krisb reopened this revision.Dec 7 2021, 1:28 PM
This revision is now accepted and ready to land.Dec 7 2021, 1:28 PM