Page MenuHomePhabricator

[analyzer] Teach the MallocChecker about Glib API for two arguments
ClosedPublic

Authored by xiangzhai on Mar 8 2017, 7:50 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

xiangzhai created this revision.Mar 8 2017, 7:50 PM
xiangzhai retitled this revision from [analyzer] Teach the MallocChecker about about Glib API for two arguments to [analyzer] Teach the MallocChecker about Glib API for two arguments.
xiangzhai updated this revision to Diff 91143.Mar 9 2017, 1:12 AM

Fix svn diff generate wrong (gmalloc.c) testcase issue.

NoQ edited edge metadata.Mar 20 2017, 7:17 AM

Hello, thanks for the patch!

It seems that ReallocMemN re-uses a lot of code from the original ReallocMem. Is it possible to re-use rather than copy-paste the code? For example, by moving the common code to a separate function like ReallocMemAux, which is then called from both functions, or turning these two functions into one with an extra boolean paramenter.

xiangzhai updated this revision to Diff 92429.EditedMar 20 2017, 10:22 PM

Hi NoQ,

Thanks for your review!

As you suggested, I rename ReallocMem to ReallocMemAux with an extra boolean paramenter SuffixWithN, please review it, thanks a lot!

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 92442.Mar 21 2017, 2:02 AM

Fix different type issue.

xiangzhai updated this revision to Diff 92580.Mar 21 2017, 6:15 PM

Fix Arg2 Expr might be nullptr issue.

xiangzhai updated this revision to Diff 92755.EditedMar 22 2017, 7:01 PM

Further reduce redundant code.

NoQ added inline comments.Apr 4 2017, 8:50 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
791–793 ↗(On Diff #92755)

You can reduce this to SVal nBlocks = C.getSVal(Blocks).

2070 ↗(On Diff #92755)

In case SuffixWithN is false, this code multiplies SVal for Arg1 by SVal of Arg1, and i do not think this is what you want. I suggest we do not initialize Arg2 or Arg2ValG in case we don't have the suffix.

Generally, i'd prefer the code for computing the buffer size to be structured as

if (SuffixWithN) {
  /* take two expressions, take two values, multiply */
} else {
  /* take one expression, take its value */
}

Then avoid the ?: operator below by always having the correct value in TotalSizeG.

The good thing about such structure is, we have only one if(), so it might be faster to execute, and all the different variants are gathered in one place, so it's easier to read.

By the way, what does G stand for?

xiangzhai updated this revision to Diff 94169.Apr 5 2017, 1:30 AM

Hi Artem,

Thanks for your review!

I updated my patch using if () {} else {} instead of ? :

And someone used Arg1ValG so I just keep formation TotalSizeG have not idea what G stands for...

Regards,
Leslie Zhai

NoQ added a comment.Apr 5 2017, 5:15 AM

I updated my patch using if () {} else {} instead of ? :

Nono, that wasn't the point, i have no preference between those. I mean, you can avoid branching completely. See the new inline comment.

And someone used Arg1ValG so I just keep formation TotalSizeG have not idea what G stands for...

Aha, that explains :)

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
904 ↗(On Diff #94169)

So, the reason for the crash in D31453 is that you forgot to initialize II_g_try_malloc_n in initIdentifierInfo(). It remains null, and therefore FunI == II_g_try_malloc_n is true whenever FD is an anonymous function (for example, an operator). For such operator, you encounter argument SVals that you cannot multiply (pointers in your case), hence you step onto the assertion.

Note how the assertion was useful for detecting the problem; this is the reason why we don't remove assertions when they fail, but try to find the root cause of the issue :)

2075 ↗(On Diff #94169)

I still think that variable names should be helpful, and it's not worth it to reuse variables just for reusing variables.

I suggest making TotalSizeG a DefinedOrUnknownSVal, and never using Arg1Val after TotalSizeG is computed, on all branches.

NoQ added inline comments.Apr 5 2017, 5:17 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
904 ↗(On Diff #94169)

I'm also noticing test failures on existing analyzer tests after applying this patch:

Failing Tests (4):

Clang :: Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
Clang :: Analysis/MismatchedDeallocator-checker-test.mm
Clang :: Analysis/NewDelete-custom.cpp
Clang :: Analysis/NewDelete-variadic.cpp

These would need to be fixed. It is likely that the problem with II_g_try_malloc_n is causing these failures, because they are related to custom operators.

xiangzhai updated this revision to Diff 94313.Apr 5 2017, 7:23 PM

Hi Artem,

Thanks for your review!

So, the reason for the crash in D31453 is that you forgot to initialize II_g_try_malloc_n in initIdentifierInfo().

Sorry for my stupid careless...

And I changed TotalSizeG to TotalSize then TotalSize.castAs<DefinedOrUnknownSVal>() as you suggested.

For double check, I run the testcase:

Clang :: Analysis/gmalloc.c
Clang :: Analysis/Malloc+MismatchedDeallocator+NewDelete.cpp
Clang :: Analysis/NewDelete-custom.cpp
Clang :: Analysis/NewDelete-variadic.cpp
k3bdevice.cpp

Regards,
Leslie Zhai

zaks.anna edited edge metadata.Apr 5 2017, 9:39 PM

Hi Leslie,

How do you run the tests? You should run all clang (or at least all analyzer) tests when testing the patches, not just select test cases.

Hi Anna,

Thanks for your correcting! Sorry for my mistake, I use make check-clang-analysis instead of just select test cases.

Regards,
Leslie Zhai

NoQ added inline comments.Apr 18 2017, 7:02 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2091–2094 ↗(On Diff #94313)

I'll try to explain again. I believe that in a better world, this line would look like this:

ProgramStateRef stateMalloc = MallocMemAux(C, CE, TotalSize, 
                                           UndefinedVal(), StatePtrIsNull);

Please change the code above so that this line, as i wrote it, worked, by computing total size correctly on all branches.

Hi Artem,

Thanks for your review! I will update my patch tomorrow, good night in my time zone :)

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 95684.Apr 18 2017, 9:43 PM
xiangzhai added a subscriber: cfe-commits.

Hi Artem,

I updated my patch as you suggested: never using Arg1Val after TotalSize is computed, on all branches. please review it, thanks a lot!

Regards,
Leslie Zhai

danielmarjamaki requested changes to this revision.Apr 19 2017, 7:56 AM
danielmarjamaki added inline comments.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
788 ↗(On Diff #95684)

I have the feeling this should be renamed. Since its purpose is to calculate the total size maybe MallocChecker::calculateBufferSize()

790 ↗(On Diff #95684)

Please begin variables with Capitals. svalBuilder,nBlocks,nBlockBytes

911 ↗(On Diff #95684)

Capital. svalBuilder

2040 ↗(On Diff #95684)

Capital variable: arg0Expr, arg0Val

2054 ↗(On Diff #95684)

is this "if (!Arg1)" needed? It seems to me that if CE->getNumArgs() returns >= 2 then CE->getArg(1) should be non-NULL. Does it crash/assert if you remove this condition?

2061 ↗(On Diff #95684)

hmm.. seems weird if CE->getArg(2) returns nullptr even though CE->getNumArgs() >= 3 .. is it possible to remove this check or does that cause some crash etc?

Generally, i'd prefer the code for computing the buffer size to be structured as

I would also prefer such code.

This revision now requires changes to proceed.Apr 19 2017, 7:56 AM
xiangzhai updated this revision to Diff 95874.EditedApr 19 2017, 8:06 PM
xiangzhai edited edge metadata.

Hi Daniel,

Thanks for your review!

I updated my patch as you suggested! Please review it, thanks a lot! but I argue that:

I have the feeling this should be renamed. Since its purpose is to calculate the total size maybe MallocChecker::calculateBufferSize()

Yes! it is difficult to suitably name variable, function, project, and even my kid :) but calculateBufferSize is not make sense:

  1. how to calculate? by evalBinOp do BO_Mul operation.
  2. for what? for bytes size so forBufferSize is ok.

Capital variable: arg0Expr, arg0Val

I hold the view that I need to respect original developers' code, and it need a Global Patch for Capital variable, just like KDE's Use nullptr everywhere

is this "if (!Arg1)" needed? It seems to me that if CE->getNumArgs() returns >= 2 then CE->getArg(1) should be non-NULL. Does it crash/assert if you remove this condition?

The same story.

Regards,
Leslie Zhai

xiangzhai updated this revision to Diff 95880.Apr 19 2017, 9:40 PM
NoQ added a comment.Apr 19 2017, 10:11 PM

Yay, now that's a lot cleaner!

Regarding style, recently we tend to ask for updating the style in the new code, and i don't think the Golden Rule (http://llvm.org/docs/CodingStandards.html#introduction) does much in our case, but i don't have a strong opinion on that. Maybe we could make a checker to make sure all SValBuilder references are called, say, SVB, and all SVal instances have names ending with Val, and have ourselves fix all the issues reported :) But because the analyzer is done by very few people, i don't think this would happen soon, but having the new code stylish is kind of nice anyway.

However, the null-check for the argument expression is also a performance issue (the compiler would never guess it's always non-null), so i believe it should definitely be removed.

xiangzhai updated this revision to Diff 95887.Apr 19 2017, 10:25 PM

Hi Artem,

Thanks for your review! I updated my patch, please review it.

Regards,
Leslie Zhai

I hold the view that I need to respect original developers' code, and it need a Global Patch for Capital variable, just like KDE's Use nullptr everywhere

Yes I was too picky. Please respect the original developers' code.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2040 ↗(On Diff #95684)

sorry this is not your code. I remove my comment.

Hi Daniel,

I respect you! the father of cppcheck and KDE's buildbot use cppcheck for example Static Analysis K3B thanks for your great job!

Regards,
Leslie Zhai

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
788 ↗(On Diff #95684)

please respond to comments by clicking on the "Reply" button for the comment. So your responce will be shown near my comment.

yes it can be hard to figure out a good name.

alright, you didn't like my suggestion then let's skip that.

do you need to start the name with "sval". does that indicate that a "sval" is returned? then that would be unusual imho.

I guess I don't have a strong opinion but I would also remove "Bin". The "Mul" says that imho.

how about evalMulForBufferSize?

xiangzhai updated this revision to Diff 95894.Apr 19 2017, 11:46 PM

SValBinMulOp -> evalMulForBufferSize

xiangzhai marked an inline comment as done.Apr 19 2017, 11:48 PM
xiangzhai added inline comments.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
788 ↗(On Diff #95684)

renamed to evalMulForBufferSize :)

xiangzhai marked an inline comment as done.Apr 19 2017, 11:48 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
337 ↗(On Diff #95894)

Thanks for renaming the function I am happy now with that name. :-)

hmm.. if you have CheckerContext parameter already then ProgramStateRef parameter seems redundant. You should be able to use C.getState().

However looking at surrounding code it seems you can provide ProgramStateRef for consistency.

I don't have a strong opinion, but I would remove this State.

xiangzhai added inline comments.Apr 20 2017, 2:38 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
337 ↗(On Diff #95894)

I will update it tomorrow! I have to gotta home to look after my kid :)

xiangzhai updated this revision to Diff 96084.Apr 20 2017, 7:51 PM

Further reduce redundant code.

xiangzhai marked 2 inline comments as done.Apr 20 2017, 7:52 PM

I don't have further comments except that I would personally rewrite:

// Get the value of the size argument.
SVal TotalSize = State->getSVal(Arg1, LCtx);
if (SuffixWithN) {
  const Expr *Arg2 = CE->getArg(2);
  TotalSize = evalMulForBufferSize(C, Arg1, Arg2);
}

to:

// Get the value of the size argument.
SVal TotalSize;
if (!SuffixWithN) {
  TotalSize = State->getSVal(Arg1, LCtx);
} else {
  TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2));
}
xiangzhai updated this revision to Diff 96101.Apr 20 2017, 11:33 PM

Further reduce redundant code.

you can ignore my comment ... LGTM

I don't want to accept this revision. Hopefully NoQ or somebody else can do that.

Never mind :)

NoQ accepted this revision.Apr 25 2017, 10:18 AM

This looks great, thanks a lot!

This revision now requires changes to proceed.Apr 25 2017, 10:18 AM

Hi Artem,

Could I commit this patch? thanks!

Regards,
Leslie Zhai

danielmarjamaki accepted this revision.Apr 25 2017, 10:15 PM

If you have svn write permission then please do it.

If you need svn write permission, please see http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

This revision is now accepted and ready to land.Apr 25 2017, 10:15 PM
This revision was automatically updated to reflect the committed changes.