Page MenuHomePhabricator

[analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
ClosedPublic

Authored by bruntib on Aug 16 2019, 1:14 AM.

Details

Summary

There are some functions which can't be given a null pointer as parameter either because it has a nonnull attribute or it is declared to have undefined behavior (e.g. strcmp()). Sometimes it is hard to determine from the checker message which parameter is null at the invocation, so now this information is included in the message.

This commit fixes https://bugs.llvm.org/show_bug.cgi?id=39358

Diff Detail

Repository
rL LLVM

Event Timeline

bruntib created this revision.Aug 16 2019, 1:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2019, 1:14 AM
bruntib retitled this revision from NonNullParamChecker and CStringChecker parameter number in checker message to [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message.Aug 16 2019, 1:44 AM
whisperity added inline comments.Aug 16 2019, 5:21 AM
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
195–196 ↗(On Diff #215539)

It seems to be as if now you're able to present which parameter is the [[nonnull]] one. Because of this, the output to the user sounds really bad and unfriendly, at least to me.

How about this:

"null pointer passed to nth parameter, but it's marked 'nonnull'"?
"null pointer passed to nth parameter expecting 'nonnull'"?

Something along these lines.

To a parameter, we're always passing arguments, so saying "as an argument" seems redundant.

And because we have the index always in our hands, we don't need to use the indefinite article.

I really like the change, thanks! Let's settle on the diagnostic message then.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
202 ↗(On Diff #215539)

I think Optional<unsigned> would be nicer. Also, checkNonNull as a function name doesn't scream about why we need an index parameter -- could you rename it to IndexOfArg or something similar?

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
195–196 ↗(On Diff #215539)

Agreed.

NoQ accepted this revision.Aug 16 2019, 1:18 PM

Thanks! I'm happy to accept this as soon as other reviewers are happy.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1548 ↗(On Diff #215539)

You could also pass a description of the parameter (eg., "source" or "destination").

This revision is now accepted and ready to land.Aug 16 2019, 1:18 PM
bruntib updated this revision to Diff 215754.Aug 17 2019, 2:45 PM

The checker message has been changed according to the suggestion.
The last parameter of checkNonNull() doesn't require a default or optional value, so it has been fixed.
The parameter has been renamed from Idx to IdxOfArg.

bruntib marked 3 inline comments as done.Aug 17 2019, 2:49 PM
bruntib added inline comments.
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
202 ↗(On Diff #215539)

The default value of IdxOfArg comes from my laziness. There were two places where I was not sure what index number should be given, but I added those too. This way no default value or Optional is needed. Omitting this information wouldn't even be reasonable, since there is always an ordinal number of the argument.

1548 ↗(On Diff #215539)

Could you please give some hint, how to include this in the message? I don't know how to do it concisely.

clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
195–196 ↗(On Diff #215539)

I used the original message and just extended it with a number. Are you sure that it is a good practice to change a checker message? I'm not against the modification, just a little afraid that somebody may rely on it. It's quite unlikely though, so I changed it :)

Szelethus accepted this revision.Aug 23 2019, 6:15 AM

@whisperity, any objections to this?

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2019, 10:56 AM
NoQ added inline comments.Tue, Oct 29, 7:08 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
295–297

It sounds like this code remained untested because tests don't match end-of-line.

I noticed it because it produces warnings that don't look like valid English, such as:

"Null pointer argument in call to memory set function 1st parameter"

Patches are welcome :)

Charusso added inline comments.Tue, Oct 29, 7:16 PM
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1548 ↗(On Diff #215539)

I have made this for that purpose:

struct CallContext {
    CallContext(Optional<unsigned> DestinationPos,
                Optional<unsigned> SourcePos = None,
                Optional<unsigned> LengthPos = None)
        : DestinationPos(DestinationPos), SourcePos(SourcePos),
          LengthPos(LengthPos) {};
  
    Optional<unsigned> DestinationPos;
    Optional<unsigned> SourcePos;
    Optional<unsigned> LengthPos;
  };
// char *strcpy(char *dest, const char *src);
{{CDF_MaybeBuiltin, "stpcpy", 2}, {&CStringChecker::evalStpcpy, {0, 1}}},

May it helps.