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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
