[Lldb-commits] [PATCH] D136938: [LLDB] Fix code breakpoints on tagged addresses

[David Spickett via Phabricator via lldb-commits]

DavidSpickett added a comment.

> What do you think about locating this change in ToAddress instead of Target::GetBreakableLoadAddress? It looks like the one caller to GetBreakableLoadAddress is Target::CreateBreakpoint(addr_t addr) - which is probably called by an SBTarget method if we want to think of the most general purpose use case. I think stripping non-addressable bits in OptionArgParser::ToAddress is the right thing to do - but I don't have an opinion about whether it should be done in Target::GetBreakableLoadAddress or not.

I had not checked the other lookup commands, so yeah this sounds good to me. Doing this fixing in fewer places is always better. Put up your change and I'll check the test from this patch against it.

> One thing to note is that I also have an SBValue method that needs to be upstreamed, SBValue::GetValueAsAddress(). So if someone has an SBValue of a function pointer and they want to take the value of that func ptr and call SBTarget::CreateBreakpoint on it, I would say "well yes, you need to use GetValueAsAddress before you pass that to CreateBreakpoint".

If I understand correctly, this would effectively run FixCodeAddress on the value? One thing that I found doing the SB API for MTE (which was shelved, but besides the point) is that there was no way to fix addresses via the API (or get access to the ABI plugin in general). I ended up just adding FixDataAddress to SBProcess as a temporary thing. So something like ValueAsAddress could be very useful.

We have this distinction (that doesn't mean much) between code and data fixes. For now a single method will work fine, I'm hoping the code/data difference never gets to a point where we have to be really careful about it.
(as you say "This process isn't using TBI, but the high bits end up being used for signing so it's the same effect.", so FixCode and FixData are the same thing in that case)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136938/new/

https://reviews.llvm.org/D136938