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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Jun 26 08:22:59 PDT 2017


labath added a comment.

In https://reviews.llvm.org/D33674#790595, @ravitheja wrote:

> Yes you can start looking at it. The thing with munmap stuff is the following, you basically don't want to give the user an opportunity to have an uninitialized or instances which have been destroyed but not deleted.
>  So I removed the Destroy function from the public space. Now the user need not worry about Destroy function.


Ok, so that was one of my reasons for doing this. The other one is internal code cleanlyness -- it's much easier to verify that the code is healthy by just looking at it when the initialization and deinitialization is close together. unique_ptr allows you to do that. In this code

  if ((result = mmap(..., size)) != MAP_FAILED)
    ptr_up = std::unique_ptr(result, mmap_deleter(size));

the init and deinit code is on adjecant line, and it's guaranteed than memory will be freed. Here:

  first_function() {
  ptr = mmap(..., size);
  ref=ArrayRef(ptr, size-1);
  ...
  }
  
  ...
  
  second_function() {
    ...
    // Remember to increment size by one
    munmap(ref.data(), ref.size()+1);
    ... 
  }

it's not so obvious because the code is far apart and you need to carry state around. To verify correctness I need to look at the two pieces of code, and then find all possible code paths between them.

> Regarding the testing strategy, we have tests at two levels, one at the SB API level and the other at the tool level.

Cool, are you going to put some of those tests in this patch?



================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:393
+  llvm::SmallVector<MutableArrayRef<uint8_t>, 2> parts = {
+      src.slice(src_cyc_index), src.drop_back(src.size() - src_cyc_index)};
+
----------------
ravitheja wrote:
> labath wrote:
> > Isn't the drop-back expression equivalent to `src.take_front(src_cyc_index)`?
> The problem with that is I don't see the documentation of some functions and I have to dig in the code.
> Which is why I did not used it.
That's fine, there are a lot of these APIs and it's not possible to remember all of them.  However, now that you know about the function, wouldn't you agree that it's cleaner to use it?


https://reviews.llvm.org/D33674





More information about the lldb-commits mailing list