[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Looks good. A little bit of cleanup. Let me know what you think of the comments.



================
Comment at: utils/clangdiag.py:17
+import os
+from subprocess import check_output as qx;
+
----------------
I would rather just do:

```
import subprocess
```

Then later use:

```
subprocess.check_output(...)
```

It makes the code easier to read.


================
Comment at: utils/clangdiag.py:20
+# State
+Breakpoints = []
+target = None
----------------
Remove? See inlined comment for line 92


================
Comment at: utils/clangdiag.py:21
+Breakpoints = []
+target = None
+diagtool = None
----------------
I would avoid making this a global. It will keep the target alive since it has a strong reference count to the underlying lldb_private::Target.


================
Comment at: utils/clangdiag.py:52
+
+    global target
+    global diagtool
----------------
Get the target from the frame instead of grabbing it from a global:
```
target = frame.thread.process.target
```
or without the shortcuts:
```
target = frame.GetThread().GetProcess().GetTarget()
```


================
Comment at: utils/clangdiag.py:66
+
+    global target
+    global diagtool
----------------
Always grab the target and don't store it in a global. so just remove this line


================
Comment at: utils/clangdiag.py:75-78
+    diagtool = os.path.join(exe.GetDirectory(), 'diagtool')
+    if not os.path.exists(diagtool):
+        print('diagtool not found along side %s' % exe)
+        return
----------------
Allow "diagtool" to be set from an option maybe? This would require the options to be passed into this function as an argument. If it doesn't get set, then modify the options to contain this default value? Then this error message can just complain about the actual path:

```
print('diagtool "%s" doesn't exist' % diagtool)
```


================
Comment at: utils/clangdiag.py:80
+
+    bp = target.BreakpointCreateByName('DiagnosticsEngine::Report')
+    bp.SetScriptCallbackFunction('clangdiag.setDiagBreakpoint')
----------------
Add name to breakpoint? See inlined comment at line 92


================
Comment at: utils/clangdiag.py:87
+def disable(debugger):
+    global target
+    global diagtool
----------------
remove and use:
```
target = debugger.GetSelectedTarget()
```


================
Comment at: utils/clangdiag.py:91-92
+    # Remove all diag breakpoints.
+    for bp in Breakpoints:
+        target.BreakpointDelete(bp.GetID())
+    target = None
----------------
Another way to do this would be to give the breakpoints you create using "target.BreakpointCreateXXX" to have a name when you create them:

```
bp = target.BreakpointCreateByName('DiagnosticsEngine::Report')
bp.AddName("llvm::Diagnostic")
```

Then here you can iterate over all breakpoints in the target:

```
for bp in target.breakpoint_iter():
  if bp.MatchesName("llvm::Diagnostic"):
    target.BreakpointDelete(bp.GetID())
```

Then you don't need a global list?



https://reviews.llvm.org/D36347