Skip to content

Commit

Permalink
[lldb][NFCI] DecodedThread::TraceItemStorage::error should own its ow…
Browse files Browse the repository at this point in the history
…n 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
  • Loading branch information
bulbazord committed Jun 8, 2023
1 parent 5df4923 commit 4bae706
Show file tree
Hide file tree
Showing 5 changed files with 10 additions and 10 deletions.
2 changes: 1 addition & 1 deletion lldb/include/lldb/Target/TraceCursor.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
10 changes: 5 additions & 5 deletions lldb/source/Plugins/Trace/intel-pt/DecodedThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions lldb/source/Plugins/Trace/intel-pt/DecodedThread.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
2 changes: 1 addition & 1 deletion lldb/source/Plugins/Trace/intel-pt/TraceCursorIntelPT.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit 4bae706

Please sign in to comment.