clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.
================
Comment at: lldb/include/lldb/API/SBProcess.h:97
+ /// Deprecated
lldb::SBThread GetSelectedThread() const;
----------------
Remove? This is not deprecated right?
================
Comment at: lldb/include/lldb/API/SBProcess.h:305-329
+ /// The name of the shared library that you want to load.
/// If image_spec is a relative path, the relative path will be
/// appended to the search paths.
/// If the image_spec is an absolute path, just the basename is used.
///
/// \param[in] paths
+ /// A list of paths to search for the library whose basename is
----------------
Revert all diffs in this file I think right?
================
Comment at: lldb/include/lldb/API/SBStructuredData.h:103
friend class SBBreakpointName;
+ friend class SBTrace;
----------------
Is this needed?
================
Comment at: lldb/include/lldb/API/SBTarget.h:856
+ /// An error explaining why the trace couldn't be created.
+ lldb::SBTrace GetTraceOrCreate(SBError &error);
+
----------------
Maybe just name this "lldb::SBTrace CreateTrace(SBError &error);". This would
return an error if the trace was already created. If we are creating the trace
we need to option to specify settings right?
================
Comment at: lldb/include/lldb/API/SBTrace.h:26-30
+ /// Start tracing a live process using a default configuration.
///
- /// \param[in] buf
- /// Buffer to write the trace data to.
+ /// \return
+ /// An error explaining any failures.
+ SBError StartProcess();
----------------
I would remove this and just use the next function. People can just default
construct a SBStructuredData and pass it in if they don't want to set any
settings.
================
Comment at: lldb/include/lldb/API/SBTrace.h:40-52
+ /// For intel-pt:
+ /// - int threadBufferSize (defaults to 4096 bytes):
+ /// Trace size in bytes per thread. It must be a power of 2 greater
+ /// than or equal to 4096 (2^12). The trace is circular keeping the
+ /// the most recent data.
+ /// - int processBufferSizeLimit (defaults to 500 MB):
+ /// Maximum total trace size per process in bytes. This limit applies
----------------
It would be nice to not document specific trace settings in this header file.
It should be possible to add a way to get the schema from a "SBTrace" object
right? Something like "const char * SBTrace::GetSchema()" and then tell users
to check that for how and what settings are available and that if none are set
that good defaults will be used?
================
Comment at: lldb/include/lldb/API/SBTrace.h:56
+ /// An error explaining any failures.
+ SBError StartProcess(const SBStructuredData &configuration);
----------------
Maybe rename to just "Start" and documentation will make it clear. We can also
overload the Start below and add a SBThread argument. See comment below.
================
Comment at: lldb/include/lldb/API/SBTrace.h:62
+ /// An error explaining any failures.
+ SBError StartThread(const SBThread &thread);
----------------
Remove this overload, and just use the one below. People can just default
construct a SBStructuredData if they don't want to modify any settings.
================
Comment at: lldb/include/lldb/API/SBTrace.h:80-81
+ /// An error explaining any failures.
+ SBError StartThread(const SBThread &thread,
+ const SBStructuredData &configuration);
----------------
Overload the "Start" method and just add a SBThread parameter to make it clear
we are tracing just a specific thread.
================
Comment at: lldb/include/lldb/API/SBTrace.h:87
+ /// An error explaining any failures.
+ SBError StopProcess();
+
----------------
Rename to "Stop"
================
Comment at: lldb/include/lldb/API/SBTrace.h:93
+ /// An error explaining any failures.
+ SBError StopThread(const SBThread &thread);
----------------
Rename to "Stop" and allow the overload to happen with the extra thread
argument. Do we have the ability to start thread 1, go for a while, start
thread 2, go for a while, stop thread 1, go for a while and then stop thread 2?
================
Comment at: lldb/include/lldb/API/SBTrace.h:102-104
+ void StopTrace(SBError &error,
+ lldb::tid_t thread_id = LLDB_INVALID_THREAD_ID);
----------------
Is this now deprecated?
================
Comment at: lldb/include/lldb/API/SBTrace.h:123
+protected:
+ lldb::TraceSP m_opaque_sp;
};
----------------
Having just one argument will change the layout of the object. We need to keep
"lldb::ProcessWP m_opaque_wp;" in from before so that layout of these objects
doesn't change. We can mark it as deprecated, but it needs to be there.
================
Comment at: lldb/include/lldb/API/SBTraceOptions.h:16
+/// Deprecated
class LLDB_API SBTraceOptions {
----------------
We can probably just mark the entire class as deprecated and avoid marking each
method.
================
Comment at: lldb/include/lldb/Target/Trace.h:235
+ /// \a llvm::Error otherwise.
+ virtual llvm::Error StartProcess(
+ StructuredData::ObjectSP configuration = StructuredData::ObjectSP()) = 0;
----------------
rename to "Start" and allow overload with next function based on parameters.
================
Comment at: lldb/include/lldb/Target/Trace.h:251
+ /// \a llvm::Error otherwise.
+ virtual llvm::Error StartThreads(
+ const std::vector<lldb::tid_t> &tids,
----------------
Rename to "Start"
================
Comment at: lldb/source/Plugins/Trace/intel-pt/constants.h:8
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_SOURCE_PLUGINS_TRACE_INTEL_PT_CONSTANTS_H
----------------
Add include header for "size_t"
================
Comment at: lldb/source/Target/Trace.cpp:294
return;
+
s.Printf(" ");
----------------
Revert diffs to this file
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
