[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Ravitheja Addepally via Phabricator via lldb-commits]

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