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

Tue Sep 1 06:11:45 PDT 2015

On Tue, Sep 1, 2015 at 8:15 AM, Daniel Marjamäki
<Daniel.Marjamaki at evidente.se> wrote:
>
> 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.

Correct, but from the user's perspective, why are we not telling them
both can be 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

I'm very sorry I didn't see this thread until now. :-( I'm adding
Richard for his thoughts on the subject as well.

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

I think that's a different beast entirely.

void f(int *p) {
  int *i = p; // is p really considered unused just because i is unused?
}

> 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.

I'm not certain about the performance of the analysis (I suspect it's
relatively cheap though), but we do not usually want off-by-default
warnings in the frontend, and I suspect that this analysis would have
to be off by default due to the chattiness on well-formed code.

Btw, since this doesn't require inter-procedural analysis, I think
this most likely sits in clangAnalysis instead of
clangStaticAnalyzerCheckers. So it would behave much like our
reachable code warnings.

~Aaron

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