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

[David Spickett via Phabricator via lldb-commits]

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