Page MenuHomePhabricator

[Analyzer] Assume const string-like globals are non-null
ClosedPublic

Authored by george.karpenkov on Oct 10 2017, 2:41 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin edited edge metadata.Oct 10 2017, 5:01 PM

Looks like a great start!

There are a bunch of minor nits inline.

The one big thing is that I think your handling of 'const char *' in typeIsConstString() isn't quite right. 'const char *' means that the pointed-to characters can't be modified but does allow modification of the variable holding the pointer. I don't think we want to treat such variables as holding non-null pointers, since anyone could assign them to NULL. On the other hand, we do want to treat variables of type 'char * const' as holding non-null pointers since the variable can't be reassigned and (presumably) it was initialized to a non-null value. In this second case the characters could potentially be modified, but that doesn't change whether the value of the pointer itself.

lib/StaticAnalyzer/Checkers/NonnilStringConstantsChecker.cpp
1 ↗(On Diff #118493)

We need to have the license preamble here.

3 ↗(On Diff #118493)

It would be good to include a motivation for why this is the right thing to do and even an example of the declarations this will trigger on.

17 ↗(On Diff #118493)

Since this applies to more than just Objective-C, I think it would be better to use 'null' instead of 'nil' in the name of the checker. Or even remove the 'Nonnil' prefix. What about 'GlobalStringConstantsChecker'?

22 ↗(On Diff #118493)

Does the constructor need to take an AnalyzerOptions?

53 ↗(On Diff #118493)

Should we just early return if location is not valid?

70 ↗(On Diff #118493)

Can you add doxygen-style comments to the implementations of these methods? I'd like to break with tradition and have comments for all new code.

71 ↗(On Diff #118493)

Note that we use capital letters for variables and parameters in Clang/LLVM.

74 ↗(On Diff #118493)

The convention Clang uses with dyn_cast and friends to to use 'auto *' in the variable declaration in order to not repeat the type name:

auto *Region = dyn_cast<VarRegion>(RegionVal->getAsRegion());
93 ↗(On Diff #118493)

Do you really want a bit-wise or here?

113 ↗(On Diff #118493)

This will leave II as uninitialized if your if/else if is not exhaustive. Are you sure that it is?

test/Analysis/nonnil-string-constants.mm
41 ↗(On Diff #118493)

What is the rationale for treating const char *v as non null?

In this scenario v can be reassigned, right?

45 ↗(On Diff #118493)

I'd also like to see a test for treating char * const v; as non null.

55 ↗(On Diff #118493)
typedef const char *str;
extern str globalStr;

allows the globalStr variable to be written to.

Did you mean:

typedef char * const str;
extern str globalStr;
george.karpenkov marked 12 inline comments as done.

Adhering to comments.

Marking requests as "done".

dcoughlin accepted this revision.Oct 10 2017, 10:18 PM

Looks good to me. Please fix the additional nits mentioned inline and commit! Also, make sure to do a pass to update the capitalization of variables throughout the file to match the LLVM coding standards. https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp
1 ↗(On Diff #118522)

The file name and description here needs to be updated for this checker.

10 ↗(On Diff #118522)

I don't think the comment in the first line here is needed.

22 ↗(On Diff #118522)

We generally don't do this kind of cross-reference in comments since they tend to get stale fast when things get moved around. There is no tool support to keep them in sync.

26 ↗(On Diff #118522)

Can this comment go?

53 ↗(On Diff #118522)

We usually put the oxygen comments on checkers on the implementation and not the interface since they aren't API and people reading the code generally look at the implementations. Can you move them to the implementations?

114 ↗(On Diff #118522)

To match the coding standards this should be auto * as well.

131 ↗(On Diff #118522)

Similar comment about auto *

133 ↗(On Diff #118522)

Similar comment about auto *.

test/Analysis/nonnull-string-constants.mm
5 ↗(On Diff #118522)

We generally don't put file paths like this in comments since they tend to go stale. It is fine to to say that these are tests for the NonnullStringConstantsChecker though.

This revision is now accepted and ready to land.Oct 10 2017, 10:18 PM
george.karpenkov marked 9 inline comments as done.Oct 11 2017, 11:38 AM
george.karpenkov added inline comments.
lib/StaticAnalyzer/Checkers/NonnullStringConstantsChecker.cpp
22 ↗(On Diff #118522)

:(
I guess I'm too spoiled by Javadoc.

53 ↗(On Diff #118522)

OK. I assume there's no good universal solution here.

george.karpenkov marked 2 inline comments as done.
This revision was automatically updated to reflect the committed changes.