labath added a comment.
I like the direction this is going in, but I see places for more cleanup. The
main three items are:
- making destruction cleaner
- avoiding global variables
- making ReadCyclicBuffer readable
the rest are just general nits.
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2551
+ if (!process_config) {
+ error = process_config.takeError();
+ return error;
----------------
`return process_config.takeError()` is two lines shorter
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2568
+lldb::user_id_t
+NativeProcessLinux::StartTracingAllThreads(const TraceOptions &config,
+ Status &error) {
----------------
This is still called trace*ALL*threads
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2640
+ if (iter == m_processor_trace_monitor.end())
+ {
+ error.SetErrorString("tracing not active for this thread");
----------------
the formatting of the bracket is wrong
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2726
+
+ LLDB_LOG(log, "UID - {0} , Thread -{1}", traceid, thread);
+
----------------
s/UID/traceid/
================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:114
+ Status StopTrace(lldb::user_id_t traceid,
+ lldb::tid_t thread = LLDB_INVALID_THREAD_ID) override;
+
----------------
having default argument values on virtual functions is dangerous. Please remove
these. AFAIK, noone is going to be calling these through a NativeProcessLinux
pointer anyway.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:33
+
+uint64_t ProcessorTraceMonitor::GetAuxBufferSize() const {
+ return m_aux.size();
----------------
Are these functions still useful?
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:129
+ LLDB_LOG(log, "failed to open Config file");
+ error.SetErrorString("file not found");
+ return error;
----------------
You can consider `return ret.getError()`, which will probably give you a more
detailed error message.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:148
+ if (error.Fail()) {
+ LLDB_LOG(log, "Status in custom config {0}", error.AsCString());
+
----------------
drop AsCString()
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:243
+ LLDB_LOG(log, "{0}:{1}:{2}:{3}", cpu_family, model, stepping,
+ vendor_id.c_str());
+
----------------
drop c_str()
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+ Status error;
----------------
I'd like this work to be done in the destructor. Just like nobody should see a
constructed-but-not-yet-initialized object, so the destroyed-but-not-destructed
state should not exist. Then you don't need to worry about setting the state of
the object to some sane values.
In fact, if you utilize std::unique_ptr capabilities, then you may not even
need a destructor at all. Just define a class
```
class munmap_delete {
size_t m_length;
public:
munmap_delete(size_t length) : m_length(length) {}
void operator()(void *ptr) { munmap(ptr, m_length); }
};
```
and then you can just declare `
`std::unique_ptr<perf_event_mmap_page, munmap_delete> mmap_base(mmap_result,
munmap_delete(mmap_size))`
This way, the allocation and deallocation code are close together, and you
don't need to write yourself `// we allocated extra page` reminders.
Since we store m_aux in a ArrayRef already, this would require duplicating the
pointer in the unique_ptr member. I am fine with that, but if you want to avoid
it you can just have a getAux() member function which sythesizes the arrayref
on demand.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:358
+#else
+ if (buffer.empty()) {
+ LLDB_LOG(log, "Empty buffer ");
----------------
ReadCyclicBuffer will handle the case of a zero-sized buffer gracefully, right?
You don't need to check the size at every level.
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:438
+ }
+ buffer = llvm::MutableArrayRef<uint8_t>(buffer.data(),
+ (buffer.size() - bytes_remaining));
----------------
`buffer = buffer.drop_back(bytes_remaining) ` ?
================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:475
+
+ if (dst.size() >= size_to_read) {
+ if (offset >= firstpart.size()) {
----------------
I think there are waaaaaay too many nested conditionals here. Let's try this
implementation for a change:
```
SmallVector<ArrayRef<uint8_t>, 2> parts{ src.slice(src_cyc_index),
src.drop_back(src_cyc_index)};
if (offset >= parts[0].size()) {
offset -= parts[0].size();
parts.drop_front();
}
parts[0] = parts[0].drop_front(offset);
```
That takes care of offset. Now on to the copy.
```
dst = dst.take_front(size_to_read);
auto next = dst.begin();
for(auto part: parts) {
size_t chunk_size = std::min(size_to_read, part.size());
next = std::copy_n(part.begin(), chunk_size, next);
size_to_read -= chunk_size;
}
```
Much easier to reason about and less than half the size of the original
implementation.
================
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;
+
----------------
labath wrote:
> 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).
I guess I wasn't too clear about this, but I meant a member variable in the
Process class. This solves the "multiple process" issue, but creates a couple
of other ones. All the call sites are now more complicated, and you have to
worry about managing the lifetime of the entries in this map. If this was a
Process member variable, those would be handled automatically (and it makes
more sense, since it is the process class that owns these traces instances).
https://reviews.llvm.org/D33674
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
