[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
@@ -897,32 +895,39 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc, } CompilerType clang_type = m_ast.CreateEnumerationType( - attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(die, nullptr), - GetOwningClangModule(die), attrs.decl, enumerator_clang_type, + attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(def_die, nullptr), + GetOwningClangModule(def_die), attrs.decl, enumerator_clang_type, attrs.is_scoped_enum); - - LinkDeclContextToDIE(TypeSystemClang::GetDeclContextForType(clang_type), die); - - type_sp = - dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr, + TypeSP type_sp = + dwarf->MakeType(def_die.GetID(), attrs.name, attrs.byte_size, nullptr, labath wrote: #96751 https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
@@ -897,32 +895,39 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc, } CompilerType clang_type = m_ast.CreateEnumerationType( - attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(die, nullptr), - GetOwningClangModule(die), attrs.decl, enumerator_clang_type, + attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(def_die, nullptr), + GetOwningClangModule(def_die), attrs.decl, enumerator_clang_type, attrs.is_scoped_enum); - - LinkDeclContextToDIE(TypeSystemClang::GetDeclContextForType(clang_type), die); - - type_sp = - dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr, + TypeSP type_sp = + dwarf->MakeType(def_die.GetID(), attrs.name, attrs.byte_size, nullptr, labath wrote: Yes, that's a very good catch. I noticed the lack of a unique map in the enum implementation, but I just assumed that means we don't care about duplicate definitions for enums. (And to a sense that's true, since it still means we will create a duplicate definition if we start with two *definition* DIEs.) This should be fairly simple to fix, I'll create a today. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
@@ -897,32 +895,39 @@ TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc, } CompilerType clang_type = m_ast.CreateEnumerationType( - attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(die, nullptr), - GetOwningClangModule(die), attrs.decl, enumerator_clang_type, + attrs.name.GetStringRef(), GetClangDeclContextContainingDIE(def_die, nullptr), + GetOwningClangModule(def_die), attrs.decl, enumerator_clang_type, attrs.is_scoped_enum); - - LinkDeclContextToDIE(TypeSystemClang::GetDeclContextForType(clang_type), die); - - type_sp = - dwarf->MakeType(die.GetID(), attrs.name, attrs.byte_size, nullptr, + TypeSP type_sp = + dwarf->MakeType(def_die.GetID(), attrs.name, attrs.byte_size, nullptr, ZequanWu wrote: If two different enum decl_die (reference the same definition def_die) were called with this function, doesn't it create two `CompilerType` and two `Type` from the same def_die? It's not a problem for `ParseStructureLikeDIE` because it will check if we have already created the type in `UniqueDWARFASTTypeMap`. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
https://github.com/labath closed https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
labath wrote: > > > This patch as-is is NFC? > > > > > > NFC_**I**_, I would say :) I don't think this should change the behavior in > > any way, but it's pretty hard to guarantee that. > > Sure enough - I take any claim as a statement of intent/belief, not of > something mathematically proved, etc. True, but in case of this code, even believing you know what it does may be a tough ask. :D > Perhaps a separate commit could add another RUN line to the existing test you > added to demonstrate the reason for the revert? Rather than worrying about > which comes first (the type unit patch or the delay patch) > > But in any case, I /think/ I understand why this patch doesn't need a test > (because this patch avoids the delay patch causing a crash (yeah, more > complex than that because the patch doesn't apply cleanly over this one) and > that crash already has a test committed) - thanks for the explanation. Correct. The reason for revert has already been established with the first test. I'll create a separate patch for the type unit bug, but it will be slightly more complicated than adding a RUN line, because, due to the bug, the lldb will print the wrong type. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
dwblaikie wrote: > > This patch as-is is NFC? > > NFC_**I**_, I would say :) I don't think this should change the behavior in > any way, but it's pretty hard to guarantee that. Sure enough - I take any claim as a statement of intent/belief, not of something mathematically proved, etc. > > (no tests) but without this patch, the other delay-definition-die patch > > would have had a problem? > > Is it possible to add a test in this patch that would exercise the thing > > that would become buggy if the delay-definition-die patch were to be > > recommitted? > > Sort of. The situation is a bit complicated, because this touches pretty much > the same code as the other patch, so the other patch will not apply cleanly > or become magically correct. It's more like a building block that enables us > to rewrite the other patch in a more robust manner -- which brings us to the > second way this is complicated: It's not that the other patch was wrong on > its own. It was just very sensitive to the other bugs. Previously, if we > failed to unique the types correctly, we would "just" get the wrong type (or > maybe no type). With the patch, that situation would trigger a hard assert. > On its own, that might even be considered a good thing (easier to find bugs), > we're it not for the fact that this made the logic of the patch very hard to > follow. So, this is my attempt to make it more straight-forward. > > As for tests, it is possible to write a test which would reproduce a crash > with the original patch, but that test is contingent on the existence of > another bug. When I reverted that patch, I added a test (in > [de3f1b6](https://github.com/llvm/llvm-project/commit/de3f1b6d68ab8a0e827db84b328803857a4f60df)) > which triggered the crash. Having a bit of a hard time following this - is the test you added the same as the test you speculated is possible to write in the prior sentence? > However, now that that bug is fixed (#95905), it does not crash anymore. Now, > I happen to know of another bug (which happens to be triggered by the same > code, only compiled with type units), but the same thing will happen once > that bug is fixed. OK - I think I'm following. > Given all of that, I don't think another test case is needed with this > particular patch. It might be interesting for the final delay patch, if we > don't fix the type unit thing by then, but I think of this patch mostly as a > cleanup. Perhaps a separate commit could add another RUN line to the existing test you added to demonstrate the reason for the revert? Rather than worrying about which comes first (the type unit patch or the delay patch) But in any case, I /think/ I understand why this patch doesn't need a test (because this patch avoids the delay patch causing a crash (yeah, more complex than that because the patch doesn't apply cleanly over this one) and that crash already has a test committed) - thanks for the explanation. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
labath wrote: > This patch as-is is NFC? NFC**I**, I would say :) I don't think this should change the behavior in any way, but it's pretty hard to guarantee that. > (no tests) but without this patch, the other delay-definition-die patch would > have had a problem? > > Is it possible to add a test in this patch that would exercise the thing that > would become buggy if the delay-definition-die patch were to be recommitted? Sort of. The situation is a bit complicated, because this touches pretty much the same code as the other patch, so the other patch will not apply cleanly or become magically correct. It's more like a building block that enables us to rewrite the other patch in a more robust manner -- which brings us to the second way this is complicated: It's not that the other patch was wrong on its own. It was just very sensitive to the other bugs. Previously, if we failed to unique the types correctly, we would "just" get the wrong type (or maybe no type). With the patch, that situation would trigger a hard assert. On its own, that might even be considered a good thing (easier to find bugs), we're it not for the fact that this made the logic of the patch very hard to follow. So, this is my attempt to make it more straight-forward. As for tests, it is possible to write a test which would reproduce a crash with the original patch, but that test is contingent on the existence of another bug. When I reverted that patch, I added a test (in de3f1b6d68ab8a0e827db84b328803857a4f60df) which triggered the crash. However, now that that bug is fixed (#95905), it does not crash anymore. Now, I happen to know of another bug (which happens to be triggered by the same code, only compiled with type units), but the same thing will happen once that bug is fixed. Given all of that, I don't think another test case is needed with this particular patch. It might be interesting for the final delay patch, if we don't fix the type unit thing by then, but I think of this patch mostly as a cleanup. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
dwblaikie wrote: This patch as-is is NFC? (no tests) but without this patch, the other delay-definition-die patch would have had a problem? Is it possible to add a test in this patch that would exercise the thing that would become buggy if the delay-definition-die patch were to be recommitted? https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
Michael137 wrote: Latest update LGTM https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
labath wrote: > Makes sense to me Do we make sure that the type lookup map is updated so we > don't re-parse when calling `ParseTypeFromDWARF` with either the declaration > or the definition DIE? Good catch. I've been meaning to add that, but I forgot. Things should still work even without the DieToType update, as the type will be caught by the UniqueDWARFASTTypeMap, but this is definitely faster. The way that the definition dies are currently handled is a bit clumsy now, but I didn't want to implement anything more elaborate as this should go away once we stop eagerly searching for the definition. https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
https://github.com/labath updated
https://github.com/llvm/llvm-project/pull/96484
>From 52db8db036c24264647340c15ec4ee6d3553cf60 Mon Sep 17 00:00:00 2001
From: Pavel Labath
Date: Mon, 24 Jun 2024 11:17:47 +0200
Subject: [PATCH 1/2] [lldb/DWARF] Remove parsing recursion when searching for
definition DIEs
If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE,
it would call FindDefinitionTypeForDIE. This returned a fully formed
type, which it achieved by recursing back into ParseStructureLikeDIE
with the definition DIE.
This obscured the control flow and caused us to repeat some work (e.g.
the UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried
to delay the definition search in #90663. After this patch, the two
ParseStructureLikeDIE calls were no longer recursive, but rather
the second call happened as a part of the CompleteType() call. This
opened the door to inconsistencies, as the second ParseStructureLikeDIE
call was not aware it was called to process a definition die for an
existing type.
To make that possible, this patch removes the recusive type resolution
from this function, and leaves just the "find definition die"
functionality. After finding the definition DIE, we just go back to the
original ParseStructureLikeDIE call, and have it finish the parsing
process with the new DIE.
While this patch is motivated by the work on delaying the definition
searching, I believe it is also useful on its own.
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 210 +-
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 187
.../SymbolFile/DWARF/SymbolFileDWARF.h| 3 +-
.../DWARF/SymbolFileDWARFDebugMap.cpp | 11 +-
.../DWARF/SymbolFileDWARFDebugMap.h | 2 +-
.../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp | 5 +-
.../SymbolFile/DWARF/SymbolFileDWARFDwo.h | 3 +-
7 files changed, 208 insertions(+), 213 deletions(-)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 52f4d765cbbd4..ad58e0cbb5a59 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -39,10 +39,12 @@
#include "lldb/Utility/StreamString.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Demangle/Demangle.h"
#include
@@ -835,54 +837,50 @@ DWARFASTParserClang::GetDIEClassTemplateParams(const
DWARFDIE &die) {
}
TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
- const DWARFDIE &die,
+ const DWARFDIE &decl_die,
ParsedDWARFTypeAttributes &attrs) {
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
- SymbolFileDWARF *dwarf = die.GetDWARF();
- const dw_tag_t tag = die.Tag();
- TypeSP type_sp;
+ SymbolFileDWARF *dwarf = decl_die.GetDWARF();
+ const dw_tag_t tag = decl_die.Tag();
+ DWARFDIE def_die;
if (attrs.is_forward_declaration) {
-type_sp = ParseTypeFromClangModule(sc, die, log);
-if (type_sp)
+if (TypeSP type_sp = ParseTypeFromClangModule(sc, decl_die, log))
return type_sp;
-type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
+def_die = dwarf->FindDefinitionDIE(decl_die);
-if (!type_sp) {
+if (!def_die) {
SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
if (debug_map_symfile) {
// We weren't able to find a full declaration in this DWARF,
// see if we have a declaration anywhere else...
-type_sp =
debug_map_symfile->FindDefinitionTypeForDWARFDeclContext(die);
+def_die = debug_map_symfile->FindDefinitionDIE(decl_die);
}
}
-if (type_sp) {
- if (log) {
-dwarf->GetObjectFile()->GetModule()->LogMessage(
-log,
-"SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
-"forward declaration, complete type is {5:x8}",
-static_cast(this), die.GetOffset(),
-DW_TAG_value_to_name(tag), tag, attrs.name.GetCString(),
-type_sp->GetID());
- }
-
- // We found a real definition for this type elsewhere so must link its
- // DeclContext to this die.
- if (clang::DeclContext *defn_decl_ctx =
- GetCachedClangDeclContextForDIE(dwarf->GetDIE(type_sp->GetID(
-LinkDeclContextToDIE(defn_decl_ctx, die);
- return type_sp;
+if (log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
+ "forward declaration, complete DIE is {5}",
+ static_c
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
https://github.com/Michael137 approved this pull request. Makes sense to me Do we make sure that the type lookup map is updated so we don't re-parse when calling `ParseTypeFromDWARF` with either the declaration or the definition DIE? https://github.com/llvm/llvm-project/pull/96484 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
llvmbot wrote:
@llvm/pr-subscribers-lldb
Author: Pavel Labath (labath)
Changes
If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE, it would
call FindDefinitionTypeForDIE. This returned a fully formed type, which it
achieved by recursing back into ParseStructureLikeDIE with the definition DIE.
This obscured the control flow and caused us to repeat some work (e.g. the
UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried to delay the
definition search in #90663. After this patch, the two
ParseStructureLikeDIE calls were no longer recursive, but rather the second
call happened as a part of the CompleteType() call. This opened the door to
inconsistencies, as the second ParseStructureLikeDIE call was not aware it was
called to process a definition die for an existing type.
To make that possible, this patch removes the recusive type resolution from
this function, and leaves just the "find definition die" functionality. After
finding the definition DIE, we just go back to the original
ParseStructureLikeDIE call, and have it finish the parsing process with the new
DIE.
While this patch is motivated by the work on delaying the definition searching,
I believe it is also useful on its own.
---
Patch is 32.26 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/96484.diff
7 Files Affected:
- (modified) lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
(+107-103)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+91-96)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h (+1-2)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
(+5-6)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
(+1-1)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp (+2-3)
- (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h (+1-2)
``diff
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 52f4d765cbbd4..ad58e0cbb5a59 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -39,10 +39,12 @@
#include "lldb/Utility/StreamString.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Demangle/Demangle.h"
#include
@@ -835,54 +837,50 @@ DWARFASTParserClang::GetDIEClassTemplateParams(const
DWARFDIE &die) {
}
TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
- const DWARFDIE &die,
+ const DWARFDIE &decl_die,
ParsedDWARFTypeAttributes &attrs) {
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
- SymbolFileDWARF *dwarf = die.GetDWARF();
- const dw_tag_t tag = die.Tag();
- TypeSP type_sp;
+ SymbolFileDWARF *dwarf = decl_die.GetDWARF();
+ const dw_tag_t tag = decl_die.Tag();
+ DWARFDIE def_die;
if (attrs.is_forward_declaration) {
-type_sp = ParseTypeFromClangModule(sc, die, log);
-if (type_sp)
+if (TypeSP type_sp = ParseTypeFromClangModule(sc, decl_die, log))
return type_sp;
-type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
+def_die = dwarf->FindDefinitionDIE(decl_die);
-if (!type_sp) {
+if (!def_die) {
SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
if (debug_map_symfile) {
// We weren't able to find a full declaration in this DWARF,
// see if we have a declaration anywhere else...
-type_sp =
debug_map_symfile->FindDefinitionTypeForDWARFDeclContext(die);
+def_die = debug_map_symfile->FindDefinitionDIE(decl_die);
}
}
-if (type_sp) {
- if (log) {
-dwarf->GetObjectFile()->GetModule()->LogMessage(
-log,
-"SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
-"forward declaration, complete type is {5:x8}",
-static_cast(this), die.GetOffset(),
-DW_TAG_value_to_name(tag), tag, attrs.name.GetCString(),
-type_sp->GetID());
- }
-
- // We found a real definition for this type elsewhere so must link its
- // DeclContext to this die.
- if (clang::DeclContext *defn_decl_ctx =
- GetCachedClangDeclContextForDIE(dwarf->GetDIE(type_sp->GetID(
-LinkDeclContextToDIE(defn_decl_ctx, die);
- return type_sp;
+if (log) {
+ dwarf->GetObjectFile()->GetModule()->LogMessage(
+ log,
+ "SymbolFileDWARF({0:p}) - {1:x16}}: {2} ({3}) type \"{4}\" is a "
+ "forward declarat
[Lldb-commits] [lldb] [lldb/DWARF] Remove parsing recursion when searching for definition DIEs (PR #96484)
https://github.com/labath created
https://github.com/llvm/llvm-project/pull/96484
If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE, it would
call FindDefinitionTypeForDIE. This returned a fully formed type, which it
achieved by recursing back into ParseStructureLikeDIE with the definition DIE.
This obscured the control flow and caused us to repeat some work (e.g. the
UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried to delay the
definition search in #90663. After this patch, the two ParseStructureLikeDIE
calls were no longer recursive, but rather the second call happened as a part
of the CompleteType() call. This opened the door to inconsistencies, as the
second ParseStructureLikeDIE call was not aware it was called to process a
definition die for an existing type.
To make that possible, this patch removes the recusive type resolution from
this function, and leaves just the "find definition die" functionality. After
finding the definition DIE, we just go back to the original
ParseStructureLikeDIE call, and have it finish the parsing process with the new
DIE.
While this patch is motivated by the work on delaying the definition searching,
I believe it is also useful on its own.
>From 52db8db036c24264647340c15ec4ee6d3553cf60 Mon Sep 17 00:00:00 2001
From: Pavel Labath
Date: Mon, 24 Jun 2024 11:17:47 +0200
Subject: [PATCH] [lldb/DWARF] Remove parsing recursion when searching for
definition DIEs
If ParseStructureLikeDIE (or ParseEnum) encountered a declaration DIE,
it would call FindDefinitionTypeForDIE. This returned a fully formed
type, which it achieved by recursing back into ParseStructureLikeDIE
with the definition DIE.
This obscured the control flow and caused us to repeat some work (e.g.
the UniqueDWARFASTTypeMap lookup), but it mostly worked until we tried
to delay the definition search in #90663. After this patch, the two
ParseStructureLikeDIE calls were no longer recursive, but rather
the second call happened as a part of the CompleteType() call. This
opened the door to inconsistencies, as the second ParseStructureLikeDIE
call was not aware it was called to process a definition die for an
existing type.
To make that possible, this patch removes the recusive type resolution
from this function, and leaves just the "find definition die"
functionality. After finding the definition DIE, we just go back to the
original ParseStructureLikeDIE call, and have it finish the parsing
process with the new DIE.
While this patch is motivated by the work on delaying the definition
searching, I believe it is also useful on its own.
---
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 210 +-
.../SymbolFile/DWARF/SymbolFileDWARF.cpp | 187
.../SymbolFile/DWARF/SymbolFileDWARF.h| 3 +-
.../DWARF/SymbolFileDWARFDebugMap.cpp | 11 +-
.../DWARF/SymbolFileDWARFDebugMap.h | 2 +-
.../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp | 5 +-
.../SymbolFile/DWARF/SymbolFileDWARFDwo.h | 3 +-
7 files changed, 208 insertions(+), 213 deletions(-)
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
index 52f4d765cbbd4..ad58e0cbb5a59 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -39,10 +39,12 @@
#include "lldb/Utility/StreamString.h"
#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/Type.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Demangle/Demangle.h"
#include
@@ -835,54 +837,50 @@ DWARFASTParserClang::GetDIEClassTemplateParams(const
DWARFDIE &die) {
}
TypeSP DWARFASTParserClang::ParseEnum(const SymbolContext &sc,
- const DWARFDIE &die,
+ const DWARFDIE &decl_die,
ParsedDWARFTypeAttributes &attrs) {
Log *log = GetLog(DWARFLog::TypeCompletion | DWARFLog::Lookups);
- SymbolFileDWARF *dwarf = die.GetDWARF();
- const dw_tag_t tag = die.Tag();
- TypeSP type_sp;
+ SymbolFileDWARF *dwarf = decl_die.GetDWARF();
+ const dw_tag_t tag = decl_die.Tag();
+ DWARFDIE def_die;
if (attrs.is_forward_declaration) {
-type_sp = ParseTypeFromClangModule(sc, die, log);
-if (type_sp)
+if (TypeSP type_sp = ParseTypeFromClangModule(sc, decl_die, log))
return type_sp;
-type_sp = dwarf->FindDefinitionTypeForDWARFDeclContext(die);
+def_die = dwarf->FindDefinitionDIE(decl_die);
-if (!type_sp) {
+if (!def_die) {
SymbolFileDWARFDebugMap *debug_map_symfile = dwarf->GetDebugMapSymfile();
if (debug_map_symfile) {
// We weren't able to find a full declaration in this DWARF,
// see if we have a dec
