subject:"\[Lldb\-commits\] \[lldb\] \[lldb\]\[DWARF\] Delay struct\/class\/union definition DIE searching when parsing declaration DIEs. \(PR #90663\)"

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-15 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

It actually still crashes at the same place even without this PR using the 
following command, you can try it on trunk:
```
$ rm -rf lldb/test/API/commands/expression/import-std-module/lldb-api/*
$ out/cmake/bin/lldb-dotest 
--lldb-module-cache-dir=out/cmake/lldb-test-build.noindex/module-cache-lldb/lldb-api
 lldb/test/API/commands/expression/import-std-module/
/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9
 /Users/zequanwu/work/llvm/lldb/test/API/dotest.py --arch arm64 --build-dir 
/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex --executable 
/Users/zequanwu/work/llvm/out/cmake/./bin/lldb --compiler 
/Users/zequanwu/work/llvm/out/cmake/./bin/clang --dsymutil 
/Users/zequanwu/work/llvm/out/cmake/./bin/dsymutil --lldb-libs-dir 
/Users/zequanwu/work/llvm/out/cmake/./lib --llvm-tools-dir 
/Users/zequanwu/work/llvm/out/cmake/./bin --libcxx-include-dir 
/Users/zequanwu/work/llvm/out/cmake/include/c++/v1 --libcxx-library-dir 
/Users/zequanwu/work/llvm/out/cmake/lib --lldb-obj-root 
/Users/zequanwu/work/llvm/out/cmake/tools/lldb 
--lldb-module-cache-dir=/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex/module-cache-lldb/lldb-api
 lldb/test/API/commands/expression/import-std-module/
lldb version 19.0.0git (g...@github.com:ZequanWu/llvm-project.git revision 
71fbbb69d63c461f391cbabf1e32cd9977c4ce68)
  clang revision 71fbbb69d63c461f391cbabf1e32cd9977c4ce68
  llvm revision 71fbbb69d63c461f391cbabf1e32cd9977c4ce68
Skipping the following test categories: ['libstdcxx', 'dwo', 'llgs', 'fork']
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dsym 
(TestDbgInfoContentForwardListFromStdModule.TestDbgInfoContentForwardList)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dwarf 
(TestDbgInfoContentForwardListFromStdModule.TestDbgInfoContentForwardList)
UNSUPPORTED: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: 
test_dwo 
(TestDbgInfoContentForwardListFromStdModule.TestDbgInfoContentForwardList) 
(test case does not fall in any category of interest for this run)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dsym 
(TestForwardListFromStdModule.TestBasicForwardList)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dwarf 
(TestForwardListFromStdModule.TestBasicForwardList)
UNSUPPORTED: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: 
test_dwo (TestForwardListFromStdModule.TestBasicForwardList) (test case does 
not fall in any category of interest for this run)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dsym 
(TestRetryWithStdModule.TestCase)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dwarf 
(TestRetryWithStdModule.TestCase)
UNSUPPORTED: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: 
test_dwo (TestRetryWithStdModule.TestCase) (test case does not fall in any 
category of interest for this run)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dsym 
(TestSharedPtrFromStdModule.TestSharedPtr)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dwarf 
(TestSharedPtrFromStdModule.TestSharedPtr)
UNSUPPORTED: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: 
test_dwo (TestSharedPtrFromStdModule.TestSharedPtr) (test case does not fall in 
any category of interest for this run)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dsym 
(TestEmptyStdModule.ImportStdModule)
PASS: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: test_dwarf 
(TestEmptyStdModule.ImportStdModule)
UNSUPPORTED: LLDB (/Users/zequanwu/work/llvm/out/cmake/bin/clang-arm64) :: 
test_dwo (TestEmptyStdModule.ImportStdModule) (test case does not fall in any 
category of interest for this run)
Assertion failed: (0 && "Invalid SLocOffset or bad function choice"), function 
getFileIDLoaded, file SourceManager.cpp, line 865.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and 
include the crash backtrace.
```

But I don't know why this change will make this crash explicitly shows up when 
running check-lldb.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-15 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

I had a fix to this:  Let `SymbolFileDWARF::GetForwardDeclCompilerTypeToDIE` do 
the same as `SymbolFileDWARF::GetUniqueDWARFASTTypeMap`: inquery 
SymbolFileDWARFDebugMap first to get the shared underlying SymbolFile so the 
map is shared among multiple SymbolFileDWARF. It's here: 
https://github.com/ZequanWu/llvm-project/commit/2172c660821e273205e7ad3a64eb7f3623b21606

It fixes those failed tests shown on the macos bot. However, I noticed that 
lldb crashes on three tests related to clang module (they also crashes when the 
fix is not given, but not crash after reverting this PR):
```
Unresolved Tests (3):
  lldb-api :: 
commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
  lldb-api :: 
commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py
```

I found it's the following commands causing crash.
```
$ out/cmake/bin/lldb 
out/cmake/lldb-test-build.noindex/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.test_dwarf/a.out
 -o "settings set symbols.clang-modules-cache-path 
/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex/module-cache-lldb/lldb-api"
 -o "settings set target.import-std-module true" -o "b 9" -o "r"  -o "expr a"
(lldb) target create 
"../cmake/lldb-test-build.noindex/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.test_dwarf/a.out"
Current executable set to 
'/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.test_dwarf/a.out'
 (arm64).
(lldb) settings set symbols.clang-modules-cache-path 
/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex/module-cache-lldb/lldb-api
(lldb) settings set target.import-std-module true
(lldb) b 9
Breakpoint 1: where = a.out`main + 104 at main.cpp:9:3, address = 
0x00012508
(lldb) r
Process 12273 launched: 
'/Users/zequanwu/work/llvm/out/cmake/lldb-test-build.noindex/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.test_dwarf/a.out'
 (arm64)
Process 12273 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
frame #0: 0x00012508 a.out`main(argc=1, argv=0x00016fdff428) at 
main.cpp:9:3
   6
   7int main(int argc, char **argv) {
   8  std::vector a = {{3}, {1}, {2}};
-> 9  return 0; // Set break point at this line.
   10   }
(lldb) expr a
Assertion failed: (0 && "Invalid SLocOffset or bad function choice"), function 
getFileIDLoaded, file SourceManager.cpp, line 865.
LLDB diagnostics will be written to 
/var/folders/jf/zylbx28s05n0d_xwqdf5jnrc00lnhs/T/diagnostics-512963
Please include the directory content when filing a bug report
[1]12267 abort  bin/lldb  -o  -o "settings set target.import-std-module 
true" -o "b 9" -o "r"
```

The clang module in 
`out/cmake/lldb-test-build.noindex/module-cache-lldb/lldb-api` was created when 
running `check-lldb`. If I delete the clang modules in that directory and run 
the above command again, it no longer crashes and able to give correct result 
(after the first run, a new clang module is created in the directory. Following 
runs of the above commands no longer crashes). So, it looks like related to 
stale clang module. If I use debug built lldb, it no longer crashes. Do you 
have any idea how to debug this crash? I'm not familiar with how clang module 
interact with type completion etc.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-14 Thread Adrian Prantl via lldb-commits


adrian-prantl wrote:

> Thanks. Can you provide instructions to repro the failure locally?

The bot log should have the cmake line and all the commands that were run there.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-14 Thread via lldb-commits


jimingham wrote:

Thanks!

Jim

> On May 13, 2024, at 6:39 PM, Zequan Wu ***@***.***> wrote:
> 
> 
> Can you take care of cleaning this up, this seems like a slightly complex 
> patch and not in an area I'm familiar with.
> 
> Yes, will do. Sorry for the mess without reverting it earlier.
> 
> —
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
> You are receiving this because you are on a team that was mentioned.
> 



https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits


jimingham wrote:

Can you take care of cleaning this up, this seems like a slightly complex patch 
and not in an area I'm familiar with.

Thanks!

Jim


> On May 13, 2024, at 6:35 PM, Zequan Wu ***@***.***> wrote:
> 
> 
> Reverting those two commits seems to have caused this build failure on Ubuntu:
> 
> You forgot to delete the newly added test 
> SymbolFile/DWARF/delayed-definition-die-searching.test in the reverting: 
> 37b8e5f 
> 
> —
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
> You are receiving this because you are on a team that was mentioned.
> 



https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

> Reverting those two commits seems to have caused this build failure on Ubuntu:

You forgot the delete the newly added test 
SymbolFile/DWARF/delayed-definition-die-searching.test in the reverting: 
https://github.com/llvm/llvm-project/commit/37b8e5feb1d065a7c474e6595bac6d2f65faeb51

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits

jimingham wrote:

Reverting those two commits seems to have caused this build failure on Ubuntu:

Step 4 (build) warnings: build (warnings)
../llvm-project/clang/lib/Lex/PPDirectives.cpp:548:28: warning: comparison of 
integers of different signs: 'unsigned int' and 'int' [-Wsign-compare]
1 warning generated.
../llvm-project/clang/lib/Frontend/TextDiagnostic.cpp:148:36: warning: 
comparison of integers of different signs: 'int' and 'unsigned int' 
[-Wsign-compare]
1 warning generated.
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1079:20: warning: 
comparison of integers of different signs: 'typename iterator_traits::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1089:24: warning: 
comparison of integers of different signs: 'typename iterator_traits::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1120:20: warning: 
comparison of integers of different signs: 'typename iterator_traits::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:1130:24: warning: 
comparison of integers of different signs: 'typename iterator_traits::difference_type' (aka 'int') and 'unsigned int' [-Wsign-compare]
../llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:1107:7: 
warning: implicit conversion from 'long long' to 'const std::time_t' (aka 
'const long') changes value from -1096193779200 to -977118720 
[-Wconstant-conversion]
../llvm-project/lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:1109:7: 
warning: implicit conversion from 'long long' to 'const std::time_t' (aka 
'const long') changes value from 971890963199 to 1228354303 
[-Wconstant-conversion]
../llvm-project/lld/MachO/SyntheticSections.cpp:2063:25: warning: comparison of 
integers of different signs: 'int' and 'const uint32_t' (aka 'const unsigned 
int') [-Wsign-compare]
1 warning generated.

Step 6 (test) failure: build (failure)
clang: warning: argument unused during compilation: 
'-fmodules-cache-path=/home/tcwg-buildbot/worker/lldb-arm-ubuntu/build/lldb-test-build.noindex/module-cache-clang/lldb-shell'
 [-Wunused-command-line-argument]

Would you mind taking a look, this is not my area of llvm...

https://lab.llvm.org/buildbot/#/builders/17/builds/53025

Thanks!

Jim

> On May 13, 2024, at 6:22 PM, Zequan Wu ***@***.***> wrote:
> 
> 
> your commit deleted that file I think, I added it back when I did the revert 
> (possibly a mistake)... It passes on my macOS system but is failing on Ubuntu 
> after the revert. I think I'll just disable it for now.
> 
> This change adds the new test, so deleting it as part of the reverting is 
> expected.
> 
> —
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
> You are receiving this because you are on a team that was mentioned.
> 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

> your commit deleted that file I think, I added it back when I did the revert 
> (possibly a mistake)...  It passes on my macOS system but is failing on 
> Ubuntu after the revert.  I think I'll just disable it for now.

This change adds the new test, so deleting it as part of the reverting is 
expected.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits


jimingham wrote:

BTW, do you know what's up with this test:

SymbolFile/DWARF/delayed-definition-die-searching.test

your commit deleted that file I think, I added it back when I did the revert 
(possibly a mistake)...  It passes on my macOS system but is failing on Ubuntu 
after the revert.  I think I'll just disable it for now.

Jim


> On May 13, 2024, at 6:09 PM, Jim Ingham ***@***.***> wrote:
> 
>> 
>> 
>> 
>>> On May 13, 2024, at 6:04 PM, Zequan Wu ***@***.***> wrote:
>>> 
>>> 
>>> I was able to reproduce the failure of these three:
>>> 
>>> lldb-api :: lang/c/forward/TestForwardDeclaration.py
>>> lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
>>> lldb-api :: types/TestRecursiveTypes.py
>>> 
>>> locally. Reverting this patch and a7eff59 
>>> 
>>>  which depended on it made the failure go away.
>>> 
>>> I reverted the patches for now to get the bots clean.
>>> 
>>> Thanks. Can you provide instructions to repro the failure locally?
>>> 
>> I didn't do anything out of the ordinary.  I'm on macOS Sonoma 14.4.1, with 
>> the latest Xcode(15.3).  I ran the CMakery this way:
>> 
>> cmake -B `pwd`/all-in-one -G Ninja -C 
>> `pwd`/llvm-project/lldb/cmake/caches/Apple-lldb-base.cmake 
>> -DLLVM_ENABLE_PROJECTS="clang;lldb" 
>> -DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;compiler-rt;libunwind" 
>> -DLIBCXX_INCLUDE_TESTS=OFF  -DCMAKE_BUILD_TYPE=Debug 
>> -DCMAKE_OSX_DEPLOYMENT_TARGET:STRING=12.3  -DLLDB_BUILD_FRAMEWORK=YES  
>> -DCMAKE_CXX_STANDARD:STRING=17 llvm-project/llvm
>> 
>> Then:
>> 
>> $ cd all-in-one
>> $ ninja check-lldb
>> 
>> and got the failure.
>> 
>> Jim
>> 
>> 
>>> @ZequanWu  in the future, if one of your 
>>> commits break a bot, make sure to revert it immediately, you can always 
>>> re-land it later with a fix or an explanation why it wasn't your commit 
>>> that broke the bots. Reverting a commit is cheap, red bots are expensive :-)
>>> 
>>> Will do.
>>> 
>>> —
>>> Reply to this email directly, view it on GitHub 
>>> , 
>>> or unsubscribe 
>>> .
>>> You are receiving this because you are on a team that was mentioned.
>>> 
>> 
>> 



https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits


jimingham wrote:



> On May 13, 2024, at 6:04 PM, Zequan Wu ***@***.***> wrote:
> 
> 
> I was able to reproduce the failure of these three:
> 
> lldb-api :: lang/c/forward/TestForwardDeclaration.py
> lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
> lldb-api :: types/TestRecursiveTypes.py
> 
> locally. Reverting this patch and a7eff59 
> 
>  which depended on it made the failure go away.
> 
> I reverted the patches for now to get the bots clean.
> 
> Thanks. Can you provide instructions to repro the failure locally?
> 
I didn't do anything out of the ordinary.  I'm on macOS Sonoma 14.4.1, with the 
latest Xcode(15.3).  I ran the CMakery this way:

cmake -B `pwd`/all-in-one -G Ninja -C 
`pwd`/llvm-project/lldb/cmake/caches/Apple-lldb-base.cmake 
-DLLVM_ENABLE_PROJECTS="clang;lldb" 
-DLLVM_ENABLE_RUNTIMES="libcxx;libcxxabi;compiler-rt;libunwind" 
-DLIBCXX_INCLUDE_TESTS=OFF  -DCMAKE_BUILD_TYPE=Debug 
-DCMAKE_OSX_DEPLOYMENT_TARGET:STRING=12.3  -DLLDB_BUILD_FRAMEWORK=YES  
-DCMAKE_CXX_STANDARD:STRING=17 llvm-project/llvm

Then:

$ cd all-in-one
$ ninja check-lldb

and got the failure.

Jim


> @ZequanWu  in the future, if one of your commits 
> break a bot, make sure to revert it immediately, you can always re-land it 
> later with a fix or an explanation why it wasn't your commit that broke the 
> bots. Reverting a commit is cheap, red bots are expensive :-)
> 
> Will do.
> 
> —
> Reply to this email directly, view it on GitHub 
> , or 
> unsubscribe 
> .
> You are receiving this because you are on a team that was mentioned.
> 



https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

> I was able to reproduce the failure of these three:
> 
> lldb-api :: lang/c/forward/TestForwardDeclaration.py
> lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
> lldb-api :: types/TestRecursiveTypes.py
>
> locally. Reverting this patch and 
> https://github.com/llvm/llvm-project/commit/a7eff59f78f08f8ef0487dfe2a136fb311af4fd0
>  which depended on it made the failure go away.
> 
> I reverted the patches for now to get the bots clean.

Thanks. Can you provide instructions to repro the failure locally?

> @ZequanWu in the future, if one of your commits break a bot, make sure to 
> revert it immediately, you can always re-land it later with a fix or an 
> explanation why it wasn't your commit that broke the bots. Reverting a commit 
> is cheap, red bots are expensive :-)

Will do. 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread Adrian Prantl via lldb-commits


adrian-prantl wrote:

@ZequanWu in the future, if one of your commits break a bot, make sure to 
revert it immediately, you can always re-land it later with a fix or an 
explanation why it wasn't your commit that broke the bots. Reverting a commit 
is cheap, red bots are expensive :-)

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits


jimingham wrote:

I was able to reproduce the failure of these three:

  lldb-api :: lang/c/forward/TestForwardDeclaration.py
  lldb-api :: lang/cpp/unique-types3/TestUniqueTypes3.py
  lldb-api :: types/TestRecursiveTypes.py

locally.  Reverting this patch and a7eff59f78f08f8ef0487dfe2a136fb311af4fd0 
which depended on it made the failure go away.

I reverted the patches for now to get the bots clean.



https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-13 Thread via lldb-commits


jimingham wrote:

I'm trying to reproduce locally as well.  It's pretty clear that this patch is 
implicated in the failure.  The first failure we saw both on the incremental 
bots and the first failure on the sanitized bots both had this patch, and no 
other really relevant ones, in the commit list.

I don't know why this is causing the failure, but it's left the bots red for 
three days now.  We really should revert this and figure out why it's failing 
before reapplying.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

> > Could this commit have broken the bots? 
> > https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/1897/
> 
> Looks like so, but I cannot repro the test failure locally.

The error message is different in current latest build 
(https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/1907/testReport/junit/lldb-api/lang_c_forward/TestForwardDeclaration_py/)
 which makes me thinking it's related to clang module being out of date?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

> Could this commit have broken the bots? 
> https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/1897/

Looks like so, but I cannot repro the test failure locally. 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Adrian Prantl via lldb-commits


adrian-prantl wrote:

Could this commit have broken the bots?
https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/1897/

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Felipe de Azevedo Piovezan via lldb-commits


felipepiovezan wrote:

AFAICT we never added new entries -- definitely not forward declarations -- to 
the table when doing the idx_parent work. Either they were already there, or 
the entry would have no parent. Would be nice to have an example to see this in 
action.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

I sent an alternative fix at https://github.com/llvm/llvm-project/pull/91799.

> The .debug_names spec states that only entries with definitions should be in 
> the .debug_names table...

Do it mean the .debug_names is implemented incorrectly?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Greg Clayton via lldb-commits


clayborg wrote:

See the `/// <<< newly added for fix` comments for the new lines

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Greg Clayton via lldb-commits


clayborg wrote:

Ok, I found the issue. `.debug_names` tables with `DW_IDX_parent` entries, 
might contain tons of entries for forward declared classes because in my 
example `std::ios_base` is the parent declaration context for `seekdir`, 
`openmode`, and `iostate` so `.debug_names` entries for these types will refer 
to another `.debug_names` for `std::ios_base` as the parent, buit this is 
messing up the table itself as now it contains entries that are not all full 
definitions. The `.debug_names` spec states that only entries with definitions 
should be in the .debug_names table... 

That being said, an easy fix is this:
```bool DebugNamesDWARFIndex::ProcessEntry(
const DebugNames::Entry ,
llvm::function_ref callback) {
  std::optional ref = ToDIERef(entry);
  if (!ref)
return true;
  SymbolFileDWARF  = *llvm::cast(
  m_module.GetSymbolFile()->GetBackingSymbolFile());
  DWARFDIE die = dwarf.GetDIE(*ref);
  if (!die)
return true;
  // Watch out for forward declarations that appear in the .debug_names tables
  // only due to being there for a DW_IDX_parent.
  if (die.GetAttributeValueAsUnsigned(DW_AT_declaration, 0)) /// <<< newly 
added for fix
return true;/// <<< newly added for fix
  return callback(die);
}
```

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

> The issue might arise from having a .debug_names table that has DW_IDX_parent 
> entries that means that there might be forward declarations included in the 
> DWARF index.

Do you mean that the searching in the type index returns a declaration DIE (but 
I expected it to always return a definition DIE if exists: 
https://github.com/llvm/llvm-project/blob/91feb130d5cd3cafce94bbaf7ad67d1542623a75/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L3135)?
  If that's the case, we get a type created from declaration DIE 
`SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext`, which makes it falls 
through the check. 

A quick fix I have in mind is to do a double-check if the dwarf_die is a 
declaration or not before: 
https://github.com/llvm/llvm-project/blob/d8e73752a5f4f79ef4293d8f446c03062010233d/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1659-L1661,
 so we won't complete it. But if there truly exists a definition DIE, it will 
never be completed because it relies on index to find definition DIE.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

> So this DIE is just a declaration. Shouldn't this code have tried to find a 
> non declaration DIE for "std::ios_base"?

Yes, I suppose `SymbolFileDWARF::CompleteType` will try to find the definition 
DIE for it before calling `DWARFASTParserClang::CompleteTypeFromDWARF`. If the 
definition DIE is not found, it's expected to do an early exit because Type is 
nullptr: 
https://github.com/llvm/llvm-project/blob/d8e73752a5f4f79ef4293d8f446c03062010233d/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1643-L1644.
 If found, we should have the definition DIE in the 
GetForwardDeclCompilerTypeToDIE map: 
https://github.com/llvm/llvm-project/blob/d8e73752a5f4f79ef4293d8f446c03062010233d/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp#L1646-L1651

Can you confirm that there exists a definition DIE for it? How does it look 
like?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Greg Clayton via lldb-commits


clayborg wrote:

Is `SymbolFileDWARF::CompleteType(...)` responsible for trying to find a 
non-declaration DIE first? The issue might arise from having a .debug_names 
table that has `DW_IDX_parent` entries that means that there might be forward 
declarations included in the DWARF index.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Greg Clayton via lldb-commits


clayborg wrote:

This is causing a clang assertion due:
```
(lldb) type lookup std::ios_base
Assertion failed: (DD && "queried property of class with no definition"), 
function data, file DeclCXX.h, line 464.
bt
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = hit program assert
frame #0: 0x000180acaa60 libsystem_kernel.dylib`__pthread_kill + 8
frame #1: 0x000180b02c20 libsystem_pthread.dylib`pthread_kill + 288
frame #2: 0x000180a0fa20 libsystem_c.dylib`abort + 180
frame #3: 0x000180a0ed10 libsystem_c.dylib`__assert_rtn + 284
frame #4: 0x00012223ba64 
LLDB`clang::CXXRecordDecl::data(this=0x000119ec5518) const at 
DeclCXX.h:464:5
frame #5: 0x0001223b7914 
LLDB`clang::CXXRecordDecl::hasUserDeclaredMoveConstructor(this=0x000119ec5518)
 const at DeclCXX.h:858:12
frame #6: 0x000122399c0c 
LLDB`lldb_private::TypeSystemClang::CompleteTagDeclarationDefinition(type=0x00050296fd10)
 at TypeSystemClang.cpp:8370:28
  * frame #7: 0x0001e4ac 
LLDB`DWARFASTParserClang::CompleteRecordType(this=0x00050296faa0, 
die=0x0001761932d0 0x0004be848030 (0x0090cdd5), 
type=0x00050296fca0, clang_type=0x00050296fd10) at 
DWARFASTParserClang.cpp:2291:3
frame #8: 0x0001f150 
LLDB`DWARFASTParserClang::CompleteTypeFromDWARF(this=0x00050296faa0, 
die=0x0001761932d0 0x0004be848030 (0x0090cdd5), 
type=0x00050296fca0, clang_type=0x00050296fd10) at 
DWARFASTParserClang.cpp:2356:12
frame #9: 0x0001222b6eac 
LLDB`lldb_private::plugin::dwarf::SymbolFileDWARF::CompleteType(this=0x00011912e218,
 compiler_type=0x00050296fd10) at SymbolFileDWARF.cpp:1659:23
frame #10: 0x0001218952e0 
LLDB`lldb_private::Type::ResolveCompilerType(this=0x00050296fca0, 
compiler_type_resolve_state=Full) at Type.cpp:715:24
frame #11: 0x000121895520 
LLDB`lldb_private::Type::GetFullCompilerType(this=0x00050296fca0) at 
Type.cpp:755:3
frame #12: 0x0001218e49c8 
LLDB`lldb_private::Language::ImageListTypeScavenger::Find_Impl(this=0x000118ebd000,
 exe_scope=0x00011a810e00, key="std::ios_base", results=size=0) at 
Language.cpp:473:43
frame #13: 0x0001218e47f8 
LLDB`lldb_private::Language::TypeScavenger::Find(this=0x000118ebd000, 
exe_scope=0x00011a810e00, key="std::ios_base", results=size=0, append=true) 
at Language.cpp:456:13
frame #14: 0x00012418c958 
LLDB`CommandObjectTypeLookup::DoExecute(this=0x000118e7a200, 
raw_command_line="std::ios_base", result=0x00016fdfe2f0) at 
CommandObjectType.cpp:2667:24
frame #15: 0x00012175fe3c 
LLDB`lldb_private::CommandObjectRaw::Execute(this=0x000118e7a200, 
args_string="std::ios_base", result=0x00016fdfe2f0) at 
CommandObject.cpp:850:7
frame #16: 0x000121733c74 
LLDB`lldb_private::CommandInterpreter::HandleCommand(this=0x000100739130, 
command_line="type lookup std::ios_base", 
lazy_add_to_history=eLazyBoolCalculate, result=0x00016fdfe2f0, 
force_repeat_command=false) at CommandInterpreter.cpp:2038:14
frame #17: 0x000121738594 
LLDB`lldb_private::CommandInterpreter::IOHandlerInputComplete(this=0x000100739130,
 io_handler=0x000118eb5918, line="type lookup std::ios_base") at 
CommandInterpreter.cpp:3118:3
frame #18: 0x0001214c9004 
LLDB`lldb_private::IOHandlerEditline::Run(this=0x000118eb5918) at 
IOHandler.cpp:600:22
frame #19: 0x00012145982c 
LLDB`lldb_private::Debugger::RunIOHandlers(this=0x00010185fe00) at 
Debugger.cpp:1092:16
frame #20: 0x000121739bc4 
LLDB`lldb_private::CommandInterpreter::RunCommandInterpreter(this=0x000100739130,
 options=0x000118e94340) at CommandInterpreter.cpp:3380:16
frame #21: 0x0001211496f0 
LLDB`lldb::SBDebugger::RunCommandInterpreter(this=0x00016fdfeb90, 
options=0x00016fdfe878) at SBDebugger.cpp:1288:14
frame #22: 0x00016338 
lldb`Driver::MainLoop(this=0x00016fdfeb70) at Driver.cpp:558:20
frame #23: 0x00016e54 lldb`main(argc=7, argv=0x00016fdff308) at 
Driver.cpp:807:26
frame #24: 0x00018077a0e0 dyld`start + 2360
```
The DIE being passed to `DWARFASTParserClang::CompleteRecordType()` looks like:
```
0x0090cdbf: DW_TAG_compile_unit
  DW_AT_producer("clang version 15.0.7 
(mononoke://mononoke.internal.tfbnw.net/fbsource 
79a342ef1fab2184ef28a1c26022c7cbaec90e83)")
  DW_AT_language(DW_LANG_C_plus_plus_14)
  DW_AT_name("fbcode/folly/detail/UniqueInstance.cpp")
  DW_AT_dwo_name
("buck-out/v2/gen/fbcode/0765d56217397ee3/hphp/hhvm/__hhvm_link__/libunique_instance.a/UniqueInstance.cpp.o.opt.o")

0x0090cdc7:   DW_TAG_namespace
DW_AT_name  ("std")

0x0090cdd5: DW_TAG_class_type
  DW_AT_name("ios_base")
  DW_AT_declaration (true)

0x0090cdd7:   DW_TAG_class_type

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-10 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu closed 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-09 Thread Pavel Labath via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK-NEXT: 
DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'

labath wrote:

I see, that makes sense. Thanks for explaining.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits



@@ -24,13 +24,16 @@ class UniqueDWARFASTType {
   UniqueDWARFASTType() : m_type_sp(), m_die(), m_declaration() {}
 
   UniqueDWARFASTType(lldb::TypeSP _sp, const DWARFDIE ,
- const Declaration , int32_t byte_size)
+ const Declaration , int32_t byte_size,
+ bool is_forward_declaration)
   : m_type_sp(type_sp), m_die(die), m_declaration(decl),
-m_byte_size(byte_size) {}
+m_byte_size(byte_size),
+m_is_forward_declaration(is_forward_declaration) {}

ZequanWu wrote:

I just removed this dead ctor.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -24,13 +24,16 @@ class UniqueDWARFASTType {
   UniqueDWARFASTType() : m_type_sp(), m_die(), m_declaration() {}
 
   UniqueDWARFASTType(lldb::TypeSP _sp, const DWARFDIE ,
- const Declaration , int32_t byte_size)
+ const Declaration , int32_t byte_size,
+ bool is_forward_declaration)
   : m_type_sp(type_sp), m_die(die), m_declaration(decl),
-m_byte_size(byte_size) {}
+m_byte_size(byte_size),
+m_is_forward_declaration(is_forward_declaration) {}

dwblaikie wrote:

Ah, cool - thanks, sorry I missed that.

If you wanted to remove the dead ctor either before or after this commit, that 
might be handy/nice.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't2' resolving forward 
declaration...
+# CHECK: (t2 >)  {}
+# CHECK: (lldb) p v2
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't1' resolving forward 
declaration...
+# CHECK: (t1)  {}
+
+#--- lldb.cmd

ZequanWu wrote:

Yeah, that would work. I have an example at 
https://discourse.llvm.org/t/rfc-delay-definition-die-searching-when-parse-a-declaration-die-for-record-type/78526/15.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't2' resolving forward 
declaration...
+# CHECK: (t2 >)  {}
+# CHECK: (lldb) p v2
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't1' resolving forward 
declaration...
+# CHECK: (t1)  {}
+
+#--- lldb.cmd

dwblaikie wrote:

Hmm - what about template parameters, even without simplified template names?

`t1` t2 should be a declaration, without looking up the definition DIE in 
this case? (though perhaps clang/LLDB doesn't do that correctly, in which case 
maybe `t1` might work)

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't2' resolving forward 
declaration...
+# CHECK: (t2 >)  {}
+# CHECK: (lldb) p v2
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't1' resolving forward 
declaration...
+# CHECK: (t1)  {}
+
+#--- lldb.cmd

ZequanWu wrote:

I tried something like this:
```
struct Bar {
  int x;
  Bar(int x): x(x) {}
};
struct Foo {
  Bar* bar;
  Foo(int x) {
bar = new Bar(x);
  }
};
```
In another CU, print variable with type `Foo*`, but it still tries to complete 
both `Foo` and `Bar` because the looking-up  child count through a pointer 
issue.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits



@@ -24,13 +24,16 @@ class UniqueDWARFASTType {
   UniqueDWARFASTType() : m_type_sp(), m_die(), m_declaration() {}
 
   UniqueDWARFASTType(lldb::TypeSP _sp, const DWARFDIE ,
- const Declaration , int32_t byte_size)
+ const Declaration , int32_t byte_size,
+ bool is_forward_declaration)
   : m_type_sp(type_sp), m_die(die), m_declaration(decl),
-m_byte_size(byte_size) {}
+m_byte_size(byte_size),
+m_is_forward_declaration(is_forward_declaration) {}

ZequanWu wrote:

It's never used. It first gets assigned at 
https://github.com/llvm/llvm-project/pull/90663/files#diff-5dd6736e4d6c38623630df16c4494c1a8b099716ee0f05c9af54b4bafb1d864eR1929
 then updated to `false` at 
https://github.com/llvm/llvm-project/pull/90663/files#diff-5dd6736e4d6c38623630df16c4494c1a8b099716ee0f05c9af54b4bafb1d864eR1808
 when we find definition DIE.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/8] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840a..7ad661c9a9d49 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -24,13 +24,16 @@ class UniqueDWARFASTType {
   UniqueDWARFASTType() : m_type_sp(), m_die(), m_declaration() {}
 
   UniqueDWARFASTType(lldb::TypeSP _sp, const DWARFDIE ,
- const Declaration , int32_t byte_size)
+ const Declaration , int32_t byte_size,
+ bool is_forward_declaration)
   : m_type_sp(type_sp), m_die(die), m_declaration(decl),
-m_byte_size(byte_size) {}
+m_byte_size(byte_size),
+m_is_forward_declaration(is_forward_declaration) {}

dwblaikie wrote:

Where is this ctor used? I can't find any calls to it.

And if it is never used, how is `m_is_forward_declaration` ever `false`, if 
it's initialized to true and this ctor is never called, I don't see any other 
place that assigns to it?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -1632,27 +1632,34 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
 return true;
   }
 
-  DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
-  if (dwarf_die) {
-// Once we start resolving this type, remove it from the forward
-// declaration map in case anyone child members or other types require this
-// type to get resolved. The type will get resolved when all of the calls
-// to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
-GetForwardDeclCompilerTypeToDIE().erase(die_it);
-
-Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
+  // Once we start resolving this type, remove it from the forward
+  // declaration map in case anyone's child members or other types require this
+  // type to get resolved.
+  DWARFDIE dwarf_die = GetDIE(die_it->second);
+  GetForwardDeclCompilerTypeToDIE().erase(die_it);
+  Type *type = nullptr;
+  if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
+type = dwarf_ast->FindDefinitionTypeForDIE(dwarf_die);
+  if (!type)
+return false;
 
-Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion);
-if (log)
-  GetObjectFile()->GetModule()->LogMessageVerboseBacktrace(
-  log, "{0:x8}: {1} ({2}) '{3}' resolving forward declaration...",
-  dwarf_die.GetID(), DW_TAG_value_to_name(dwarf_die.Tag()),
-  dwarf_die.Tag(), type->GetName().AsCString());
-assert(compiler_type);
-if (DWARFASTParser *dwarf_ast = GetDWARFParser(*dwarf_die.GetCU()))
-  return dwarf_ast->CompleteTypeFromDWARF(dwarf_die, type, compiler_type);
+  die_it = GetForwardDeclCompilerTypeToDIE().find(
+  compiler_type_no_qualifiers.GetOpaqueQualType());
+  if (die_it != GetForwardDeclCompilerTypeToDIE().end()) {
+dwarf_die = GetDIE(die_it->getSecond());
+GetForwardDeclCompilerTypeToDIE().erase(die_it);
   }
-  return false;
+
+  Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion);
+  if (log)

dwblaikie wrote:

```
if (Log *log = GetLog(DWARFLog::DebugInfo | DWARFLog::TypeCompletion))
```

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't2' resolving forward 
declaration...
+# CHECK: (t2 >)  {}
+# CHECK: (lldb) p v2
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't1'
+# CHECK: DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_base_type 
(DW_TAG_base_type) name = 'int'
+# CHECK: DW_TAG_structure_type (DW_TAG_structure_type) 't1' resolving forward 
declaration...
+# CHECK: (t1)  {}
+
+#--- lldb.cmd

dwblaikie wrote:

Might be better to test this without `-gsimple-template-names`, because it's a 
more generally valuable feature than that?

Oh, but that "look for the child count through a pointer" issue comes up and 
it's hard to reach this situation without `-gsimple-template-names`? (Maybe 
indirectly - if you print a struct with a pointer - perhaps that doesn't do the 
child count searching through the pointer?)

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread David Blaikie via lldb-commits



@@ -16,61 +16,66 @@ using namespace lldb_private::plugin::dwarf;
 bool UniqueDWARFASTTypeList::Find(const DWARFDIE ,
   const lldb_private::Declaration ,
   const int32_t byte_size,
+  bool is_forward_declaration,
   UniqueDWARFASTType ) const {
   for (const UniqueDWARFASTType  : m_collection) {
 // Make sure the tags match
 if (udt.m_die.Tag() == die.Tag()) {
-  // Validate byte sizes of both types only if both are valid.
-  if (udt.m_byte_size < 0 || byte_size < 0 ||
-  udt.m_byte_size == byte_size) {
-// Make sure the file and line match
-if (udt.m_declaration == decl) {
-  // The type has the same name, and was defined on the same file and
-  // line. Now verify all of the parent DIEs match.
-  DWARFDIE parent_arg_die = die.GetParent();
-  DWARFDIE parent_pos_die = udt.m_die.GetParent();
-  bool match = true;
-  bool done = false;
-  while (!done && match && parent_arg_die && parent_pos_die) {
-const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
-const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
-if (parent_arg_tag == parent_pos_tag) {
-  switch (parent_arg_tag) {
-  case DW_TAG_class_type:
-  case DW_TAG_structure_type:
-  case DW_TAG_union_type:
-  case DW_TAG_namespace: {
-const char *parent_arg_die_name = parent_arg_die.GetName();
-if (parent_arg_die_name ==
-nullptr) // Anonymous (i.e. no-name) struct
-{
-  match = false;
-} else {
-  const char *parent_pos_die_name = parent_pos_die.GetName();
-  if (parent_pos_die_name == nullptr ||
-  ((parent_arg_die_name != parent_pos_die_name) &&
-   strcmp(parent_arg_die_name, parent_pos_die_name)))
-match = false;
-}
-  } break;
-
-  case DW_TAG_compile_unit:
-  case DW_TAG_partial_unit:
-done = true;
-break;
-  default:
-break;
-  }
+  // If they are not both definition DIEs or both declaration DIEs, then
+  // don't check for byte size and declaration location, because 
declaration
+  // DIEs usually don't have those info.
+  bool matching_size_declaration =
+  udt.m_is_forward_declaration != is_forward_declaration
+  ? true
+  : (udt.m_byte_size < 0 || byte_size < 0 ||
+ udt.m_byte_size == byte_size) &&
+udt.m_declaration == decl;
+  if (!matching_size_declaration)
+continue;
+  // The type has the same name, and was defined on the same file and
+  // line. Now verify all of the parent DIEs match.
+  DWARFDIE parent_arg_die = die.GetParent();
+  DWARFDIE parent_pos_die = udt.m_die.GetParent();
+  bool match = true;
+  bool done = false;
+  while (!done && match && parent_arg_die && parent_pos_die) {
+const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
+const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
+if (parent_arg_tag == parent_pos_tag) {
+  switch (parent_arg_tag) {
+  case DW_TAG_class_type:
+  case DW_TAG_structure_type:
+  case DW_TAG_union_type:
+  case DW_TAG_namespace: {
+const char *parent_arg_die_name = parent_arg_die.GetName();
+if (parent_arg_die_name ==
+nullptr) // Anonymous (i.e. no-name) struct
+{

dwblaikie wrote:

The comment between the `)` and `{` creates some weird line wrapping, maybe put 
the comment inside the `{` block?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

Will leave it open for few days in case anyone has more comments.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK-NEXT: 
DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'

ZequanWu wrote:

Updated to use `CHECK`. Unfortunately, it doesn't work with `image dump ast` 
because this change doesn't affect  when type completion happens. So, after `p 
v1`, w/wo this change, lldb just creates `ClassTemplateSpecializationDecl` for 
`t1` with no definition. When doing `p v2', lldb tries to complete the 
`t1` decl. This change only moves the definition searching to the time 
when completing a type.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/7] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840a..7ad661c9a9d49 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Pavel Labath via lldb-commits



@@ -0,0 +1,40 @@
+# Test definition DIE searching is delayed until complete type is required.
+
+# RUN: split-file %s %t
+# RUN: %clangxx_host %t/main.cpp %t/t1_def.cpp -g -gsimple-template-names -o 
%t.out
+# RUN: %lldb -b %t.out -s %t/lldb.cmd | FileCheck %s
+
+# CHECK:  (lldb) p v1
+# CHECK-NEXT: 
DWARFASTParserClang::ParseTypeFromDWARF{{.*}}DW_TAG_structure_type 
(DW_TAG_structure_type) name = 't2'

labath wrote:

CHECK-NEXT on log output sounds like a very bad idea, as the test can be broken 
by adding a random log statement. For what you're trying to do, a plain CHECK 
should suffice (though i'd also consider if this can be checked using something 
less volatile than log messages -- for example, you might be able to use `image 
dump ast` to compare the clang ast of the type before/after completion).

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Pavel Labath via lldb-commits


https://github.com/labath edited https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-07 Thread Pavel Labath via lldb-commits


https://github.com/labath approved this pull request.


https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-06 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

> You could enable logging and check for specific logging after steps. In the 
> test I described above if you just print the "Foo *foo" variable, it won't 
> need to complete the definition, you could check for logging, and then if you 
> print "*foo", then it should complete the definition and you should see 
> logging. Not sure if that is what you meant

Added a test with simple template names. 

The example you gave doesn't delay definition DIE searching because even when 
`p foo`, lldb calls `ValueObject::GetNumChildren` which requires complete type, 
which I think is also a point of unnecessary operation. The 
`ValueObjectPrinter::PrintChildrenIfNeeded` below ultimately calls 
`GetNumChildren`: 
https://github.com/llvm/llvm-project/blob/1241e7692a466ceb420be2780f1c3e8bbab7d469/lldb/source/DataFormatters/ValueObjectPrinter.cpp#L90-L94.
 I don't see a setting to disable this. If we could avoid this, this patch 
could further reduce unnecessary definition DIE searching when user just want 
to print the value of a pointer (declaration DIE should be enough). 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-06 Thread Zequan Wu via lldb-commits



@@ -1667,13 +1791,40 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
 }
 
 if (dwarf->GetUniqueDWARFASTTypeMap().Find(
-unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
-*unique_ast_entry_up)) {
+unique_typename, die, unique_decl, byte_size,
+attrs.is_forward_declaration, *unique_ast_entry_up)) {
   type_sp = unique_ast_entry_up->m_type_sp;
   if (type_sp) {
 dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
 LinkDeclContextToDIE(
 GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+if (!attrs.is_forward_declaration) {
+  // If the DIE being parsed in this function is a definition and the
+  // entry in the map is a declaration, then we need to update the 
entry
+  // to point to the definition DIE.
+  if (unique_ast_entry_up->m_is_forward_declaration) {
+unique_ast_entry_up->m_die = die;
+unique_ast_entry_up->m_byte_size = byte_size;
+unique_ast_entry_up->m_declaration = unique_decl;
+unique_ast_entry_up->m_is_forward_declaration = false;
+// Need to update Type ID to refer to the definition DIE. because
+// it's used in ParseSubroutine to determine if we need to copy cxx
+// method types from a declaration DIE to this definition DIE.
+type_sp->SetID(die.GetID());
+clang_type = type_sp->GetForwardCompilerType();
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);
+
+CompilerType compiler_type_no_qualifiers =
+ClangUtil::RemoveFastQualifiers(clang_type);
+auto result = dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(

ZequanWu wrote:

It doesn't compile, because the value of the map is a DIERef, which doesn't 
have zero-argument constructor (can only constructed with 1 or 3 arguments). 
DIERef is a reference to a parsed DIE, so adding a zero-argument constructor 
for it doesn't quite make sense.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-06 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/6] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840af..7ad661c9a9d49b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-06 Thread Pavel Labath via lldb-commits



@@ -1667,13 +1791,40 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
 }
 
 if (dwarf->GetUniqueDWARFASTTypeMap().Find(
-unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
-*unique_ast_entry_up)) {
+unique_typename, die, unique_decl, byte_size,
+attrs.is_forward_declaration, *unique_ast_entry_up)) {
   type_sp = unique_ast_entry_up->m_type_sp;
   if (type_sp) {
 dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
 LinkDeclContextToDIE(
 GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+if (!attrs.is_forward_declaration) {
+  // If the DIE being parsed in this function is a definition and the
+  // entry in the map is a declaration, then we need to update the 
entry
+  // to point to the definition DIE.
+  if (unique_ast_entry_up->m_is_forward_declaration) {
+unique_ast_entry_up->m_die = die;
+unique_ast_entry_up->m_byte_size = byte_size;
+unique_ast_entry_up->m_declaration = unique_decl;
+unique_ast_entry_up->m_is_forward_declaration = false;
+// Need to update Type ID to refer to the definition DIE. because
+// it's used in ParseSubroutine to determine if we need to copy cxx
+// method types from a declaration DIE to this definition DIE.
+type_sp->SetID(die.GetID());
+clang_type = type_sp->GetForwardCompilerType();
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);
+
+CompilerType compiler_type_no_qualifiers =
+ClangUtil::RemoveFastQualifiers(clang_type);
+auto result = dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(

labath wrote:

The if below is just implementing overwrite semantics, but maps already have 
APIs for that. How about 
`dwarf->GetForwardDeclCompilerTypeToDIE()[compiler_type_no_qualifiers.GetOpaqueQualType()]
 = *die.GetDIERef()` ?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-06 Thread Pavel Labath via lldb-commits



@@ -154,6 +154,27 @@ static bool TagIsRecordType(dw_tag_t tag) {
   }
 }
 
+static bool
+IsForwardDeclaration(const lldb_private::plugin::dwarf::DWARFDIE ,

labath wrote:

This shouldn't be necessary here, as this file has `using namespace 
lldb_private::plugin::dwarf` at the top.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits

clayborg wrote:

> The tests your described testing this change doesn't break things by delaying 
> definition DIE searching, which I think is already covered by existing tests 
> (created for other purposes, but also covers this case). I was thinking about 
> testing the definition DIE searching is actually delayed. Maybe there isn't a 
> way to do it.

You could enable logging and check for specific logging after steps. In the 
test I described above if you just print the "Foo *foo" variable, it won't need 
to complete the definition, you could check for logging, and then if you print 
"*foo", then it should complete the definition and you should see logging. Not 
sure if that is what you meant

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/5] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840af..7ad661c9a9d49b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


ZequanWu wrote:

> > > > > Is any of it testable?
> > > > 
> > > > 
> > > > Good question. Though this is mostly meant to be "NFC" (with very large 
> > > > quotes), I can imagine us doing something like forcing the parsing of a 
> > > > specific type (`type lookup ` ?), and then checking that the 
> > > > module ast (`image dump ast`) does _not_ contain specific types -- as 
> > > > that's basically what we're trying to achieve.
> > > 
> > > 
> > > Yea that could work. But if it's going to be very painful or fragile to 
> > > test then don't let that hold back the PR
> > 
> > 
> > In terms of testing, since this only delays definition DIE searching not 
> > type completion, we need to construct a test so that lldb finds the 
> > declaration DIE first without trigger a type completion on it and somehow 
> > test the incomplete type. The first part is tricky. I'm not sure how to 
> > achieve it.
> 
> You should be able to make a test case with two files. One file contains the 
> class definition and the other uses only a forward declaration. You could 
> test by running the binary stopping only in the one with the forward 
> declaration. So file 1 would be something like `foo.cpp` containing
> 
> ```
> struct Foo { 
>   int value;
>   Foo(int v): value(v) {}
> };
> 
> Foo *CreateFoo() {
>   return new Foo(1);
> }
> ```
> 
> Then having `main.cpp` contain:
> 
> ```
> struct Foo; // Forward declare Foo
> // Prototypes that don't need Foo definition
> Foo *CreateFoo();
> 
> int main() {
>   Foo *foo = CreateFoo();
>   return 0; // Breakpoint 1 here
> }
> ```
> 
> Then run to "Breakpoint 1 here" and then run `frame variable foo`. Foo won't 
> need to be completed for this. But if you then run `frame variable *foo`, it 
> should cause the type to be completed and show the instance variable 
> correctly.

The tests your described testing this change doesn't break things by delaying 
definition DIE searching, which I think is already covered by existing tests 
(created for other purposes, but also covers this case). I was thinking about 
testing the definition DIE searching is actually delayed. Maybe there isn't a 
way to do it.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits

clayborg wrote:

> > > > Is any of it testable?
> > > 
> > > 
> > > Good question. Though this is mostly meant to be "NFC" (with very large 
> > > quotes), I can imagine us doing something like forcing the parsing of a 
> > > specific type (`type lookup ` ?), and then checking that the 
> > > module ast (`image dump ast`) does _not_ contain specific types -- as 
> > > that's basically what we're trying to achieve.
> > 
> > 
> > Yea that could work. But if it's going to be very painful or fragile to 
> > test then don't let that hold back the PR
> 
> In terms of testing, since this only delays definition DIE searching not type 
> completion, we need to construct a test so that lldb finds the declaration 
> DIE first without trigger a type completion on it and somehow test the 
> incomplete type. The first part is tricky. I'm not sure how to achieve it.

You should be able to make a test case with two files. One file contains the 
class definition and the other uses only a forward declaration. You could test 
by running the binary stopping only in the one with the forward declaration. So 
file 1 would be something like `foo.cpp` containing
```
struct Foo { 
  int value;
  Foo(int v): value(v) {}
};

Foo *CreateFoo() {
  return new Foo(1);
}

```
Then having `main.cpp` contain:
```
struct Foo; // Forward declare Foo
// Prototypes that don't need Foo definition
void Increment(Foo *); 
Foo *CreateFoo();

int main() {
  Foo *foo = CreateFoo();
  return 0; // Breakpoint 1 here
}
```
Then run to "Breakpoint 1 here" and then run `frame variable foo`. Foo won't 
need to be completed for this. But if you then run `frame variable *foo`, it 
should cause the type to be completed and show the instance variable correctly.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits



@@ -154,6 +154,27 @@ static bool TagIsRecordType(dw_tag_t tag) {
   }
 }
 
+static bool
+IsForwardDeclaration(const lldb_private::plugin::dwarf::DWARFDIE ,
+ const ParsedDWARFTypeAttributes ,
+ LanguageType cu_language) {
+  if (attrs.byte_size && *attrs.byte_size == 0 && attrs.name &&

clayborg wrote:

Do we want to check `attrs.is_forward_declaration` before doing any of this and 
quick return? Then only do this more expensive `if` check if 
`attrs.is_forward_declaration` is false?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits


https://github.com/clayborg commented:

Looks pretty good to me as long as the test suite is happy.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits



@@ -1631,27 +1631,34 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
 return true;
   }
 
-  DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
-  if (dwarf_die) {
-// Once we start resolving this type, remove it from the forward
-// declaration map in case anyone child members or other types require this
-// type to get resolved. The type will get resolved when all of the calls
-// to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
-GetForwardDeclCompilerTypeToDIE().erase(die_it);
-
-Type *type = GetDIEToType().lookup(dwarf_die.GetDIE());
+  // Once we start resolving this type, remove it from the forward
+  // declaration map in case anyone child members or other types require this

clayborg wrote:

s/anyone/anyone's/ to fix bad previous comment

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Greg Clayton via lldb-commits


https://github.com/clayborg edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits

ZequanWu wrote:

> > > Is any of it testable?
> > 
> > 
> > Good question. Though this is mostly meant to be "NFC" (with very large 
> > quotes), I can imagine us doing something like forcing the parsing of a 
> > specific type (`type lookup ` ?), and then checking that the 
> > module ast (`image dump ast`) does _not_ contain specific types -- as 
> > that's basically what we're trying to achieve.
> 
> Yea that could work. But if it's going to be very painful or fragile to test 
> then don't let that hold back the PR

In terms of testing, since this only delays definition DIE searching not type 
completion, we need to construct a test so that lldb finds the declaration DIE 
first without trigger a type completion on it and somehow test the incomplete 
type. The first part is tricky. I'm not sure how to achieve it.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -1664,13 +1793,40 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
 }
 
 if (dwarf->GetUniqueDWARFASTTypeMap().Find(
-unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
-*unique_ast_entry_up)) {
+unique_typename, die, unique_decl, byte_size,
+attrs.is_forward_declaration, *unique_ast_entry_up)) {
   type_sp = unique_ast_entry_up->m_type_sp;
   if (type_sp) {
 dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
 LinkDeclContextToDIE(
 GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+if (!attrs.is_forward_declaration) {
+  // If the parameter DIE is definition and the entry in the map is

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -1921,38 +1970,33 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
   GetClangASTImporter().SetRecordLayout(record_decl, layout);
 }
   }
-} else if (clang_type_was_created) {
-  // Start the definition if the class is not objective C since the
-  // underlying decls respond to isCompleteDefinition(). Objective
-  // C decls don't respond to isCompleteDefinition() so we can't
-  // start the declaration definition right away. For C++
-  // class/union/structs we want to start the definition in case the
-  // class is needed as the declaration context for a contained class
-  // or type without the need to complete that type..
-
-  if (attrs.class_language != eLanguageTypeObjC &&
-  attrs.class_language != eLanguageTypeObjC_plus_plus)
-TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
-  // Leave this as a forward declaration until we need to know the
-  // details of the type. lldb_private::Type will automatically call
-  // the SymbolFile virtual function
-  // "SymbolFileDWARF::CompleteType(Type *)" When the definition
-  // needs to be defined.
-  assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
- ClangUtil::RemoveFastQualifiers(clang_type)
- .GetOpaqueQualType()) &&
- "Type already in the forward declaration map!");
-  // Can't assume m_ast.GetSymbolFile() is actually a
-  // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
-  // binaries.
-  dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
-  ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
-  *die.GetDIERef());
-  m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
 }
+// Start the definition if the class is not objective C since the
+// underlying decls respond to isCompleteDefinition(). Objective
+// C decls don't respond to isCompleteDefinition() so we can't
+// start the declaration definition right away. For C++
+// class/union/structs we want to start the definition in case the
+// class is needed as the declaration context for a contained class
+// or type without the need to complete that type..
+
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);
   }
 
+  // Leave this as a forward declaration until we need to know the

ZequanWu wrote:

I didn't modify the original comments. It doesn't make sense to me as well. I 
append "If this is a declaration DIE" in front of this comment. 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu commented:

> Though this is mostly meant to be "NFC" (with very large quotes)

Yeah, this is mostly "NFC". A noticeable difference is we now set the type 
created from declaration with `TypeSystemClang::SetHasExternalStorage` without 
knowing if there's a definition or not. Based on my knowledge, it simply tells 
clang that the type can be completed by external AST source and ultimately 
calls `SymbolFileDWARF::CompleteType` to complete it. If there isn't one, we 
just fail complete it. Maybe we should also force completion of the type in 
this case?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -108,6 +108,9 @@ class DWARFASTParserClang : public 
lldb_private::plugin::dwarf::DWARFASTParser {
   lldb_private::ConstString GetDIEClassTemplateParams(
   const lldb_private::plugin::dwarf::DWARFDIE ) override;

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -1921,38 +1970,33 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
   GetClangASTImporter().SetRecordLayout(record_decl, layout);
 }
   }
-} else if (clang_type_was_created) {
-  // Start the definition if the class is not objective C since the
-  // underlying decls respond to isCompleteDefinition(). Objective
-  // C decls don't respond to isCompleteDefinition() so we can't
-  // start the declaration definition right away. For C++
-  // class/union/structs we want to start the definition in case the
-  // class is needed as the declaration context for a contained class
-  // or type without the need to complete that type..
-
-  if (attrs.class_language != eLanguageTypeObjC &&
-  attrs.class_language != eLanguageTypeObjC_plus_plus)
-TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
-  // Leave this as a forward declaration until we need to know the
-  // details of the type. lldb_private::Type will automatically call
-  // the SymbolFile virtual function
-  // "SymbolFileDWARF::CompleteType(Type *)" When the definition
-  // needs to be defined.
-  assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
- ClangUtil::RemoveFastQualifiers(clang_type)
- .GetOpaqueQualType()) &&
- "Type already in the forward declaration map!");
-  // Can't assume m_ast.GetSymbolFile() is actually a
-  // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
-  // binaries.
-  dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
-  ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
-  *die.GetDIERef());
-  m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
 }
+// Start the definition if the class is not objective C since the
+// underlying decls respond to isCompleteDefinition(). Objective
+// C decls don't respond to isCompleteDefinition() so we can't
+// start the declaration definition right away. For C++
+// class/union/structs we want to start the definition in case the
+// class is needed as the declaration context for a contained class
+// or type without the need to complete that type..
+
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);

ZequanWu wrote:

Oh, I didn't notice it. Moved this into an else branch.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -60,6 +60,12 @@ class DWARFASTParser {
 
   virtual ConstString GetDIEClassTemplateParams(const DWARFDIE ) = 0;
 
+  // Return true if we found the definition DIE for it. is_forward_declaration
+  // is set to true if the parameter die is a declaration.
+  virtual bool
+  FindDefinitionDIE(const lldb_private::plugin::dwarf::DWARFDIE ,

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const 
DWARFDIE ) {
   return qualified_name;
 }
 

ZequanWu wrote:

Yes, it does more than finding the DIE. Updated to return `Type*` and renamed 
to `FindDefinitionTypeForDIE`.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -16,60 +16,65 @@ using namespace lldb_private::plugin::dwarf;
 bool UniqueDWARFASTTypeList::Find(const DWARFDIE ,
   const lldb_private::Declaration ,
   const int32_t byte_size,
+  bool is_forward_declaration,
   UniqueDWARFASTType ) const {
   for (const UniqueDWARFASTType  : m_collection) {
 // Make sure the tags match
 if (udt.m_die.Tag() == die.Tag()) {
-  // Validate byte sizes of both types only if both are valid.
-  if (udt.m_byte_size < 0 || byte_size < 0 ||
-  udt.m_byte_size == byte_size) {
-// Make sure the file and line match
-if (udt.m_declaration == decl) {
-  // The type has the same name, and was defined on the same file and
-  // line. Now verify all of the parent DIEs match.
-  DWARFDIE parent_arg_die = die.GetParent();
-  DWARFDIE parent_pos_die = udt.m_die.GetParent();
-  bool match = true;
-  bool done = false;
-  while (!done && match && parent_arg_die && parent_pos_die) {
-const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
-const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
-if (parent_arg_tag == parent_pos_tag) {
-  switch (parent_arg_tag) {
-  case DW_TAG_class_type:
-  case DW_TAG_structure_type:
-  case DW_TAG_union_type:
-  case DW_TAG_namespace: {
-const char *parent_arg_die_name = parent_arg_die.GetName();
-if (parent_arg_die_name ==
-nullptr) // Anonymous (i.e. no-name) struct
-{
+  // If they are not both definition DIEs or both declaration DIEs, then
+  // don't check for byte size and declaration location, because 
declaration
+  // DIEs usually don't have those info.
+  bool matching_size_declaration =
+  udt.m_is_forward_declaration != is_forward_declaration
+  ? true
+  : (udt.m_byte_size < 0 || byte_size < 0 ||
+ udt.m_byte_size == byte_size) &&
+udt.m_declaration == decl;
+  if (matching_size_declaration) {

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const 
DWARFDIE ) {
   return qualified_name;
 }
 
+bool DWARFASTParserClang::FindDefinitionDIE(const DWARFDIE ,
+bool _forward_declaration) {

ZequanWu wrote:

I was about to use it in SymbolFileDWARF::CompleteType: 
https://github.com/llvm/llvm-project/pull/90663/files#diff-edef3a65d5d569bbb75a4158d35b827aa5d42ee03ccd3b0c1d4f354afa12210cR1645.
 But with some refactor change, it's no longer useful anymore. 

Removed.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits



@@ -249,11 +270,10 @@ static void ForcefullyCompleteType(CompilerType type) {
 /// This function serves a similar purpose as RequireCompleteType above, but it
 /// avoids completing the type if it is not immediately necessary. It only
 /// ensures we _can_ complete the type later.
-static void PrepareContextToReceiveMembers(TypeSystemClang ,
-   ClangASTImporter _importer,
-   clang::DeclContext *decl_ctx,
-   DWARFDIE die,
-   const char *type_name_cstr) {
+void DWARFASTParserClang::PrepareContextToReceiveMembers(
+TypeSystemClang , clang::DeclContext *decl_ctx,

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/4] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840af..7ad661c9a9d49b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits


Michael137 wrote:

> > Is any of it testable?
> 
> Good question. Though this is mostly meant to be "NFC" (with very large 
> quotes), I can imagine us doing something like forcing the parsing of a 
> specific type (`type lookup ` ?), and then checking that the 
> module ast (`image dump ast`) does _not_ contain specific types -- as that's 
> basically what we're trying to achieve.

Yea that could work. But if it's going to be very painful or fragile to test 
then don't let that hold back the PR

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits


https://github.com/Michael137 edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits


labath wrote:

> Is any of it testable?

Good question. Though this is mostly meant to be "NFC" (with very large 
quotes), I can imagine us doing something like forcing the parsing of a 
specific type (`type lookup ` ?), and then checking that the module 
ast (`image dump ast`) does *not* contain specific types -- as that's basically 
what we're trying to achieve.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits



@@ -16,60 +16,65 @@ using namespace lldb_private::plugin::dwarf;
 bool UniqueDWARFASTTypeList::Find(const DWARFDIE ,
   const lldb_private::Declaration ,
   const int32_t byte_size,
+  bool is_forward_declaration,
   UniqueDWARFASTType ) const {
   for (const UniqueDWARFASTType  : m_collection) {
 // Make sure the tags match
 if (udt.m_die.Tag() == die.Tag()) {
-  // Validate byte sizes of both types only if both are valid.
-  if (udt.m_byte_size < 0 || byte_size < 0 ||
-  udt.m_byte_size == byte_size) {
-// Make sure the file and line match
-if (udt.m_declaration == decl) {
-  // The type has the same name, and was defined on the same file and
-  // line. Now verify all of the parent DIEs match.
-  DWARFDIE parent_arg_die = die.GetParent();
-  DWARFDIE parent_pos_die = udt.m_die.GetParent();
-  bool match = true;
-  bool done = false;
-  while (!done && match && parent_arg_die && parent_pos_die) {
-const dw_tag_t parent_arg_tag = parent_arg_die.Tag();
-const dw_tag_t parent_pos_tag = parent_pos_die.Tag();
-if (parent_arg_tag == parent_pos_tag) {
-  switch (parent_arg_tag) {
-  case DW_TAG_class_type:
-  case DW_TAG_structure_type:
-  case DW_TAG_union_type:
-  case DW_TAG_namespace: {
-const char *parent_arg_die_name = parent_arg_die.GetName();
-if (parent_arg_die_name ==
-nullptr) // Anonymous (i.e. no-name) struct
-{
+  // If they are not both definition DIEs or both declaration DIEs, then
+  // don't check for byte size and declaration location, because 
declaration
+  // DIEs usually don't have those info.
+  bool matching_size_declaration =
+  udt.m_is_forward_declaration != is_forward_declaration
+  ? true
+  : (udt.m_byte_size < 0 || byte_size < 0 ||
+ udt.m_byte_size == byte_size) &&
+udt.m_declaration == decl;
+  if (matching_size_declaration) {

Michael137 wrote:

For readability, can we do:
```
if (!match_size_declaration)
  continue;
```
?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits


https://github.com/Michael137 commented:

The idea makes sense and I like that we could factor things out of 
`ParseStructureLikeDIE`, so generally LGTM (module Pavel's comments). Is any of 
it testable?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits


https://github.com/Michael137 edited 
https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Michael Buch via lldb-commits



@@ -1664,13 +1793,40 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
 }
 
 if (dwarf->GetUniqueDWARFASTTypeMap().Find(
-unique_typename, die, unique_decl, attrs.byte_size.value_or(-1),
-*unique_ast_entry_up)) {
+unique_typename, die, unique_decl, byte_size,
+attrs.is_forward_declaration, *unique_ast_entry_up)) {
   type_sp = unique_ast_entry_up->m_type_sp;
   if (type_sp) {
 dwarf->GetDIEToType()[die.GetDIE()] = type_sp.get();
 LinkDeclContextToDIE(
 GetCachedClangDeclContextForDIE(unique_ast_entry_up->m_die), die);
+if (!attrs.is_forward_declaration) {
+  // If the parameter DIE is definition and the entry in the map is

Michael137 wrote:

I find `parameter DIE` a bit confusing. Made me think of templates or 
functions. Could we just reference the parameter, e.g., `If the 'die' being 
parsed is a definition ...` or something like that

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -60,6 +60,12 @@ class DWARFASTParser {
 
   virtual ConstString GetDIEClassTemplateParams(const DWARFDIE ) = 0;
 
+  // Return true if we found the definition DIE for it. is_forward_declaration
+  // is set to true if the parameter die is a declaration.
+  virtual bool
+  FindDefinitionDIE(const lldb_private::plugin::dwarf::DWARFDIE ,

labath wrote:

Delete lldb_private::plugin::dwarf::

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -1921,38 +1970,33 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
   GetClangASTImporter().SetRecordLayout(record_decl, layout);
 }
   }
-} else if (clang_type_was_created) {
-  // Start the definition if the class is not objective C since the
-  // underlying decls respond to isCompleteDefinition(). Objective
-  // C decls don't respond to isCompleteDefinition() so we can't
-  // start the declaration definition right away. For C++
-  // class/union/structs we want to start the definition in case the
-  // class is needed as the declaration context for a contained class
-  // or type without the need to complete that type..
-
-  if (attrs.class_language != eLanguageTypeObjC &&
-  attrs.class_language != eLanguageTypeObjC_plus_plus)
-TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
-  // Leave this as a forward declaration until we need to know the
-  // details of the type. lldb_private::Type will automatically call
-  // the SymbolFile virtual function
-  // "SymbolFileDWARF::CompleteType(Type *)" When the definition
-  // needs to be defined.
-  assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
- ClangUtil::RemoveFastQualifiers(clang_type)
- .GetOpaqueQualType()) &&
- "Type already in the forward declaration map!");
-  // Can't assume m_ast.GetSymbolFile() is actually a
-  // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
-  // binaries.
-  dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
-  ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
-  *die.GetDIERef());
-  m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
 }
+// Start the definition if the class is not objective C since the
+// underlying decls respond to isCompleteDefinition(). Objective
+// C decls don't respond to isCompleteDefinition() so we can't
+// start the declaration definition right away. For C++
+// class/union/structs we want to start the definition in case the
+// class is needed as the declaration context for a contained class
+// or type without the need to complete that type..
+
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);
   }
 
+  // Leave this as a forward declaration until we need to know the

labath wrote:

I'm having trouble reconciling this comment with the one above (line 1974). How 
can we leave this as a forward declaration if we have already (conditionally, 
if we are processing a definition DIE of a non-objc type) started its 
definition (line 1984). What exactly is this trying to say?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -108,6 +108,9 @@ class DWARFASTParserClang : public 
lldb_private::plugin::dwarf::DWARFASTParser {
   lldb_private::ConstString GetDIEClassTemplateParams(
   const lldb_private::plugin::dwarf::DWARFDIE ) override;

labath wrote:

delete `lldb_private::plugin::dwarf::` (and elsewhere in this file)

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -1921,38 +1970,33 @@ DWARFASTParserClang::ParseStructureLikeDIE(const 
SymbolContext ,
   GetClangASTImporter().SetRecordLayout(record_decl, layout);
 }
   }
-} else if (clang_type_was_created) {
-  // Start the definition if the class is not objective C since the
-  // underlying decls respond to isCompleteDefinition(). Objective
-  // C decls don't respond to isCompleteDefinition() so we can't
-  // start the declaration definition right away. For C++
-  // class/union/structs we want to start the definition in case the
-  // class is needed as the declaration context for a contained class
-  // or type without the need to complete that type..
-
-  if (attrs.class_language != eLanguageTypeObjC &&
-  attrs.class_language != eLanguageTypeObjC_plus_plus)
-TypeSystemClang::StartTagDeclarationDefinition(clang_type);
-
-  // Leave this as a forward declaration until we need to know the
-  // details of the type. lldb_private::Type will automatically call
-  // the SymbolFile virtual function
-  // "SymbolFileDWARF::CompleteType(Type *)" When the definition
-  // needs to be defined.
-  assert(!dwarf->GetForwardDeclCompilerTypeToDIE().count(
- ClangUtil::RemoveFastQualifiers(clang_type)
- .GetOpaqueQualType()) &&
- "Type already in the forward declaration map!");
-  // Can't assume m_ast.GetSymbolFile() is actually a
-  // SymbolFileDWARF, it can be a SymbolFileDWARFDebugMap for Apple
-  // binaries.
-  dwarf->GetForwardDeclCompilerTypeToDIE().try_emplace(
-  ClangUtil::RemoveFastQualifiers(clang_type).GetOpaqueQualType(),
-  *die.GetDIERef());
-  m_ast.SetHasExternalStorage(clang_type.GetOpaqueQualType(), true);
 }
+// Start the definition if the class is not objective C since the
+// underlying decls respond to isCompleteDefinition(). Objective
+// C decls don't respond to isCompleteDefinition() so we can't
+// start the declaration definition right away. For C++
+// class/union/structs we want to start the definition in case the
+// class is needed as the declaration context for a contained class
+// or type without the need to complete that type..
+
+if (attrs.class_language != eLanguageTypeObjC &&
+attrs.class_language != eLanguageTypeObjC_plus_plus)
+  TypeSystemClang::StartTagDeclarationDefinition(clang_type);

labath wrote:

Is it a problem that `clang_type` can already have its definition started (and 
completed) on line 1944 (if the DIE has no children)? Should this maybe be in 
some sort of an else branch?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const 
DWARFDIE ) {
   return qualified_name;
 }
 
+bool DWARFASTParserClang::FindDefinitionDIE(const DWARFDIE ,
+bool _forward_declaration) {

labath wrote:

I'm having trouble finding where is this by-ref argument actually used in the 
caller. Can we get rid of it (perhaps by moving the `IsForwardDeclaration` call 
to the caller)?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits


https://github.com/labath commented:

Modulo comments, this makes sense to me (as much as that can ever be said about 
this code), but it could definitely use a second (third?) pair of eyes. 
Michael, what do you make of this?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -249,11 +270,10 @@ static void ForcefullyCompleteType(CompilerType type) {
 /// This function serves a similar purpose as RequireCompleteType above, but it
 /// avoids completing the type if it is not immediately necessary. It only
 /// ensures we _can_ complete the type later.
-static void PrepareContextToReceiveMembers(TypeSystemClang ,
-   ClangASTImporter _importer,
-   clang::DeclContext *decl_ctx,
-   DWARFDIE die,
-   const char *type_name_cstr) {
+void DWARFASTParserClang::PrepareContextToReceiveMembers(
+TypeSystemClang , clang::DeclContext *decl_ctx,

labath wrote:

This arg is not necessary now that this is a member

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits


https://github.com/labath edited https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-03 Thread Pavel Labath via lldb-commits



@@ -1632,6 +1669,96 @@ DWARFASTParserClang::GetCPlusPlusQualifiedName(const 
DWARFDIE ) {
   return qualified_name;
 }
 

labath wrote:

I am bothered by this name. I would expect that something called 
`FindDefinitionDIE` returns a DWARFDIE (or something along those lines).  
Instead it returns bool, and appears to do a lot more than simply finding a DIE 
(it constructs the full clang ast for it, iiuc).

How about, mirroring the SymbolFileDWARF apis this wraps, we call this 
something like `FindDefinitionTypeForDIE` (and then actually have the function 
return the parsed Type object)?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Zequan Wu via lldb-commits



@@ -1631,13 +1631,19 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
 return true;
   }
 
-  DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
+  DWARFDIE dwarf_die = FindDefinitionDIE(GetDIE(die_it->getSecond()));
   if (dwarf_die) {
 // Once we start resolving this type, remove it from the forward
 // declaration map in case anyone child members or other types require this
 // type to get resolved. The type will get resolved when all of the calls
 // to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
-GetForwardDeclCompilerTypeToDIE().erase(die_it);
+// Need to get a new iterator because FindDefinitionDIE might add new

ZequanWu wrote:

Now, we change `m_forward_decl_compiler_type_to_die` to be updated with 
definition DIE. So, we may need to erase twice, because the first erase is 
always necessary if we failed to find definition DIE for it. The second erase 
is because calling FindDefinitionDIE might add new entry to the definition DIE 
because the first one removed it.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Zequan Wu via lldb-commits



@@ -1654,6 +1660,99 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
   return false;
 }
 
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE ) {
+  auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+  if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+return GetDIE(def_die_it->getSecond());
+
+  ParsedDWARFTypeAttributes attrs(die);
+  const dw_tag_t tag = die.Tag();
+  TypeSP type_sp;
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  if (log) {
+GetObjectFile()->GetModule()->LogMessage(
+log,
+"SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+"forward declaration DIE, trying to find definition DIE",
+static_cast(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+attrs.name.GetCString());
+  }
+  // We haven't parse definition die for this type, starting to search for it.
+  // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()
+  // map will have the new mapping from this declaration die to definition die.
+  if (attrs.class_language == eLanguageTypeObjC ||

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Zequan Wu via lldb-commits



@@ -533,9 +540,16 @@ class SymbolFileDWARF : public SymbolFileCommon {
   NameToOffsetMap m_function_scope_qualified_name_map;
   std::unique_ptr m_ranges;
   UniqueDWARFASTTypeMap m_unique_ast_type_map;
+  // A map from DIE to lldb_private::Type. For record type, the key might be
+  // either declaration DIE or definition DIE.
   DIEToTypePtr m_die_to_type;
   DIEToVariableSP m_die_to_variable_sp;
+  // A map from CompilerType to the struct/class/union/enum DIE (might be a
+  // declaration or a definition) that is used to construct it.
   CompilerTypeToDIE m_forward_decl_compiler_type_to_die;
+  // A map from a struct/class/union/enum DIE (might be a declaration or a
+  // definition) to its definition DIE.
+  DIEToDIE m_die_to_def_die;

ZequanWu wrote:

Done.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Zequan Wu via lldb-commits


https://github.com/ZequanWu updated 
https://github.com/llvm/llvm-project/pull/90663

>From 4e83099b593e66f12dc21be5fbac5279e03e Mon Sep 17 00:00:00 2001
From: Zequan Wu 
Date: Tue, 30 Apr 2024 16:23:11 -0400
Subject: [PATCH 1/3] [lldb][DWARF] Delay struct/class/union definition DIE
 searching when parsing declaration DIEs.

---
 .../SymbolFile/DWARF/DWARFASTParserClang.cpp  | 270 +++---
 .../SymbolFile/DWARF/SymbolFileDWARF.cpp  | 105 ++-
 .../SymbolFile/DWARF/SymbolFileDWARF.h|  14 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp   |   5 +
 .../SymbolFile/DWARF/SymbolFileDWARFDwo.h |   2 +
 .../SymbolFile/DWARF/UniqueDWARFASTType.cpp   |  95 +++---
 .../SymbolFile/DWARF/UniqueDWARFASTType.h |  21 +-
 7 files changed, 296 insertions(+), 216 deletions(-)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index bea11e0e3840af..7ad661c9a9d49b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -252,7 +252,8 @@ static void ForcefullyCompleteType(CompilerType type) {
 static void PrepareContextToReceiveMembers(TypeSystemClang ,
ClangASTImporter _importer,
clang::DeclContext *decl_ctx,
-   DWARFDIE die,
+   const DWARFDIE _ctx_die,
+   const DWARFDIE ,
const char *type_name_cstr) {
   auto *tag_decl_ctx = clang::dyn_cast(decl_ctx);
   if (!tag_decl_ctx)
@@ -279,6 +280,13 @@ static void PrepareContextToReceiveMembers(TypeSystemClang 
,
 type_name_cstr ? type_name_cstr : "", die.GetOffset());
   }
 
+  // By searching for the definition DIE of the decl_ctx type, we will either:
+  // 1. Found the the definition DIE and start its definition with
+  // TypeSystemClang::StartTagDeclarationDefinition.
+  // 2. Unable to find it, then need to forcefully complete it.
+  die.GetDWARF()->FindDefinitionDIE(decl_ctx_die);
+  if (tag_decl_ctx->isCompleteDefinition() || tag_decl_ctx->isBeingDefined())
+return;
   // We don't have a type definition and/or the import failed. We must
   // forcefully complete the type to avoid crashes.
   ForcefullyCompleteType(type);
@@ -620,10 +628,11 @@ DWARFASTParserClang::ParseTypeModifier(const 
SymbolContext ,
   if (tag == DW_TAG_typedef) {
 // DeclContext will be populated when the clang type is materialized in
 // Type::ResolveCompilerType.
-PrepareContextToReceiveMembers(
-m_ast, GetClangASTImporter(),
-GetClangDeclContextContainingDIE(die, nullptr), die,
-attrs.name.GetCString());
+DWARFDIE decl_ctx_die;
+clang::DeclContext *decl_ctx =
+GetClangDeclContextContainingDIE(die, _ctx_die);
+PrepareContextToReceiveMembers(m_ast, GetClangASTImporter(), decl_ctx,
+   decl_ctx_die, die, attrs.name.GetCString());
 
 if (attrs.type.IsValid()) {
   // Try to parse a typedef from the (DWARF embedded in the) Clang
@@ -1100,32 +1109,6 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
 // struct and see if this is actually a C++ method
 Type *class_type = dwarf->ResolveType(decl_ctx_die);
 if (class_type) {
-  if (class_type->GetID() != decl_ctx_die.GetID() ||
-  IsClangModuleFwdDecl(decl_ctx_die)) {
-
-// We uniqued the parent class of this function to another
-// class so we now need to associate all dies under
-// "decl_ctx_die" to DIEs in the DIE for "class_type"...
-DWARFDIE class_type_die = dwarf->GetDIE(class_type->GetID());
-
-if (class_type_die) {
-  std::vector failures;
-
-  CopyUniqueClassMethodTypes(decl_ctx_die, class_type_die,
- class_type, failures);
-
-  // FIXME do something with these failures that's
-  // smarter than just dropping them on the ground.
-  // Unfortunately classes don't like having stuff added
-  // to them after their definitions are complete...
-
-  Type *type_ptr = dwarf->GetDIEToType()[die.GetDIE()];
-  if (type_ptr && type_ptr != DIE_IS_BEING_PARSED) {
-return type_ptr->shared_from_this();
-  }
-}
-  }
-
   if (attrs.specification.IsValid()) {
 // We have a specification which we are going to base our
 // function prototype off of, so we need this type to be
@@ -1260,6 +1243,39 @@ DWARFASTParserClang::ParseSubroutine(const DWARFDIE ,
   }
 }
   }
+  // By here, we should have already completed the c++ class_type
+  //

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -1654,6 +1660,99 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
   return false;
 }
 
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE ) {
+  auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+  if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+return GetDIE(def_die_it->getSecond());
+
+  ParsedDWARFTypeAttributes attrs(die);
+  const dw_tag_t tag = die.Tag();
+  TypeSP type_sp;
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  if (log) {
+GetObjectFile()->GetModule()->LogMessage(
+log,
+"SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+"forward declaration DIE, trying to find definition DIE",
+static_cast(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+attrs.name.GetCString());
+  }
+  // We haven't parse definition die for this type, starting to search for it.
+  // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()
+  // map will have the new mapping from this declaration die to definition die.
+  if (attrs.class_language == eLanguageTypeObjC ||

labath wrote:

I realize this is a bit fuzzy, and there are already some arguably 
clang-specific pieces of code in SymbolFileDWARF, but the main reason I am 
bringing this up is because this code (some of it at least) has previously been 
in DWARFASTParserClang, and now it isn't -- which seems like a regression to me.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -1654,6 +1660,99 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
   return false;
 }
 
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE ) {
+  auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+  if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+return GetDIE(def_die_it->getSecond());
+
+  ParsedDWARFTypeAttributes attrs(die);
+  const dw_tag_t tag = die.Tag();
+  TypeSP type_sp;
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  if (log) {
+GetObjectFile()->GetModule()->LogMessage(
+log,
+"SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+"forward declaration DIE, trying to find definition DIE",
+static_cast(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+attrs.name.GetCString());
+  }
+  // We haven't parse definition die for this type, starting to search for it.
+  // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()
+  // map will have the new mapping from this declaration die to definition die.
+  if (attrs.class_language == eLanguageTypeObjC ||

labath wrote:

Definitely anything that is gated on `language == eLanguageType*C*` checks, but 
a case could also be made that the whole declaration-to-definition mapping 
should be within the purview of the DWARFASTParser instance responsible for the 
specific language. Having the logic split between an ASTParser (a 
language-specific class) and SymbolFileDWARF (language-agnostic) forces all 
languages to work in the same way. I don't know how much of a problem would 
that be in practice, but it would definitely make the code nicer (single 
responsibility principle, and all) if the logic lived in a central place.

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Zequan Wu via lldb-commits



@@ -1654,6 +1660,99 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
   return false;
 }
 
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE ) {
+  auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+  if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+return GetDIE(def_die_it->getSecond());
+
+  ParsedDWARFTypeAttributes attrs(die);
+  const dw_tag_t tag = die.Tag();
+  TypeSP type_sp;
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  if (log) {
+GetObjectFile()->GetModule()->LogMessage(
+log,
+"SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+"forward declaration DIE, trying to find definition DIE",
+static_cast(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+attrs.name.GetCString());
+  }
+  // We haven't parse definition die for this type, starting to search for it.
+  // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()
+  // map will have the new mapping from this declaration die to definition die.
+  if (attrs.class_language == eLanguageTypeObjC ||

ZequanWu wrote:

Which part of this function is clang-specific code? 

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -533,9 +540,16 @@ class SymbolFileDWARF : public SymbolFileCommon {
   NameToOffsetMap m_function_scope_qualified_name_map;
   std::unique_ptr m_ranges;
   UniqueDWARFASTTypeMap m_unique_ast_type_map;
+  // A map from DIE to lldb_private::Type. For record type, the key might be
+  // either declaration DIE or definition DIE.
   DIEToTypePtr m_die_to_type;
   DIEToVariableSP m_die_to_variable_sp;
+  // A map from CompilerType to the struct/class/union/enum DIE (might be a
+  // declaration or a definition) that is used to construct it.
   CompilerTypeToDIE m_forward_decl_compiler_type_to_die;
+  // A map from a struct/class/union/enum DIE (might be a declaration or a
+  // definition) to its definition DIE.
+  DIEToDIE m_die_to_def_die;

labath wrote:

Would it be possible to avoid creating the extra map, for example by modifying 
the `m_forward_decl_compiler_type_to_die` to point to the definition DIE -- 
after that DIE has been located?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -533,9 +540,16 @@ class SymbolFileDWARF : public SymbolFileCommon {
   NameToOffsetMap m_function_scope_qualified_name_map;
   std::unique_ptr m_ranges;
   UniqueDWARFASTTypeMap m_unique_ast_type_map;
+  // A map from DIE to lldb_private::Type. For record type, the key might be
+  // either declaration DIE or definition DIE.
   DIEToTypePtr m_die_to_type;
   DIEToVariableSP m_die_to_variable_sp;
+  // A map from CompilerType to the struct/class/union/enum DIE (might be a
+  // declaration or a definition) that is used to construct it.
   CompilerTypeToDIE m_forward_decl_compiler_type_to_die;
+  // A map from a struct/class/union/enum DIE (might be a declaration or a
+  // definition) to its definition DIE.
+  DIEToDIE m_die_to_def_die;

labath wrote:

(I also can't help but wonder why do we need that 
`m_forward_decl_compiler_type_to_die` in the first place, given that the 
`ClangASTMetadata` associated with the type already contains a user ID, which 
should allow us to retrieve original DIE. I know this isn't related to your 
patch, I'm just writing it in case you or someone has any thoughts on this.)

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -1654,6 +1660,99 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
   return false;
 }
 
+DWARFDIE SymbolFileDWARF::FindDefinitionDIE(const DWARFDIE ) {
+  auto def_die_it = GetDeclarationDIEToDefinitionDIE().find(die.GetDIE());
+  if (def_die_it != GetDeclarationDIEToDefinitionDIE().end())
+return GetDIE(def_die_it->getSecond());
+
+  ParsedDWARFTypeAttributes attrs(die);
+  const dw_tag_t tag = die.Tag();
+  TypeSP type_sp;
+  Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
+  if (log) {
+GetObjectFile()->GetModule()->LogMessage(
+log,
+"SymbolFileDWARF({0:p}) - {1:x16}: {2} type \"{3}\" is a "
+"forward declaration DIE, trying to find definition DIE",
+static_cast(this), die.GetOffset(), DW_TAG_value_to_name(tag),
+attrs.name.GetCString());
+  }
+  // We haven't parse definition die for this type, starting to search for it.
+  // After we found the definition die, the GetDeclarationDIEToDefinitionDIE()
+  // map will have the new mapping from this declaration die to definition die.
+  if (attrs.class_language == eLanguageTypeObjC ||

labath wrote:

I'm not sure how big of a problem this is (as `CompleteType` above already 
contains some clang-specific code), but moving some clang-specific code out of 
`DWARFASTParserClang` and into `SymbolFileDWARF` is less than ideal, as the 
latter is also used with non-clang/non-C languages. Would it be possible to 
keep this code `DWARFASTParserClang` somehow?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] [lldb][DWARF] Delay struct/class/union definition DIE searching when parsing declaration DIEs. (PR #90663)

2024-05-02 Thread Pavel Labath via lldb-commits



@@ -1631,13 +1631,19 @@ bool SymbolFileDWARF::CompleteType(CompilerType 
_type) {
 return true;
   }
 
-  DWARFDIE dwarf_die = GetDIE(die_it->getSecond());
+  DWARFDIE dwarf_die = FindDefinitionDIE(GetDIE(die_it->getSecond()));
   if (dwarf_die) {
 // Once we start resolving this type, remove it from the forward
 // declaration map in case anyone child members or other types require this
 // type to get resolved. The type will get resolved when all of the calls
 // to SymbolFileDWARF::ResolveClangOpaqueTypeDefinition are done.
-GetForwardDeclCompilerTypeToDIE().erase(die_it);
+// Need to get a new iterator because FindDefinitionDIE might add new

labath wrote:

Would it be possible to erase the iterator before calling FindDefinitionDIE?

https://github.com/llvm/llvm-project/pull/90663
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

1 2 >

1 - 100 of 109 matches

Mail list logo