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