[Lldb-commits] [PATCH] D103500: [trace][intel-pt] Create basic SB API

[Vedant Kumar via Phabricator via lldb-commits]

vsk requested changes to this revision.
vsk added a subscriber: mib.
vsk added a comment.
This revision now requires changes to proceed.

Thanks, excited to see this work progress!



================
Comment at: lldb/include/lldb/Target/Target.h:1129
   ///   The trace object. It might be undefined.
   lldb::TraceSP &GetTrace();
 
----------------
See my other note about returning references to shared pointers. This would be nice to clean up either before or after landing this patch.


================
Comment at: lldb/include/lldb/Target/Target.h:1132
+  /// Create a \a Trace object for the current target using the using the
+  /// default supported tracing technology for this process. This
+  ///
----------------
Dangling 'This'?


================
Comment at: lldb/include/lldb/Target/Target.h:1137
+  ///     the trace couldn't be created.
+  llvm::Expected<lldb::TraceSP &> CreateTrace();
+
----------------
I don't think returning a `TraceSP &` is a good practice. This returns a reference to a SP: unless the SP is stored in a long-lived instance (which would defeat the purpose of using shared pointers in the first place), the returned reference will be dangling.

It might make more sense to either return the shared pointer (lldb::TraceSP) directly, or to return a bare pointer/reference.


================
Comment at: lldb/include/lldb/Target/Trace.h:267
   ///     \a llvm::Error otherwise.
-  llvm::Error StopThreads(const std::vector<lldb::tid_t> &tids);
+  llvm::Error Stop(const std::vector<lldb::tid_t> &tids);
 
----------------
Consider using ArrayRef<lldb::tid_t>, as this permits the caller to use a larger variety of vector-like containers.


================
Comment at: lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py:11
+    def wrapper(*args, **kwargs):
+        USE_SB_API = True
+        func(*args, **kwargs)
----------------
I don't think this works. In this wrapper, "USE_SB_API" is a local variable, *not* a reference to a TraceIntelPTTestCaseBase instance's "USE_SB_API" field.

To see why, consider the following example -- it prints "False" twice:
```
def make_wrapper(func):
    def wrapper(*args):
        FLAG = True
        func(*args)
        FLAG = False
        func(*args)
    return wrapper

class Klass:
    FLAG = False

    @make_wrapper
    def test(self):
        print(self.FLAG)

Klass().test()
```

How did you verify that the USE_SB_API = True case behaves as expected?


================
Comment at: lldb/source/API/SBTarget.cpp:2458
 
+lldb::SBTrace SBTarget::GetTrace() {
+  LLDB_RECORD_METHOD_NO_ARGS(lldb::SBTrace, SBTarget, GetTrace);
----------------
Paging @mib - would you mind taking a closer look at these API/*.cpp changes? You've got more experience than I do in this area :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103500