[lldb][SymbolFileDWARFDebugMap] Introduce enum to indicate whether to continue iteration of object files #87344

Michael137 · 2024-04-02T13:02:41Z

This patch introduces a new IterationMarker enum (happy to take alternative name suggestions), which callbacks, like the one in SymbolFileDWARFDebugMap::ForEachSymbolFile, can return in order to indicate whether the caller should continue iterating or bail.

For now this patch just changes the ForEachSymbolFile callback to use this new enum. In the future we could change the various DWARFIndex::GetXXX callbacks to do the same.

This makes the callbacks easier to read and hopefully reduces the chance of bugs like #87177.

… continue iteration of object files This patch introduces a new `IterationMarker` enum (happy to take alternative name suggestions), which callbacks, like the one in `SymbolFileDWARFDebugMap::ForEachSymbolFile` can return in order to indicate whether the caller should continue iterating or bail. For now this patch just changes the `ForEachSymbolFile` callback to use this new enum. In the future we could change the various `DWARFIndex::GetXXX` callbacks to do the same. This makes the callbacks easier to read and hopefully reduces the chance of bugs like llvm#87177.

llvmbot · 2024-04-02T13:03:18Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

This patch introduces a new IterationMarker enum (happy to take alternative name suggestions), which callbacks, like the one in SymbolFileDWARFDebugMap::ForEachSymbolFile can return in order to indicate whether the caller should continue iterating or bail.

For now this patch just changes the ForEachSymbolFile callback to use this new enum. In the future we could change the various DWARFIndex::GetXXX callbacks to do the same.

This makes the callbacks easier to read and hopefully reduces the chance of bugs like #87177.

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

3 Files Affected:

(modified) lldb/include/lldb/lldb-enumerations.h (+7)
(modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp (+38-35)
(modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h (+4-2)

diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 646f7bfda98475..eea5f44c0e192c 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1339,6 +1339,13 @@ enum AddressMaskRange {
   eAddressMaskRangeAll = eAddressMaskRangeAny,
 };
 
+/// Useful for callbacks whose return type indicates
+/// whether to continue iteration or short-circuit.
+enum class IterationMarker {
+  eContinue = 0,
+  eStop,
+};
+
 } // namespace lldb
 
 #endif // LLDB_LLDB_ENUMERATIONS_H
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
index 4bc2cfd60688a8..d40be5d718aa6e 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -37,6 +37,7 @@
 
 #include "LogChannelDWARF.h"
 #include "SymbolFileDWARF.h"
+#include "lldb/lldb-enumerations.h"
 
 #include <memory>
 #include <optional>
@@ -803,13 +804,13 @@ SymbolFileDWARFDebugMap::GetDynamicArrayInfoForUID(
 bool SymbolFileDWARFDebugMap::CompleteType(CompilerType &compiler_type) {
   bool success = false;
   if (compiler_type) {
-    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
       if (oso_dwarf->HasForwardDeclForCompilerType(compiler_type)) {
         oso_dwarf->CompleteType(compiler_type);
         success = true;
-        return true;
+        return IterationMarker::eStop;
       }
-      return false;
+      return IterationMarker::eContinue;
     });
   }
   return success;
@@ -915,7 +916,7 @@ void SymbolFileDWARFDebugMap::FindGlobalVariables(
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   uint32_t total_matches = 0;
 
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     const uint32_t old_size = variables.GetSize();
     oso_dwarf->FindGlobalVariables(name, parent_decl_ctx, max_matches,
                                    variables);
@@ -925,18 +926,18 @@ void SymbolFileDWARFDebugMap::FindGlobalVariables(
 
       // Are we getting all matches?
       if (max_matches == UINT32_MAX)
-        return false; // Yep, continue getting everything
+        return IterationMarker::eContinue; // Yep, continue getting everything
 
       // If we have found enough matches, lets get out
       if (max_matches >= total_matches)
-        return true;
+        return IterationMarker::eStop;
 
       // Update the max matches for any subsequent calls to find globals in any
       // other object files with DWARF
       max_matches -= oso_matches;
     }
 
-    return false;
+    return IterationMarker::eContinue;
   });
 }
 
@@ -945,7 +946,7 @@ void SymbolFileDWARFDebugMap::FindGlobalVariables(
     VariableList &variables) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   uint32_t total_matches = 0;
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     const uint32_t old_size = variables.GetSize();
     oso_dwarf->FindGlobalVariables(regex, max_matches, variables);
 
@@ -955,18 +956,18 @@ void SymbolFileDWARFDebugMap::FindGlobalVariables(
 
       // Are we getting all matches?
       if (max_matches == UINT32_MAX)
-        return false; // Yep, continue getting everything
+        return IterationMarker::eContinue; // Yep, continue getting everything
 
       // If we have found enough matches, lets get out
       if (max_matches >= total_matches)
-        return true;
+        return IterationMarker::eStop;
 
       // Update the max matches for any subsequent calls to find globals in any
       // other object files with DWARF
       max_matches -= oso_matches;
     }
 
-    return false;
+    return IterationMarker::eContinue;
   });
 }
 
@@ -1071,7 +1072,7 @@ void SymbolFileDWARFDebugMap::FindFunctions(
   LLDB_SCOPED_TIMERF("SymbolFileDWARFDebugMap::FindFunctions (name = %s)",
                      lookup_info.GetLookupName().GetCString());
 
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     uint32_t sc_idx = sc_list.GetSize();
     oso_dwarf->FindFunctions(lookup_info, parent_decl_ctx, include_inlines,
                              sc_list);
@@ -1079,7 +1080,7 @@ void SymbolFileDWARFDebugMap::FindFunctions(
       RemoveFunctionsWithModuleNotEqualTo(m_objfile_sp->GetModule(), sc_list,
                                           sc_idx);
     }
-    return false;
+    return IterationMarker::eContinue;
   });
 }
 
@@ -1090,7 +1091,7 @@ void SymbolFileDWARFDebugMap::FindFunctions(const RegularExpression &regex,
   LLDB_SCOPED_TIMERF("SymbolFileDWARFDebugMap::FindFunctions (regex = '%s')",
                      regex.GetText().str().c_str());
 
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     uint32_t sc_idx = sc_list.GetSize();
 
     oso_dwarf->FindFunctions(regex, include_inlines, sc_list);
@@ -1098,7 +1099,7 @@ void SymbolFileDWARFDebugMap::FindFunctions(const RegularExpression &regex,
       RemoveFunctionsWithModuleNotEqualTo(m_objfile_sp->GetModule(), sc_list,
                                           sc_idx);
     }
-    return false;
+    return IterationMarker::eContinue;
   });
 }
 
@@ -1121,9 +1122,9 @@ void SymbolFileDWARFDebugMap::GetTypes(SymbolContextScope *sc_scope,
         oso_dwarf->GetTypes(sc_scope, type_mask, type_list);
     }
   } else {
-    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
       oso_dwarf->GetTypes(sc_scope, type_mask, type_list);
-      return false;
+      return IterationMarker::eContinue;
     });
   }
 }
@@ -1141,9 +1142,9 @@ SymbolFileDWARFDebugMap::ParseCallEdgesInFunction(
 TypeSP SymbolFileDWARFDebugMap::FindDefinitionTypeForDWARFDeclContext(
     const DWARFDIE &die) {
   TypeSP type_sp;
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     type_sp = oso_dwarf->FindDefinitionTypeForDWARFDeclContext(die);
-    return ((bool)type_sp);
+    return type_sp ? IterationMarker::eStop : IterationMarker::eContinue;
   });
   return type_sp;
 }
@@ -1152,13 +1153,13 @@ bool SymbolFileDWARFDebugMap::Supports_DW_AT_APPLE_objc_complete_type(
     SymbolFileDWARF *skip_dwarf_oso) {
   if (m_supports_DW_AT_APPLE_objc_complete_type == eLazyBoolCalculate) {
     m_supports_DW_AT_APPLE_objc_complete_type = eLazyBoolNo;
-    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
       if (skip_dwarf_oso != oso_dwarf &&
           oso_dwarf->Supports_DW_AT_APPLE_objc_complete_type(nullptr)) {
         m_supports_DW_AT_APPLE_objc_complete_type = eLazyBoolYes;
-        return true;
+        return IterationMarker::eStop;
       }
-      return false;
+      return IterationMarker::eContinue;
     });
   }
   return m_supports_DW_AT_APPLE_objc_complete_type == eLazyBoolYes;
@@ -1217,10 +1218,10 @@ TypeSP SymbolFileDWARFDebugMap::FindCompleteObjCDefinitionTypeForDIE(
   if (!must_be_implementation) {
     TypeSP type_sp;
 
-    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+    ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
       type_sp = oso_dwarf->FindCompleteObjCDefinitionTypeForDIE(
           die, type_name, must_be_implementation);
-      return (bool)type_sp;
+      return type_sp ? IterationMarker::eStop : IterationMarker::eContinue;
     });
 
     return type_sp;
@@ -1231,9 +1232,10 @@ TypeSP SymbolFileDWARFDebugMap::FindCompleteObjCDefinitionTypeForDIE(
 void SymbolFileDWARFDebugMap::FindTypes(const TypeQuery &query,
                                         TypeResults &results) {
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     oso_dwarf->FindTypes(query, results);
-    return results.Done(query); // Keep iterating if we aren't done.
+    return results.Done(query) ? IterationMarker::eStop
+                               : IterationMarker::eContinue;
   });
 }
 
@@ -1243,23 +1245,24 @@ CompilerDeclContext SymbolFileDWARFDebugMap::FindNamespace(
   std::lock_guard<std::recursive_mutex> guard(GetModuleMutex());
   CompilerDeclContext matching_namespace;
 
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     matching_namespace =
         oso_dwarf->FindNamespace(name, parent_decl_ctx, only_root_namespaces);
 
-    return (bool)matching_namespace;
+    return matching_namespace ? IterationMarker::eStop
+                              : IterationMarker::eContinue;
   });
 
   return matching_namespace;
 }
 
 void SymbolFileDWARFDebugMap::DumpClangAST(Stream &s) {
-  ForEachSymbolFile([&s](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&s](SymbolFileDWARF *oso_dwarf) {
     oso_dwarf->DumpClangAST(s);
     // The underlying assumption is that DumpClangAST(...) will obtain the
     // AST from the underlying TypeSystem and therefore we only need to do
     // this once and can stop after the first iteration hence we return true.
-    return true;
+    return IterationMarker::eStop;
   });
 }
 
@@ -1391,7 +1394,7 @@ void SymbolFileDWARFDebugMap::ParseDeclsForContext(
     lldb_private::CompilerDeclContext decl_ctx) {
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     oso_dwarf->ParseDeclsForContext(decl_ctx);
-    return false; // Keep iterating
+    return IterationMarker::eContinue;
   });
 }
 
@@ -1519,14 +1522,14 @@ SymbolFileDWARFDebugMap::AddOSOARanges(SymbolFileDWARF *dwarf2Data,
 
 ModuleList SymbolFileDWARFDebugMap::GetDebugInfoModules() {
   ModuleList oso_modules;
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     ObjectFile *oso_objfile = oso_dwarf->GetObjectFile();
     if (oso_objfile) {
       ModuleSP module_sp = oso_objfile->GetModule();
       if (module_sp)
         oso_modules.Append(module_sp);
     }
-    return false; // Keep iterating
+    return IterationMarker::eContinue;
   });
   return oso_modules;
 }
@@ -1579,8 +1582,8 @@ Status SymbolFileDWARFDebugMap::CalculateFrameVariableError(StackFrame &frame) {
 void SymbolFileDWARFDebugMap::GetCompileOptions(
     std::unordered_map<lldb::CompUnitSP, lldb_private::Args> &args) {
 
-  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
+  ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) {
     oso_dwarf->GetCompileOptions(args);
-    return false;
+    return IterationMarker::eContinue;
   });
 }
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
index d639ee500080d5..de3c1b45597a28 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
@@ -20,6 +20,7 @@
 
 #include "UniqueDWARFASTType.h"
 #include "lldb/Utility/StructuredData.h"
+#include "lldb/lldb-enumerations.h"
 
 class DWARFASTParserClang;
 
@@ -235,11 +236,12 @@ class SymbolFileDWARFDebugMap : public SymbolFileCommon {
 
   // If closure returns "false", iteration continues.  If it returns
   // "true", iteration terminates.
-  void ForEachSymbolFile(std::function<bool(SymbolFileDWARF *)> closure) {
+  void ForEachSymbolFile(
+      std::function<lldb::IterationMarker(SymbolFileDWARF *)> closure) {
     for (uint32_t oso_idx = 0, num_oso_idxs = m_compile_unit_infos.size();
          oso_idx < num_oso_idxs; ++oso_idx) {
       if (SymbolFileDWARF *oso_dwarf = GetSymbolFileByOSOIndex(oso_idx)) {
-        if (closure(oso_dwarf))
+        if (closure(oso_dwarf) == lldb::IterationMarker::eStop)
           return;
       }
     }

felipepiovezan

LGTM! Thanks for doing it, this was much needed

Michael137 · 2024-04-02T16:02:25Z

lldb/include/lldb/lldb-enumerations.h

@@ -1339,6 +1339,13 @@ enum AddressMaskRange {
  eAddressMaskRangeAll = eAddressMaskRangeAny,
 };

+/// Useful for callbacks whose return type indicates
+/// whether to continue iteration or short-circuit.
+enum class IterationMarker {


Maybe IterationAction could be better? @jimingham @clayborg Any thoughts?

IterationAction seems more descriptive

adrian-prantl · 2024-04-02T17:43:49Z

$ git grep ForEach lldb/source | grep \.h:
lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.h:  virtual void ForEachMacro(
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.h:  void ForEachDispatchFunction(std::function<void(lldb::addr_t, 
lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.h:  void ForEach(std::function<bool(const Device &)> f);
lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h:  void ForEachCore(std::function<void(lldb::cpu_id_t cpu_id,
lldb/source/Plugins/Process/Linux/IntelPTMultiCoreTrace.h:  void ForEachCore(std::function<void(lldb::cpu_id_t cpu_id,
lldb/source/Plugins/Process/Linux/IntelPTThreadTraceCollection.h:  void ForEachThread(std::function<void(lldb::tid_t tid,
lldb/source/Plugins/SymbolFile/DWARF/NameToDIE.h:  ForEach(std::function<bool(ConstString name, const DIERef &die_ref)> const
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h:  bool ForEachExternalModule(CompileUnit &, llvm::DenseSet<SymbolFile *> &,
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h:  bool ForEachExternalModule(CompileUnit &, llvm::DenseSet<SymbolFile *> &,
lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h:  void ForEachSymbolFile(std::function<bool(SymbolFileDWARF *)> closure) {
lldb/source/Plugins/Trace/intel-pt/TaskTimer.h:  void ForEachTimedTask(std::function<void(const std::string &name,
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:  void ForEachEnumerator(

adrian-prantl · 2024-04-02T17:44:26Z

Couple more candidates listed above, I think this is a great improvement!

clayborg · 2024-04-02T18:23:48Z

lldb/include/lldb/lldb-enumerations.h

+enum class IterationAction {
+  eContinue = 0,
+  eStop,
+};
+


We don't need this in the public API, so this should be moved to lldb-private-enumerations.h

since this is a enum class do we need the leading e character? So it would just be:

enum class IterationAction { Continue = 0, Stop, };

Michael137 requested a review from JDevlieghere as a code owner April 2, 2024 13:02

Michael137 requested review from clayborg and felipepiovezan April 2, 2024 13:02

llvmbot added the lldb label Apr 2, 2024

felipepiovezan approved these changes Apr 2, 2024

View reviewed changes

Michael137 commented Apr 2, 2024

View reviewed changes

Michael137 added 3 commits April 2, 2024 17:30

fixup! fix ParseDeclsForContext

24afa86

fixup! IterationMarker->IterationAction; fix doxygen comment

6263eec

fixup! clang-format

1980cd4

adrian-prantl self-requested a review April 2, 2024 17:44

adrian-prantl approved these changes Apr 2, 2024

View reviewed changes

clayborg requested changes Apr 2, 2024

View reviewed changes

fixup! move enum to lldb_private; rename enum values

03c6973

clayborg approved these changes Apr 2, 2024

View reviewed changes

Michael137 merged commit e61d6b7 into llvm:main Apr 2, 2024

Michael137 deleted the lldb/iteration-indicator-enum branch April 2, 2024 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][SymbolFileDWARFDebugMap] Introduce enum to indicate whether to continue iteration of object files #87344

[lldb][SymbolFileDWARFDebugMap] Introduce enum to indicate whether to continue iteration of object files #87344

Uh oh!

Michael137 commented Apr 2, 2024 •

edited

Loading

Uh oh!

llvmbot commented Apr 2, 2024

Uh oh!

felipepiovezan left a comment

Uh oh!

Michael137 Apr 2, 2024

Uh oh!

felipepiovezan Apr 2, 2024

Uh oh!

adrian-prantl commented Apr 2, 2024

Uh oh!

adrian-prantl commented Apr 2, 2024

Uh oh!

clayborg Apr 2, 2024

Uh oh!

Michael137 Apr 2, 2024

Uh oh!

Uh oh!

[lldb][SymbolFileDWARFDebugMap] Introduce enum to indicate whether to continue iteration of object files #87344

[lldb][SymbolFileDWARFDebugMap] Introduce enum to indicate whether to continue iteration of object files #87344

Uh oh!

Conversation

Michael137 commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 2, 2024

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

Michael137 Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

adrian-prantl commented Apr 2, 2024

Uh oh!

adrian-prantl commented Apr 2, 2024

Uh oh!

clayborg Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Michael137 Apr 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Michael137 commented Apr 2, 2024 •

edited

Loading