[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/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+  Status error;
----------------
ravitheja wrote:
> labath wrote:
> > I'd like this work to be done in the destructor. Just like nobody should see a constructed-but-not-yet-initialized object, so the destroyed-but-not-destructed state should not exist. Then you don't need to worry about setting the state of the object to some sane values.
> > 
> > In fact, if you utilize std::unique_ptr capabilities, then you may not even need a destructor at all. Just define a class
> > ```
> > class munmap_delete {
> >   size_t m_length;
> > 
> > public:
> >   munmap_delete(size_t length) : m_length(length) {}
> >   void operator()(void *ptr) { munmap(ptr, m_length); }
> > };
> > ```
> > 
> > and then you can just declare  `
> > `std::unique_ptr<perf_event_mmap_page, munmap_delete> mmap_base(mmap_result, munmap_delete(mmap_size))`
> > 
> > This way, the allocation and deallocation code are close together, and you don't need to write yourself `// we allocated extra page` reminders.
> > 
> > Since we store m_aux in a ArrayRef already, this would require duplicating the pointer in the unique_ptr member. I am fine with that, but if you want to avoid it you can just have a getAux() member function which sythesizes the arrayref on demand.
> I can get the desired effect, by moving this function to the destructor, but we won't be able to catch munmap errors. I guess logging should be fine ? right now we were able to report errors in munmap.
They only way I see that munmap can fail is if you pass it a garbage argument.
```
 On success, munmap() returns 0, on failure -1, and errno is set (probably to EINVAL). :D
```
That is a programmer error, so I would be fine with even asserting on it, but logging and ignoring is fine as well. It's not like you can do anything better at that point anyway -- you're already ignoring the return value of Destroy...


https://reviews.llvm.org/D33674