On Sun, 7 May 2017, Trevor Saunders wrote:
On Sun, May 07, 2017 at 07:32:48PM +0200, Marc Glisse wrote:
On Sun, 7 May 2017, tbsaunde+...@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.
Ah, right, for the warning it indeed makes sense to be pre-inlining, as it
isn't clear you could use unique_ptr if the call to free was not directly
in this function.
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.
I fear that there are many cases where you need more PRE / DOM / SRA / ...
for the SSA_NAME to end up in a place where malloc and free match, but if
you already detect some cases, that's good.
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.
It seems that the differences will be larger than I originally expected,
so it isn't clear the gain would be worth it.
While I remember, one (easy) case you may want to handle is:
p = malloc(n);
if (p != 0) ...
where the else branch does not need to contain a call to free.
(maybe one can add strdup and similar functions later, but better start
with few functions for now)
--
Marc Glisse
