ravitheja added inline comments.
================ Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2847 + +Status NativeProcessLinux::StopProcessorTracingOnProcess(lldb::user_id_t uid) { + Status ret_error; ---------------- labath wrote: > 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. Well I wanted this function to be capable of being called from other contexts as well, but i never ended up using that way. ================ Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:2940-2943 +Status NativeProcessLinux::ProcessorTraceMonitor::StartTrace( + lldb::pid_t pid, lldb::tid_t tid, TraceOptions &config) { + Status error; + Log *log(ProcessPOSIXLog::GetLogIfAllCategoriesSet(POSIX_LOG_PTRACE)); ---------------- zturner wrote: > What happens if you call this function twice in a row? Or from different > threads? Is that something you care about? > What happens if you call this function twice in a row Second call will fail, > Or from different threads If called for different threads, then they will start to be traced, ofcourse assuming they are not being traced. ================ Comment at: source/Plugins/Process/Linux/NativeProcessLinux.cpp:3056 + + auto BufferOrError = getProcFile("cpuinfo"); + if (!BufferOrError) { ---------------- labath wrote: > I don't see a getProcFile overload with this signature (although I am not > opposed to adding one). Are you sure this compiles? Yes sorry I forgot to add this new file. ================ Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:319-320 + int m_fd; + void *m_mmap_data; + void *m_mmap_aux; + void *m_mmap_base; ---------------- labath wrote: > ravitheja wrote: > > zturner wrote: > > > Instead of having > > > ``` > > > void *m_mmap_data; > > > void *m_mmap_aux; > > > void *m_mmap_base; > > > ``` > > > > > > and then doing some ugly casting every time someone calls > > > `getAuxBufferSize` or `getDataBufferSize`, instead just write: > > > > > > ``` > > > MutableArrayRef<uint8_t> m_mmap_data; > > > MutableArrayRef<uint8_t> m_mmap_aux; > > > ``` > > > > > > and initializing these array refs up front using the size from the > > > header. This way you don't have to worry about anyone using the buffers > > > incorrectly, and the `ReadPerfTraceAux(size_t offset)` function no longer > > > needs to return a `Status`, but instead it can just return > > > `MutableArrayRef<uint8_t>` since nothing can go wrong. > > As mentioned in my previous comments, the m_mmap_data and m_mmap_aux are > > cyclic buffers and unfortunately the kernel keeps overwriting the buffer in > > a cyclic manner. The last position written by the kernel can only be > > obtained by inspecting the data_head and aux_head fields in the > > perf_event_mmap_page structure (pointed by m_mmap_base). > > > > Because of this scenario you see the ugly type casting. Now regarding the > > casting in getAuxBufferSize and getDataBufferSize I did not want to store > > the buffer sizes as even if store them since they are not the biggest > > contributors to the total type casting ugliness. plus if the correct sizes > > are already store in the perf_event_mmap_page structure why store them > > myself ?. > > > > Now regarding ReadPerfTraceAux(size_t offset) , i still need the size > > argument coz what if the user does not want to read the complete data from > > offset to the end and just wants a chunk in between the offset and the end ? > So, if this refers to a structure of type `perf_event_mmap_page`, why let the > type of this be `perf_event_mmap_page *`? That way you can have just one > cast, when you initialize the member, and not each time you access it. The thing is this structure is only available from a certain version of the perf_event.h. Although I have put macro guards everywhere I use this structure. But on previous Linux versions it won't be there, so I was afraid if i would run into issues. https://reviews.llvm.org/D33674 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits