[Lldb-commits] [PATCH] D143215: Separate Process::GetWatchpointSupportInfo into two methods to get the two separate pieces of information

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added inline comments.


================
Comment at: lldb/source/Target/Process.cpp:2373
+    reported_after = false;
+  return reported_after;
+}
----------------
Emmmer wrote:
> DavidSpickett wrote:
> > DavidSpickett wrote:
> > > 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?
> > @Emmmer any idea?
> It seems standard RISC-V uses the `before` mode, that is 
> > The action for this trigger will be taken just before the instruction that triggered it is committed, but after all preceding instructions are committed.
> 
> If I understand this code correctly (correct me if I was wrong), we should return `false` for RISC-V. 
> 
> But in the debug-spec [1], RISC-V seems to allow both `before` and `after` modes, which can be detected through CSRs. When debug-spec lands, we may change the code accordingly.
> 
> [1]: https://github.com/riscv/riscv-debug-spec/blob/6309972901417a0fa72d812d2ffe89e432f00dff/xml/hwbp_registers.xml#L365
Thanks!  I'll add isRISCV() to the architectures we default to "watchpoints received before instruction executes".  This can be overridden by a gdb remote stub which adds the lldb-extension key to `qHostInfo` response.  although it sounds like it could technically be per *process*??? in which case the key should really be sent in `qProcessInfo`.  The remote stub should be able to send `qHostInfo` before it has attached to any specific process.


================
Comment at: lldb/source/Target/StopInfo.cpp:833
-      m_should_stop = true;
-      return m_should_stop;
-    }
----------------
DavidSpickett wrote:
> 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?
Previously, this would fetch the number of watchpoints available (if the lldb-extended packet was provided) and it would fetch whether watchpoints are received before or after (if the qHostInfo includes the lldb-extended key), and you would get an error if the former was unavailable.  (I think the latter would default to "watchpoints are after").  This `m_should_stop = true` is the behavior when watchpoints are received after the instruction has executed; this is the crux of the bug I was trying to fix, where lldb would not instruction step over the instruction and re-add it, when the packet declaring the number of watchpoints was unimplemented.  


================
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;
----------------
DavidSpickett wrote:
> 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.
It is a bit confusing.  The layers that return the `optional<uint32_t>` will return the actual number of hardware watchpoints available, including 0 if that is the correct value.  Or a `nullopt` to indicate that it could not be retrieved. So in this method, we got back the number of watchpoints available, and it was said to be 0.  lldb won't actually check against this number if you try to set one, so even if we got a bogus value here, you could try to set a watchpoint and see if it works.  `SBProcess::GetNumSupportedHardwareWatchpoints()` takes an SBError out arg & returns a uint32_t.  It can return 0 to mean "could not get number of watchpoints" or "there are zero watchpoints" but the SBError tells you whether it was the failure case or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143215