[PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

[Daniel Marjamäki via cfe-commits]

Hello!

> The checker isn't currently path sensitive as it doesn't pay attention
> to control flow graphs or how pointer values flow through a function
> body. I suppose this is a matter of scope more than anything; I see a
> logical extension of this functionality being with local variables as
> well as parameters. So, for instance:
>
>void f(int *p) {
>  int *i = p;
>  std::cout << *i;
>}

Imho that path analysis is not very interesting. The "p" can't be const until we see that "i" is const.

> This is true, for the current design of the patch, static analysis is
> less useful because the path sensitivity doesn't matter. But that also
> suggests if we wanted to add such a thing to the static analyzer
> someday, we'd have two ways of getting the same information if we
> stuck this in the frontend. It seems more logical to me to set this up
> as a static analysis checker so that we can extend it to be path
> sensitive under the same flag.

I wish we would have had this discussion earlier. Now I am not eager to rewrite it. for information this design has passed dev without comments:
http://lists.llvm.org/pipermail/cfe-dev/2015-August/044547.html

do you want that Wunused-parameter is moved from the frontend too? otherwise there will be similar path-sensitive analysis in the frontend anyway.

if we talk about the user interface.. imho it would be nice that this is a compiler warning. the analysis is quick and there should be little noise.

> Btw, since I forgot to mention it before, I think this is a great idea
> in general, thank you for working on it! :-)

Thanks! This is appreciated.

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se

________________________________________
Från: Aaron Ballman [aaron.ballman at gmail.com]
Skickat: den 31 augusti 2015 21:20
Till: reviews+D12359+public+799c9f67cbbb768e at reviews.llvm.org
Kopia: Daniel Marjamäki; stephan.bergmann.secondary at googlemail.com; cfe-commits
Ämne: Re: [PATCH] D12359: New warning -Wnonconst-parameter when a pointer parameter can be const

On Mon, Aug 31, 2015 at 2:58 PM, Daniel Marjamäki
<daniel.marjamaki at evidente.se> wrote:
> danielmarjamaki added a comment.
>
> In http://reviews.llvm.org/D12359#236334, @aaron.ballman wrote:
>
>> I have concerns about this being a frontend warning. The true positive rate seems rather high given the benign nature of the diagnostic, and the false negative rate is quite high. This seems like it would make more sense as a path-sensitive static analyzer warning instead of a frontend warning, as that would justify the slightly high true positive rate, and rectify quite a bit of the false negative rate.
>
>
> I don't understand. The checker is path sensitive, isn't it? Do you see some problem that I don't?

> The checker isn't currently path sensitive as it doesn't pay attention
> to control flow graphs or how pointer values flow through a function
> body. I suppose this is a matter of scope more than anything; I see a
> logical extension of this functionality being with local variables as
> well as parameters. So, for instance:

void f(int *p) {
  int *i = p;
  std::cout << *i;
}

I think code like the above should tell the user that both p and i can be const.

> It will warn if there is no write anywhere in the function. Except as I wrote, for some cases where #ifdef is used, but moving it to static analysis won't help.

This is true, for the current design of the patch, static analysis is
less useful because the path sensitivity doesn't matter. But that also
suggests if we wanted to add such a thing to the static analyzer
someday, we'd have two ways of getting the same information if we
stuck this in the frontend. It seems more logical to me to set this up
as a static analysis checker so that we can extend it to be path
sensitive under the same flag.

>
>> Have you tried running this over the Clang and LLVM code bases? How many diagnostics does it produce?
>
>
> Not yet. I'll do that.
>
>
> ================
> Comment at: test/Sema/warn-nonconst-parameter.c:8
> @@ +7,3 @@
> +//
> +// It does not warn about pointers to records or function pointers.
> +
> ----------------
> aaron.ballman wrote:
>> How does it handle cases like:
>>
>> void g(int);
>> void f(volatile int *p) {
>>   int j = *p; // Should not warn
>>   int i = p[0]; // Should not warn
>>   g(*p); // Should not warn
>> }
>>
>> void h(int *p) {
>>   int i = p ? *p : 0; // Should warn
>> }
>>
> ok interesting. I have never seen a volatile pointer argument before. but technically I believe we should warn about f(). the function only reads p. maybe for stylistic reasons it would look weird to say that it's both volatile and const, is that why we should not warn?

I was thinking that we should not warn in those cases because volatile
values can be changed in ways unknown to the implementation. But now
that I'm a bit more awake, I'm not as convinced of that as I
previously was. I'm not certain that const plays into that.

Btw, since I forgot to mention it before, I think this is a great idea
in general, thank you for working on it! :-)

~Aaron