DavidSpickett added a comment.
> Change lldb from depending on the remote gdb stub to tell it whether
> watchpoints are received before or after the instruction, to knowing the
> architectures where watchpoint exceptions are received before the instruction
> executes, and allow the remote gdb stub to override this behavior in its
> qHostInfo response.
Sounds good to me. Can you simplify the commit message some now that you've got
the changes you want?
Seems like the main things are:
- Split up querying watchpoint reporting style, and number of watchpoint slots.
- Have process determine the reporting style by target, with the option for the
remote to override. Meaning this query never fails.
- Only conclude that watchpoints are unsupported if the remote tells us that it
has 0 slots.
(the last one I think we always did but good for context)
================
Comment at: lldb/source/Target/Process.cpp:2360
+ std::optional<bool> subclass_override = DoGetWatchpointReportedAfter();
+ if (subclass_override) {
+ return *subclass_override;
----------------
Can do `if (std::optional<bool> subclass....) {...` here.
Also no `{}` for single line if.
================
Comment at: lldb/source/Target/Process.cpp:2367
+ return reported_after;
+ }
+ llvm::Triple triple = arch.GetTriple();
----------------
No `{}` needed here too.
================
Comment at: lldb/source/Target/Process.cpp:2373
+ reported_after = false;
+ return reported_after;
+}
----------------
Would this be any clearer with multiple returns? Or one giant return, but the
logic is a bit cryptic then.
```
const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
return true;
llvm::Triple triple = arch.GetTriple();
return !(triple.isMIPS() || triple.isPPC64() || triple.isAArch64() ||
triple.isArmMClass() || triple.isARM());
```
Better as:
```
const ArchSpec &arch = GetTarget().GetArchitecture();
if (!arch.IsValid())
return false;
llvm::Triple triple = arch.GetTriple();
if (triple.isMIPS() || triple.isPPC64() || triple.isAArch64() ||
triple.isArmMClass() || triple.isARM())
return false;
return true;
```
Also, do we know what RISC-V does?
================
Comment at: lldb/source/Target/StopInfo.cpp:833
- m_should_stop = true;
- return m_should_stop;
- }
----------------
This is no longer needed because `GetWatchpointReportedAfter` will fall back to
the process' defaults if the GDB layer does not/can not provide a value,
correct?
================
Comment at: lldb/source/Target/Target.cpp:804
"Target supports (%u) hardware watchpoint slots.\n",
- num_supported_hardware_watchpoints);
+ *num_supported_hardware_watchpoints);
return false;
----------------
This should just append "Target supports 0 hardware...".
In theory a compiler could figure that out but also it's a bit confusing
because it makes the reader wonder what value other than 0 is supposed to end
up here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143215/new/
https://reviews.llvm.org/D143215
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
