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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
