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