1/2: [nfc] [lldb] Unindent code

[jankratochvil]

Summary

It removes some needless deep indentation and some redundant statements.
It prepares the code for a more clean next patch - DWARF index callbacks D77327.

[kwk]

Overall looks good to me except for one larger logic change. Maybe a your comment can clarify this. The code was in a very bad shape before, given the countless amounts of shortcuts you could take.

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

2014

I assume this can be removed because you're iterating over num_matches == 0 when it's empty. But I cannot tell about the DWARFDebugInfo &debug_info = dwarf->DebugInfo(); and how costly this call is and if it makes sense to run when the offsets are empty.

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

524 ↗

(On Diff #254579)

Why is the if no longer needed?

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

2061

Nice shortcut.

2943

This looks like a logic change and I wonder if it should go in a separate patch, maybe?

[aprantl]

I am generally very okay with this change, though some changes look suspicious, as @kwk noted.

[jankratochvil]

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

2014

I did not put an attention to it, thanks for catching it. Personally I think it is OK but when it has been brought up I am fixing this possible performance regression. SymbolFileDWARF::DebugInfo() contains some llvm::call_once(m_info_once_flag which does have some cost.

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

524 ↗

(On Diff #254579)

It will now run DWARFMappedHash::ExtractDIEArray on empty array which is cheap, it does nothing.

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

2943

This should be NFC, do I miss something?
From:

if (a == b) {
  c;
}

The patch changes it to:

if (a != b)
  continue;
c;

[shafik]

I feel like this should have been split in two. It feels like there are straight forward obviously correct changes and a several others that require some thinking about but look correct.

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

524 ↗

(On Diff #254579)

If we stick with this change, please add a comment explaining this. It is not obvious and someone later on may come and change it back thinking this is a bug.

[aprantl]

In D77326#1958082, @shafik wrote:

I feel like this should have been split in two. It feels like there are straight forward obviously correct changes and a several others that require some thinking about but look correct.

That's a really good point. I agree. Can you split all changes of one kind into separate patches?

[kwk]

No need to split this into multiples revisions. It is just a rewrite of some odd nesting.

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

2943

Ah, now I see that it was previously a block on the right side of the diff :).

[jankratochvil]

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

524 ↗

(On Diff #254579)

I have rather dropped this part of this refactorization patch. It will look more logical together with changes of the next part D77327. Thanks for the comments.

[jankratochvil]

I have checked in the really obvious parts as rG80237523193d.
The remainder is left here - although I do not see there more problematic parts now.

[clayborg]

Does anyone know why harbormaster is useless?

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

2014

not costly at all. This will be fine.

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

70–71 ↗

(On Diff #254579)

This logic is easy to understand by reading the code.

70–71 ↗

(On Diff #254579)

This logic is harder to follow. Maybe:

if (!(die_tag == 0 || die_tag == DW_TAG_class_type || die_tag == DW_TAG_structure_type))
  continue;

[jankratochvil]

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

70–71 ↗

(On Diff #254579)

OK, changed it.
I will follow this also IMO more readable form, I changed it since a 10 years old review but that does not matter anymore.

[labath]

Looks good to me, but let's wait for @shafik and @aprantl to take a look.

[aprantl]

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

2015

Can this be made into a range-based for loop in a separate commit?

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

2057–2063

same here (later)

2915

These two continues IMO are a bit confusing to read this way. Perhaps in this case an if (log) block with just one continue at the end is easier to read. Up to you.

2951

I think I'd prefer a return over a break, (iff they are equivalent!).

[shafik]

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

70 ↗

(On Diff #255012)

I also feel like the previous version was more readable. Is there another gain that I am missing?

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

2065–2071

This whole switch feels overly clever, how about:

if (die.Tag() != DW_TAG_variable)
  continue;

[jankratochvil]

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

2015

In D77327 it gets changed into a callback anyway so method_die_offsets no longer exists there.

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

70 ↗

(On Diff #255012)

I find both versions OK but this patch is going to revert my change in rG80237523193d so everyone should be happy.

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

2057–2063

In D77327 it gets changed into a callback anyway so die_offsets no longer exists there.

2915

OK, changed back to if (log) { ... }. I even was not sure with that.

[jankratochvil]

Implemented all the review comments.

[jankratochvil]

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

2935

This break cannot be changed to return as in D77327 it will become a callback lambda.

2951

Wrote above - This break cannot be changed to return as in D77327 it will become a callback lambda.

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

for (const DWARFDIE &die : member_function_dies)
       dwarf->ResolveType(die);
 
     if (class_language == eLanguageTypeObjC) {
       ConstString class_name(clang_type.GetTypeName());
       if (class_name) {
         DIEArray method_die_offsets;
         dwarf->GetObjCMethodDIEOffsets(class_name, method_die_offsets);
 
-        if (!method_die_offsets.empty()) {
-          DWARFDebugInfo &debug_info = dwarf->DebugInfo();
+        DWARFDebugInfo *debug_infop = nullptr;
 
-          const size_t num_matches = method_die_offsets.size();
-          for (size_t i = 0; i < num_matches; ++i) {
-            const DIERef &die_ref = method_die_offsets[i];
-            DWARFDIE method_die = debug_info.GetDIE(die_ref);
+          if (!debug_infop)
+            debug_infop = &dwarf->DebugInfo();
+          DWARFDIE method_die = debug_infop->GetDIE(die_ref);
 
-            if (method_die)
-              method_die.ResolveType();
-          }
-        }
 
         for (DelayedPropertyList::iterator pi = delayed_properties.begin(),
                                            pe = delayed_properties.end();
              pi != pe; ++pi)
           pi->Finalize();
       }
     }
 

lldb/source/Plugins/SymbolFile/DWARF/DWARFDeclContext.h

public:
 
   DWARFDeclContext() : m_entries(), m_language(lldb::eLanguageTypeUnknown) {}
 
   void AppendDeclContext(dw_tag_t tag, const char *name) {
     m_entries.push_back(Entry(tag, name));
   }
 
   bool operator==(const DWARFDeclContext &rhs) const;
+  bool operator!=(const DWARFDeclContext &rhs) const { return !(*this == rhs); }
 
   uint32_t GetSize() const { return m_entries.size(); }
 
   Entry &operator[](uint32_t idx) {
     // "idx" must be valid
     return m_entries[idx];
   }
 

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

void SymbolFileDWARF::FindGlobalVariables(
 
   if (!CPlusPlusLanguage::ExtractContextAndIdentifier(name.GetCString(),
                                                       context, basename))
     basename = name.GetStringRef();
 
   DIEArray die_offsets;
   m_index->GetGlobalVariables(ConstString(basename), die_offsets);
   const size_t num_die_matches = die_offsets.size();
-  if (num_die_matches) {
-    SymbolContext sc;
-    sc.module_sp = m_objfile_sp->GetModule();
-    assert(sc.module_sp);
+
 
-    // Loop invariant: Variables up to this index have been checked for context
-    // matches.
-    uint32_t pruned_idx = original_size;
 
-    bool done = false;
-    for (size_t i = 0; i < num_die_matches && !done; ++i) {
-      const DIERef &die_ref = die_offsets[i];
-      DWARFDIE die = GetDIE(die_ref);
+  for (size_t i = 0; i < num_die_matches; ++i) {
+    if (!die) {
+      m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
+      continue;
+    }
 
-      if (die) {
-        switch (die.Tag()) {
-        default:
-        case DW_TAG_subprogram:
-        case DW_TAG_inlined_subroutine:
-        case DW_TAG_try_block:
-        case DW_TAG_catch_block:
-          break;
+      continue;
 
-        case DW_TAG_variable: {
-          auto *dwarf_cu = llvm::dyn_cast<DWARFCompileUnit>(die.GetCU());
-          if (!dwarf_cu)
-            continue;
-          sc.comp_unit = GetCompUnitForDWARFCompUnit(*dwarf_cu);
+    case DW_TAG_variable:
+      break;
+    }
 
-          if (parent_decl_ctx) {
-            if (DWARFASTParser *dwarf_ast = GetDWARFParser(*die.GetCU())) {
-              CompilerDeclContext actual_parent_decl_ctx =
-                  dwarf_ast->GetDeclContextContainingUIDFromDWARF(die);
-              if (!actual_parent_decl_ctx ||
-                  actual_parent_decl_ctx != parent_decl_ctx)
-                continue;
-            }
-          }
 
-          ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false,
-                         &variables);
-          while (pruned_idx < variables.GetSize()) {
-            VariableSP var_sp = variables.GetVariableAtIndex(pruned_idx);
-            if (name_is_mangled ||
-                var_sp->GetName().GetStringRef().contains(name.GetStringRef()))
-              ++pruned_idx;
-            else
-              variables.RemoveVariableAtIndex(pruned_idx);
-          }
+    ParseVariables(sc, die, LLDB_INVALID_ADDRESS, false, false, &variables);
 
-          if (variables.GetSize() - original_size >= max_matches)
-            done = true;
-        } break;
-        }
-      } else {
-        m_index->ReportInvalidDIERef(die_ref, name.GetStringRef());
-      }
-    }
+      break;
   }
 
   // Return the number of variable that were appended to the list
   const uint32_t num_matches = variables.GetSize() - original_size;
   if (log && num_matches > 0) {
     GetObjectFile()->GetModule()->LogMessage(
         log,
         "SymbolFileDWARF::FindGlobalVariables (name=\"%s\", "
TypeSP SymbolFileDWARF::FindCompleteObjCDefinitionTypeForDIE(
   if (!type_name || (must_be_implementation && !GetObjCClassSymbol(type_name)))
     return type_sp;
 
   DIEArray die_offsets;
   m_index->GetCompleteObjCClass(type_name, must_be_implementation, die_offsets);
 
   const size_t num_matches = die_offsets.size();
 
-  if (num_matches) {
-    for (size_t i = 0; i < num_matches; ++i) {
-      const DIERef &die_ref = die_offsets[i];
-      DWARFDIE type_die = GetDIE(die_ref);
+    if (!type_die) {
+      m_index->ReportInvalidDIERef(die_ref, type_name.GetStringRef());
+      continue;
+    }
 
-      if (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)
+      continue;
 
-        if (try_resolving_type) {
-          if (must_be_implementation &&
-              type_die.Supports_DW_AT_APPLE_objc_complete_type())
-            try_resolving_type = type_die.GetAttributeValueAsUnsigned(
-                DW_AT_APPLE_objc_complete_type, 0);
+    if (!try_resolving_type)
+      continue;
 
-          if (try_resolving_type) {
-            Type *resolved_type = ResolveType(type_die, false, true);
-            if (resolved_type && resolved_type != DIE_IS_BEING_PARSED) {
-              DEBUG_PRINTF("resolved 0x%8.8" PRIx64 " from %s to 0x%8.8" PRIx64
-                           " (cu 0x%8.8" PRIx64 ")\n",
-                           die.GetID(),
-                           m_objfile_sp->GetFileSpec().GetFilename().AsCString(
-                               "<Unknown>"),
-                           type_die.GetID(), type_cu->GetID());
+    if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
+      continue;
+
+    DEBUG_PRINTF(
+        "resolved 0x%8.8" PRIx64 " from %s to 0x%8.8" PRIx64
+        m_objfile_sp->GetFileSpec().GetFilename().AsCString("<Unknown>"),
 
-              if (die)
-                GetDIEToType()[die.GetDIE()] = resolved_type;
-              type_sp = resolved_type->shared_from_this();
-              break;
-            }
-          }
-        }
-      } else {
-        m_index->ReportInvalidDIERef(die_ref, type_name.GetStringRef());
-      }
-    }
-  }
   return type_sp;
 }
 
 // This function helps to ensure that the declaration contexts match for two
 // different DIEs. Often times debug information will refer to a forward
 // declaration of a type (the equivalent of "struct my_struct;". There will
 // often be a declaration of that type elsewhere that has the full definition.
 // When we go looking for the full type "my_struct", we will find one or more
if (type_name) {
           LLDB_LOG_ERROR(
               lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_SYMBOLS),
               std::move(err), "Cannot get TypeSystem for language {}",
               Language::GetNameForLanguageType(language));
         } else {
           type_system = &type_system_or_err.get();
         }
       }
-      if (num_matches) {
-        for (size_t i = 0; i < num_matches; ++i) {
-          const DIERef &die_ref = die_offsets[i];
-          DWARFDIE type_die = GetDIE(die_ref);
+        if (!type_die) {
+          m_index->ReportInvalidDIERef(die_ref, type_name.GetStringRef());
+          continue;
+        }
 
-          if (type_die) {
-            // Make sure type_die's langauge matches the type system we are
-            // looking for. We don't want to find a "Foo" type from Java if we
-            // are looking for a "Foo" type for C, C++, ObjC, or ObjC++.
-            if (type_system &&
-                !type_system->SupportsLanguage(GetLanguage(*type_die.GetCU())))
-              continue;
-            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;
-              }
-            }
 
-            if (try_resolving_type) {
-              DWARFDeclContext type_dwarf_decl_ctx =
-                  GetDWARFDeclContext(type_die);
-
-              if (log) {
-                GetObjectFile()->GetModule()->LogMessage(
-                    log,
-                    "SymbolFileDWARF::"
-                    "FindDefinitionTypeForDWARFDeclContext(tag=%s, "
-                    "qualified-name='%s') trying die=0x%8.8x (%s)",
-                    DW_TAG_value_to_name(dwarf_decl_ctx[0].tag),
-                    dwarf_decl_ctx.GetQualifiedName(), type_die.GetOffset(),
-                    type_dwarf_decl_ctx.GetQualifiedName());
-              }
+        if (!try_resolving_type) {
+          if (!log)
+            continue;
+          std::string qualified_name;
+          type_die.GetQualifiedName(qualified_name);
+              "qualified-name='%s') ignoring die=0x%8.8x (%s)",
+              qualified_name.c_str());
+          continue;
 
-              // Make sure the decl contexts match all the way up
-              if (dwarf_decl_ctx == type_dwarf_decl_ctx) {
-                Type *resolved_type = ResolveType(type_die, false);
-                if (resolved_type && resolved_type != DIE_IS_BEING_PARSED) {
-                  type_sp = resolved_type->shared_from_this();
-                  break;
-                }
-              }
-            } else {
-              if (log) {
-                std::string qualified_name;
-                type_die.GetQualifiedName(qualified_name);
-                GetObjectFile()->GetModule()->LogMessage(
-                    log,
-                    "SymbolFileDWARF::"
-                    "FindDefinitionTypeForDWARFDeclContext(tag=%s, "
-                    "qualified-name='%s') ignoring die=0x%8.8x (%s)",
-                    DW_TAG_value_to_name(dwarf_decl_ctx[0].tag),
-                    dwarf_decl_ctx.GetQualifiedName(), type_die.GetOffset(),
-                    qualified_name.c_str());
-              }
-            }
-          } else {
-            m_index->ReportInvalidDIERef(die_ref, type_name.GetStringRef());
-          }
+        DWARFDeclContext type_dwarf_decl_ctx = GetDWARFDeclContext(type_die);
+
+              "qualified-name='%s') trying die=0x%8.8x (%s)",
+              type_dwarf_decl_ctx.GetQualifiedName());
         }
+
+        // Make sure the decl contexts match all the way up
+        if (dwarf_decl_ctx != type_dwarf_decl_ctx)
+          continue;
+
+        Type *resolved_type = ResolveType(type_die, false);
+        if (!resolved_type || resolved_type == DIE_IS_BEING_PARSED)
+          continue;
+
+        type_sp = resolved_type->shared_from_this();
+        break;
       }
     }
   }
   return type_sp;
 }
 
 TypeSP SymbolFileDWARF::ParseType(const SymbolContext &sc, const DWARFDIE &die,
                                   bool *type_is_new_ptr) {