[Lldb-commits] [lldb] 4bae706 - [lldb][NFCI] DecodedThread::TraceItemStorage::error should own its own data

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-06-08T12:19:14-07:00
New Revision: 4bae706682d5b621884ff2f7014ba4eb8cb636aa

URL: https://github.com/llvm/llvm-project/commit/4bae706682d5b621884ff2f7014ba4eb8cb636aa
DIFF: https://github.com/llvm/llvm-project/commit/4bae706682d5b621884ff2f7014ba4eb8cb636aa.diff

LOG: [lldb][NFCI] DecodedThread::TraceItemStorage::error should own its own data

The way it works now, it stores a `const char *` that it does not
explicitly own. It's owned by the ConstString StringPool. This is purely
to manage its lifetime, we don't really benefit from deduplication (nor
should we try to, they are errors). We also don't really benefit from
quick comparisons.

This may make the size of TraceItemStorage larger, but you have to pay
the cost of owning the data somewhere. The ConstString StringPool is an
attractive choice but ultimately a poor one.

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

Added: 
    

Modified: 
    lldb/include/lldb/Target/TraceCursor.h
    lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
    lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
    lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
    lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h

Removed: 
    


################################################################################
diff  --git a/lldb/include/lldb/Target/TraceCursor.h b/lldb/include/lldb/Target/TraceCursor.h
index cfc25b0cedc90..f5546d8284d37 100644
--- a/lldb/include/lldb/Target/TraceCursor.h
+++ b/lldb/include/lldb/Target/TraceCursor.h
@@ -217,7 +217,7 @@ class TraceCursor {
 
   /// \return
   ///     The error message the cursor is pointing at.
-  virtual const char *GetError() const = 0;
+  virtual llvm::StringRef GetError() const = 0;
 
   /// \return
   ///     Whether the cursor points to an event or not.

diff  --git a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
index 58cd2290a8b71..17f8f51bdf0e0 100644
--- a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
@@ -186,14 +186,12 @@ void DecodedThread::AppendInstruction(const pt_insn &insn) {
 }
 
 void DecodedThread::AppendError(const IntelPTError &error) {
-  CreateNewTraceItem(lldb::eTraceItemKindError).error =
-      ConstString(error.message()).AsCString();
+  CreateNewTraceItem(lldb::eTraceItemKindError).error = error.message();
   m_error_stats.RecordError(/*fatal=*/false);
 }
 
 void DecodedThread::AppendCustomError(StringRef err, bool fatal) {
-  CreateNewTraceItem(lldb::eTraceItemKindError).error =
-      ConstString(err).AsCString();
+  CreateNewTraceItem(lldb::eTraceItemKindError).error = err.str();
   m_error_stats.RecordError(fatal);
 }
 
@@ -238,7 +236,9 @@ DecodedThread::GetItemKindByIndex(uint64_t item_index) const {
   return static_cast<lldb::TraceItemKind>(m_item_kinds[item_index]);
 }
 
-const char *DecodedThread::GetErrorByIndex(uint64_t item_index) const {
+llvm::StringRef DecodedThread::GetErrorByIndex(uint64_t item_index) const {
+  if (item_index >= m_item_data.size())
+    return llvm::StringRef();
   return m_item_data[item_index].error;
 }
 

diff  --git a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
index b2a674aca1826..5745cdb67ab68 100644
--- a/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
+++ b/lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
@@ -159,7 +159,7 @@ class DecodedThread : public std::enable_shared_from_this<DecodedThread> {
 
   /// \return
   ///   The error associated with a given trace item.
-  const char *GetErrorByIndex(uint64_t item_index) const;
+  llvm::StringRef GetErrorByIndex(uint64_t item_index) const;
 
   /// \return
   ///   The trace item kind given an item index.
@@ -275,7 +275,7 @@ class DecodedThread : public std::enable_shared_from_this<DecodedThread> {
     lldb::TraceEvent event;
 
     /// The string message of this item if it's an error
-    const char *error;
+    std::string error;
   };
 
   /// Create a new trace item.

diff  --git a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
index 32e0bad0d19b9..66d342196cf10 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
@@ -95,7 +95,7 @@ lldb::TraceItemKind TraceCursorIntelPT::GetItemKind() const {
   return m_decoded_thread_sp->GetItemKindByIndex(m_pos);
 }
 
-const char *TraceCursorIntelPT::GetError() const {
+llvm::StringRef TraceCursorIntelPT::GetError() const {
   return m_decoded_thread_sp->GetErrorByIndex(m_pos);
 }
 

diff  --git a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
index 173426faacce1..14240d9d00285 100644
--- a/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
+++ b/lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
@@ -28,7 +28,7 @@ class TraceCursorIntelPT : public TraceCursor {
 
   bool HasValue() const override;
 
-  const char *GetError() const override;
+  llvm::StringRef GetError() const override;
 
   lldb::addr_t GetLoadAddress() const override;