Since its important to know whether a function frees memory (even if its a reallocating function!), I used two CallDescriptionMaps to merge all CallDescriptions into it. MemFunctionInfoTy no longer makes sense, it may never have, but for now, it would be more of a distraction then anything else.
It may be more precise to call this isCMemCall, because that's the question it answers.
Ugh, no idea whats happening here, I'll be honest. I sweat to dig a bit deeper to find out, though I didn't want to bother delaying the publishing this patch for a feature not documented, or used anywhere.
Whenever i see a (CE, State) pair, it screams CallEvent to me. That said, i'm worried that State in these callbacks isn't necessarily equal to C.getState() (the latter, by the way, is always equal to the CallEvent's .getState() - that's a relief, right?), so if you'll ever be in the mood to check that, that'd be great :)
I think alloca deserves CDF_MaybeBuiltin. This would also probably allow you to remove the second line.
These are also builtins.
And these too.
I think this is about the (lack of) support for __attribute__((malloc)).
It should be always equal to it. I'll change it.
Actually, BuiltinFunctionChecker uses evalCall to create an AllocaRegion for __builtin_alloca. I spent an hour when writing this patch to track a crash down when I initially made this CDF_MaybeBuiltin :)
Hmmm, I tried making this take a (CallEvent, CheckerContext), but it bloated the code for little gain, since every handler function would start with the retrieval of the state and the call expression. That said, I could cascade these down to FreeMemAux, MallocMemAux, etc, to also take this pair instead, but I'll be honest, I don't see point until something actually breaks.
This is the standard way in the checkers: almost every handler function starts with the retrieval of the state from the CheckerContext. The only reason for an extra State parameter is that sometimes we create more states in the lower level functions but only add the final one to the CheckerContext as a new transition. Does something like this happen here?
Maybe return !!FreeingMemFnMap.lookup(Call)?
Same here: why not just return FreeingMemFnMap.lookup(Call) || NonFreeingMemFn.lookup(Call)?
I think my strdup comments weren't addressed >.<
I agree with @baloghadamsoftware here. If you pass State explicitly, it looks like an indication for the reader that this State may be different from C.getState(). If in practice they're the same, i'd rather do C.getState() in every method than confuse the reader.
Also do you remember what makes us query CallExpr so often? Given that CallEvent includes CallExpr, we should be able to expose everything we need as CallEvent methods. Say, we should be able to replace MallocMemAux(C, CE, CE->getArg(0), ...) with MallocMemAux(C, Call, Call.getArgExpr(0), ...) and only do Call.getOriginExpr() once when we report a bug.
I've spent a lot of time on this code, and I don't think it'd be a great idea to do everything in a single step. Collapsing CallExpr, ProgramState into CallEvent is a shockingly a non-trivial task. While I'm happy to not commit this patch before uploading a solution to that problem, I'd prefer (and I think you would too!) to do them in followup patches.
The point of this revision was to drop in CallDescriptionMap -- I like to think that it introduces no new problems, it just doesn't solve many existing ones :^)
edit: I will soon address the inlines and other comments with an update
I'll upload followups where I'm addressing these issues -- to keep things simple, I decided against increasing this patch's scope. I won't commit until those are also accepted.
I wish for a third map, something like ReallocationMap. Other than that it is a great direction, I love it. Thanks!
I think about the opposite, passing around arguments by templates is not cool. They meant to be arguments.
This type of callback simply could be a third CallDescriptionMap for future profeness.
Looks wonderful, thanks!
+1! It's accidental that these values are currently known at compile time.
Yup, or even better, returns, because this code is very fragile because it'll do horrible things if we accidentally invoke two callbacks at once.
Closed by commit rG703a1b8caf09: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap (authored by ldionne, committed by Szelethus). · Explain Why
This is really strange -- it looks like 703a1b8caf09 was attributed to me in git, but I have nothing to do with this change. How did you commit the patch? Is that some Phabricator bug?
$ git show --pretty=full 703a1b8caf09
commit 703a1b8caf09a5262a45c2179b8131922f71cf25 Author: Louis Dionne <firstname.lastname@example.org> Commit: Kirstóf Umann <email@example.com> [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap ...
Not a phabricator bug. Maybe @Szelethus's local git somehow got in a weird state(?)
Ugh, I squashed my local commits and pushed, it seems like for some reason it made you the author... it has happened to me before (always with you, for some reason), but was able to notice and correct it. There is little we can do about this, right?
Its been a source of endless misery :)
Always with me? Maybe you should double-check that you don't have something weird in your git config, aliases or other? I don't know *why* it would be me in particular, but the fact that it is consistently the same person is kind of suspicious.