[Lldb-commits] [lldb] 035217f - Allow signposts to take advantage of deferred string substitution

Adrian Prantl via lldb-commits lldb-commits at lists.llvm.org
Mon Jun 14 16:53:49 PDT 2021


Author: Adrian Prantl
Date: 2021-06-14T16:53:41-07:00
New Revision: 035217ff515b8ecdc871e39fa840f3cba1b9cec7

URL: https://github.com/llvm/llvm-project/commit/035217ff515b8ecdc871e39fa840f3cba1b9cec7
DIFF: https://github.com/llvm/llvm-project/commit/035217ff515b8ecdc871e39fa840f3cba1b9cec7.diff

LOG: Allow signposts to take advantage of deferred string substitution

One nice feature of the os_signpost API is that format string
substitutions happen in the consumer, not the logging
application. LLVM's current Signpost class doesn't take advantage of
this though and instead always uses a static "Begin/End %s" format
string.

This patch uses variadic macros to allow the API to be used as
intended. Unfortunately, the primary use-case I had in mind (the
LLDB_SCOPED_TIMER() macro) does not get much better from this, because
__PRETTY_FUNCTION__ is *not* a macro, but a static string, so
signposts created by LLDB_SCOPED_TIMER() still use a static "%s"
format string. At least LLDB_SCOPED_TIMERF() works as intended.

This reapplies the previously reverted patch with additional include
order fixes for non-modular builds of LLDB.

Differential Revision: https://reviews.llvm.org/D103575

Added: 
    

Modified: 
    lldb/include/lldb/Utility/Timer.h
    lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
    lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
    lldb/source/Utility/Timer.cpp
    llvm/include/llvm/Support/Signposts.h
    llvm/lib/Support/Signposts.cpp
    llvm/lib/Support/Timer.cpp

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Utility/Timer.h b/lldb/include/lldb/Utility/Timer.h
index 2b39881de89ec..ae30e719aa419 100644
--- a/lldb/include/lldb/Utility/Timer.h
+++ b/lldb/include/lldb/Utility/Timer.h
@@ -9,11 +9,17 @@
 #ifndef LLDB_UTILITY_TIMER_H
 #define LLDB_UTILITY_TIMER_H
 
-#include "lldb/lldb-defines.h"
+#include "llvm/Config/config.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/Chrono.h"
+#include "llvm/Support/Signposts.h"
 #include <atomic>
 #include <cstdint>
 
+namespace llvm {
+  class SignpostEmitter;
+}
+
 namespace lldb_private {
 class Stream;
 
@@ -72,15 +78,28 @@ class Timer {
   const Timer &operator=(const Timer &) = delete;
 };
 
+llvm::SignpostEmitter &GetSignposts();
+
 } // namespace lldb_private
 
 // Use a format string because LLVM_PRETTY_FUNCTION might not be a string
 // literal.
 #define LLDB_SCOPED_TIMER()                                                    \
   static ::lldb_private::Timer::Category _cat(LLVM_PRETTY_FUNCTION);           \
-  ::lldb_private::Timer _scoped_timer(_cat, "%s", LLVM_PRETTY_FUNCTION)
-#define LLDB_SCOPED_TIMERF(...)                                                \
+  ::lldb_private::Timer _scoped_timer(_cat, "%s", LLVM_PRETTY_FUNCTION);       \
+  SIGNPOST_EMITTER_START_INTERVAL(::lldb_private::GetSignposts(),              \
+                                  &_scoped_timer, "%s", LLVM_PRETTY_FUNCTION); \
+  auto _scoped_signpost = llvm::make_scope_exit([&_scoped_timer]() {           \
+    ::lldb_private::GetSignposts().endInterval(&_scoped_timer);                \
+  })
+
+#define LLDB_SCOPED_TIMERF(FMT, ...)                                           \
   static ::lldb_private::Timer::Category _cat(LLVM_PRETTY_FUNCTION);           \
-  ::lldb_private::Timer _scoped_timer(_cat, __VA_ARGS__)
+  ::lldb_private::Timer _scoped_timer(_cat, FMT, __VA_ARGS__);                 \
+  SIGNPOST_EMITTER_START_INTERVAL(::lldb_private::GetSignposts(),              \
+                                  &_scoped_timer, FMT, __VA_ARGS__);           \
+  auto _scoped_signpost = llvm::make_scope_exit([&_scoped_timer]() {           \
+    ::lldb_private::GetSignposts().endInterval(&_scoped_timer);                \
+  })
 
 #endif // LLDB_UTILITY_TIMER_H

diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index e015de5ba117c..35898da280bde 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -21,6 +21,7 @@
 #include "lldb/Core/Section.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Host/Host.h"
+#include "lldb/Host/SafeMachO.h"
 #include "lldb/Symbol/DWARFCallFrameInfo.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Target/DynamicLoader.h"
@@ -42,8 +43,6 @@
 #include "lldb/Utility/Timer.h"
 #include "lldb/Utility/UUID.h"
 
-#include "lldb/Host/SafeMachO.h"
-
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -64,65 +63,48 @@
 
 #include <memory>
 
+#if LLVM_SUPPORT_XCODE_SIGNPOSTS
 // Unfortunately the signpost header pulls in the system MachO header, too.
-#ifdef CPU_TYPE_ARM
 #undef CPU_TYPE_ARM
-#endif
-#ifdef CPU_TYPE_ARM64
 #undef CPU_TYPE_ARM64
-#endif
-#ifdef CPU_TYPE_ARM64_32
 #undef CPU_TYPE_ARM64_32
-#endif
-#ifdef CPU_TYPE_I386
 #undef CPU_TYPE_I386
-#endif
-#ifdef CPU_TYPE_X86_64
 #undef CPU_TYPE_X86_64
-#endif
-#ifdef MH_DYLINKER
+#undef MH_BINDATLOAD
+#undef MH_BUNDLE
+#undef MH_CIGAM
+#undef MH_CIGAM_64
+#undef MH_CORE
+#undef MH_DSYM
+#undef MH_DYLDLINK
+#undef MH_DYLIB
+#undef MH_DYLIB_STUB
 #undef MH_DYLINKER
-#endif
-#ifdef MH_OBJECT
+#undef MH_DYLINKER
+#undef MH_EXECUTE
+#undef MH_FVMLIB
+#undef MH_INCRLINK
+#undef MH_KEXT_BUNDLE
+#undef MH_MAGIC
+#undef MH_MAGIC_64
+#undef MH_NOUNDEFS
 #undef MH_OBJECT
-#endif
-#ifdef LC_VERSION_MIN_MACOSX
+#undef MH_OBJECT
+#undef MH_PRELOAD
+
+#undef LC_BUILD_VERSION
 #undef LC_VERSION_MIN_MACOSX
-#endif
-#ifdef LC_VERSION_MIN_IPHONEOS
 #undef LC_VERSION_MIN_IPHONEOS
-#endif
-#ifdef LC_VERSION_MIN_TVOS
 #undef LC_VERSION_MIN_TVOS
-#endif
-#ifdef LC_VERSION_MIN_WATCHOS
 #undef LC_VERSION_MIN_WATCHOS
-#endif
-#ifdef LC_BUILD_VERSION
-#undef LC_BUILD_VERSION
-#endif
-#ifdef PLATFORM_MACOS
+
 #undef PLATFORM_MACOS
-#endif
-#ifdef PLATFORM_MACCATALYST
 #undef PLATFORM_MACCATALYST
-#endif
-#ifdef PLATFORM_IOS
 #undef PLATFORM_IOS
-#endif
-#ifdef PLATFORM_IOSSIMULATOR
 #undef PLATFORM_IOSSIMULATOR
-#endif
-#ifdef PLATFORM_TVOS
 #undef PLATFORM_TVOS
-#endif
-#ifdef PLATFORM_TVOSSIMULATOR
 #undef PLATFORM_TVOSSIMULATOR
-#endif
-#ifdef PLATFORM_WATCHOS
 #undef PLATFORM_WATCHOS
-#endif
-#ifdef PLATFORM_WATCHOSSIMULATOR
 #undef PLATFORM_WATCHOSSIMULATOR
 #endif
 

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
index 44d07d44a6da9..42ccfd8ff986b 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -16,7 +16,6 @@
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Utility/RangeMap.h"
 #include "lldb/Utility/RegularExpression.h"
-#include "lldb/Utility/Timer.h"
 
 //#define DEBUG_OSO_DMAP // DO NOT CHECKIN WITH THIS NOT COMMENTED OUT
 #if defined(DEBUG_OSO_DMAP)
@@ -34,6 +33,9 @@
 #include "LogChannelDWARF.h"
 #include "SymbolFileDWARF.h"
 
+// Work around the fact that Timer.h pulls in the system Mach-O headers.
+#include "lldb/Utility/Timer.h"
+
 #include <memory>
 
 using namespace lldb;

diff  --git a/lldb/source/Utility/Timer.cpp b/lldb/source/Utility/Timer.cpp
index 2f3afe4c87037..b59ce3b9f5563 100644
--- a/lldb/source/Utility/Timer.cpp
+++ b/lldb/source/Utility/Timer.cpp
@@ -33,6 +33,8 @@ static std::atomic<Timer::Category *> g_categories;
 /// Allows llvm::Timer to emit signposts when supported.
 static llvm::ManagedStatic<llvm::SignpostEmitter> Signposts;
 
+llvm::SignpostEmitter &lldb_private::GetSignposts() { return *Signposts; }
+
 std::atomic<bool> Timer::g_quiet(true);
 std::atomic<unsigned> Timer::g_display_depth(0);
 static std::mutex &GetFileMutex() {
@@ -59,7 +61,6 @@ void Timer::SetQuiet(bool value) { g_quiet = value; }
 
 Timer::Timer(Timer::Category &category, const char *format, ...)
     : m_category(category), m_total_start(std::chrono::steady_clock::now()) {
-  Signposts->startInterval(this, m_category.GetName());
   TimerStack &stack = GetTimerStackForCurrentThread();
 
   stack.push_back(this);
@@ -86,8 +87,6 @@ Timer::~Timer() {
   auto total_dur = stop_time - m_total_start;
   auto timer_dur = total_dur - m_child_duration;
 
-  Signposts->endInterval(this, m_category.GetName());
-
   TimerStack &stack = GetTimerStackForCurrentThread();
   if (g_quiet && stack.size() <= g_display_depth) {
     std::lock_guard<std::mutex> lock(GetFileMutex());

diff  --git a/llvm/include/llvm/Support/Signposts.h b/llvm/include/llvm/Support/Signposts.h
index d31f3c1e6eb55..6a656fb2e899f 100644
--- a/llvm/include/llvm/Support/Signposts.h
+++ b/llvm/include/llvm/Support/Signposts.h
@@ -18,8 +18,17 @@
 #define LLVM_SUPPORT_SIGNPOSTS_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Config/config.h"
 #include <memory>
 
+#if LLVM_SUPPORT_XCODE_SIGNPOSTS
+#include <Availability.h>
+#include <os/signpost.h>
+#endif
+
+#define SIGNPOSTS_AVAILABLE()                                                  \
+  __builtin_available(macos 10.14, iOS 12, tvOS 12, watchOS 5, *)
+
 namespace llvm {
 class SignpostEmitterImpl;
 
@@ -36,8 +45,33 @@ class SignpostEmitter {
 
   /// Begin a signposted interval for a given object.
   void startInterval(const void *O, StringRef Name);
+
+#if LLVM_SUPPORT_XCODE_SIGNPOSTS
+  os_log_t &getLogger() const;
+  os_signpost_id_t getSignpostForObject(const void *O);
+#endif
+
+  /// A macro to take advantage of the special format string handling
+  /// in the os_signpost API. The format string substitution is
+  /// deferred to the log consumer and done outside of the
+  /// application.
+#if LLVM_SUPPORT_XCODE_SIGNPOSTS
+#define SIGNPOST_EMITTER_START_INTERVAL(SIGNPOST_EMITTER, O, ...)              \
+  do {                                                                         \
+    if ((SIGNPOST_EMITTER).isEnabled())                                        \
+      if (SIGNPOSTS_AVAILABLE())                                               \
+        os_signpost_interval_begin((SIGNPOST_EMITTER).getLogger(),             \
+                                   (SIGNPOST_EMITTER).getSignpostForObject(O), \
+                                   "LLVM Timers", __VA_ARGS__);                \
+  } while (0)
+#else
+#define SIGNPOST_EMITTER_START_INTERVAL(SIGNPOST_EMITTER, O, ...)              \
+  do {                                                                         \
+  } while (0)
+#endif
+
   /// End a signposted interval for a given object.
-  void endInterval(const void *O, StringRef Name);
+  void endInterval(const void *O);
 };
 
 } // end namespace llvm

diff  --git a/llvm/lib/Support/Signposts.cpp b/llvm/lib/Support/Signposts.cpp
index 7bf7b46c1047c..efa283c0ade51 100644
--- a/llvm/lib/Support/Signposts.cpp
+++ b/llvm/lib/Support/Signposts.cpp
@@ -10,19 +10,14 @@
 #include "llvm/Support/Signposts.h"
 #include "llvm/Support/Timer.h"
 
-#include "llvm/Config/config.h"
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/Support/Mutex.h"
-#include <Availability.h>
-#include <os/signpost.h>
 #endif // if LLVM_SUPPORT_XCODE_SIGNPOSTS
 
 using namespace llvm;
 
 #if LLVM_SUPPORT_XCODE_SIGNPOSTS
-#define SIGNPOSTS_AVAILABLE()                                                  \
-  __builtin_available(macos 10.14, iOS 12, tvOS 12, watchOS 5, *)
 namespace {
 os_log_t *LogCreator() {
   os_log_t *X = new os_log_t;
@@ -40,13 +35,13 @@ struct LogDeleter {
 namespace llvm {
 class SignpostEmitterImpl {
   using LogPtrTy = std::unique_ptr<os_log_t, LogDeleter>;
-  using LogTy = LogPtrTy::element_type;
 
   LogPtrTy SignpostLog;
   DenseMap<const void *, os_signpost_id_t> Signposts;
   sys::SmartMutex<true> Mutex;
 
-  LogTy &getLogger() const { return *SignpostLog; }
+public:
+  os_log_t &getLogger() const { return *SignpostLog; }
   os_signpost_id_t getSignpostForObject(const void *O) {
     sys::SmartScopedLock<true> Lock(Mutex);
     const auto &I = Signposts.find(O);
@@ -60,7 +55,6 @@ class SignpostEmitterImpl {
     return Inserted.first->second;
   }
 
-public:
   SignpostEmitterImpl() : SignpostLog(LogCreator()) {}
 
   bool isEnabled() const {
@@ -79,7 +73,7 @@ class SignpostEmitterImpl {
     }
   }
 
-  void endInterval(const void *O, llvm::StringRef Name) {
+  void endInterval(const void *O) {
     if (isEnabled()) {
       if (SIGNPOSTS_AVAILABLE()) {
         // Both strings used here are required to be constant literal strings.
@@ -125,10 +119,17 @@ void SignpostEmitter::startInterval(const void *O, StringRef Name) {
 #endif // if !HAVE_ANY_SIGNPOST_IMPL
 }
 
-void SignpostEmitter::endInterval(const void *O, StringRef Name) {
+#if HAVE_ANY_SIGNPOST_IMPL
+os_log_t &SignpostEmitter::getLogger() const { return Impl->getLogger(); }
+os_signpost_id_t SignpostEmitter::getSignpostForObject(const void *O) {
+  return Impl->getSignpostForObject(O);
+}
+#endif
+
+void SignpostEmitter::endInterval(const void *O) {
 #if HAVE_ANY_SIGNPOST_IMPL
   if (Impl == nullptr)
     return;
-  Impl->endInterval(O, Name);
+  Impl->endInterval(O);
 #endif // if !HAVE_ANY_SIGNPOST_IMPL
 }

diff  --git a/llvm/lib/Support/Timer.cpp b/llvm/lib/Support/Timer.cpp
index 6e592dbc0ba1a..8d421db4f7b15 100644
--- a/llvm/lib/Support/Timer.cpp
+++ b/llvm/lib/Support/Timer.cpp
@@ -174,7 +174,7 @@ void Timer::stopTimer() {
   Running = false;
   Time += TimeRecord::getCurrentTime(false);
   Time -= StartTime;
-  Signposts->endInterval(this, getName());
+  Signposts->endInterval(this);
 }
 
 void Timer::clear() {


        


More information about the lldb-commits mailing list