Skip to content
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

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

Merged
merged 8 commits into from
Sep 13, 2024
Merged

Avoid expression evaluation in libStdC++ std::vector<bool> synthetic children provider #108414

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6e84ab9
Fix StartDebuggingRequestHandler/ReplModeRequestHandler in lldb-dap
Aug 19, 2024
b23f3ea
Merge branch 'llvm:main' into main
jeffreytan81 Aug 21, 2024
b8ec0fb
Merge branch 'llvm:main' into main
jeffreytan81 Aug 26, 2024
1a350e4
Merge branch 'llvm:main' into main
jeffreytan81 Sep 12, 2024
d56c9b3
Add CreateBoolValue API
Sep 12, 2024
37a5ffa
Fix typo
Sep 12, 2024
257cd0d
Use ValueObject::CreateValueObjectFromBool() API
Sep 12, 2024
713242a
Remove SetAddressTypeOfChildren() call
Sep 13, 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
6 changes: 1 addition & 5 deletions lldb/examples/synthetic/gnu_libstdcpp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,11 +473,7 @@ def get_child_at_index(self, index):
"[" + str(index) + "]", element_offset, element_type
)
bit = element.GetValueAsUnsigned(0) & (1 << bit_offset)
if bit != 0:
value_expr = "(bool)true"
else:
value_expr = "(bool)false"
return self.valobj.CreateValueFromExpression("[%d]" % index, value_expr)
return self.valobj.CreateBooleanValue("[%d]" % index, bool(bit))

def update(self):
try:
Expand Down
2 changes: 2 additions & 0 deletions lldb/include/lldb/API/SBValue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ class LLDB_API SBValue {
lldb::SBValue CreateValueFromData(const char *name, lldb::SBData data,
lldb::SBType type);

lldb::SBValue CreateBoolValue(const char *name, bool value);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a blurb this is created from data not address


/// Get a child value by index from a value.
///
/// Structs, unions, classes, arrays and pointers have child
Expand Down
27 changes: 27 additions & 0 deletions lldb/source/API/SBValue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,33 @@ lldb::SBValue SBValue::CreateValueFromData(const char *name, SBData data,
return sb_value;
}

lldb::SBValue SBValue::CreateBoolValue(const char *name, bool value) {
LLDB_INSTRUMENT_VA(this, name);

lldb::SBValue sb_value;
lldb::ValueObjectSP new_value_sp;
ValueLocker locker;
lldb::ValueObjectSP value_sp(GetSP(locker));
ProcessSP process_sp = m_opaque_sp->GetProcessSP();
lldb::SBTarget target = GetTarget();
if (!target.IsValid())
return sb_value;
lldb::SBType boolean_type = target.GetBasicType(lldb::eBasicTypeBool);
lldb::TypeImplSP type_impl_sp(boolean_type.GetSP());
if (value_sp && process_sp && type_impl_sp) {
int data_buf[1] = {value ? 1 : 0};
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a data buffer the only option here because of the bitwise logic? I ask because we could just have the address point to a singular byte instead right?

lldb::SBData data = lldb::SBData::CreateDataFromSInt32Array(
process_sp->GetByteOrder(), sizeof(data_buf[0]), data_buf,
sizeof(data_buf) / sizeof(data_buf[0]));
ExecutionContext exe_ctx(value_sp->GetExecutionContextRef());
new_value_sp = ValueObject::CreateValueObjectFromData(
name, **data, exe_ctx, type_impl_sp->GetCompilerType(true));
new_value_sp->SetAddressTypeOfChildren(eAddressTypeLoad);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did the lack of this line cause problems? bool values can't have children, so it's a bit surprising you had to specify this.

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 copied it from SBValue::CreateValueFromData(). I actually do not accurately know its meaning, so simply keep it here for safety purpose. I will remove it since test still succeeds after removing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem comes with ValueObjects that live in lldb host memory for instance ValueObjectConstResult "history" entities or ValueObject made from DataExtractors. If you have such a result value that includes pointers that you copied up from the target memory, even though the ValueObject itself lives in lldb host memory, the pointer children still point into the target (are load addresses). This API states that that is true for this ValueObject.

It's not relevant for ValueObjects that can't have pointer children.

}
sb_value.SetSP(new_value_sp);
return sb_value;
}

SBValue SBValue::GetChildAtIndex(uint32_t idx) {
LLDB_INSTRUMENT_VA(this, idx);

Expand Down
Loading