[lldb] Change ValueObject::AddressOf() to return Expected (NFC) #106831
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-lldb Author: AbdAlRahman Gad (AbdAlRahmanGad) ChangesFull diff: https://github.com/llvm/llvm-project/pull/106831.diff 19 Files Affected:
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
index 93eb3e8f590f4e..2a5358fd5b0276 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -625,7 +625,7 @@ class ValueObject {
/// (e.g. sythetic child provider).
virtual lldb::ValueObjectSP Clone(ConstString new_name);
- virtual lldb::ValueObjectSP AddressOf(Status &error);
+ virtual llvm::Expected<lldb::ValueObjectSP> AddressOf(Status &error);
virtual lldb::addr_t GetLiveAddress() { return LLDB_INVALID_ADDRESS; }
diff --git a/lldb/include/lldb/Core/ValueObjectConstResult.h b/lldb/include/lldb/Core/ValueObjectConstResult.h
index d3b3362bd0e9ec..6ae6f8624cc4af 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResult.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResult.h
@@ -83,7 +83,7 @@ class ValueObjectConstResult : public ValueObject {
uint32_t offset, const CompilerType &type, bool can_create,
ConstString name_const_str = ConstString()) override;
- lldb::ValueObjectSP AddressOf(Status &error) override;
+ llvm::Expected<lldb::ValueObjectSP> AddressOf(Status &error) override;
lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
AddressType *address_type = nullptr) override;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultCast.h b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
index 911a08363b3935..40528150fd852c 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultCast.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultCast.h
@@ -43,7 +43,7 @@ class ValueObjectConstResultCast : public ValueObjectCast {
uint32_t offset, const CompilerType &type, bool can_create,
ConstString name_const_str = ConstString()) override;
- lldb::ValueObjectSP AddressOf(Status &error) override;
+ llvm::Expected<lldb::ValueObjectSP> AddressOf(Status &error) override;
size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
uint32_t item_count = 1) override;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultChild.h b/lldb/include/lldb/Core/ValueObjectConstResultChild.h
index 71a3c53befe786..f6c4689c02ebd0 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultChild.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultChild.h
@@ -49,7 +49,7 @@ class ValueObjectConstResultChild : public ValueObjectChild {
uint32_t offset, const CompilerType &type, bool can_create,
ConstString name_const_str = ConstString()) override;
- lldb::ValueObjectSP AddressOf(Status &error) override;
+ llvm::Expected<lldb::ValueObjectSP> AddressOf(Status &error) override;
lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
AddressType *address_type = nullptr) override;
diff --git a/lldb/include/lldb/Core/ValueObjectConstResultImpl.h b/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
index 68ba8ae7fba206..ff87b7d9898042 100644
--- a/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
+++ b/lldb/include/lldb/Core/ValueObjectConstResultImpl.h
@@ -46,7 +46,7 @@ class ValueObjectConstResultImpl {
bool can_create,
ConstString name_const_str = ConstString());
- lldb::ValueObjectSP AddressOf(Status &error);
+ llvm::Expected<lldb::ValueObjectSP> AddressOf(Status &error);
lldb::addr_t GetLiveAddress() { return m_live_address; }
diff --git a/lldb/source/API/SBValue.cpp b/lldb/source/API/SBValue.cpp
index df0e82b6523fbd..4f86b5d9a7d018 100644
--- a/lldb/source/API/SBValue.cpp
+++ b/lldb/source/API/SBValue.cpp
@@ -1289,8 +1289,14 @@ lldb::SBValue SBValue::AddressOf() {
lldb::ValueObjectSP value_sp(GetSP(locker));
if (value_sp) {
Status error;
- sb_value.SetSP(value_sp->AddressOf(error), GetPreferDynamicValue(),
- GetPreferSyntheticValue());
+ auto address_of_valobj_sp_or_err = value_sp->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ sb_value.SetSP(*address_of_valobj_sp_or_err, GetPreferDynamicValue(),
+ GetPreferSyntheticValue());
}
return sb_value;
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index d56bd004e63c7e..e147417e9fe9e1 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -2204,7 +2204,14 @@ ValueObjectSP ValueObject::GetValueForExpressionPath(
if (*final_task_on_target ==
ValueObject::eExpressionPathAftermathTakeAddress) {
Status error;
- ValueObjectSP final_value = ret_val->AddressOf(error);
+ ValueObjectSP final_value;
+ auto address_of_valobj_sp_or_err = ret_val->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ final_value = *address_of_valobj_sp_or_err;
if (error.Fail() || !final_value.get()) {
if (reason_to_stop)
*reason_to_stop =
@@ -2895,7 +2902,7 @@ ValueObjectSP ValueObject::Dereference(Status &error) {
}
}
-ValueObjectSP ValueObject::AddressOf(Status &error) {
+llvm::Expected<lldb::ValueObjectSP> ValueObject::AddressOf(Status &error) {
if (m_addr_of_valobj_sp)
return m_addr_of_valobj_sp;
diff --git a/lldb/source/Core/ValueObjectConstResult.cpp b/lldb/source/Core/ValueObjectConstResult.cpp
index 879d3c3f6b0372..f4e017bbb425c7 100644
--- a/lldb/source/Core/ValueObjectConstResult.cpp
+++ b/lldb/source/Core/ValueObjectConstResult.cpp
@@ -258,7 +258,8 @@ lldb::ValueObjectSP ValueObjectConstResult::GetSyntheticChildAtOffset(
name_const_str);
}
-lldb::ValueObjectSP ValueObjectConstResult::AddressOf(Status &error) {
+llvm::Expected<lldb::ValueObjectSP>
+ValueObjectConstResult::AddressOf(Status &error) {
return m_impl.AddressOf(error);
}
diff --git a/lldb/source/Core/ValueObjectConstResultCast.cpp b/lldb/source/Core/ValueObjectConstResultCast.cpp
index bf7a12dc682366..0a3d80b695f090 100644
--- a/lldb/source/Core/ValueObjectConstResultCast.cpp
+++ b/lldb/source/Core/ValueObjectConstResultCast.cpp
@@ -40,7 +40,8 @@ lldb::ValueObjectSP ValueObjectConstResultCast::GetSyntheticChildAtOffset(
name_const_str);
}
-lldb::ValueObjectSP ValueObjectConstResultCast::AddressOf(Status &error) {
+llvm::Expected<lldb::ValueObjectSP>
+ValueObjectConstResultCast::AddressOf(Status &error) {
return m_impl.AddressOf(error);
}
diff --git a/lldb/source/Core/ValueObjectConstResultChild.cpp b/lldb/source/Core/ValueObjectConstResultChild.cpp
index 39fc0c9fbb35b8..aaf775601e7107 100644
--- a/lldb/source/Core/ValueObjectConstResultChild.cpp
+++ b/lldb/source/Core/ValueObjectConstResultChild.cpp
@@ -47,7 +47,8 @@ lldb::ValueObjectSP ValueObjectConstResultChild::GetSyntheticChildAtOffset(
name_const_str);
}
-lldb::ValueObjectSP ValueObjectConstResultChild::AddressOf(Status &error) {
+llvm::Expected<lldb::ValueObjectSP>
+ValueObjectConstResultChild::AddressOf(Status &error) {
return m_impl.AddressOf(error);
}
diff --git a/lldb/source/Core/ValueObjectConstResultImpl.cpp b/lldb/source/Core/ValueObjectConstResultImpl.cpp
index 2a7c9077007653..a38931dcc27bdf 100644
--- a/lldb/source/Core/ValueObjectConstResultImpl.cpp
+++ b/lldb/source/Core/ValueObjectConstResultImpl.cpp
@@ -168,7 +168,8 @@ lldb::ValueObjectSP ValueObjectConstResultImpl::GetSyntheticChildAtOffset(
offset, type, can_create, name_const_str);
}
-lldb::ValueObjectSP ValueObjectConstResultImpl::AddressOf(Status &error) {
+llvm::Expected<lldb::ValueObjectSP>
+ValueObjectConstResultImpl::AddressOf(Status &error) {
if (m_address_of_backend.get() != nullptr)
return m_address_of_backend;
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
index f994d025043352..a6bcd3434d477c 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
@@ -786,7 +786,14 @@ void ClangExpressionDeclMap::LookUpLldbClass(NameSearchContext &context) {
if (m_ctx_obj) {
Status status;
- lldb::ValueObjectSP ctx_obj_ptr = m_ctx_obj->AddressOf(status);
+ lldb::ValueObjectSP ctx_obj_ptr;
+ auto address_of_valobj_sp_or_err = m_ctx_obj->AddressOf(status);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ ctx_obj_ptr = *address_of_valobj_sp_or_err;
if (!ctx_obj_ptr || status.Fail())
return;
@@ -888,7 +895,14 @@ void ClangExpressionDeclMap::LookUpLldbObjCClass(NameSearchContext &context) {
if (m_ctx_obj) {
Status status;
- lldb::ValueObjectSP ctx_obj_ptr = m_ctx_obj->AddressOf(status);
+ lldb::ValueObjectSP ctx_obj_ptr;
+ auto address_of_valobj_sp_or_err = m_ctx_obj->AddressOf(status);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ ctx_obj_ptr = *address_of_valobj_sp_or_err;
if (!ctx_obj_ptr || status.Fail())
return;
diff --git a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
index 5e63d1d7b21453..37576dd6c05e8b 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/Coroutines.cpp
@@ -192,7 +192,14 @@ lldb_private::formatters::StdlibCoroutineHandleSyntheticFrontEnd::Update() {
lldb::ValueObjectSP promise = CreateValueObjectFromAddress(
"promise", frame_ptr_addr + 2 * ptr_size, exe_ctx, promise_type);
Status error;
- lldb::ValueObjectSP promisePtr = promise->AddressOf(error);
+ lldb::ValueObjectSP promisePtr;
+ auto address_of_valobj_sp_or_err = promise->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ promisePtr = *address_of_valobj_sp_or_err;
if (error.Success())
m_promise_ptr_sp = promisePtr->Clone(ConstString("promise"));
diff --git a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
index d7cfeb30557c36..f671b31d29a44f 100644
--- a/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
+++ b/lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp
@@ -288,7 +288,14 @@ lldb::ChildCacheState ForwardListFrontEnd::Update() {
AbstractListFrontEnd::Update();
Status err;
- ValueObjectSP backend_addr(m_backend.AddressOf(err));
+ ValueObjectSP backend_addr;
+ auto address_of_valobj_sp_or_err = m_backend.AddressOf(err);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ backend_addr = *address_of_valobj_sp_or_err;
if (err.Fail() || !backend_addr)
return lldb::ChildCacheState::eRefetch;
@@ -400,7 +407,14 @@ lldb::ChildCacheState ListFrontEnd::Update() {
m_node_address = 0;
Status err;
- ValueObjectSP backend_addr(m_backend.AddressOf(err));
+ ValueObjectSP backend_addr;
+ auto address_of_valobj_sp_or_err = m_backend.AddressOf(err);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ backend_addr = *address_of_valobj_sp_or_err;
if (err.Fail() || !backend_addr)
return lldb::ChildCacheState::eRefetch;
m_node_address = backend_addr->GetValueAsUnsigned(0);
diff --git a/lldb/source/Plugins/Language/ObjC/NSArray.cpp b/lldb/source/Plugins/Language/ObjC/NSArray.cpp
index 67d0cd08f51aeb..7f1fa55323f0d4 100644
--- a/lldb/source/Plugins/Language/ObjC/NSArray.cpp
+++ b/lldb/source/Plugins/Language/ObjC/NSArray.cpp
@@ -805,7 +805,13 @@ lldb_private::formatters::NSArraySyntheticFrontEndCreator(
if (flags.IsClear(eTypeIsPointer)) {
Status error;
- valobj_sp = valobj_sp->AddressOf(error);
+ auto address_of_valobj_sp_or_err = valobj_sp->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ valobj_sp = *address_of_valobj_sp_or_err;
if (error.Fail() || !valobj_sp)
return nullptr;
}
diff --git a/lldb/source/Plugins/Language/ObjC/NSDictionary.cpp b/lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
index ec6fd756394a2f..1789c1d3f708ba 100644
--- a/lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
+++ b/lldb/source/Plugins/Language/ObjC/NSDictionary.cpp
@@ -527,7 +527,13 @@ lldb_private::formatters::NSDictionarySyntheticFrontEndCreator(
if (flags.IsClear(eTypeIsPointer)) {
Status error;
- valobj_sp = valobj_sp->AddressOf(error);
+ auto address_of_valobj_sp_or_err = valobj_sp->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ valobj_sp = *address_of_valobj_sp_or_err;
if (error.Fail() || !valobj_sp)
return nullptr;
}
diff --git a/lldb/source/Plugins/Language/ObjC/NSSet.cpp b/lldb/source/Plugins/Language/ObjC/NSSet.cpp
index 7d0a6a507211f8..2f9b4efa3e8b29 100644
--- a/lldb/source/Plugins/Language/ObjC/NSSet.cpp
+++ b/lldb/source/Plugins/Language/ObjC/NSSet.cpp
@@ -347,7 +347,13 @@ lldb_private::formatters::NSSetSyntheticFrontEndCreator(
if (flags.IsClear(eTypeIsPointer)) {
Status error;
- valobj_sp = valobj_sp->AddressOf(error);
+ auto address_of_valobj_sp_or_err = valobj_sp->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ valobj_sp = *address_of_valobj_sp_or_err;
if (error.Fail() || !valobj_sp)
return nullptr;
}
diff --git a/lldb/source/Symbol/Variable.cpp b/lldb/source/Symbol/Variable.cpp
index d4e1ce43ef1f16..c95b969425fe01 100644
--- a/lldb/source/Symbol/Variable.cpp
+++ b/lldb/source/Symbol/Variable.cpp
@@ -361,8 +361,15 @@ Status Variable::GetValuesForVariableExpressionPath(
if (error.Success()) {
for (uint32_t i = 0; i < valobj_list.GetSize();) {
Status tmp_error;
- ValueObjectSP valobj_sp(
- valobj_list.GetValueObjectAtIndex(i)->AddressOf(tmp_error));
+ ValueObjectSP valobj_sp;
+ auto address_of_valobj_sp_or_err =
+ valobj_list.GetValueObjectAtIndex(i)->AddressOf(tmp_error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ valobj_sp = *address_of_valobj_sp_or_err;
if (tmp_error.Fail()) {
variable_list.RemoveVariableAtIndex(i);
valobj_list.RemoveValueObjectAtIndex(i);
diff --git a/lldb/source/Target/StackFrame.cpp b/lldb/source/Target/StackFrame.cpp
index e35a4c318d358f..181ab85fa5982c 100644
--- a/lldb/source/Target/StackFrame.cpp
+++ b/lldb/source/Target/StackFrame.cpp
@@ -1072,7 +1072,14 @@ ValueObjectSP StackFrame::GetValueForVariableExpressionPath(
ValueObjectSP deref_valobj_sp(valobj_sp->Dereference(error));
valobj_sp = deref_valobj_sp;
} else if (address_of) {
- ValueObjectSP address_of_valobj_sp(valobj_sp->AddressOf(error));
+ ValueObjectSP address_of_valobj_sp;
+ auto address_of_valobj_sp_or_err = valobj_sp->AddressOf(error);
+ if (!address_of_valobj_sp_or_err)
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Object),
+ address_of_valobj_sp_or_err.takeError(),
+ "unable to get the address of the value object");
+ else
+ address_of_valobj_sp = *address_of_valobj_sp_or_err;
valobj_sp = address_of_valobj_sp;
}
}
|
|
Are you planning to do this more generally? There are lots of API's that return ValueObjects that might or might not succeed, and it's not clear to me how AddressOf is special in this regard. If we're planning on changing how we hand out ValueObjects more generally in the API then we should probably discuss that (maybe an RFC) and make a plan rather than just dive into it piecemeal. |
|
This change was suggested by @adrian-prantl as good first issue for me. it's similar to this Pull Request. You can read the conversation here https://discourse.llvm.org/t/rich-disassembler-for-lldb/76952/14 |
|
re-ping since It's been more than a week I hope I didn’t come across as too pushy or intrusive. |
No Jim was just out a little last week and is catching up still. I don't have an important opinion here, but I don't know if it adds a lot to have an llvm::Expected for a ValueObjectSP. The methods return an empty shared pointer to indicate that no valueobject was created, e.g.
(my approval is not necessary for this, I'm just throwing in my two cents) |
|
I don't think this is the right way to handle ValueObjects in particular. Since ValueObjects carry their errors along with them, there's really no good excuse ever to return an empty ValueObjectSP. We do that in some places, but it really makes the interface awkward: currently you have to check first whether you got a ValueObjectSP, then you have to check it's error to see if you can use it. I don't want to add yet another way you have to check for errors in ValueObjects but returning Expected. In particular, AddressOf should never be returning an empty ValueObjectSP, since there's always an interesting reason why this failed if it did. If anything we should be asserting at the end of AddressOf if somebody is trying to get away with returning an empty ValueObjectSP. Adding an Expected makes it seem legit to return an empty ValueObjectSP, so that is going in the wrong direction. If we are going to move in the direction of making ValueObject's in particular more secure to use, we should make it so that there are no more interfaces that return ValueObjectSP's, just ValueObjects. I'm not sure whether this would work, because of reference counting objects as they are returned, so we may have to do something cruder. But really the answer is not to let ourselves return empty ValueObjectSP's. That way we'll always be ensured that if producing a ValueObject fails, we'll have a reason. So I don't think that wrapping Expected around ValueObjectSP's is the right way to go here. |
|
You also have to be careful when treating Errors in ValueObjects, since it is not always the case that a ValueObject that returns To be clear, I'm have no problem with rationalizing the error handling for ValueObjects, but we need to have a higher level plan first, I don't think going in piecemeal like this is going to be productive. |
|
As a new contributor, I don't have an opinion on this, but I'm more than happy to help out with additional tasks whenever needed. |
|
This is more an architectural issue. Right now, we're always handing out ValueObjectSP's which means there are two ways of indicating failure, having an empty ValueObjectSP, and having a ValueObjectSP that has a ValueObject in it, but it's Error state is set. That's pretty awkward at times, and we should come up with a better scheme than that, but I'm not sure what that is without thinking more about it. That doesn't seem so much like a new contributor task. However, there are lots of other API's in lldb that fit the pattern Adrian referred to that don't relate to ValueObjects. Those are likely to be much less problematic. |
|
Could you please recommend something to me? If you don't have something specific in mind right now, should I look it up and then ask if it is suitable to work on before I begin? |
The conversion of error handling behaviors was Adrian's project, I haven't thought about it in any detail, so I don't have any suggestions off the bat. Maybe @adrian-prantl has some non-ValueObject candidates in mind? Otherwise, look around and then ask is probably the best course. |
No description provided.