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

[Pavel Labath via Phabricator via lldb-commits]

labath 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,
----------------
ravitheja wrote:
> 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.
I believe zachary did not understand what the function does. The position is indeed necessary. However, I believe the prototype I suggested will work for you.

As for the function itself, I think it is way over-engineered. More than half of the function is sanity-checking, and more than half of the tests are tests of the sanity checks.

With ArrayRef, the function could be implemented as:
```
auto remaining = buffer.drop_front(position);
if(remaining.size() > dest.size())
  std::copy(remaining.begin(), dest.size(), dest.begin());
else
  std::copy_n(buffer.begin(), remaining.size() - dest.size(), 
    std::copy(remaining.begin(), remaining.end(), dest.begin());
```
(this will be slightly more complicated if you need to handle the `dest.size() > buffer.size()` case, which I am not sure if you need)



================
Comment at: source/Plugins/Process/Linux/NativeProcessLinux.h:160
+  enum PTErrorCode {
+    FileNotFound = 0x23,
+    ThreadNotTraced,
----------------
ravitheja wrote:
> 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.
If they're completely arbitrary, then how about starting with number one?

Not being able to send complex error messages across the gdb-protocol is a bit of a drag. I would not be opposed to adding such a facility, as I wished for that a couple of times in past. If you wish to do that, then we should start a separate discussion.


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


https://reviews.llvm.org/D33674