[lldb] Make StatsDuration thread-safe

[labath]

Summary

std::chrono::duration types are not thread-safe, and they cannot be
concurrently updated from multiple threads. Currently, we were doing
such a thing (only) in the DWARF indexing code
(DWARFUnit::ExtractDIEsRWLocked), but I think it can easily happen that
someone else tries to update another statistic like this without
bothering to check for thread safety.

This patch changes the StatsDuration type from a simple typedef into a
class in its own right. The class stores the duration internally as
std::atomic<uint64_t> (so it can be updated atomically), but presents it
to its users as the usual chrono type (duration<float>).

[clayborg]

Thanks for finding a solution. I tried originally doing a std::atomic<double> but those are not supported. As long as you have verified that storing at a uint64_t reports the same kinds of durations and that they are accurate, I am good with this.

[labath]

The precision is now limited to microseconds (arbitrary choice that I though should be sufficient) whereas previously it was... floating. There are no changes besides that.

lldb/include/lldb/Breakpoint/Breakpoint.h

public:
   // target - primarily from the dummy target to prime new targets.
   static lldb::BreakpointSP CopyFromBreakpoint(lldb::TargetSP new_target,
       const Breakpoint &bp_to_copy_from);
 
   /// Get statistics associated with this breakpoint in JSON format.
   llvm::json::Value GetStatistics();
 
   /// Get the time it took to resolve all locations in this breakpoint.
-  StatsDuration GetResolveTime() const { return m_resolve_time; }
+  StatsDuration::Duration GetResolveTime() const { return m_resolve_time; }
 
 protected:
   friend class Target;
   // Protected Methods
 
   /// Constructors and Destructors
   /// Only the Target can make a breakpoint, and it owns the breakpoint
   /// lifespans. The constructor takes a filter and a resolver.  Up in Target
private:
 
   /// Number of times this breakpoint has been hit. This is kept separately
   /// from the locations hit counts, since locations can go away when their
   /// backing library gets unloaded, and we would lose hit counts.
   StoppointHitCounter m_hit_counter;
 
   BreakpointName::Permissions m_permissions;
 
-  StatsDuration m_resolve_time{0.0};
+  StatsDuration m_resolve_time;
 
   void SendBreakpointChangedEvent(lldb::BreakpointEventType eventKind);
 
   void SendBreakpointChangedEvent(BreakpointEventData *data);
 
   Breakpoint(const Breakpoint &) = delete;
   const Breakpoint &operator=(const Breakpoint &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_BREAKPOINT_BREAKPOINT_H

lldb/include/lldb/Core/Module.h

protected:
   std::atomic<bool> m_did_load_symfile{false};
   std::atomic<bool> m_did_set_uuid{false};
   mutable bool m_file_has_changed : 1,
       m_first_file_changed_log : 1; /// See if the module was modified after it
                                     /// was initially opened.
   /// We store a symbol table parse time duration here because we might have
   /// an object file and a symbol file which both have symbol tables. The parse
   /// time for the symbol tables can be aggregated here.
-  StatsDuration m_symtab_parse_time{0.0};
+  StatsDuration m_symtab_parse_time;
   /// We store a symbol named index time duration here because we might have
   /// an object file and a symbol file which both have symbol tables. The parse
   /// time for the symbol tables can be aggregated here.
-  StatsDuration m_symtab_index_time{0.0};
+  StatsDuration m_symtab_index_time;
 
   /// Resolve a file or load virtual address.
   ///
   /// Tries to resolve \a vm_addr as a file address (if \a
   /// vm_addr_is_file_addr is true) or as a load address if \a
   /// vm_addr_is_file_addr is false) in the symbol vendor. \a resolve_scope
   /// indicates what clients wish to resolve and can be used to limit the
   /// scope of what is parsed.

lldb/include/lldb/Symbol/SymbolFile.h

public:
   /// function will iterate over all sections in a module and add up their
   /// debug info only section byte sizes.
   virtual uint64_t GetDebugInfoSize();
 
   /// Return the time taken to parse the debug information.
   ///
   /// \returns 0.0 if no information has been parsed or if there is
   /// no computational cost to parsing the debug information.
-  virtual StatsDuration GetDebugInfoParseTime() { return StatsDuration(0.0); }
+  virtual StatsDuration::Duration GetDebugInfoParseTime() { return {}; }
 
   /// Return the time it took to index the debug information in the object
   /// file.
   ///
   /// \returns 0.0 if the file doesn't need to be indexed or if it
   /// hasn't been indexed yet, or a valid duration if it has.
-  virtual StatsDuration GetDebugInfoIndexTime() { return StatsDuration(0.0); }
+  virtual StatsDuration::Duration GetDebugInfoIndexTime() { return {}; }
 
   /// Accessors for the bool that indicates if the debug info index was loaded
   /// from, or saved to the module index cache.
   ///
   /// In statistics it is handy to know if a module's debug info was loaded from
   /// or saved to the cache. When the debug info index is loaded from the cache
   /// startup times can be faster. When the cache is enabled and the debug info
   /// index is saved to the cache, debug sessions can be slower. These accessors

lldb/include/lldb/Target/Statistics.h

 //===-- Statistics.h --------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_STATISTICS_H
 #define LLDB_TARGET_STATISTICS_H
 
-#include <chrono>
-#include <string>
-#include <vector>
-
 #include "lldb/Utility/Stream.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/Support/JSON.h"
+#include <atomic>
+#include <chrono>
+#include <string>
+#include <vector>
 
 namespace lldb_private {
 
 using StatsClock = std::chrono::high_resolution_clock;
-using StatsDuration = std::chrono::duration<double>;
 using StatsTimepoint = std::chrono::time_point<StatsClock>;
 
+class StatsDuration {
+public:
+  using Duration = std::chrono::duration<double>;
+
+  Duration get() const {
+    return Duration(InternalDuration(value.load(std::memory_order_relaxed)));
+  }
+  operator Duration() const { return get(); }
+
+  StatsDuration &operator+=(Duration dur) {
+    value.fetch_add(std::chrono::duration_cast<InternalDuration>(dur).count(),
+                    std::memory_order_relaxed);
+    return *this;
+  }
+
+private:
+  using InternalDuration = std::chrono::duration<uint64_t, std::micro>;
+  std::atomic<uint64_t> value{0};
+};
+
 /// A class that measures elapsed time in an exception safe way.
 ///
 /// This is a RAII class is designed to help gather timing statistics within
 /// LLDB where objects have optional Duration variables that get updated with
 /// elapsed times. This helps LLDB measure statistics for many things that are
 /// then reported in LLDB commands.
 ///
 /// Objects that need to measure elapsed times should have a variable of type
public:
   /// Elapsed time in seconds to increment when this object goes out of scope.
   StatsDuration &m_elapsed_time;
 
 public:
   ElapsedTime(StatsDuration &opt_time) : m_elapsed_time(opt_time) {
     m_start_time = StatsClock::now();
   }
   ~ElapsedTime() {
-    StatsDuration elapsed = StatsClock::now() - m_start_time;
+    StatsClock::duration elapsed = StatsClock::now() - m_start_time;
     m_elapsed_time += elapsed;
   }
 };
 
 /// A class to count success/fail statistics.
 struct StatsSuccessFail {
   StatsSuccessFail(llvm::StringRef n) : name(n.str()) {}
 
public:
   void SetFirstPrivateStopTime();
   void SetFirstPublicStopTime();
 
   StatsDuration &GetCreateTime() { return m_create_time; }
   StatsSuccessFail &GetExpressionStats() { return m_expr_eval; }
   StatsSuccessFail &GetFrameVariableStats() { return m_frame_var; }
 
 protected:
-  StatsDuration m_create_time{0.0};
+  StatsDuration m_create_time;
   llvm::Optional<StatsTimepoint> m_launch_or_attach_time;
   llvm::Optional<StatsTimepoint> m_first_private_stop_time;
   llvm::Optional<StatsTimepoint> m_first_public_stop_time;
   StatsSuccessFail m_expr_eval{"expressionEvaluation"};
   StatsSuccessFail m_frame_var{"frameVariable"};
   std::vector<intptr_t> m_module_identifiers;
   void CollectStats(Target &target);
 };

lldb/source/Breakpoint/Breakpoint.cpp

Breakpoint::BreakpointEventData::GetBreakpointLocationAtIndexFromEvent(
   }
 
   return bp_loc_sp;
 }
 
 json::Value Breakpoint::GetStatistics() {
   json::Object bp;
   bp.try_emplace("id", GetID());
-  bp.try_emplace("resolveTime", m_resolve_time.count());
+  bp.try_emplace("resolveTime", m_resolve_time.get().count());
   bp.try_emplace("numLocations", (int64_t)GetNumLocations());
   bp.try_emplace("numResolvedLocations", (int64_t)GetNumResolvedLocations());
   bp.try_emplace("internal", IsInternal());
   if (!m_kind_description.empty())
     bp.try_emplace("kindDescription", m_kind_description);
   // Put the full structured data for reproducing this breakpoint in a key/value
   // pair named "details". This allows the breakpoint's details to be visible
   // in the stats in case we need to reproduce a breakpoint that has long 

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

GetFunctions(ConstString name, SymbolFileDWARF &dwarf,
                uint32_t name_type_mask,
                llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
   virtual void
   GetFunctions(const RegularExpression &regex,
                llvm::function_ref<bool(DWARFDIE die)> callback) = 0;
 
   virtual void Dump(Stream &s) = 0;
 
-  StatsDuration GetIndexTime() { return m_index_time; }
+  StatsDuration::Duration GetIndexTime() { return m_index_time; }
 
 protected:
   Module &m_module;
-  StatsDuration m_index_time{0.0};
+  StatsDuration m_index_time;
 
   /// Helper function implementing common logic for processing function dies. If
   /// the function given by "ref" matches search criteria given by
   /// "parent_decl_ctx" and "name_type_mask", it is inserted into the "dies"
   /// vector.
   bool ProcessFunctionDIE(llvm::StringRef name, DIERef ref,
                           SymbolFileDWARF &dwarf,
                           const CompilerDeclContext &parent_decl_ctx,

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

public:
   static DWARFDeclContext GetDWARFDeclContext(const DWARFDIE &die);
 
   static lldb::LanguageType LanguageTypeFromDWARF(uint64_t val);
 
   static lldb::LanguageType GetLanguage(DWARFUnit &unit);
   /// Same as GetLanguage() but reports all C++ versions as C++ (no version).
   static lldb::LanguageType GetLanguageFamily(DWARFUnit &unit);
 
-  lldb_private::StatsDuration GetDebugInfoParseTime() override {
+  lldb_private::StatsDuration::Duration GetDebugInfoParseTime() override {
     return m_parse_time;
   }
-  lldb_private::StatsDuration GetDebugInfoIndexTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoIndexTime() override;
 
   lldb_private::StatsDuration &GetDebugInfoParseTimeRef() {
     return m_parse_time;
   }
 
 protected:
   typedef llvm::DenseMap<const DWARFDebugInfoEntry *, lldb_private::Type *>
       DIEToTypePtr;
protected:
   std::vector<uint32_t> m_lldb_cu_to_dwarf_unit;
   /// DWARF does not provide a good way for traditional (concatenating) linkers
   /// to invalidate debug info describing dead-stripped code. These linkers will
   /// keep the debug info but resolve any addresses referring to such code as
   /// zero (BFD) or a small positive integer (zero + relocation addend -- GOLD).
   /// Try to filter out this debug info by comparing it to the lowest code
   /// address in the module.
   lldb::addr_t m_first_code_address = LLDB_INVALID_ADDRESS;
-  lldb_private::StatsDuration m_parse_time{0.0};
+  lldb_private::StatsDuration m_parse_time;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_SYMBOLFILE_DWARF_SYMBOLFILEDWARF_H

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

 
 LanguageType SymbolFileDWARF::GetLanguageFamily(DWARFUnit &unit) {
   auto lang = (llvm::dwarf::SourceLanguage)unit.GetDWARFLanguageType();
   if (llvm::dwarf::isCPlusPlus(lang))
     lang = DW_LANG_C_plus_plus;
   return LanguageTypeFromDWARF(lang);
 }
 
-StatsDuration SymbolFileDWARF::GetDebugInfoIndexTime() {
+StatsDuration::Duration SymbolFileDWARF::GetDebugInfoIndexTime() {
   if (m_index)
     return m_index->GetIndexTime();
-  return StatsDuration(0.0);
+  return {};
 }

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

public:
   ParseCallEdgesInFunction(lldb_private::UserID func_id) override;
 
   void DumpClangAST(lldb_private::Stream &s) override;
 
   // PluginInterface protocol
   llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
 
   uint64_t GetDebugInfoSize() override;
-  lldb_private::StatsDuration GetDebugInfoParseTime() override;
-  lldb_private::StatsDuration GetDebugInfoIndexTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoParseTime() override;
+  lldb_private::StatsDuration::Duration GetDebugInfoIndexTime() override;
 
 protected:
   enum { kHaveInitializedOSOs = (1 << 0), kNumFlags };
 
   friend class DebugMapModule;
   friend class DWARFASTParserClang;
   friend class DWARFCompileUnit;
   friend class SymbolFileDWARF;

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

ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     SectionList *section_list = module_sp->GetSectionList();
     if (section_list)
       debug_info_size += section_list->GetDebugInfoSize();
     return false; // Keep iterating
   });
   return debug_info_size;
 }
 
-StatsDuration SymbolFileDWARFDebugMap::GetDebugInfoParseTime() {
-  StatsDuration elapsed(0.0);
+StatsDuration::Duration SymbolFileDWARFDebugMap::GetDebugInfoParseTime() {
+  StatsDuration::Duration elapsed(0.0);
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     ObjectFile *oso_objfile = oso_dwarf->GetObjectFile();
     if (oso_objfile) {
       ModuleSP module_sp = oso_objfile->GetModule();
       if (module_sp) {
         SymbolFile *symfile = module_sp->GetSymbolFile();
         if (symfile)
           elapsed += symfile->GetDebugInfoParseTime();
       }
     }
     return false; // Keep iterating
   });
   return elapsed;
 }
 
-StatsDuration SymbolFileDWARFDebugMap::GetDebugInfoIndexTime() {
-  StatsDuration elapsed(0.0);
+StatsDuration::Duration SymbolFileDWARFDebugMap::GetDebugInfoIndexTime() {
+  StatsDuration::Duration elapsed(0.0);
   ForEachSymbolFile([&](SymbolFileDWARF *oso_dwarf) -> bool {
     ObjectFile *oso_objfile = oso_dwarf->GetObjectFile();
     if (oso_objfile) {
       ModuleSP module_sp = oso_objfile->GetModule();
       if (module_sp) {
         SymbolFile *symfile = module_sp->GetSymbolFile();
         if (symfile)
           elapsed += symfile->GetDebugInfoIndexTime();
       }
     }
     return false; // Keep iterating
   });
   return elapsed;
 }

lldb/source/Target/Statistics.cpp

else
     obj.try_emplace(key, llvm::json::fixUTF8(str));
 }
 
 json::Value StatsSuccessFail::ToJSON() const {
   return json::Object{{"successes", successes}, {"failures", failures}};
 }
 
 static double elapsed(const StatsTimepoint &start, const StatsTimepoint &end) {
-  StatsDuration elapsed = end.time_since_epoch() - start.time_since_epoch();
+  StatsDuration::Duration elapsed =
+      end.time_since_epoch() - start.time_since_epoch();
   return elapsed.count();
 }
 
 void TargetStats::CollectStats(Target &target) {
   m_module_identifiers.clear();
   for (ModuleSP module_sp : target.GetImages().Modules())
     m_module_identifiers.emplace_back((intptr_t)module_sp.get());
 }
double elapsed_time =
         elapsed(*m_launch_or_attach_time, *m_first_private_stop_time);
     target_metrics_json.try_emplace("launchOrAttachTime", elapsed_time);
   }
   if (m_launch_or_attach_time && m_first_public_stop_time) {
     double elapsed_time =
         elapsed(*m_launch_or_attach_time, *m_first_public_stop_time);
     target_metrics_json.try_emplace("firstStopTime", elapsed_time);
   }
-  target_metrics_json.try_emplace("targetCreateTime", m_create_time.count());
+  target_metrics_json.try_emplace("targetCreateTime",
+                                  m_create_time.get().count());
 
   json::Array breakpoints_array;
   double totalBreakpointResolveTime = 0.0;
   // Rport both the normal breakpoint list and the internal breakpoint list.
   for (int i = 0; i < 2; ++i) {
     BreakpointList &breakpoints = target.GetBreakpointList(i == 1);
     std::unique_lock<std::recursive_mutex> lock;
     breakpoints.GetListMutex(lock);
for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
     module_stat.path = module->GetFileSpec().GetPath();
     if (ConstString object_name = module->GetObjectName()) {
       module_stat.path.append(1, '(');
       module_stat.path.append(object_name.GetStringRef().str());
       module_stat.path.append(1, ')');
     }
     module_stat.uuid = module->GetUUID().GetAsString();
     module_stat.triple = module->GetArchitecture().GetTriple().str();
-    module_stat.symtab_parse_time = module->GetSymtabParseTime().count();
-    module_stat.symtab_index_time = module->GetSymtabIndexTime().count();
+    module_stat.symtab_parse_time = module->GetSymtabParseTime().get().count();
+    module_stat.symtab_index_time = module->GetSymtabIndexTime().get().count();
     Symtab *symtab = module->GetSymtab();
     if (symtab) {
       module_stat.symtab_loaded_from_cache = symtab->GetWasLoadedFromCache();
       if (module_stat.symtab_loaded_from_cache)
         ++symtabs_loaded;
       module_stat.symtab_saved_to_cache = symtab->GetWasSavedToCache();
       if (module_stat.symtab_saved_to_cache)
         ++symtabs_saved;