Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions lldb/include/lldb/Core/ValueObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,19 @@ class ValueObject {

virtual int64_t GetValueAsSigned(int64_t fail_value, bool *success = nullptr);

llvm::APSInt GetValueAsAPSInt();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if these errors matter to users then llvm::Expected<llvm::APSInt> would be the right choice. Otherwise std::optional is the way to go.


llvm::APFloat GetValueAsFloat();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here


bool GetValueAsBool();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need GetValueAsBool() or can we just use GetValueAsAPSInt() and make sure bool gets converted to a suitable integer?

Copy link
Contributor Author

@cmtice cmtice Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Update the value of the current object to be the integer in the 'value'
/// parameter.
void UpdateIntegerValue(const llvm::APInt &value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


/// Assign the integer value 'new_val_sp' to the current object.
void UpdateIntegerValue(lldb::ValueObjectSP new_val_sp);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


virtual bool SetValueFromCString(const char *value_str, Status &error);

/// Return the module associated with this value object in case the value is
Expand Down Expand Up @@ -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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::ValueObjectSP CastDerivedToBaseType(CompilerType type,
const std::vector<uint32_t> &idx);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::ValueObjectSP CastBaseToDerivedType(CompilerType type, uint64_t offset);

lldb::ValueObjectSP CastScalarToBasicType(CompilerType type, Status &error);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as Jim made below where the returned lldb::ValueObjectSP can have its error set correctly.

Also, we have a bunch of CastXXXXToXXXX calls added below. Do we really need all of these? Can we just have one lldb::ValueObjectSP Cast(CompilerType type) function and deduce what should be done internally and fail gracefully?

Or if we do need functions specific to Scalar and Enum we can add just lldb::ValueObjectSP CastScalar(CompilerType type) and lldb::ValueObjectSP CastEnum(CompilerType type).

It would be great if we just have a single lldb::ValueObjectSP Cast(CompilerType type) function that works for everything. We should be able to figure out internally how things are stored (scalar, enum would be deduced from the current ValueObject, and figure out what we are casting it to with by looking at CompilerType


lldb::ValueObjectSP CastEnumToBasicType(CompilerType type);

lldb::ValueObjectSP CastPointerToBasicType(CompilerType type);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is BasicType in this context? I know there's lldb::BasicType, but these methods take CompilerTypes.


lldb::ValueObjectSP CastIntegerOrEnumToEnumType(CompilerType type);

lldb::ValueObjectSP CastFloatToEnumType(CompilerType type, Status &error);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need an error parameter for anything that returns a ValueObject. ValueObjects carry their errors in the ValueObject (ValueObject::GetError()).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some doxygen comments to these methods? It would help clarify the intent behind them as well as the purpose of some of the parameter (e.g. What does idx correspond to in CastDerivedToBaseType?)


/// 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.
Expand Down Expand Up @@ -668,6 +699,32 @@ class ValueObject {
CreateValueObjectFromData(llvm::StringRef name, const DataExtractor &data,
const ExecutionContext &exe_ctx, CompilerType type);

static lldb::ValueObjectSP
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

CreateValueObjectFromBytes(lldb::TargetSP target_sp, const void *bytes,
CompilerType type);

static lldb::ValueObjectSP CreateValueObjectFromBytes(lldb::TargetSP target,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to be able to add a name for the value object? Some the creation APIs allow users to specify names and the native synthetic child providers could use these APIs if they can specify a name.

const void *bytes,
lldb::BasicType type);

static lldb::ValueObjectSP CreateValueObjectFromAPInt(lldb::TargetSP target,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for adding a name parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

const llvm::APInt &v,
CompilerType type);

static lldb::ValueObjectSP
CreateValueObjectFromAPFloat(lldb::TargetSP target, const llvm::APFloat &v,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto for adding a name parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CompilerType type);

static lldb::ValueObjectSP CreateValueObjectFromPointer(lldb::TargetSP target,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different from CreateValueObjectFromAddress?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

uintptr_t addr,
CompilerType type);

static lldb::ValueObjectSP CreateValueObjectFromBool(lldb::TargetSP target,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

bool value);

static lldb::ValueObjectSP CreateValueObjectFromNullptr(lldb::TargetSP target,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

CompilerType type);

lldb::ValueObjectSP Persist();

/// Returns true if this is a char* or a char[] if it is a char* and
Expand Down Expand Up @@ -719,6 +776,10 @@ class ValueObject {
ClearUserVisibleData(eClearUserVisibleDataItemsSummary);
}

void SetDerefValobj(ValueObject *deref) { m_deref_valobj = deref; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Synthetic child providers also have a way to provide the deref ValueObject dynamically. How do these two ways of doing that job work together?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


ValueObject *GetDerefValobj() { return m_deref_valobj; }

void SetValueFormat(lldb::TypeFormatImplSP format) {
m_type_format_sp = std::move(format);
ClearUserVisibleData(eClearUserVisibleDataItemsValue);
Expand Down
Loading