On Mon, May 08, 2017 at 10:28:13AM +0200, Marc Glisse wrote: > 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, you probably could, you'd just need to pass it around some. > > > 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. So, I've been using a hacked up version of this to find places to use auto_bitmap. I ended up following places where one ssa name is assigned to another, and moving this pass after the ccp within all_early_optimizations. The pass location change was because at the earlier location there was places where the ssa name from the allocation was assigned to a vAR_DECL, that was ssa'd away at the later point. So it may actually be that optimization helps more than I thought, but I couldn't see an immediate reason we needed to be still using variables instead of ssa names in the one spot I looked at. > > > 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. yeah, it seems to me there's a couple parts an iterator over all statements that matches if a statement meets some requirements. That may be generalizable? Then there's a function that checks if you can get from one block to another without going through a given set of blocks. There's also a thing to find where a function is called with some arguments which I'm not sure if that's generally useful. Finally there's some glue holding specific checkers and the rest together. > 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. yeah, that's another thing to try and cover, though I suspect there's a lot of xmalloc like things out there that will have the !p case end in a noreturn block, which would already skip them from the analysis. > (maybe one can add strdup and similar functions later, but better start with > few functions for now) yeah, I think there's a bunch of things to explore in the space, but we should probably start by getting some of it finished ;) Trev > > -- > Marc Glisse
