[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.h:156
+  // ---------------------------------------------------------------------
+  static size_t ReadCyclicBuffer(void *buf, size_t buf_size, void *cyc_buf,
+                                 size_t cyc_buf_size, size_t cyc_start,
----------------
zturner wrote:
> labath wrote:
> > How about ` void ReadCyclicBuffer(ArrayRef<uint8_t> buffer, size_t position, MutableArrayRef<uint8_t> &dest)`
> Better yet, drop the `position` argument entirely.  `ReadCyclicBuffer(src, N, dest)` is equivalent to `ReadCyclicBuffer(src.drop_front(N), dest);`
So there are couple of things i would like to mention ->
1) The data and aux buffers allocated are cyclic in nature, so we need to consider the cyc_start or starting index in order to correctly read them.
2) The function ReadCyclicBuffer is very generic and is capable of reading a certain chunk of memory starting from a certain offset.
3) Now I could have kept this function in the private domain so that the cyc_start need not be passed everytime, but then I also wanted to unit test this function.

Now keeping these in mind my questions are the following ->
@zturner @labath  I need the offset and cyc_start , both are required so the position argument won't suffice ?

Please correct me if i am wrong.


================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+  enum PTErrorCode {
+    FileNotFound = 0x23,
+    ThreadNotTraced,
----------------
labath wrote:
> This seems suspicious. How did you come to choose this number?
Sometime ago I asked in the devlist about the error codes in lldb and got the answer that they were completely arbitrary, so 0x23 has no special meaning. The question that I would like to ask is do u think these error codes should be here ? coz in https://reviews.llvm.org/D33035 it was suggested by @clayborg that tools working on top of the SB API's should only receive Error Strings and not codes.

Currently anyone who would encounter these codes would have to refer here for their meaning. Also the remote packets don't allow me to send Error Strings instead of error codes.


================
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;
----------------
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 ?


https://reviews.llvm.org/D33674