jasonmolenda added inline comments.
================
Comment at: lldb/source/API/SBValue.cpp:936
+ if (ABISP abi_sp = process_sp->GetABI())
+ return abi_sp->FixCodeAddress(ret_val);
+ return ret_val;
----------------
DavidSpickett wrote:
> We've talked about this before, and for now I think the result of `FixCode`
> and `FixData` are the same for debug purposes. In the event that that ever
> changes, our path is probably to document this as assuming a code address,
> then adding specific `ValueAsCodeAddress` and `ValueAsDataAddress`, correct?
>
> (which sounds good to me, just checking we've thought that through)
>
> You could document that in the docstring but there's a good chance it would
> cause more confusion than it's worth when at this time, it's not something
> users have to care about.
Yeah, this is a good point that I punted on a bit. On Darwin we are only using
TCR_EL1.T0SZ to tell us how many bits are used for addressing; there's no
distinction between code and data for us. I guess I should worry about someone
defining a `FixCodeAddress` method in an ABI plugin that clears lower bits when
instruction alignment is required. It may be that it's safer to use
FixDataAddress if I have to choose between the two without any context. In our
internal implementation in the apple sources, we only had FixAddress which were
used for both. What do you think?
================
Comment at: lldb/source/Core/ValueObject.cpp:1469
case Value::ValueType::HostAddress:
case Value::ValueType::LoadAddress:
----------------
DavidSpickett wrote:
> What even is a host address, and do we care here?
> ```
> /// A host address value (for memory in the process that < A is
> /// using liblldb).
> HostAddress
> ```
> I'd have to check more but could that be an issue if I was debugging from x86
> (where the non-address bits must be 1 or 0) to AArch64?
>
> Maybe it is just included here so we cover all enum values and in fact,
> addresses in this path will always be from the debugee.
Excellent point. ValueObjects can point to host memory for sure. We can have a
problem with AArch64 to AArch64 debug session; macs run in a larger address
space than iOS devices for instance, so masking off the target iOS bits from a
mac side address could make it invalid.
================
Comment at: lldb/source/Core/ValueObject.cpp:3012
if (pointer_type) {
+ if (Process *process = exe_ctx.GetProcessPtr()) {
+ if (ABISP abi_sp = process->GetABI()) {
----------------
DavidSpickett wrote:
> Probably best on its own change but I wonder if it's time to add
> Process::Fix<...>Address to dedupe all these conditionals.
Yes, this is a good idea. I will do this separately.
================
Comment at: lldb/source/DataFormatters/ValueObjectPrinter.cpp:432
+ if (stripped != m_valobj->GetPointerValue()) {
+ m_stream->Printf(" (actual=0x%" PRIx64 ")", stripped);
+ }
----------------
DavidSpickett wrote:
> Is there a way to cover this with a test? I think there is one for the
> `(actual=` part already, does that cover this part too?
Good point. And actually, this codepath was never firing because both things
it compared to were stripping the non-addressable bits off. Fixed this method,
added to the test case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142792/new/
https://reviews.llvm.org/D142792
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
