JDevlieghere created this revision.
JDevlieghere added reviewers: labath, davide, jingham.
JDevlieghere added a project: LLDB.
Herald added a subscriber: abidh.
When running the test suite with the instrumentation macros, I noticed two
lldb-mi tests regressed. The issue was the copy constructor of SBLineEntry.
Without the macros the returned value would be elided, but with the macros the
copy constructor was called. The latter used `IsValid` to determine whether the
underlying opaque pointer should be set. This is likely a remnant of when
IsValid would only check the validity of the smart pointer. In SBLineEntry
however, it actually forwards to LineEntry::IsValid().
So what happened here was that because of the macros the copy constructor was
called. The opaque pointer was valid but the LineEntry didn't consider itself
valid. So the copied-to object was default initialized.
This patch replaces all checks for IsValid in copy (assignment) constructors
with checks for the opaque pointer itself.
Repository:
rLLDB LLDB
https://reviews.llvm.org/D58946
Files:
lldb/source/API/SBAddress.cpp
lldb/source/API/SBDeclaration.cpp
lldb/source/API/SBError.cpp
lldb/source/API/SBExpressionOptions.cpp
lldb/source/API/SBLineEntry.cpp
lldb/source/API/SBProcessInfo.cpp
lldb/source/API/SBStringList.cpp
lldb/source/API/SBSymbolContext.cpp
lldb/source/API/SBTypeEnumMember.cpp
Index: lldb/source/API/SBTypeEnumMember.cpp
===================================================================
--- lldb/source/API/SBTypeEnumMember.cpp
+++ lldb/source/API/SBTypeEnumMember.cpp
@@ -22,23 +22,25 @@
SBTypeEnumMember::SBTypeEnumMember() : m_opaque_sp() {}
SBTypeEnumMember::~SBTypeEnumMember() {}
+
SBTypeEnumMember::SBTypeEnumMember(
const lldb::TypeEnumMemberImplSP &enum_member_sp)
: m_opaque_sp(enum_member_sp) {}
SBTypeEnumMember::SBTypeEnumMember(const SBTypeEnumMember &rhs)
: m_opaque_sp() {
- if (this != &rhs) {
- if (rhs.IsValid())
- m_opaque_sp = std::make_shared<TypeEnumMemberImpl>(rhs.ref());
- }
+ m_opaque_sp = std::make_shared<TypeEnumMemberImpl>(rhs.ref());
}
SBTypeEnumMember &SBTypeEnumMember::operator=(const SBTypeEnumMember &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- m_opaque_sp = std::make_shared<TypeEnumMemberImpl>(rhs.ref());
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_sp)
+ m_opaque_sp = std::make_shared<TypeEnumMemberImpl>(*rhs.m_opaque_sp);
+ else
+ m_opaque_sp.reset(); // Mirror the default constructor.
+
return *this;
}
Index: lldb/source/API/SBSymbolContext.cpp
===================================================================
--- lldb/source/API/SBSymbolContext.cpp
+++ lldb/source/API/SBSymbolContext.cpp
@@ -21,38 +21,33 @@
SBSymbolContext::SBSymbolContext(const SymbolContext *sc_ptr) : m_opaque_up() {
if (sc_ptr)
- m_opaque_up.reset(new SymbolContext(*sc_ptr));
+ m_opaque_up = llvm::make_unique<SymbolContext>(*sc_ptr);
}
SBSymbolContext::SBSymbolContext(const SBSymbolContext &rhs) : m_opaque_up() {
- if (rhs.IsValid()) {
- if (m_opaque_up)
- *m_opaque_up = *rhs.m_opaque_up;
- else
- ref() = *rhs.m_opaque_up;
- }
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<SymbolContext>(*rhs.m_opaque_up);
}
SBSymbolContext::~SBSymbolContext() {}
const SBSymbolContext &SBSymbolContext::operator=(const SBSymbolContext &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- m_opaque_up.reset(new lldb_private::SymbolContext(*rhs.m_opaque_up));
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<SymbolContext>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset(); // Mirror the default constructor.
+
return *this;
}
void SBSymbolContext::SetSymbolContext(const SymbolContext *sc_ptr) {
- if (sc_ptr) {
- if (m_opaque_up)
- *m_opaque_up = *sc_ptr;
- else
- m_opaque_up.reset(new SymbolContext(*sc_ptr));
- } else {
- if (m_opaque_up)
- m_opaque_up->Clear(true);
- }
+ if (sc_ptr)
+ m_opaque_up = llvm::make_unique<SymbolContext>(*sc_ptr);
+ else
+ m_opaque_up->Clear(true);
}
bool SBSymbolContext::IsValid() const { return m_opaque_up != NULL; }
Index: lldb/source/API/SBStringList.cpp
===================================================================
--- lldb/source/API/SBStringList.cpp
+++ lldb/source/API/SBStringList.cpp
@@ -18,21 +18,23 @@
SBStringList::SBStringList(const lldb_private::StringList *lldb_strings_ptr)
: m_opaque_up() {
if (lldb_strings_ptr)
- m_opaque_up.reset(new lldb_private::StringList(*lldb_strings_ptr));
+ m_opaque_up = llvm::make_unique<StringList>(*lldb_strings_ptr);
}
SBStringList::SBStringList(const SBStringList &rhs) : m_opaque_up() {
- if (rhs.IsValid())
- m_opaque_up.reset(new lldb_private::StringList(*rhs));
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<StringList>(*rhs.m_opaque_up);
}
const SBStringList &SBStringList::operator=(const SBStringList &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- m_opaque_up.reset(new lldb_private::StringList(*rhs));
- else
- m_opaque_up.reset();
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<StringList>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset(); // Mirror the default constructor.
+
return *this;
}
Index: lldb/source/API/SBProcessInfo.cpp
===================================================================
--- lldb/source/API/SBProcessInfo.cpp
+++ lldb/source/API/SBProcessInfo.cpp
@@ -17,20 +17,21 @@
SBProcessInfo::SBProcessInfo() : m_opaque_up() {}
SBProcessInfo::SBProcessInfo(const SBProcessInfo &rhs) : m_opaque_up() {
- if (rhs.IsValid()) {
- ref() = *rhs.m_opaque_up;
- }
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<ProcessInstanceInfo>(*rhs.m_opaque_up);
}
SBProcessInfo::~SBProcessInfo() {}
SBProcessInfo &SBProcessInfo::operator=(const SBProcessInfo &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- ref() = *rhs.m_opaque_up;
- else
- m_opaque_up.reset();
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<ProcessInstanceInfo>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset(); // Mirror the default constructor.
+
return *this;
}
Index: lldb/source/API/SBLineEntry.cpp
===================================================================
--- lldb/source/API/SBLineEntry.cpp
+++ lldb/source/API/SBLineEntry.cpp
@@ -21,28 +21,30 @@
SBLineEntry::SBLineEntry() : m_opaque_up() {}
SBLineEntry::SBLineEntry(const SBLineEntry &rhs) : m_opaque_up() {
- if (rhs.IsValid())
- ref() = rhs.ref();
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<LineEntry>(*rhs.m_opaque_up);
}
SBLineEntry::SBLineEntry(const lldb_private::LineEntry *lldb_object_ptr)
: m_opaque_up() {
if (lldb_object_ptr)
- ref() = *lldb_object_ptr;
+ m_opaque_up = llvm::make_unique<LineEntry>(*lldb_object_ptr);
}
const SBLineEntry &SBLineEntry::operator=(const SBLineEntry &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- ref() = rhs.ref();
- else
- m_opaque_up.reset();
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<LineEntry>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset();
+
return *this;
}
void SBLineEntry::SetLineEntry(const lldb_private::LineEntry &lldb_object_ref) {
- ref() = lldb_object_ref;
+ m_opaque_up = llvm::make_unique<LineEntry>(lldb_object_ref);
}
SBLineEntry::~SBLineEntry() {}
Index: lldb/source/API/SBExpressionOptions.cpp
===================================================================
--- lldb/source/API/SBExpressionOptions.cpp
+++ lldb/source/API/SBExpressionOptions.cpp
@@ -18,16 +18,24 @@
SBExpressionOptions::SBExpressionOptions()
: m_opaque_up(new EvaluateExpressionOptions()) {}
-SBExpressionOptions::SBExpressionOptions(const SBExpressionOptions &rhs) {
- m_opaque_up.reset(new EvaluateExpressionOptions());
- *(m_opaque_up.get()) = rhs.ref();
+SBExpressionOptions::SBExpressionOptions(const SBExpressionOptions &rhs)
+ : m_opaque_up(new EvaluateExpressionOptions()) {
+ if (rhs.m_opaque_up)
+ m_opaque_up =
+ llvm::make_unique<EvaluateExpressionOptions>(*rhs.m_opaque_up);
}
const SBExpressionOptions &SBExpressionOptions::
operator=(const SBExpressionOptions &rhs) {
- if (this != &rhs) {
- this->ref() = rhs.ref();
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up =
+ llvm::make_unique<EvaluateExpressionOptions>(*rhs.m_opaque_up);
+ else
+ m_opaque_up = llvm::make_unique<EvaluateExpressionOptions>();
+
return *this;
}
Index: lldb/source/API/SBError.cpp
===================================================================
--- lldb/source/API/SBError.cpp
+++ lldb/source/API/SBError.cpp
@@ -19,20 +19,20 @@
SBError::SBError() : m_opaque_up() {}
SBError::SBError(const SBError &rhs) : m_opaque_up() {
- if (rhs.IsValid())
- m_opaque_up.reset(new Status(*rhs));
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Status>(*rhs.m_opaque_up);
}
SBError::~SBError() {}
const SBError &SBError::operator=(const SBError &rhs) {
- if (rhs.IsValid()) {
- if (m_opaque_up)
- *m_opaque_up = *rhs;
- else
- m_opaque_up.reset(new Status(*rhs));
- } else
- m_opaque_up.reset();
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Status>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset(); // Mirror the default constructor.
return *this;
}
Index: lldb/source/API/SBDeclaration.cpp
===================================================================
--- lldb/source/API/SBDeclaration.cpp
+++ lldb/source/API/SBDeclaration.cpp
@@ -21,23 +21,25 @@
SBDeclaration::SBDeclaration() : m_opaque_up() {}
SBDeclaration::SBDeclaration(const SBDeclaration &rhs) : m_opaque_up() {
- if (rhs.IsValid())
- ref() = rhs.ref();
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Declaration>(*rhs.m_opaque_up);
}
SBDeclaration::SBDeclaration(const lldb_private::Declaration *lldb_object_ptr)
: m_opaque_up() {
if (lldb_object_ptr)
- ref() = *lldb_object_ptr;
+ m_opaque_up = llvm::make_unique<Declaration>(*lldb_object_ptr);
}
const SBDeclaration &SBDeclaration::operator=(const SBDeclaration &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- ref() = rhs.ref();
- else
- m_opaque_up.reset();
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Declaration>(*rhs.m_opaque_up);
+ else
+ m_opaque_up.reset(); // Mirror the default constructor.
+
return *this;
}
Index: lldb/source/API/SBAddress.cpp
===================================================================
--- lldb/source/API/SBAddress.cpp
+++ lldb/source/API/SBAddress.cpp
@@ -29,8 +29,8 @@
}
SBAddress::SBAddress(const SBAddress &rhs) : m_opaque_up(new Address()) {
- if (rhs.IsValid())
- ref() = rhs.ref();
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Address>(*rhs.m_opaque_up);
}
SBAddress::SBAddress(lldb::SBSection section, lldb::addr_t offset)
@@ -45,12 +45,14 @@
SBAddress::~SBAddress() {}
const SBAddress &SBAddress::operator=(const SBAddress &rhs) {
- if (this != &rhs) {
- if (rhs.IsValid())
- ref() = rhs.ref();
- else
- m_opaque_up.reset(new Address());
- }
+ if (this == &rhs)
+ return *this;
+
+ if (rhs.m_opaque_up)
+ m_opaque_up = llvm::make_unique<Address>(*rhs.m_opaque_up);
+ else
+ m_opaque_up = llvm::make_unique<Address>();
+
return *this;
}
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
