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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
