labath added a comment.
Second round of comments. I still see a lot of places with redundant checks for
what appear to be operational invariants. I'd like to get those cleared up to
make the code flow better.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:611
+
+ if (ProcessorTraceMonitor::GetProcessTraceID() != LLDB_INVALID_UID) {
+ auto traceMonitor = ProcessorTraceMonitor::Create(
----------------
Every call to AddThread is followed by this block. Is there a reason we cannot
just move it inside the AddThread function?
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:621
+ LLDB_LOG(log, "{0} error in create m_code {1} m_string {2}",
+ __FUNCTION__, error2.GetError(), error2.AsCString());
+ }
----------------
LLDB_LOG will already print the function name (same thing everywhere else you
use `__FUNCTION__`), so you don't need to write it out yourself. Also the
AsCString is unnecessary, as Status has a pretty-printer.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2534
+ size_t offset) {
+ TraceOptions trace_options;
+ Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE));
----------------
unused variable
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2627
+ if (!traceMonitor) {
+ Status error2(traceMonitor.takeError());
+ LLDB_LOG(log, "{0} error in create m_code {1} m_string {2}", __FUNCTION__,
----------------
As of r305462 you can write `error = traceMonitor.takeError()`, so you
shouldn't need the extra variable.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2644
+ if (iter != m_processor_trace_monitor.end())
+ error = StopTrace(iter->second->GetTraceID(), thread);
+
----------------
Should you delete the iterator from the map after this? In fact, the mere act
of deleting the iterator should probably be what triggers the trace stop, in
line with RAII principles.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2678
+ Status error;
+ if (ProcessorTraceMonitor::GetProcessTraceID() == LLDB_INVALID_UID) {
+ error.SetError(ProcessNotBeingTraced, eErrorTypeGeneric);
----------------
This is dead code. The caller has already checked this condition. This should
be at most an assertion.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:263
+ // is sufficient to obtain the actual ProcessorTrace instance.
+ ProcessorTraceMonitorSP LookupProcessorTraceInstance(lldb::user_id_t traceid,
+ lldb::tid_t thread,
----------------
ErrorOr<T> is a better way to return a value *or* an error. Although, in this
case, it sounds like you're really only interested in the found-or-not-found
distinction, so you could just drop the Status argument and let a null return
value signify an error.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:277
+
+ llvm::DenseMap<lldb::tid_t, ProcessorTraceMonitorSP>
+ m_processor_trace_monitor;
----------------
I'd like to downgrade these to unique pointers to ProcessTraceMonitor. There's
no reason for these to ever outlive or escape the process instance, so it's
natural to say they are strongly owned by it. In other places where you use
ProcessorTraceMonitorSP you can just use regular pointers or references
(depending on whether they can be null or not).
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:282
+ // same process user id.
+ llvm::DenseSet<lldb::tid_t> m_pt_traced_thread_group;
};
----------------
I am confused about the purpose of this member variable. As far as I can tell,
it will always contain *either* all of the threads in the process *or* none of
them. Is there a situation where this is not the case?
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:158
+ LLDB_LOG(log, "ProcessorTrace failed to open Config file");
+ error.SetError(FileNotFound, eErrorTypePOSIX);
+ return error;
----------------
eErrorTypePOSIX is used for errno error values. Please don't try to pass your
invented error codes as these.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:321
+ if (!pt_monitor_sp) {
+ LLDB_LOG(log, "NativeProcessLinux {0}", "Alloc failed");
+ error.SetError(AllocFailed, eErrorTypeGeneric);
----------------
dead code
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:412
+#ifndef PERF_ATTR_SIZE_VER5
+ LLDB_LOG(log, "ProcessorTrace {0} Not supported ", __FUNCTION__);
+ error.SetError(PerfEventNotSupported, eErrorTypeGeneric);
----------------
Just put llvm_unreachable here. There shouldn't be any way to construct this
object if tracing is not supported.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:480
+ if (base == nullptr) {
+ LLDB_LOG(log, "ProcessorTrace {0} Null pointer ", __FUNCTION__);
+ error.SetError(NullPointer, eErrorTypeGeneric);
----------------
Dead code? The construction of the object shouldn't have succeeded if you
failed to allocate the memory or initialize the trace.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:20
+
+namespace lldb_private {
+
----------------
All the other files in the folder are in an additional process_linux namespace.
This file should go there as well.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:70
+ void *m_mmap_data;
+ void *m_mmap_aux;
+ void *m_mmap_base;
----------------
Make this MutableArrayRef<uint8_t>. Then you don't need the ugly cast when
calling readcyclicbuffer, nor the `GetAuxBufferSize` function (as it just
becomes `m_mmap_aux.size()`). Same for `m_mmap_data`.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.h:79
+ // Trace id of trace instance corresponding to the process.
+ static lldb::user_id_t m_pt_process_traceid;
+
----------------
Instead of global variables (we may want to debug more than one process
eventually), we could have a member variable `std::pair<user_id_t,
TraceOptions> m_process_trace;` (you can have a special class for that if you
don't like .first and .second everywhere).
================
Comment at: unittests/Linux/ProcessorTraceTest.cpp:1
+//===-- ProcessorTraceTest.cpp -------------------------------- -*- C++
-*-===//
+//
----------------
You seem to have forgotten to delete this.
================
Comment at: unittests/Process/Linux/ProcessorTraceTest.cpp:41
+ bytes_read = ReadCylicBufferWrapper(
+ nullptr, sizeof(smaller_buffer), cyclic_buffer, sizeof(cyclic_buffer), 3,
+ 0);
----------------
I don't understand why you're testing these. Why would anyone create an
ArrayRef pointing to null? If you get handed an array ref, you should be free
to assume it points to valid data.
https://reviews.llvm.org/D33674
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
