Re: [PATCH, RFC] warn about raw pointers that can be unique_ptr<T>

[Trevor Saunders <tbsaunde at tbsaunde dot org>]

On Sun, May 07, 2017 at 07:32:48PM +0200, Marc Glisse wrote:
> On Sun, 7 May 2017, tbsaunde+gcc@tbsaunde.org wrote:
> 
> > This is a start at warning about various resource allocation issues that can be
> > improved.  Currently it only warns about functions that call malloc and then
> > always pass the resulting pointer to free().
> > 
> > It should be pretty simple to extend this to new/delete and new[]/delete[], as
> > well as checking that in simple cases the pairing is correct.  However it
> > wasn't obvious to me how to tell if a function call is to a allocating operator
> > new.  We probably don't want to warn about placement new since complicated
> > things can be happening there.  There is the DECL_IS_OPERATOR_NEW() but that
> > doesn't seem to cover class specific operator new overloads, and maybe even
> > custom ones at global scope?
> > 
> > Other things that may be reasonable to try and include would be open / close
> > and locks.
> > 
> > It might also be good to warn about functions that take or return unique
> > ownership of resources, but I'm not quiet sure how to handle functions that
> > allocate or deallocate shared resources.
> 
> Interesting.
> 
> 1) How do you avoid warning for code that already uses unique_ptr? After
> inlining, it will look just like code calling new, and delete on all paths.

Currently by running early enough that no inline has happened.  I'm not
sure how valuable it would be to try and find things post inlining
instead.

> Well, currently you only handle malloc/free, but if you provide an inline
> implementation of new/delete that forwards to malloc/free, the issue should
> already appear. Or maybe the pass is very early? But then the compiler will
> likely seldom notice that the argument to free is actually the return of
> malloc.

I wonder how true this last bit is, sure we could find some more things
after more optimization, but its not obvious to me that it would be so
much to really be worth it.  Looking at gcc itself there's plenty of
places where this should already be enough with tracking places where
the pointer is coppied from one ssa name to another.  I feel like the
big areas of improvement are warning about functions that take or return
ownership of things.

> 2) In some sense, we would want to warn when some path is missing the call
> to free, but it seems very hard to distinguish the cases where it is
> accidentally missing from those where it is done on purpose. Maybe detect
> cases where the only paths missing free are through exceptions?

I didn't think about exceptions, but I'd been thinking of trying to warn
about variables we prove don't escape in alias analysis, but that would
probably be a small set of all allocations.

> 3) This seems very similar to the conditions needed to transform a call to
> malloc into a stack allocation. I was experimenting with this some years
> ago, see for instance
> https://gcc.gnu.org/ml/gcc-patches/2013-11/msg01049.html (it is probably not
> the most recent message on the subject, since the main code moved to CCP
> afterwards http://geometrica.saclay.inria.fr/team/Marc.Glisse/tmp/heapstack,
> but I can't locate more recent messages at the moment, and it was more a
> prototype than a ready patch). Do you think the infrastructure could be
> shared between your warning and such an optimization? Or are the goals
> likely to diverge too soon for sharing to be practical?

yeah, some of the issues are certainly similar.  I'm not sure if there
would be things worth sharing probably easiest to find out by just
trying it.

Trev

> 
> -- 
> Marc Glisse