jasonmolenda added a comment.

Thanks for all the helpful comments, I'll update the patch.



================
Comment at: 
lldb/tools/debugserver/source/MacOSX/arm64/DNBArchImplARM64.cpp:833-835
+std::vector<DNBArchMachARM64::watchpoint_spec>
+DNBArchMachARM64::AlignRequestedWatchpoint(nub_addr_t user_addr,
+                                           nub_size_t user_size) {
----------------
JDevlieghere wrote:
> Do you ever expect this to return more than two watchpoints? Seems like this 
> could be a `struct` that holds two optional `WatchpointSpec`s. I don't feel 
> strongly about it, but it looks like you never iterate over the list and the 
> way you have to check after the recursive call is a bit awkward. 
> 
> edit: nvm, it looks like you do actually iterate over them in 
> `EnableHardwareWatchpoint`
I was thinking about how this current scheme only ever splits 1 watchpoint into 
2, but a future design that could expand a user requested watch into a larger 
number of hardware watchpoints would expand it further.  If a user asks to 
watch a 32 byte object, and the target only supports 8 byte watchpoints, and 
the object is doubleword aligned, we could watch it with 4 hardware 
watchpoints.  The older code in debugserver was written in terms of "one or 
two" but I switched to a vector of hardware implementable watchpoints that may 
expand as we evaluate the hardware capabilities of the target/stub.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149040

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to