Skip to content

[LLDB] Add more helper functions to ValueObject class. #87197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Jun 13, 2024
Merged

[LLDB] Add more helper functions to ValueObject class. #87197

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
68cb68d
[LLDB] Add more helper functions to ValueObject class.
cmtice Mar 31, 2024
b6a1e23
Merge branch 'main' of github.com:llvm/llvm-project into dil-valobj-2
cmtice Apr 8, 2024
145e74b
[LLDB] Add more helper functions to ValueObject class.
cmtice Apr 8, 2024
22e1b06
[LLDB] Fix review comments for "CreateValueObjectFrom..." functions.
cmtice Apr 23, 2024
b8eee30
Merge remote-tracking branch 'origin/main' into dil-valobj-2
cmtice Apr 23, 2024
8709f70
Merge remote-tracking branch 'origin/main' into dil-valobj-2
cmtice May 19, 2024
1e41bfa
[LLDB] Add more helper functions to ValueObject class.
cmtice May 22, 2024
28748a9
[LLDB] Add more helper functions to ValueObject.
cmtice Jun 5, 2024
499a097
Fix comment.
cmtice Jun 6, 2024
fa25850
[LLDB] Add more helper functions to ValueObject class.
cmtice Jun 6, 2024
d4e7f69
[LLDB] Add more helper functions to ValueObject class.
cmtice Jun 6, 2024
6ca00e8
[LLDB] Add more helper functions to ValueObject class.
cmtice Jun 6, 2024
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
44 changes: 32 additions & 12 deletions lldb/source/Core/ValueObject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1093,7 +1093,9 @@ llvm::Expected<llvm::APSInt> ValueObject::GetValueAsAPSInt() {
// Make sure the type can be converted to an APSInt.
if (!GetCompilerType().IsInteger() &&
!GetCompilerType().IsScopedEnumerationType() &&
!GetCompilerType().IsEnumerationType() &&
!GetCompilerType().IsPointerType() &&
!GetCompilerType().IsNullPtrType() &&
!GetCompilerType().IsReferenceType() && !GetCompilerType().IsBoolean())
return llvm::make_error<llvm::StringError>(
"type cannot be converted to APSInt", llvm::inconvertibleErrorCode());
Expand Down Expand Up @@ -3015,11 +3017,12 @@ llvm::Expected<lldb::ValueObjectSP> ValueObject::CastDerivedToBaseType(

lldb::TargetSP target = GetTargetSP();
// The `value` can be a pointer, but GetChildAtIndex works for pointers too.
lldb::ValueObjectSP inner_value;
lldb::ValueObjectSP inner_value = GetSP();

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);
Copy link
Collaborator

@jimingham jimingham Jun 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cmtice cmtice Jun 5, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@jimingham jimingham Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cmtice cmtice Jun 6, 2024

Choose a reason for hiding this comment

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

Done.

inner_value =
inner_value->GetChildAtIndex(i, /*can_create_synthetic*/ true);

// At this point type of `inner_value` should be the dereferenced target
// type.
Expand Down Expand Up @@ -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) {
Copy link
Collaborator

@jimingham jimingham Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@cmtice cmtice Jun 6, 2024

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@jimingham jimingham Jun 6, 2024

Choose a reason for hiding this comment

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

That's better, but now the type.IsBoolean is entirely pointless, since you would never get to this code in that case.

Copy link
Contributor Author

@cmtice cmtice Jun 6, 2024

Choose a reason for hiding this comment

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

Whoops! Good catch; I've fixed that now.

m_error.SetErrorString(
"target type cannot be smaller than the pointer type");
return GetSP();
}
if (!type.IsInteger()) {
m_error.SetErrorString("target tyep must be an integer");
m_error.SetErrorString("target type must be an integer");
return GetSP();
}
}
Expand All @@ -3159,7 +3162,9 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
return ValueObject::CreateValueObjectFromBool(
target, !float_value_or_err->isZero(), "result");
else {
m_error.SetErrorString("cannot get value as APFloat");
m_error.SetErrorStringWithFormat(
"cannot get value as APFloat: %s",
llvm::toString(float_value_or_err.takeError()).c_str());
return GetSP();
}
}
Expand All @@ -3176,7 +3181,10 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
return ValueObject::CreateValueObjectFromAPInt(target, ext, type,
"result");
} else {
m_error.SetErrorString("cannot get value as APSInt");
m_error.SetErrorStringWithFormat(
"cannot get value as APSInt: %s",
llvm::toString(int_value_or_err.takeError()).c_str());
;
return GetSP();
}
} else if (is_scalar && is_float) {
Expand All @@ -3191,7 +3199,9 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
// Casting floating point values that are out of bounds of the target
// type is undefined behaviour.
if (status & llvm::APFloatBase::opInvalidOp) {
m_error.SetErrorString("invalid type cast detected");
m_error.SetErrorStringWithFormat(
"invalid type cast detected: %s",
llvm::toString(float_value_or_err.takeError()).c_str());
return GetSP();
}
return ValueObject::CreateValueObjectFromAPInt(target, integer, type,
Expand All @@ -3212,7 +3222,9 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
return ValueObject::CreateValueObjectFromAPFloat(target, f, type,
"result");
} else {
m_error.SetErrorString("cannot get value as APSInt");
m_error.SetErrorStringWithFormat(
"cannot get value as APSInt: %s",
llvm::toString(int_value_or_err.takeError()).c_str());
return GetSP();
}
} else {
Expand All @@ -3225,7 +3237,9 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
return ValueObject::CreateValueObjectFromAPFloat(target, f, type,
"result");
} else {
m_error.SetErrorString("cannot get value as APSInt");
m_error.SetErrorStringWithFormat(
"cannot get value as APSInt: %s",
llvm::toString(int_value_or_err.takeError()).c_str());
return GetSP();
}
}
Expand All @@ -3238,7 +3252,9 @@ lldb::ValueObjectSP ValueObject::CastToBasicType(CompilerType type) {
return ValueObject::CreateValueObjectFromAPFloat(target, f, type,
"result");
} else {
m_error.SetErrorString("cannot get value as APFloat");
m_error.SetErrorStringWithFormat(
"cannot get value as APFloat: %s",
llvm::toString(float_value_or_err.takeError()).c_str());
return GetSP();
}
}
Expand Down Expand Up @@ -3280,7 +3296,9 @@ lldb::ValueObjectSP ValueObject::CastToEnumType(CompilerType type) {
// Casting floating point values that are out of bounds of the target
// type is undefined behaviour.
if (status & llvm::APFloatBase::opInvalidOp) {
m_error.SetErrorString("invalid type cast detected");
m_error.SetErrorStringWithFormat(
"invalid type cast detected: %s",
llvm::toString(value_or_err.takeError()).c_str());
return GetSP();
}
return ValueObject::CreateValueObjectFromAPInt(target, integer, type,
Expand All @@ -3297,7 +3315,9 @@ lldb::ValueObjectSP ValueObject::CastToEnumType(CompilerType type) {
return ValueObject::CreateValueObjectFromAPInt(target, ext, type,
"result");
} else {
m_error.SetErrorString("cannot get value as APSInt");
m_error.SetErrorStringWithFormat(
"cannot get value as APSInt: %s",
llvm::toString(value_or_err.takeError()).c_str());
return GetSP();
}
}
Expand Down