[SymbolFileDWARF][NFC] Remove duplicated code checking for type tags #74773

felipepiovezan · 2023-12-07T22:19:21Z

There was duplicated (and complex) code querying whether tags were type-like tags (i.e. class or struct); this has been factored out into a helper function.

There was also a comment about not comparing identical DIEs without ever performing that check; this comment has been removed. It was likely a result of copy paste from another function in this same file which actually does that check.

There was duplicated (and complex) code querying whether tags were type-like tags (i.e. class or struct); this has been factored out into a helper function. There was also a comment about not comparing identical DIEs without ever performing that check; this comment has been removed. It was likely a result of copy paste from another function in this same file which actually does that check.

llvmbot · 2023-12-07T22:19:48Z

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

There was duplicated (and complex) code querying whether tags were type-like tags (i.e. class or struct); this has been factored out into a helper function.

There was also a comment about not comparing identical DIEs without ever performing that check; this comment has been removed. It was likely a result of copy paste from another function in this same file which actually does that check.

Full diff: https://github.com/llvm/llvm-project/pull/74773.diff

1 Files Affected:

(modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+13-45)

diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index d4cc26a3c329b..541d8d75f2187 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -130,6 +130,10 @@ class PluginProperties : public Properties {
   }
 };
 
+bool IsTypeTag(llvm::dwarf::Tag Tag) {
+  return Tag == llvm::dwarf::Tag::DW_TAG_class_type ||
+         Tag == llvm::dwarf::Tag::DW_TAG_structure_type;
+}
 } // namespace
 
 static PluginProperties &GetGlobalPluginProperties() {
@@ -2947,29 +2951,18 @@ TypeSP SymbolFileDWARF::FindCompleteObjCDefinitionTypeForDIE(
 
   m_index->GetCompleteObjCClass(
       type_name, must_be_implementation, [&](DWARFDIE type_die) {
-        bool try_resolving_type = false;
-
         // Don't try and resolve the DIE we are looking for with the DIE
         // itself!
-        if (type_die != die) {
-          switch (type_die.Tag()) {
-          case DW_TAG_class_type:
-          case DW_TAG_structure_type:
-            try_resolving_type = true;
-            break;
-          default:
-            break;
-          }
-        }
-        if (!try_resolving_type)
+        if (type_die == die || !IsTypeTag(type_die.Tag()))
           return true;
 
         if (must_be_implementation &&
-            type_die.Supports_DW_AT_APPLE_objc_complete_type())
-          try_resolving_type = type_die.GetAttributeValueAsUnsigned(
+            type_die.Supports_DW_AT_APPLE_objc_complete_type()) {
+          bool try_resolving_type = type_die.GetAttributeValueAsUnsigned(
               DW_AT_APPLE_objc_complete_type, 0);
-        if (!try_resolving_type)
-          return true;
+          if (!try_resolving_type)
+            return true;
+        }
 
         Type *resolved_type = ResolveType(type_die, false, true);
         if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
@@ -3128,36 +3121,11 @@ SymbolFileDWARF::FindDefinitionTypeForDWARFDeclContext(const DWARFDIE &die) {
       if (type_system &&
           !type_system->SupportsLanguage(GetLanguage(*type_die.GetCU())))
         return true;
-      bool try_resolving_type = false;
 
-      // Don't try and resolve the DIE we are looking for with the DIE
-      // itself!
       const dw_tag_t type_tag = type_die.Tag();
-      // Make sure the tags match
-      if (type_tag == tag) {
-        // The tags match, lets try resolving this type
-        try_resolving_type = true;
-      } else {
-        // The tags don't match, but we need to watch our for a forward
-        // declaration for a struct and ("struct foo") ends up being a
-        // class ("class foo { ... };") or vice versa.
-        switch (type_tag) {
-        case DW_TAG_class_type:
-          // We had a "class foo", see if we ended up with a "struct foo
-          // { ... };"
-          try_resolving_type = (tag == DW_TAG_structure_type);
-          break;
-        case DW_TAG_structure_type:
-          // We had a "struct foo", see if we ended up with a "class foo
-          // { ... };"
-          try_resolving_type = (tag == DW_TAG_class_type);
-          break;
-        default:
-          // Tags don't match, don't event try to resolve using this type
-          // whose name matches....
-          break;
-        }
-      }
+      // Resolve the type if both have the same tag or {class, struct} tags.
+      bool try_resolving_type =
+          type_tag == tag || (IsTypeTag(type_tag) && IsTypeTag(tag));
 
       if (!try_resolving_type) {
         if (log) {

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

Michael137

LGTM

…e tags

bulbazord

LGTM

clayborg · 2023-12-08T07:57:28Z

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

-      }
+      // Resolve the type if both have the same tag or {class, struct} tags.
+      const bool try_resolving_type =
+          type_tag == tag || (IsTypeTag(type_tag) && IsTypeTag(tag));


The second call to IsTypeTag(tag) is redundant since you already checked for equality right before it.

I'm not sure I follow, the logic is the opposite: equality failed, so we have to test whether both are "class-ish" tags. If one is a class but the other is a namespace, we need this expression to be false

clayborg · 2023-12-08T07:59:20Z

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

@@ -130,6 +130,10 @@ class PluginProperties : public Properties {
  }
 };

+bool IsTypeTag(llvm::dwarf::Tag Tag) {


Make static and rename to IsClassOrStructType. There are many other type tags in DWARF, so this doesn't make it clear that this funciton is only looking for DW_TAG_class_type or DW_TAG_structure_type.

I'll rename it.

Regarding static, this function, as well as most functions defined above it, are in an anonymous namespace

Do you prefer if I take it out of the namespace and make it static? In llvm-project/llvm I would have done it because of the programming guidelines

In the interest of expediency, I've moved it out of the anonymous namespace and made it static

…for type tags

clayborg

Thanks for the quick change, and I need to check sources for the anonymous namespace before I comment next time!

…lvm#74773) There was duplicated (and complex) code querying whether tags were type-like tags (i.e. class or struct); this has been factored out into a helper function. There was also a comment about not comparing identical DIEs without ever performing that check; this comment has been removed. It was likely a result of copy paste from another function in this same file which actually does that check. (cherry picked from commit 162248c)

felipepiovezan requested a review from clayborg December 7, 2023 22:19

felipepiovezan requested a review from JDevlieghere as a code owner December 7, 2023 22:19

llvmbot added the lldb label Dec 7, 2023

Michael137 reviewed Dec 7, 2023

View reviewed changes

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Outdated Show resolved Hide resolved

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp Outdated Show resolved Hide resolved

Michael137 approved these changes Dec 7, 2023

View reviewed changes

fixup! [SymbolFileDWARF][NFC] Remove duplicated code checking for typ…

0225efc

…e tags

bulbazord approved these changes Dec 7, 2023

View reviewed changes

clayborg requested changes Dec 8, 2023

View reviewed changes

fixup! fixup! [SymbolFileDWARF][NFC] Remove duplicated code checking …

bc6f379

…for type tags

felipepiovezan requested a review from clayborg December 8, 2023 14:40

clayborg approved these changes Dec 8, 2023

View reviewed changes

felipepiovezan merged commit 162248c into llvm:main Dec 11, 2023

felipepiovezan deleted the felipe/simplify_is_type branch December 11, 2023 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SymbolFileDWARF][NFC] Remove duplicated code checking for type tags #74773

[SymbolFileDWARF][NFC] Remove duplicated code checking for type tags #74773

Uh oh!

felipepiovezan commented Dec 7, 2023

Uh oh!

llvmbot commented Dec 7, 2023

Uh oh!

Uh oh!

Uh oh!

Michael137 left a comment

Uh oh!

bulbazord left a comment

Uh oh!

clayborg Dec 8, 2023

Uh oh!

felipepiovezan Dec 8, 2023 •

edited

Loading

Uh oh!

clayborg Dec 8, 2023

Uh oh!

felipepiovezan Dec 8, 2023 •

edited

Loading

Uh oh!

felipepiovezan Dec 8, 2023 •

edited

Loading

Uh oh!

felipepiovezan Dec 8, 2023

Uh oh!

clayborg left a comment

Uh oh!

Uh oh!

[SymbolFileDWARF][NFC] Remove duplicated code checking for type tags #74773

[SymbolFileDWARF][NFC] Remove duplicated code checking for type tags #74773

Uh oh!

Conversation

felipepiovezan commented Dec 7, 2023

Uh oh!

llvmbot commented Dec 7, 2023

Uh oh!

Uh oh!

Uh oh!

Michael137 left a comment

Choose a reason for hiding this comment

Uh oh!

bulbazord left a comment

Choose a reason for hiding this comment

Uh oh!

clayborg Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felipepiovezan Dec 8, 2023 •

edited

Loading

felipepiovezan Dec 8, 2023 •

edited

Loading

felipepiovezan Dec 8, 2023 •

edited

Loading