Page MenuHomePhabricator

[analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap
ClosedPublic

Authored by Szelethus on Sep 27 2019, 3:18 PM.

Details

Summary

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.

Diff Detail

Event Timeline

Szelethus created this revision.Sep 27 2019, 3:18 PM
Szelethus marked 2 inline comments as done.Sep 27 2019, 3:23 PM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
900–902

It may be more precise to call this isCMemCall, because that's the question it answers.

1488

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.

NoQ marked an inline comment as done.Sep 27 2019, 6:42 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

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 :)

397–398

I think alloca deserves CDF_MaybeBuiltin. This would also probably allow you to remove the second line.

402–404

These are also builtins.

407–408

And these too.

1488

I think this is about the (lack of) support for __attribute__((malloc)).

Szelethus marked 2 inline comments as done.Sep 28 2019, 4:51 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

It should be always equal to it. I'll change it.

397–398

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 :)

NoQ added inline comments.Sep 28 2019, 7:46 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
397–398

Oh misery.

Szelethus marked 5 inline comments as done.Oct 2 2019, 2:38 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

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.

Szelethus updated this revision to Diff 222794.Oct 2 2019, 3:21 AM
Szelethus marked an inline comment as done.

Rebase.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1488

This should be an NFC change, and this part of the code will have to be rewritten anyways if we want a more serious annotation guided analysis.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

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?

887

Maybe return !!FreeingMemFnMap.lookup(Call)?

901

Same here: why not just return FreeingMemFnMap.lookup(Call) || NonFreeingMemFn.lookup(Call)?

NoQ added a comment.Nov 7 2019, 5:09 PM

I think my strdup comments weren't addressed >.<

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

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.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
887

I think the code changed after I wrote this comment, it is not valid anymore.

901

Here as well. Disregard these comments.

Szelethus marked 5 inline comments as done.EditedMar 1 2020, 8:43 AM

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

Szelethus updated this revision to Diff 247512.Mar 1 2020, 11:45 AM
Szelethus added a reviewer: balazske.
Szelethus set the repository for this revision to rG LLVM Github Monorepo.

Address inlines.

Szelethus marked 4 inline comments as done.Mar 1 2020, 11:47 AM
Szelethus added inline comments.
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
378–379

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.

balazske added inline comments.Mar 2 2020, 6:08 AM
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
358

The CHECK_FN could be used even here?

template <bool ShouldFreeOnFail>
CHECK_FN(checkRealloc)
1207

If these cases are exclusive the code looks better when else statements are added?

I wish for a third map, something like ReallocationMap. Other than that it is a great direction, I love it. Thanks!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
358

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.

Charusso accepted this revision.Mar 3 2020, 3:36 AM
This revision is now accepted and ready to land.Mar 3 2020, 3:36 AM
Szelethus marked an inline comment as done.Mar 6 2020, 8:04 AM

I wish for a third map, something like ReallocationMap. Other than that it is a great direction, I love it. Thanks!

Hah, that is a neat idea.

NoQ accepted this revision.Mar 15 2020, 10:16 PM

Looks wonderful, thanks!

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
358

passing around arguments by templates is not cool. They meant to be arguments.

+1! It's accidental that these values are currently known at compile time.

1207

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.

This revision was automatically updated to reflect the committed changes.
Szelethus marked 5 inline comments as done.

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?

NoQ added a comment.Mar 30 2020, 9:39 AM

$ git show --pretty=full 703a1b8caf09

commit 703a1b8caf09a5262a45c2179b8131922f71cf25
Author: Louis Dionne <ldionne@apple.com>
Commit: Kirstóf Umann <dkszelethus@gmail.com>

    [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

    ...

Not a phabricator bug. Maybe @Szelethus's local git somehow got in a weird state(?)

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?

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?

In D68165#1950212, @NoQ wrote:

Not a phabricator bug. Maybe @Szelethus's local git somehow got in a weird state(?)

Its been a source of endless misery :)

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.

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.

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.

Will do. Sorry for the trouble!