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