[LLDB] Add more helper functions to ValueObject class. #87197
Conversation
Create additional helper functions for the ValueObject class, for: - returning the value as an APSInt or APFloat - additional type casting options - additional ways to create ValueObjects from various types of data - dereferencing a ValueObject These helper functions are needed for implementing the Data Inspection Language, described in https://discourse.llvm.org/t/rfc-data-inspection-language/69893
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesCreate additional helper functions for the ValueObject class, for:
These helper functions are needed for implementing the Data Inspection Language, described in Patch is 20.16 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/87197.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Core/ValueObject.h b/lldb/include/lldb/Core/ValueObject.h
index e7e35e2b2bffc0..0c8dbf384a326c 100644
--- a/lldb/include/lldb/Core/ValueObject.h
+++ b/lldb/include/lldb/Core/ValueObject.h
@@ -441,6 +441,19 @@ class ValueObject {
virtual int64_t GetValueAsSigned(int64_t fail_value, bool *success = nullptr);
+ llvm::APSInt GetValueAsAPSInt();
+
+ llvm::APFloat GetValueAsFloat();
+
+ bool GetValueAsBool();
+
+ /// Update the value of the current object to be the integer in the 'value'
+ /// parameter.
+ void UpdateIntegerValue(const llvm::APInt &value);
+
+ /// Assign the integer value 'new_val_sp' to the current object.
+ void UpdateIntegerValue(lldb::ValueObjectSP new_val_sp);
+
virtual bool SetValueFromCString(const char *value_str, Status &error);
/// Return the module associated with this value object in case the value is
@@ -618,6 +631,24 @@ class ValueObject {
virtual lldb::ValueObjectSP CastPointerType(const char *name,
lldb::TypeSP &type_sp);
+ /// Return the target load address assocaited with this value object.
+ lldb::addr_t GetLoadAddress();
+
+ lldb::ValueObjectSP CastDerivedToBaseType(CompilerType type,
+ const std::vector<uint32_t> &idx);
+
+ lldb::ValueObjectSP CastBaseToDerivedType(CompilerType type, uint64_t offset);
+
+ lldb::ValueObjectSP CastScalarToBasicType(CompilerType type, Status &error);
+
+ lldb::ValueObjectSP CastEnumToBasicType(CompilerType type);
+
+ lldb::ValueObjectSP CastPointerToBasicType(CompilerType type);
+
+ lldb::ValueObjectSP CastIntegerOrEnumToEnumType(CompilerType type);
+
+ lldb::ValueObjectSP CastFloatToEnumType(CompilerType type, Status &error);
+
/// If this object represents a C++ class with a vtable, return an object
/// that represents the virtual function table. If the object isn't a class
/// with a vtable, return a valid ValueObject with the error set correctly.
@@ -668,6 +699,32 @@ class ValueObject {
CreateValueObjectFromData(llvm::StringRef name, const DataExtractor &data,
const ExecutionContext &exe_ctx, CompilerType type);
+ static lldb::ValueObjectSP
+ CreateValueObjectFromBytes(lldb::TargetSP target_sp, const void *bytes,
+ CompilerType type);
+
+ static lldb::ValueObjectSP CreateValueObjectFromBytes(lldb::TargetSP target,
+ const void *bytes,
+ lldb::BasicType type);
+
+ static lldb::ValueObjectSP CreateValueObjectFromAPInt(lldb::TargetSP target,
+ const llvm::APInt &v,
+ CompilerType type);
+
+ static lldb::ValueObjectSP
+ CreateValueObjectFromAPFloat(lldb::TargetSP target, const llvm::APFloat &v,
+ CompilerType type);
+
+ static lldb::ValueObjectSP CreateValueObjectFromPointer(lldb::TargetSP target,
+ uintptr_t addr,
+ CompilerType type);
+
+ static lldb::ValueObjectSP CreateValueObjectFromBool(lldb::TargetSP target,
+ bool value);
+
+ static lldb::ValueObjectSP CreateValueObjectFromNullptr(lldb::TargetSP target,
+ CompilerType type);
+
lldb::ValueObjectSP Persist();
/// Returns true if this is a char* or a char[] if it is a char* and
@@ -719,6 +776,10 @@ class ValueObject {
ClearUserVisibleData(eClearUserVisibleDataItemsSummary);
}
+ void SetDerefValobj(ValueObject *deref) { m_deref_valobj = deref; }
+
+ ValueObject *GetDerefValobj() { return m_deref_valobj; }
+
void SetValueFormat(lldb::TypeFormatImplSP format) {
m_type_format_sp = std::move(format);
ClearUserVisibleData(eClearUserVisibleDataItemsValue);
diff --git a/lldb/source/Core/ValueObject.cpp b/lldb/source/Core/ValueObject.cpp
index f39bd07a255366..70cd3bdece8a40 100644
--- a/lldb/source/Core/ValueObject.cpp
+++ b/lldb/source/Core/ValueObject.cpp
@@ -1089,6 +1089,116 @@ int64_t ValueObject::GetValueAsSigned(int64_t fail_value, bool *success) {
return fail_value;
}
+llvm::APSInt ValueObject::GetValueAsAPSInt() {
+ lldb::TargetSP target = GetTargetSP();
+ uint64_t byte_size = 0;
+ if (auto temp = GetCompilerType().GetByteSize(target.get()))
+ byte_size = temp.value();
+
+ unsigned bit_width = static_cast<unsigned>(byte_size * CHAR_BIT);
+ bool success = true;
+ uint64_t fail_value = 0;
+ uint64_t ret_val = GetValueAsUnsigned(fail_value, &success);
+ uint64_t new_value = fail_value;
+ if (success)
+ new_value = ret_val;
+ bool is_signed = GetCompilerType().IsSigned();
+
+ return llvm::APSInt(llvm::APInt(bit_width, new_value, is_signed), !is_signed);
+}
+
+llvm::APFloat ValueObject::GetValueAsFloat() {
+ lldb::BasicType basic_type =
+ GetCompilerType().GetCanonicalType().GetBasicTypeEnumeration();
+ lldb::DataExtractorSP data_sp(new DataExtractor());
+ Status error;
+
+ switch (basic_type) {
+ case lldb::eBasicTypeFloat: {
+ float v = 0;
+ GetData(*data_sp, error);
+ assert(error.Success() && "Unable to read float data from value");
+
+ lldb::offset_t offset = 0;
+ uint32_t old_offset = offset;
+ void *ok = nullptr;
+ ok = data_sp->GetU8(&offset, (void *)&v, sizeof(float));
+ assert(offset != old_offset && ok != nullptr && "unable to read data");
+
+ return llvm::APFloat(v);
+ }
+ case lldb::eBasicTypeDouble:
+ // No way to get more precision at the moment.
+ case lldb::eBasicTypeLongDouble: {
+ double v = 0;
+ GetData(*data_sp, error);
+ assert(error.Success() && "Unable to read long double data from value");
+
+ lldb::offset_t offset = 0;
+ uint32_t old_offset = offset;
+ void *ok = nullptr;
+ ok = data_sp->GetU8(&offset, (void *)&v, sizeof(double));
+ assert(offset != old_offset && ok != nullptr && "unable to read data");
+
+ return llvm::APFloat(v);
+ }
+ default:
+ return llvm::APFloat(NAN);
+ }
+}
+
+bool ValueObject::GetValueAsBool() {
+ CompilerType val_type = GetCompilerType();
+ if (val_type.IsInteger() || val_type.IsUnscopedEnumerationType() ||
+ val_type.IsPointerType()) {
+ return GetValueAsAPSInt().getBoolValue();
+ }
+ if (val_type.IsFloat()) {
+ return GetValueAsFloat().isNonZero();
+ }
+ if (val_type.IsArrayType()) {
+ lldb::ValueObjectSP new_val =
+ ValueObject::ValueObject::CreateValueObjectFromAddress(
+ GetName().GetStringRef(), GetAddressOf(), GetExecutionContextRef(),
+ val_type);
+ return new_val->GetValueAsUnsigned(0) != 0;
+ }
+ return false;
+}
+
+void ValueObject::UpdateIntegerValue(const llvm::APInt &value) {
+ lldb::TargetSP target = GetTargetSP();
+ uint64_t byte_size = 0;
+ if (auto temp = GetCompilerType().GetByteSize(target.get()))
+ byte_size = temp.value();
+
+ assert(value.getBitWidth() == byte_size * CHAR_BIT &&
+ "illegal argument: new value should be of the same size");
+
+ lldb::DataExtractorSP data_sp;
+ Status error;
+ data_sp->SetData(value.getRawData(), byte_size,
+ target->GetArchitecture().GetByteOrder());
+ data_sp->SetAddressByteSize(
+ static_cast<uint8_t>(target->GetArchitecture().GetAddressByteSize()));
+ SetData(*data_sp, error);
+}
+
+void ValueObject::UpdateIntegerValue(lldb::ValueObjectSP new_val_sp) {
+ CompilerType new_val_type = new_val_sp->GetCompilerType();
+ assert((new_val_type.IsInteger() || new_val_type.IsFloat() ||
+ new_val_type.IsPointerType()) &&
+ "illegal argument: new value should be of the same size");
+
+ if (new_val_type.IsInteger()) {
+ UpdateIntegerValue(new_val_sp->GetValueAsAPSInt());
+ } else if (new_val_type.IsFloat()) {
+ UpdateIntegerValue(new_val_sp->GetValueAsFloat().bitcastToAPInt());
+ } else if (new_val_type.IsPointerType()) {
+ UpdateIntegerValue(llvm::APInt(64, new_val_sp->GetValueAsUnsigned(0)));
+ }
+}
+
// if any more "special cases" are added to
// ValueObject::DumpPrintableRepresentation() please keep this call up to date
// by returning true for your new special cases. We will eventually move to
@@ -2809,6 +2919,243 @@ ValueObjectSP ValueObject::CastPointerType(const char *name, TypeSP &type_sp) {
return valobj_sp;
}
+lldb::addr_t ValueObject::GetLoadAddress() {
+ lldb::addr_t addr_value = LLDB_INVALID_ADDRESS;
+ lldb::TargetSP target_sp = GetTargetSP();
+ if (target_sp) {
+ const bool scalar_is_load_address = true;
+ AddressType addr_type;
+ addr_value = GetAddressOf(scalar_is_load_address, &addr_type);
+ if (addr_type == eAddressTypeFile) {
+ lldb::ModuleSP module_sp(GetModule());
+ if (!module_sp)
+ addr_value = LLDB_INVALID_ADDRESS;
+ else {
+ Address tmp_addr;
+ module_sp->ResolveFileAddress(addr_value, tmp_addr);
+ addr_value = tmp_addr.GetLoadAddress(target_sp.get());
+ }
+ } else if (addr_type == eAddressTypeHost || addr_type == eAddressTypeHost)
+ addr_value = LLDB_INVALID_ADDRESS;
+ }
+ return addr_value;
+}
+
+lldb::ValueObjectSP
+ValueObject::CastDerivedToBaseType(CompilerType type,
+ const std::vector<uint32_t> &idx) {
+
+ lldb::TargetSP target = GetTargetSP();
+ assert((type.IsPointerType() || type.IsReferenceType()) &&
+ "invalid ast: target type should be a pointer or a reference");
+ assert(!idx.empty() && "invalid ast: children sequence should be non-empty");
+
+ // The `value` can be a pointer, but GetChildAtIndex works for pointers too.
+ lldb::ValueObjectSP inner_value;
+
+ for (const uint32_t i : idx) {
+ // Force static value, otherwise we can end up with the "real" type.
+ inner_value = GetChildAtIndex(i, /*can_create_synthetic*/ false);
+ }
+
+ // At this point type of `inner_value` should be the dereferenced target type.
+ CompilerType inner_value_type = inner_value->GetCompilerType();
+ if (type.IsPointerType()) {
+ assert(inner_value_type.CompareTypes(type.GetPointeeType()) &&
+ "casted value doesn't match the desired type");
+
+ uintptr_t addr = inner_value->GetLoadAddress();
+ return ValueObject::CreateValueObjectFromPointer(target, addr, type);
+ }
+
+ // At this point the target type should be a reference.
+ assert(inner_value_type.CompareTypes(type.GetNonReferenceType()) &&
+ "casted value doesn't match the desired type");
+
+ return lldb::ValueObjectSP(inner_value->Cast(type.GetNonReferenceType()));
+}
+
+lldb::ValueObjectSP ValueObject::CastBaseToDerivedType(CompilerType type,
+ uint64_t offset) {
+ lldb::TargetSP target = GetTargetSP();
+
+ assert((type.IsPointerType() || type.IsReferenceType()) &&
+ "invalid ast: target type should be a pointer or a reference");
+
+ auto pointer_type =
+ type.IsPointerType() ? type : type.GetNonReferenceType().GetPointerType();
+
+ uintptr_t addr =
+ type.IsPointerType() ? GetValueAsUnsigned(0) : GetLoadAddress();
+
+ lldb::ValueObjectSP value = ValueObject::CreateValueObjectFromPointer(
+ target, addr - offset, pointer_type);
+
+ if (type.IsPointerType()) {
+ return value;
+ }
+
+ // At this point the target type is a reference. Since `value` is a pointer,
+ // it has to be dereferenced.
+ Status error;
+ return value->Dereference(error);
+}
+
+lldb::ValueObjectSP ValueObject::CastScalarToBasicType(CompilerType type,
+ Status &error) {
+ assert(type.IsScalarType() && "target type must be an scalar");
+ assert(GetCompilerType().IsScalarType() && "argument must be a scalar");
+
+ lldb::TargetSP target = GetTargetSP();
+ if (type.IsBoolean()) {
+ if (GetCompilerType().IsInteger()) {
+ return ValueObject::CreateValueObjectFromBool(target,
+ GetValueAsUnsigned(0) != 0);
+ }
+ if (GetCompilerType().IsFloat()) {
+ return ValueObject::CreateValueObjectFromBool(
+ target, !GetValueAsFloat().isZero());
+ }
+ }
+ if (type.IsInteger()) {
+ if (GetCompilerType().IsInteger()) {
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ byte_size = temp.value();
+ llvm::APSInt ext = GetValueAsAPSInt().extOrTrunc(byte_size * CHAR_BIT);
+ return ValueObject::CreateValueObjectFromAPInt(target, ext, type);
+ }
+ if (GetCompilerType().IsFloat()) {
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ byte_size = temp.value();
+ llvm::APSInt integer(byte_size * CHAR_BIT, !type.IsSigned());
+ bool is_exact;
+ llvm::APFloatBase::opStatus status = GetValueAsFloat().convertToInteger(
+ integer, llvm::APFloat::rmTowardZero, &is_exact);
+
+ // Casting floating point values that are out of bounds of the target type
+ // is undefined behaviour.
+ if (status & llvm::APFloatBase::opInvalidOp) {
+ error.SetErrorString("invalid type cast detected");
+ }
+
+ return ValueObject::CreateValueObjectFromAPInt(target, integer, type);
+ }
+ }
+ if (type.IsFloat()) {
+ if (GetCompilerType().IsInteger()) {
+ Scalar scalar_int(GetValueAsAPSInt());
+ llvm::APFloat f = scalar_int.CreateAPFloatFromAPSInt(
+ type.GetCanonicalType().GetBasicTypeEnumeration());
+ return ValueObject::CreateValueObjectFromAPFloat(target, f, type);
+ }
+ if (GetCompilerType().IsFloat()) {
+ Scalar scalar_float(GetValueAsFloat());
+ llvm::APFloat f = scalar_float.CreateAPFloatFromAPFloat(
+ type.GetCanonicalType().GetBasicTypeEnumeration());
+ return ValueObject::CreateValueObjectFromAPFloat(target, f, type);
+ }
+ }
+ assert(false && "invalid target type: must be a scalar");
+ return lldb::ValueObjectSP();
+}
+
+lldb::ValueObjectSP ValueObject::CastEnumToBasicType(CompilerType type) {
+ lldb::TargetSP target = GetTargetSP();
+
+ assert(type.IsScalarType() && "target type must be a scalar");
+ assert(GetCompilerType().IsEnumerationType() && "argument must be an enum");
+
+ if (type.IsBoolean()) {
+ return ValueObject::CreateValueObjectFromBool(target,
+ GetValueAsUnsigned(0) != 0);
+ }
+
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ byte_size = temp.value();
+ // Get the value as APSInt and extend or truncate it to the requested size.
+ llvm::APSInt ext = GetValueAsAPSInt().extOrTrunc(byte_size * CHAR_BIT);
+
+ if (type.IsInteger()) {
+ return ValueObject::CreateValueObjectFromAPInt(target, ext, type);
+ }
+ if (type.IsFloat()) {
+ Scalar scalar_int(ext);
+ llvm::APFloat f = scalar_int.CreateAPFloatFromAPSInt(
+ type.GetCanonicalType().GetBasicTypeEnumeration());
+ return ValueObject::CreateValueObjectFromAPFloat(target, f, type);
+ }
+ assert(false && "invalid target type: must be a scalar");
+ return lldb::ValueObjectSP();
+}
+
+lldb::ValueObjectSP ValueObject::CastPointerToBasicType(CompilerType type) {
+ lldb::TargetSP target = GetTargetSP();
+
+ uint64_t type_byte_size = 0;
+ uint64_t val_byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ type_byte_size = temp.value();
+ if (auto temp = GetCompilerType().GetByteSize(target.get()))
+ val_byte_size = temp.value();
+ assert(type.IsInteger() && "target type must be an integer");
+ assert((type.IsBoolean() || type_byte_size >= val_byte_size) &&
+ "target type cannot be smaller than the pointer type");
+
+ if (type.IsBoolean()) {
+ return ValueObject::CreateValueObjectFromBool(target,
+ GetValueAsUnsigned(0) != 0);
+ }
+
+ // Get the value as APSInt and extend or truncate it to the requested size.
+ llvm::APSInt ext = GetValueAsAPSInt().extOrTrunc(type_byte_size * CHAR_BIT);
+ return ValueObject::CreateValueObjectFromAPInt(target, ext, type);
+}
+
+lldb::ValueObjectSP
+ValueObject::CastIntegerOrEnumToEnumType(CompilerType type) {
+ lldb::TargetSP target = GetTargetSP();
+
+ assert(type.IsEnumerationType() && "target type must be an enum");
+ assert((GetCompilerType().IsInteger() ||
+ GetCompilerType().IsEnumerationType()) &&
+ "argument must be an integer or an enum");
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ byte_size = temp.value();
+
+ // Get the value as APSInt and extend or truncate it to the requested size.
+ llvm::APSInt ext = GetValueAsAPSInt().extOrTrunc(byte_size * CHAR_BIT);
+ return ValueObject::CreateValueObjectFromAPInt(target, ext, type);
+}
+
+lldb::ValueObjectSP ValueObject::CastFloatToEnumType(CompilerType type,
+ Status &error) {
+ lldb::TargetSP target = GetTargetSP();
+
+ assert(type.IsEnumerationType() && "target type must be an enum");
+ assert(GetCompilerType().IsFloat() && "argument must be a float");
+
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target.get()))
+ byte_size = temp.value();
+ llvm::APSInt integer(byte_size * CHAR_BIT, !type.IsSigned());
+ bool is_exact;
+
+ llvm::APFloatBase::opStatus status = GetValueAsFloat().convertToInteger(
+ integer, llvm::APFloat::rmTowardZero, &is_exact);
+
+ // Casting floating point values that are out of bounds of the target type
+ // is undefined behaviour.
+ if (status & llvm::APFloatBase::opInvalidOp) {
+ error.SetErrorString("invalid type cast detected");
+ }
+
+ return ValueObject::CreateValueObjectFromAPInt(target, integer, type);
+}
+
ValueObject::EvaluationPoint::EvaluationPoint() : m_mod_id(), m_exe_ctx_ref() {}
ValueObject::EvaluationPoint::EvaluationPoint(ExecutionContextScope *exe_scope,
@@ -3031,6 +3378,64 @@ lldb::ValueObjectSP ValueObject::CreateValueObjectFromData(
return new_value_sp;
}
+lldb::ValueObjectSP
+ValueObject::CreateValueObjectFromBytes(lldb::TargetSP target_sp,
+ const void *bytes, CompilerType type) {
+ ExecutionContext exe_ctx(
+ ExecutionContextRef(ExecutionContext(target_sp.get(), false)));
+ uint64_t byte_size = 0;
+ if (auto temp = type.GetByteSize(target_sp.get()))
+ byte_size = temp.value();
+ lldb::DataExtractorSP data_sp = std::make_shared<DataExtractor>(
+ bytes, byte_size, target_sp->GetArchitecture().GetByteOrder(),
+ static_cast<uint8_t>(target_sp->GetArchitecture().GetAddressByteSize()));
+ lldb::ValueObjectSP value =
+ ValueObject::CreateValueObjectFromData("result", *data_sp, exe_ctx, type);
+ return value;
+}
+
+lldb::ValueObjectSP ValueObject::CreateValueObjectFromBytes(
+ lldb::TargetSP target, const void *bytes, lldb::BasicType type) {
+ CompilerType target_type;
+ if (target) {
+ for (auto type_system_sp : target->GetScratchTypeSystems())
+ if (auto compiler_type = type_system_sp->GetBasicTypeFromAST(type)) {
+ target_type = compiler_type;
+ break;
+ }
+ }
+ return CreateValueObjectFromBytes(target, bytes, target_type);
+}
+
+lldb::ValueObjectSP ValueObject::CreateValueObjectFromAPInt(
+ lldb::TargetSP target, const llvm::APInt &v, CompilerType type) {
+ return CreateValueObjectFromBytes(target, v.getRawData(), type);
+}
+
+lldb::ValueObjectSP ValueObject::CreateValueObjectFromAPFloat(
+ lldb::TargetSP target, const llvm::APFloat &v, CompilerType type) {
+ return CreateValueObjectFromAPInt(target, v.bitcastToAPInt(), type);
+}
+
+lldb::ValueObjectSP
+ValueObject::CreateValueObjectFromPointer(lldb::TargetSP target, uintptr_t addr,
+ CompilerType type) {
+ return CreateValueObjectFromBytes(target, &addr, type);
+}
+
+lldb::ValueObjectSP
+ValueObject::CreateValueObjectFromBool(lldb::TargetSP target, bool value) {
+ return CreateValueObjectFromBytes(target, &value, lldb::eBasicTypeBool);
+}
+
+lldb::ValueObjectSP
+ValueObject::CreateValueObjectFromNullptr(lldb::TargetSP target,
+ CompilerType type) {
+ assert(type.IsNullPtrType() && "target type must be nullptr");
+ uintptr_t z...
[truncated]
|
lldb/include/lldb/Core/ValueObject.h
Outdated
| @@ -441,6 +441,19 @@ class ValueObject { | |||
|
|
|||
| virtual int64_t GetValueAsSigned(int64_t fail_value, bool *success = nullptr); | |||
|
|
|||
| llvm::APSInt GetValueAsAPSInt(); | |||
There was a problem hiding this comment.
Not all SBValues are representable as Int's or Floats, Bool's etc. But I don't see any way to report errors with these new API's.
There was a problem hiding this comment.
if these errors matter to users then llvm::Expected<llvm::APSInt> would be the right choice. Otherwise std::optional is the way to go.
lldb/include/lldb/Core/ValueObject.h
Outdated
|
|
||
| lldb::ValueObjectSP CastIntegerOrEnumToEnumType(CompilerType type); | ||
|
|
||
| lldb::ValueObjectSP CastFloatToEnumType(CompilerType type, Status &error); |
There was a problem hiding this comment.
You don't need an error parameter for anything that returns a ValueObject. ValueObjects carry their errors in the ValueObject (ValueObject::GetError()).
lldb/include/lldb/Core/ValueObject.h
Outdated
| @@ -668,6 +699,32 @@ class ValueObject { | |||
| CreateValueObjectFromData(llvm::StringRef name, const DataExtractor &data, | |||
| const ExecutionContext &exe_ctx, CompilerType type); | |||
|
|
|||
| static lldb::ValueObjectSP | |||
There was a problem hiding this comment.
Given targets can be big or little-endian, I'm a little worried about a "const void *bytes" that says nothing about what is in the bytes. This is what DataBuffer's are for, why couldn't you use them?
There was a problem hiding this comment.
I agree on this, but why can't we just use the above CreateValueObjectFromData(llvm::StringRef name, const DataExtractor &data, const ExecutionContext &exe_ctx, CompilerType type);? The DataExtractor in the mentioned existing API can own or share a pointer to data. Would the "const void *bytes" always be copied into the internal data in the ValueObject in this case?
There was a problem hiding this comment.
CreateValueObjectFromBytes was being called from CreateValueObjectfrom{APInt,Pointer,Bool,Nullptr}, receivng the pointer to the appropriate values in the input parameter, putting the bytes into a DataExtractor, then calling CreateValueObjectFromData with the DataExtractor. I thought that would handle things like correct endianness, etc. However, I will move the "putting the bytes into the DataExtractor" into the callers for CreateValueObjectFromBytes & call CreateValueObjectFromData directly from each of them. This leads to a bit of code duplication, but I can see that it's safer. (This eliminates the CreateValueObjectFromBytes functions altogether).
lldb/include/lldb/Core/ValueObject.h
Outdated
| @@ -441,6 +441,19 @@ class ValueObject { | |||
|
|
|||
| virtual int64_t GetValueAsSigned(int64_t fail_value, bool *success = nullptr); | |||
|
|
|||
| llvm::APSInt GetValueAsAPSInt(); | |||
|
|
|||
| llvm::APFloat GetValueAsFloat(); | |||
lldb/include/lldb/Core/ValueObject.h
Outdated
|
|
||
| llvm::APFloat GetValueAsFloat(); | ||
|
|
||
| bool GetValueAsBool(); |
lldb/include/lldb/Core/ValueObject.h
Outdated
| CreateValueObjectFromBytes(lldb::TargetSP target_sp, const void *bytes, | ||
| CompilerType type); | ||
|
|
||
| static lldb::ValueObjectSP CreateValueObjectFromBytes(lldb::TargetSP target, |
There was a problem hiding this comment.
These functions could all benefit from Doxygen comments. If we can use a StringRef, or DataExtractor/Buffer instead a raw pointer, that would likely also be safer.
lldb/include/lldb/Core/ValueObject.h
Outdated
| CreateValueObjectFromAPFloat(lldb::TargetSP target, const llvm::APFloat &v, | ||
| CompilerType type); | ||
|
|
||
| static lldb::ValueObjectSP CreateValueObjectFromPointer(lldb::TargetSP target, |
There was a problem hiding this comment.
How is this different from CreateValueObjectFromAddress?
There was a problem hiding this comment.
I agree, we already have this API unless this is doing things differently. That other API allows a name to be specified and is used for synthetic children, so you should be able to use it and specify no name
There was a problem hiding this comment.
"CreateValueObjectFromPointer" is different from "CreateValueObjectFromAddresss" In that the former creates a value object whose value is the pointer address; the latter creates a value object whose value is whatever the pointer points to (it dereferences the pointer). I can update CreateValueObjectFromAddress to take a parameter telling it whether to dereference the pointer or use the pointer address as the value, if you would prefer.
| @@ -719,6 +776,10 @@ class ValueObject { | |||
| ClearUserVisibleData(eClearUserVisibleDataItemsSummary); | |||
| } | |||
|
|
|||
| void SetDerefValobj(ValueObject *deref) { m_deref_valobj = deref; } | |||
There was a problem hiding this comment.
Synthetic child providers also have a way to provide the deref ValueObject dynamically. How do these two ways of doing that job work together?
There was a problem hiding this comment.
I use the synthetic child providers way of getting the deref ValueObject to get the value I use for my call to SetDerefValobj (when I'm converting a ValueObject containing a SmartPointer into a ValueObject containing the pointer the SmartPointer was referencing).
There was a problem hiding this comment.
+1 to all of Jim and Adrian's comments.
High level comments:
-
I'm a little concerned about the use of assertions in this patch. I generally like assertions when they are used to assert internal consistency. It looks like you're using them as an error-checking and parameter-checking mechanism. This seems pretty fragile, especially since it will be difficult to enforce that these functions are called "correctly" in all cases from all contributors.
-
There isn't a lot of documentation. It would be helpful to know the expected behavior for a lot of these methods (especially for people like me who aren't super familiar with the ValueObject class and all of its tendrils).
-
Is there a way you can add some tests for the new functionality? I'm not sure how you could trigger these new methods from an API test, but a unit test might be possible?
lldb/source/Core/ValueObject.cpp
Outdated
| new_value = ret_val; | ||
| bool is_signed = GetCompilerType().IsSigned(); | ||
|
|
||
| return llvm::APSInt(llvm::APInt(bit_width, new_value, is_signed), !is_signed); |
There was a problem hiding this comment.
If GetValueAsUnsigned fails, this will give you an APSInt that looks like 0, meaning there's no way to distinguish between a ValueObject that represents 0 and a failure value.
Suggestion: Either return an invalid APSInt (via default constructor) or return a std::optional<APSInt> or llvm::Expected<APSInt> if you want an error message back.
There was a problem hiding this comment.
The call to GetValueAsUnsigned included a boolean, 'success' which would be set correctly if the call fails, and which I was checking upon return. But, if I use Greg's code from above then I omit the call to GetValueAsUnsigned, anyway. And yes, I am converting these GetValueAs... functions to return an llvm::Expected<> value.
lldb/source/Core/ValueObject.cpp
Outdated
| return llvm::APFloat(v); | ||
| } | ||
| default: | ||
| return llvm::APFloat(NAN); |
There was a problem hiding this comment.
Where is NAN coming from? I don't see it in this file or the APFloat header.
There was a problem hiding this comment.
I think it was coming from a stdlib definition; but I'm updating the code to set an error message here instead.
lldb/source/Core/ValueObject.cpp
Outdated
| return llvm::APSInt(llvm::APInt(bit_width, new_value, is_signed), !is_signed); | ||
| } | ||
|
|
||
| llvm::APFloat ValueObject::GetValueAsFloat() { |
There was a problem hiding this comment.
2 issues and a small nitpick:
Nit: The name could be GetValueAsAPFloat to differentiate between it and a potential function that would return a float/double?
- I don't think assertions are the right way to make sure we have good data.
GetDatahas error handling through itsStatus &out parameter, we should handle that appropriately instead of asserting that we got good data. What happens when we compile out the assertions for release builds? Reading data from theDataExtractorwill fail and crash regardless. - This function has no way of returning a value that would indicate failure.
Suggestion: Return std::optional<llvm::APFloat> or llvm::Expected<llvm::APFloat>. There's no way to return an invalid APFloat from looking at the header, so that doesn't appear to be an option. Then if any of the steps fail, you can return either std::nullopt or an appropriate llvm::Error.
lldb/source/Core/ValueObject.cpp
Outdated
| if (auto temp = GetCompilerType().GetByteSize(target.get())) | ||
| byte_size = temp.value(); | ||
|
|
||
| assert(value.getBitWidth() == byte_size * CHAR_BIT && |
There was a problem hiding this comment.
Instead of asserting this, why not have this function return an llvm::Error? If this invariant is not met, this can return this exact error message.
There was a problem hiding this comment.
yes, no asserts and add error propagation.
lldb/source/Core/ValueObject.cpp
Outdated
| assert(type.IsEnumerationType() && "target type must be an enum"); | ||
| assert((GetCompilerType().IsInteger() || | ||
| GetCompilerType().IsEnumerationType()) && | ||
| "argument must be an integer or an enum"); |
lldb/source/Core/ValueObject.cpp
Outdated
| lldb::TargetSP target = GetTargetSP(); | ||
|
|
||
| assert(type.IsEnumerationType() && "target type must be an enum"); | ||
| assert(GetCompilerType().IsFloat() && "argument must be a float"); |
lldb/source/Core/ValueObject.cpp
Outdated
| lldb::ValueObjectSP | ||
| ValueObject::CreateValueObjectFromNullptr(lldb::TargetSP target, | ||
| CompilerType type) { | ||
| assert(type.IsNullPtrType() && "target type must be nullptr"); |
lldb/include/lldb/Core/ValueObject.h
Outdated
|
|
||
| lldb::ValueObjectSP CastEnumToBasicType(CompilerType type); | ||
|
|
||
| lldb::ValueObjectSP CastPointerToBasicType(CompilerType type); |
There was a problem hiding this comment.
What is BasicType in this context? I know there's lldb::BasicType, but these methods take CompilerTypes.
lldb/include/lldb/Core/ValueObject.h
Outdated
| CreateValueObjectFromBytes(lldb::TargetSP target_sp, const void *bytes, | ||
| CompilerType type); | ||
|
|
||
| static lldb::ValueObjectSP CreateValueObjectFromBytes(lldb::TargetSP target, |
lldb/include/lldb/Core/ValueObject.h
Outdated
|
|
||
| llvm::APFloat GetValueAsFloat(); | ||
|
|
||
| bool GetValueAsBool(); |
There was a problem hiding this comment.
Do we need GetValueAsBool() or can we just use GetValueAsAPSInt() and make sure bool gets converted to a suitable integer?
There was a problem hiding this comment.
I would prefer to have GetValueAsBool(). Otherwise I will need to do integer-to-bool conversions in the places where I call this function; also it makes the code more clear, readable and maintainable. Also, my function handles more object types than just integers. But if you feel very strongly about this, I can fall back on GetValueAsAPSInt.
lldb/include/lldb/Core/ValueObject.h
Outdated
|
|
||
| /// Update the value of the current object to be the integer in the 'value' | ||
| /// parameter. | ||
| void UpdateIntegerValue(const llvm::APInt &value); |
There was a problem hiding this comment.
Should we name this similar to SetValueFromCString(...)? Something like: SetValue(const llvm::APInt &value) or SetValueFromInteger(const llvm::APInt &value)? Do we want an error to be returned in case this fails?
lldb/include/lldb/Core/ValueObject.h
Outdated
| /// Assign the integer value 'new_val_sp' to the current object. | ||
| void UpdateIntegerValue(lldb::ValueObjectSP new_val_sp); |
There was a problem hiding this comment.
What does this function actually do? Are we wanting the current ValueObject to copy the integer value from lldb::ValueObjectSP new_val_sp? Seems like we should just be able to use the above void UpdateIntegerValue(const llvm::APInt &value);? Can you elaborate on why this is needed?
There was a problem hiding this comment.
Yes, this assigns the value from new_val_sp to the current (integer) ValueObject. The main part of this function does call the preceding one. Because new_val_sp can actually contain an integer, a float or a pointer, there's some slightly different manipulations of the value that needs to happen before calling the preceding function (take a look at the code in ValueObject.cpp), which is why this function was written.
| @@ -618,6 +631,24 @@ class ValueObject { | |||
| virtual lldb::ValueObjectSP CastPointerType(const char *name, | |||
| lldb::TypeSP &type_sp); | |||
|
|
|||
| /// Return the target load address assocaited with this value object. | |||
| lldb::addr_t GetLoadAddress(); | |||
There was a problem hiding this comment.
Is this function just a copy of the contents of lldb::addr_t SBValue::GetLoadAddress()? If so, we should also fix that function to call this function. Might be nice to have this return std::optional<lldb::addr_t> for internal use as if the ValueObject is a variable that is in a register, this will need to return LLDB_INVALID_ADDRESS.
There was a problem hiding this comment.
Yes, this is based on SBValue::GetLoadAddress; I'll update that one to call this one. As written, it already does return LLDB_INVALID_ADDRESS in certain cases...I'm not sure what you want me to do differently (probably better to discuss this at the implementation rather than the declaration).
lldb/include/lldb/Core/ValueObject.h
Outdated
| lldb::addr_t GetLoadAddress(); | ||
|
|
||
| lldb::ValueObjectSP CastDerivedToBaseType(CompilerType type, | ||
| const std::vector<uint32_t> &idx); |
There was a problem hiding this comment.
Maybe use const llvm::ArrayRef<uint32_t> instead of const std::vector<uint32_t>? Some headerdoc on what this does might be nice. I would guess idx contains a base class index path? Maybe renamed idx to something more descriptive?
lldb/source/Core/ValueObject.cpp
Outdated
| if (val_type.IsArrayType()) { | ||
| lldb::ValueObjectSP new_val = | ||
| ValueObject::ValueObject::CreateValueObjectFromAddress( | ||
| GetName().GetStringRef(), GetAddressOf(), GetExecutionContextRef(), | ||
| val_type); | ||
| return new_val->GetValueAsUnsigned(0) != 0; | ||
| } |
There was a problem hiding this comment.
What is this attempting to do? It looks like this is taking an array type and returning an array pointer and trying to return true if the array is in memory?
There was a problem hiding this comment.
Hm...What I was trying to do (and I can see I didn't do it well here) is to check to see if the array had a real value(pointer address) or not. I'm updating the code to be cleaner and more clear.
lldb/source/Core/ValueObject.cpp
Outdated
| if (auto temp = GetCompilerType().GetByteSize(target.get())) | ||
| byte_size = temp.value(); | ||
|
|
||
| assert(value.getBitWidth() == byte_size * CHAR_BIT && |
There was a problem hiding this comment.
yes, no asserts and add error propagation.
lldb/source/Core/ValueObject.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| void ValueObject::UpdateIntegerValue(const llvm::APInt &value) { |
There was a problem hiding this comment.
If I look at this API, I would expect this function to change the integer value of any ValueObject. The value object might have a variable that lives in memory (on the stack or heap), or if it lives in a register (update the register) or if there is already a local buffer, replace the bytes in the local buffer.
This function seems to replace the integer value always with a local buffer?
There was a problem hiding this comment.
The intent is to only call this function on ValueObjects that are intermediate objects in the new parser/evaluator, not ValueObjects that are associated with program variables. I'll add a check for that while I'm updating the other asserts & error checks. So no, this function should not update values in memory, heap or register, only in the ValueObject itself.
lldb/source/Core/ValueObject.cpp
Outdated
| lldb::DataExtractorSP data_sp; | ||
| Status error; | ||
| data_sp->SetData(value.getRawData(), byte_size, | ||
| target->GetArchitecture().GetByteOrder()); |
There was a problem hiding this comment.
This will crash, data_sp is never constructed or assigned. We also need to figure out if we are going to want the integer value to be replaced always with a local copy, or updated in the actual program memory, register, or local buffer.
I realize many of these APIs are probably only intended to be used in the new ValueObject based expression parser, but there are APIs being added to ValueObject so they should work regardless of where they come from or originate.
There was a problem hiding this comment.
Just for the record: This call is not crashing for me; I am calling this code in my light-weight parser/evaluator, and it seems to work just fine.
I am ok with changing this code, if you really think it should be changed, but I'd appreciate suggestions on how to make this work "properly" -- I've been looking through ValueObject.cpp & ValueObject.h and not seeing much that looks helpful to me. This does seem to be the way SBValue::SetData works (which was the original basis for this function...)
lldb/source/Core/ValueObject.cpp
Outdated
| module_sp->ResolveFileAddress(addr_value, tmp_addr); | ||
| addr_value = tmp_addr.GetLoadAddress(target_sp.get()); | ||
| } | ||
| } else if (addr_type == eAddressTypeHost || addr_type == eAddressTypeHost) |
There was a problem hiding this comment.
Double check for eAddressTypeHost, I am guessing the second one was meant to be eAddressTypeInvalid. We should also change lldb::addr_t SBValue::GetLoadAddress() to call this function
|
How does this do error handling? If I pass in new_val_sp which contains a "struct Foo" - so really not convertible to an integer, this will fail. Presumably when that happens, you just back out of setting the original ValueObject's value? So it doesn't seem appropriate to use that ValueObject's Status object to report the error. And it wasn't new_value_sp's fault you used it incorrectly, so you also can't change its Status.
So I think in this case you do need to pass in an Status object.
In general, it seems like these additions have not thought very much about error reporting...
Jim
… On Apr 2, 2024, at 8:38 PM, cmtice ***@***.***> wrote:
@cmtice commented on this pull request.
In lldb/include/lldb/Core/ValueObject.h <#87197 (comment)>:
> + /// Assign the integer value 'new_val_sp' to the current object.
+ void UpdateIntegerValue(lldb::ValueObjectSP new_val_sp);
Yes, this assigns the value from new_val_sp to the current (integer) ValueObject. The main part of this function does call the preceding one. Because new_val_sp can actually contain an integer, a float or a pointer, there's some slightly different manipulations of the value that needs to happen before calling the preceding function (take a look at the code in ValueObject.cpp), which is why this function was written.
—
Reply to this email directly, view it on GitHub <#87197 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW5CJAYIC6EDCGLDVYTY3N2RZAVCNFSM6AAAAABFQTAPTWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZVGQZDEMBRHE>.
You are receiving this because you are on a team that was mentioned.
|
|
Most of my error checking was being done in the code that calls these functions, rather than in the functions directly, but I see your point. I will update the functions to do the error checking & reporting directly themselves. |
Fix various issues in GetValueAsAPSInt, GetValueAsAPFloat, GetValueAsBool, UpdateIntegerValue (renamed to SetValueFromInteger), and GetLoadAddress: - Added error checking & reporting. - Removed asserts. - Added doxygen comments. - Renamed UpdateIntegerValue to SetValueFromInteger - Renamed GetValueAsFloat to GetValueAsAPFloat - Updated code in GetValue... functions to use existing Scalar values where possibled. - Updated SBValue::GetLoadAddress to call ValueObject::GetLoadAddress.
|
I think I've addressed all the review comments for GetValueAsAPSInt, GetValueAsAPFloat, GetValueAsBool, UpdateIntegerValue (now SetValueFromInteger) and GetLoadAddress. I apologize if I missed anything. Please take another look at those functions. I'm still working on fixing the issues with the other functions. :-) |
- Update CreateValueObjectFromAddress to optionally not dereference the address before creating the value (i.e. create a value object containing the addres itself). This removes the need for CreateValueObjectFromPointer. - Remove CreateValueObjectFromPointer - Remove CreateValueObjectFromBytes functions, moving the DataExtractor into the callers & having them call CreateValueObjectFromData directly. - Add 'name' parameter to the next CreateValueObjectFrom... functions. - Remove assert stetement. - Add Doxygen comments.
|
I've now addressed the comments for the "CreateValueObjectFrom..." functions (I think). I will not be able to work on this for the next couple of weeks, but I'll get back to it and fix the rest as soon as I can. |
Add doxygen comments for cast functions; reduce number of cast functions by combining some; fix error reporting in cast functions and remove all assertions.
|
I'm sorry this has taken me so long, but I believe I have addressed/fixed everyone's comments now; if I missed any please let me know. Please review this PR again. |
Fix remaining issues wit 'cast' functions: - Update parameter type checking - Update error messages & error status checking - Correctly update 'inner_value' in 'CastDerivedToBaseType'
|
I've updated with a few minor fixes. I've addressed all the review comments (I think). I've verified/tested that all the casting, etc. works properly. Please review/approve this. Thanks! |
lldb/source/Core/ValueObject.cpp
Outdated
|
|
||
| for (const uint32_t i : base_type_indices) | ||
| // Force static value, otherwise we can end up with the "real" type. | ||
| inner_value = GetChildAtIndex(i, /*can_create_synthetic*/ false); |
There was a problem hiding this comment.
I don't understand this change. How is getting this->GetSP()->GetChildAtIndex different from this->GetChildAtIndex?
The other change here seems to be to change can_create_synthetic from false to true. IIUC, the comment directly above this change says why it chose false here, though it is a somewhat confusing comment. Since you seem to disagree with that comment, you should probably change the comment to say why you think true is the right value here.
There was a problem hiding this comment.
The difference is that now in each loop iteration the rhs is 'inner_loop->GetChildAtIndex...', which will be different each iteration through the loop; before it was just 'GetChildAtIndex', which did not change on each iteration.
re false->true: The original lldb-eval code on which that was based used 'false', but it was also making the call through the SB API interface (SBValues, SBTypes, etc). I found that the cast test cases passed with lldb-eval when this value was false, but failed in my translated, migrated version. Setting this to true allows those failing test cases to pass.
I don't have any easy way to add those test cases to this PR because they all use 'frame variable' (using my DIL implementation) for doing the casts...But I do have them, and I am running (and passing) them.
There was a problem hiding this comment.
Ah, I missed that I was looking at a diff against your original diff which didn't make sense. In context the code seems fine.
But then that also showed that you are the author of the "somewhat confusing comment". Can you make that comment more helpful? Suppressing the synthetic child production would force this child to get its type directly from the CompilerType, which I would call the "real" child. So that comment seems to make sense when we passed true but doesn't make sense when passing true.
You don't need to introduce tests for this at this stage if that's dependent on other changes, but this seems like a tricky point, so it should have a comment. But this one doesn't make sense to me. And it doesn't explain why getting the real type would be bad.
| @@ -3138,13 +3141,13 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) { | |||
| val_byte_size = temp.value(); | |||
|
|
|||
| if (is_pointer) { | |||
| if (type.IsBoolean() && type_byte_size < val_byte_size) { | |||
| if (!type.IsBoolean() && type_byte_size < val_byte_size) { | |||
There was a problem hiding this comment.
I'm not really sure what the original code intended with that type.IsBoolean? Makes no sense we would only return the "too small" error when the incoming type is Boolean.
But with this code, if I pass 32 bit float (on a 64 bit pointers system), the error I'll get is that it's too small. OTOH, if I pass a 64 bit float, I'll get "must be integer". That's confusing. If you aren't planning to take anything but integers, it seems like you should check that first, then check the size.
Re-order checks for input type and size in CastToBasicType.
| if (!type.IsInteger()) { | ||
| m_error.SetErrorString("target type must be an integer"); | ||
| return GetSP(); | ||
| } | ||
| if (!type.IsBoolean() && type_byte_size < val_byte_size) { |
There was a problem hiding this comment.
That's better, but now the type.IsBoolean is entirely pointless, since you would never get to this code in that case.
There was a problem hiding this comment.
Whoops! Good catch; I've fixed that now.
Fix type-checking error introduced with previous re-ordering fix.
lldb/source/Core/ValueObject.cpp
Outdated
| uint64_t num_bits = 0; | ||
| if (auto temp = new_val_sp->GetCompilerType().GetBitSize(target.get())) | ||
| num_bits = temp.value(); | ||
| // SetValueFromInteger(llvm::APInt(64, int_val), error); |
There was a problem hiding this comment.
Is this leftover from some experiment?
There was a problem hiding this comment.
Oh, yes. I thought I had found and removed all of those, but it looks like I missed one. I'll remove it.
|
There was one comment that should probably be removed. This looks okay to me at this stage. |
Delete a commented-out line of code.
Jim, If this looks ok to you, could you officially mark it as approved please? |
|
Review ping? Does this look ok to everyone now? I would really like to get this PR committed (I have more waiting behind it). Thanks in advance! |
Create additional helper functions for the ValueObject class, for: - returning the value as an APSInt or APFloat - additional type casting options - additional ways to create ValueObjects from various types of data - dereferencing a ValueObject These helper functions are needed for implementing the Data Inspection Language, described in https://discourse.llvm.org/t/rfc-data-inspection-language/69893
|
|
||
| if (!type.IsScalarType()) { | ||
| m_error.SetErrorString("target type must be a scalar"); | ||
| return GetSP(); |
There was a problem hiding this comment.
Hi @cmtice! I just came across this code and I don't think this API makes sense. Since CastToBasicType creates a new casted ValueObject I think this method should be const and not modify the error of the original value when the cast fails. I think it should return a ValueObjectConstResult with the error instead.
There was a problem hiding this comment.
Ok, I have tried to do what I think you were asking me to do. Please see #117401
Create additional helper functions for the ValueObject class, for:
These helper functions are needed for implementing the Data Inspection Language, described in
https://discourse.llvm.org/t/rfc-data-inspection-language/69893