This is an archive of the discontinued LLVM Phabricator instance.

Fix for DWARF parsing to better handle auto return type for member functions
Needs ReviewPublic

Authored by shafik on Jul 7 2021, 10:41 AM.

Details

Summary

Currently when we have a member function that has an auto return type and the definition is out of line we generate two DWARF DIE.
One for the declaration and a second one for the definition. We don't currently lookup the definition in order to replace the auto return type with a real one.

This modifies DWARFASTParserClang to detect we have a function with deduced return type and lookup the
definition to obtain the correct return type and adjust the FunctionDecl to reflect this.

Diff Detail

Event Timeline

shafik created this revision.Jul 7 2021, 10:41 AM
shafik requested review of this revision.Jul 7 2021, 10:41 AM

I think it would be best to split out the Clang change into a separately tested patch.

clang/lib/CodeGen/CGDebugInfo.cpp
1694 ↗(On Diff #356843)

typo: definition (2x)

I think it would be best to split out the Clang change into a separately tested patch.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2783

This block looks like it's more complicated than it needs to be. Could you just say

if (other_die != die)
      if (other_die.Tag()) != DW_TAG_subprogram)
       return false;

or am I missing something?

aprantl added inline comments.Jul 7 2021, 10:53 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2783

Definitely missed something :-)

if (other_die == die || (other_die.Tag()) != DW_TAG_subprogram))
    return false;

Could LLDB find the linkage name on the declaration, look that name up in the symbol table, and find the DW_TAG_subprogram DIE for the symbol's start address?

shafik updated this revision to Diff 357578.Jul 9 2021, 11:12 AM
shafik added reviewers: jingham, jasonmolenda.

Changing approach based on Adrian's comments.

@aprantl after your comments and discussion offline I changed my approach to do this lookup using the symbol table and it worked out.

The main issue with the first approach was that gcc would also have to be updated in order for them to change their approach to generating debug info as well and that felt a lot harder to justify.

This is also a simpler approach, it requires fewer changes.

Currently when we have a member function that has an auto return type and the definition is out of line we generate two DWARF DIE.
One for the declaration and a second one for the definition, the definition holds the deduced type but does not contain a DW_AT_name nor
a DW_AT_linkage_name so there was not way to look up the definition DIE.

Regarding the DWARF, the definition DIE should have a DW_AT_specification that points back to the declaration; is that missing here?
I'm not familiar with LLDB so it's likely I'm misunderstanding the problem.

shafik edited the summary of this revision. (Show Details)Jul 9 2021, 11:20 AM

Currently when we have a member function that has an auto return type and the definition is out of line we generate two DWARF DIE.
One for the declaration and a second one for the definition, the definition holds the deduced type but does not contain a DW_AT_name nor
a DW_AT_linkage_name so there was not way to look up the definition DIE.

Regarding the DWARF, the definition DIE should have a DW_AT_specification that points back to the declaration; is that missing here?
I'm not familiar with LLDB so it's likely I'm misunderstanding the problem.

Apologies, I should have updated the description as well, portions of that description are out of date with the new approach I just updated to.

teemperor requested changes to this revision.Jul 12 2021, 11:23 AM

I think this looks good, thanks for fixing this! I believe there should be a few more tests for all the corner cases we can run into here (those tests can all be just Python API tests, no need for having them all in assembly format). Also I think lambdas where triggering the original issue, so that would also be nice to have in a test.

I started writing some example tests but this is crashing for me with:

Unexpected undeduced type!
UNREACHABLE executed at /home/teemperor/work/ci/llvm-project/clang/lib/CodeGen/CodeGenTypes.cpp:624!
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
Stack dump:
0.	Program arguments: ./bin/lldb ././lldb-test-build.noindex/lang/cpp/auto_return/TestCppAutoReturn.test_dwarf/a.out
 #0 0x000055f540b5e611 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (./bin/lldb+0x34611)
 #1 0x000055f540b5c6b0 llvm::sys::RunSignalHandlers() (./bin/lldb+0x326b0)
 #2 0x000055f540b5efb6 SignalHandler(int) (./bin/lldb+0x34fb6)
 #3 0x00007fe3c3484870 __restore_rt (/usr/lib/libpthread.so.0+0x13870)
 #4 0x00007fe3bd2e3d22 raise (/usr/lib/libc.so.6+0x3cd22)
 #5 0x00007fe3bd2cd862 abort (/usr/lib/libc.so.6+0x26862)
 #6 0x00007fe3becf0df1 (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x146adf1)
 #7 0x00007fe3bf7cbdda clang::CodeGen::CodeGenTypes::ConvertType(clang::QualType) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1f45dda)
 #8 0x00007fe3bf7caf7e clang::CodeGen::CodeGenTypes::ConvertTypeForMem(clang::QualType, bool) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1f44f7e)
 #9 0x00007fe3bf951dd8 clang::CodeGen::CodeGenModule::getOrCreateStaticVarDecl(clang::VarDecl const&, llvm::GlobalValue::LinkageTypes) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20cbdd8)
#10 0x00007fe3bf950b10 clang::CodeGen::CodeGenFunction::EmitStaticVarDecl(clang::VarDecl const&, llvm::GlobalValue::LinkageTypes) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20cab10)
#11 0x00007fe3bf9508f3 clang::CodeGen::CodeGenFunction::EmitVarDecl(clang::VarDecl const&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20ca8f3)
#12 0x00007fe3bf950458 clang::CodeGen::CodeGenFunction::EmitDecl(clang::Decl const&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x20ca458)
#13 0x00007fe3bf6f887c clang::CodeGen::CodeGenFunction::EmitDeclStmt(clang::DeclStmt const&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e7287c)
#14 0x00007fe3bf6ee2d8 clang::CodeGen::CodeGenFunction::EmitSimpleStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e682d8)
#15 0x00007fe3bf6eda89 clang::CodeGen::CodeGenFunction::EmitStmt(clang::Stmt const*, llvm::ArrayRef<clang::Attr const*>) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e67a89)
#16 0x00007fe3bf6f96b0 clang::CodeGen::CodeGenFunction::EmitCompoundStmtWithoutScope(clang::CompoundStmt const&, bool, clang::CodeGen::AggValueSlot) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e736b0)
#17 0x00007fe3bf7585b4 clang::CodeGen::CodeGenFunction::EmitFunctionBody(clang::Stmt const*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ed25b4)
#18 0x00007fe3bf7591de clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, llvm::Function*, clang::CodeGen::CGFunctionInfo const&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ed31de)
#19 0x00007fe3bf77a51f clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef451f)
#20 0x00007fe3bf77222d clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1eec22d)
#21 0x00007fe3bf7773bb clang::CodeGen::CodeGenModule::EmitGlobal(clang::GlobalDecl) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef13bb)
#22 0x00007fe3bf77e039 clang::CodeGen::CodeGenModule::EmitTopLevelDecl(clang::Decl*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1ef8039)
#23 0x00007fe3bf6ab280 (anonymous namespace)::CodeGeneratorImpl::HandleTopLevelDecl(clang::DeclGroupRef) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1e25280)
#24 0x00007fe3bf549632 lldb_private::ASTResultSynthesizer::HandleTopLevelDecl(clang::DeclGroupRef) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1cc3632)
#25 0x00007fe3c018a558 clang::ParseAST(clang::Sema&, bool, bool) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x2904558)
#26 0x00007fe3bf55d493 lldb_private::ClangExpressionParser::ParseInternal(lldb_private::DiagnosticManager&, clang::CodeCompleteConsumer*, unsigned int, unsigned int) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1cd7493)
#27 0x00007fe3bf540c11 lldb_private::ClangUserExpression::TryParse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContextScope*, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1cbac11)
#28 0x00007fe3bf541346 lldb_private::ClangUserExpression::Parse(lldb_private::DiagnosticManager&, lldb_private::ExecutionContext&, lldb_private::ExecutionPolicy, bool, bool) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1cbb346)
#29 0x00007fe3be8013d6 lldb_private::UserExpression::Evaluate(lldb_private::ExecutionContext&, lldb_private::EvaluateExpressionOptions const&, llvm::StringRef, llvm::StringRef, std::shared_ptr<lldb_private::ValueObject>&, lldb_private::Status&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, lldb_private::ValueObject*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xf7b3d6)
#30 0x00007fe3be927184 lldb_private::Target::EvaluateExpression(llvm::StringRef, lldb_private::ExecutionContextScope*, std::shared_ptr<lldb_private::ValueObject>&, lldb_private::EvaluateExpressionOptions const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >*, lldb_private::ValueObject*) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x10a1184)
#31 0x00007fe3bf473723 lldb_private::CommandObjectExpression::EvaluateExpression(llvm::StringRef, lldb_private::Stream&, lldb_private::Stream&, lldb_private::CommandReturnObject&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1bed723)
#32 0x00007fe3bf4746ae lldb_private::CommandObjectExpression::DoExecute(llvm::StringRef, lldb_private::CommandReturnObject&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0x1bee6ae)
#33 0x00007fe3be848635 lldb_private::CommandObjectRaw::Execute(char const*, lldb_private::CommandReturnObject&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xfc2635)
#34 0x00007fe3be83d0a8 lldb_private::CommandInterpreter::HandleCommand(char const*, lldb_private::LazyBool, lldb_private::CommandReturnObject&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xfb70a8)
#35 0x00007fe3be840f58 lldb_private::CommandInterpreter::IOHandlerInputComplete(lldb_private::IOHandler&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xfbaf58)
#36 0x00007fe3be781f18 lldb_private::IOHandlerEditline::Run() (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xefbf18)
#37 0x00007fe3be762cc4 lldb_private::Debugger::RunIOHandlers() (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xedccc4)
#38 0x00007fe3be842c19 lldb_private::CommandInterpreter::RunCommandInterpreter(lldb_private::CommandInterpreterRunOptions&) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xfbcc19)
#39 0x00007fe3be2a16bd lldb::SBDebugger::RunCommandInterpreter(bool, bool) (/home/teemperor/work/ci/build/bin/../lib/liblldb.so.13git+0xa1b6bd)
#40 0x000055f540b39d0c Driver::MainLoop() (./bin/lldb+0xfd0c)

This is the diff for the test:

diff --git a/lldb/test/API/lang/cpp/auto_return/Makefile b/lldb/test/API/lang/cpp/auto_return/Makefile
new file mode 100644
index 000000000000..a43ca68df298
--- /dev/null
+++ b/lldb/test/API/lang/cpp/auto_return/Makefile
@@ -0,0 +1,4 @@
+CXX_SOURCES := main.cpp other.cpp
+CXXFLAGS_EXTRAS := -std=c++17
+
+include Makefile.rules
diff --git a/lldb/test/API/lang/cpp/auto_return/TestCppAutoReturn.py b/lldb/test/API/lang/cpp/auto_return/TestCppAutoReturn.py
new file mode 100644
index 000000000000..49ff5a9460dd
--- /dev/null
+++ b/lldb/test/API/lang/cpp/auto_return/TestCppAutoReturn.py
@@ -0,0 +1,18 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+class TestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    def test(self):
+        self.build()
+        lldbutil.run_to_source_breakpoint(self, "// break here", lldb.SBFileSpec("main.cpp"))
+
+        # Member access
+        self.expect_expr("C.ret()", result_type="int", result_value="1")
+
+        lldbutil.run_to_source_breakpoint(self, "// break also here", lldb.SBFileSpec("other.cpp"))
+        self.expect_expr("b.ret()", result_type="int", result_value="1")
diff --git a/lldb/test/API/lang/cpp/auto_return/main.cpp b/lldb/test/API/lang/cpp/auto_return/main.cpp
new file mode 100644
index 000000000000..3629c7b0c8f4
--- /dev/null
+++ b/lldb/test/API/lang/cpp/auto_return/main.cpp
@@ -0,0 +1,18 @@
+void other();
+
+namespace {
+struct Bla {
+  auto ret();
+};
+
+auto Bla::ret() {
+  auto x = [](int bla){ return bla; };
+  return x(1);
+}
+}
+
+int main() {
+  Bla C;
+  other();
+  return C.ret(); // break here
+}
diff --git a/lldb/test/API/lang/cpp/auto_return/other.cpp b/lldb/test/API/lang/cpp/auto_return/other.cpp
new file mode 100644
index 000000000000..315049e16ff0
--- /dev/null
+++ b/lldb/test/API/lang/cpp/auto_return/other.cpp
@@ -0,0 +1,15 @@
+namespace {
+struct Bla {
+  auto ret();
+};
+
+auto Bla::ret() {
+  auto x = [](int bla){ return bla; };
+  return x(2);
+}
+}
+
+void other() {
+  Bla b;
+  b.ret(); // // break also here
+}
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
435

If this is virtual then I guess SymbolFileDWARFDwo should overload it?

This revision now requires changes to proceed.Jul 12 2021, 11:23 AM
clayborg requested changes to this revision.Jul 12 2021, 1:33 PM
clayborg added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
1359

s/defintion/definition/

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2767

type

2779

check dcu to ensure it isn't NULL

shafik updated this revision to Diff 358373.Jul 13 2021, 11:41 AM
  • Modified FindTypeForAutoReturnForDIE to take into account if we have multiple symbols with the same name.
  • Modified ParseSubroutine to take into account that case we get the definition first, this can happen when we set a breakpoint inside the definition.
  • Added new tests suggested by Raphael.
shafik marked 4 inline comments as done.Jul 13 2021, 11:42 AM
shafik added inline comments.Jul 13 2021, 12:15 PM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
435

I am actually not sure if this has to be virtual or not, let me dig into this and see if there would be any difference for SymbolFileDWARFDwo or not.

teemperor requested changes to this revision.Jul 13 2021, 2:54 PM

I guess Phabricator just sent this back to review automatically when you updated the diff? Just mark this as 'needs review' again when this is ready for review. I'll send this back in the meantime to clear my review queue.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
897

unused variable

946

unrelated change?

This revision now requires changes to proceed.Jul 13 2021, 2:54 PM
JDevlieghere added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
403–405

What's the purpose of this? Do we expect to see the type attribute more than once?

898–899

You're doing dyn_cast_or_null but then below you're dereferencing ts unconditionally?

1363–1365

LLVM likes to put these variables in the if-clause, which I personally really like because it conveys the scope without hurting readability.

1367–1370
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2762–2765

I know this pattern is common in LLDB, but I really dislike it because you have to backtrack all the way to the beginning of the function to know if type_sp has been modified in any way. When I write code like, this I tend to use return {}; to make it clear I'm returning a default constructed instance of the return type. That also makes it clear where we're actually returning a non-default instance by just looking at the returns.

2808
shafik updated this revision to Diff 358773.Jul 14 2021, 4:00 PM
shafik marked 8 inline comments as done.
  • Removed virtual from FindTypeForAutoReturnForDIE
  • Added missing nullptr checks
  • Modernized the code
shafik added inline comments.Jul 14 2021, 4:51 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
403–405

Good question, the first iteration was done by @teemperor and then I modified it heavily but I did not dig into this change to deeply but I was pretty sure when we first talked about there was a good reason for it.

1363–1365

Good catch, I was changing this code around a lot and forgot to go back and clean this up.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2762–2765

Good catch, I originally based this on FindCompleteObjCDefinitionTypeForDIE and stuck with the idiom w/o thinking about it.

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
435

Since this is based on the same approach in GetObjCClassSymbol and that is not virtual I am going to drop it.

teemperor requested changes to this revision.Jul 15 2021, 2:37 AM
This comment was removed by teemperor.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
403–405

ParsedDWARFTypeAttributes iterates not just over the attributes in the current DIE, but also follows DW_AT_specification and then iterates the attributes there. But the DW_AT_specification of the definition points to the declaration with the auto type, so without this change we would first assign type to the real type from the definition and then when we iterate over the attributes of the declaration we would overwrite the real type with the auto type.

In short: without this change ParsedDWARFTypeAttributes(definition_die).type would be auto instead of the real type.

This revision now requires changes to proceed.Jul 15 2021, 2:37 AM
aprantl added inline comments.Jul 15 2021, 10:10 AM
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
2783

should there be an early exit if we found other_die?

teemperor resigned from this revision.Jul 16 2021, 11:41 AM
This revision now requires review to proceed.Jul 16 2021, 11:41 AM
shafik updated this revision to Diff 427081.May 4 2022, 11:55 AM
shafik removed reviewers: teemperor, jingham, jasonmolenda.
  • Expanded test
  • applied clang-format
Herald added a project: Restricted Project. · View Herald TranscriptMay 4 2022, 11:55 AM
aprantl accepted this revision.Jun 7 2022, 1:28 PM
aprantl added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
403

Could you add a comment explaining why this is necessary? (see conversation below)