labath added a comment.
First batch of comments from me, I think I will have some more once I gain more
insight into this. The main one is about the constructor/initialize,
destructor/destroy duality, which we should abolish. The rest is mostly
stylistic.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:618
+ traceMonitor);
+ if (traceMonitor.get() != nullptr) {
+ traceMonitor->setUserID(m_pt_process_uid);
----------------
`if(traceMonitor)`
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:619
+ if (traceMonitor.get() != nullptr) {
+ traceMonitor->setUserID(m_pt_process_uid);
+ m_pt_traced_thread_group.insert(thread_sp->GetID());
----------------
As far as, I can tell, every call to `StartProcessorTracing` is followed by
setUserId. Can we move the logic of setting the id inside the
`StartProcessorTracing` function
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2483
+template <typename T>
+static Status ReadFromFile(const char *filename, T &read,
+ std::ios_base::fmtflags mode = (std::ios_base::hex |
----------------
Please use llvm file API for this (see how `getProcFile` works -- it's just a
very thin wrapper around the llvm functions)
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2510
+ if (thread == LLDB_INVALID_THREAD_ID && uid == m_pt_process_uid) {
+ if (log)
+ log->Printf("NativeProcessLinux %s_thread not specified: %" PRIu64,
----------------
Please use LLDB_LOG (here and everywhere else in this patch)
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847
+
+Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) {
+ Status ret_error;
----------------
You are calling this function with uid == m_pt_process_uid, which means this
parameter is redundant, and leads to redundant sanity checks. Please remove it.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2965
+
+ config.setTraceBufferSize(bufsize);
+ config.setMetaDataBufferSize(metabufsize);
----------------
You are modifying the config, but the caller is ignoring these modifications.
If you don't need these, we could remove the modifications, make the config
argument `const` and end up with a much cleaner interface.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3056
+
+ auto BufferOrError = getProcFile("cpuinfo");
+ if (!BufferOrError) {
----------------
I don't see a getProcFile overload with this signature (although I am not
opposed to adding one). Are you sure this compiles?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3064
+ std::tie(Line, Rest) = Rest.split('\n');
+ {
+ SmallVector<StringRef, 2> columns;
----------------
Is this scope necessary? I find it just confuses the reader...
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3070
+ continue; // continue searching
+
+ if (log)
----------------
add: `columns[1] = columns[1].trim();`
Then you can skip calling trim everywhere else.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3075
+
+ if ((columns[0].find("cpu family") != std::string::npos) &&
+ columns[1].trim(" ").getAsInteger(10, cpu_family))
----------------
columns[0].contains(foo)
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3100
+ (stepping != static_cast<uint64_t>(-1)) && (!vendor_id.empty()))
+ break; // we are done
+ }
----------------
return here. Then you can avoid duplicating the check outside the loop.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:156
+ // ---------------------------------------------------------------------
+ static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+ size_t cyc_buf_size, size_t cyc_start,
----------------
How about ` void ReadCyclicBuffer(ArrayRef<uint8_t> buffer, size_t position,
MutableArrayRef<uint8_t> &dest)`
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+ enum PTErrorCode {
+ FileNotFound = 0x23,
+ ThreadNotTraced,
----------------
This seems suspicious. How did you come to choose this number?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:317
+ // ---------------------------------------------------------------------
+ class ProcessorTraceMonitor {
+ int m_fd;
----------------
Please move this into a separate file. NativeProcessLinux.cpp is big enough as
it is.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:331
+ public:
+ ProcessorTraceMonitor()
+ : m_fd(-1), m_mmap_data(nullptr), m_mmap_aux(nullptr),
----------------
This would be much cleaner, if we could avoid creating an "invalid trace"
object. The way that's usually done is to have a factory function, returning
say llvm::Expected<std::unique_ptr<T>> (or even llvm::Expected<T> if you are
able to make your type movable easily). The factory function then does all the
operations that can fail and returns an error in this case. Once it has
everything ready, it just calls the constructor, forwarding all the necessary
arguments. The constructor can perform any additional non-fallable
initialization, if necessary.
This way all the code inside and around the object can assume that once it has
an object of this type, it is a valid object. We don't have many examples of
this pattern, but you can for example look at MinidumpParser::Create or the
code in (D32930) for guidance. I am also working on a patch to switch
NativeProcess creation to use this pattern.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:348
+
+ void setUserID(lldb::user_id_t uid) { m_uid = uid; }
+
----------------
I believe we decided to call this "trace ID"
================
Comment at: unittests/CMakeLists.txt:77
+endif()
+add_subdirectory(Linux)
----------------
Please put this under unittests/Process/Linux (to match
unittests/Process/gdb-remote)
================
Comment at: unittests/Linux/CMakeLists.txt:1
+include_directories(${LLDB_SOURCE_DIR}/source/Plugins/Process/Linux)
+
----------------
Delete this. Just include the files as "Plugins/Process/Linux/XXX.h"
https://reviews.llvm.org/D33674
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
