Re: Fwd: r198063 - Warn on mismatched parentheses in memcmp and friends.

2016-10-18 Thread Richard Smith via cfe-commits 
On Tue, Oct 18, 2016 at 9:31 AM, Nico Weber  wrote:

> Did you find real-world code triggering this, or did you happen to see the
> implementation of the warning? I'm not sure if omitting just the note or
> the whole warning is more useful in practice. Intuitively I'd say that
> people "never" will compare the result of memcpy, so I'd guess that it
> doesn't matter much what we pick and omitting the warning completely is
> fine, but I'm not sure.
>
I found this when implementing core issue 1512, under which the suggested
"fixed" code doesn't compile any more.


> On Oct 17, 2016 1:44 PM, "Richard Smith"  wrote:
>
>> [Re-send to correct addresses.]
>>
>> On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber  wrote:
>>
>>> Author: nico
>>> Date: Thu Dec 26 17:38:39 2013
>>> New Revision: 198063
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=198063=rev
>>> Log:
>>> Warn on mismatched parentheses in memcmp and friends.
>>>
>>> Thisadds a new warning that warns on code like this:
>>>
>>>   if (memcmp(a, b, sizeof(a) != 0))
>>>
>>> The warning looks like:
>>>
>>> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison
>>> [-Wmemsize-comparison]
>>>   if (memcmp(a, b, sizeof(a) != 0))
>>>~~^~~~
>>> test4.cc:5:7: note: did you mean to compare the result of 'memcmp'
>>> instead?
>>>   if (memcmp(a, b, sizeof(a) != 0))
>>>   ^  ~
>>> )
>>> test4.cc:5:20: note: explicitly cast the argument to size_t to silence
>>> this warning
>>>   if (memcmp(a, b, sizeof(a) != 0))
>>>^
>>>(size_t)( )
>>> 1 warning generated.
>>>
>>> This found 2 bugs in chromium and has 0 false positives on both chromium
>>> and
>>> llvm.
>>>
>>> The idea of triggering this warning on a binop in the size argument is
>>> due to
>>> rnk.
>>>
>>>
>>> Added:
>>> cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp
>>> Modified:
>>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>>> Basic/DiagnosticSemaKinds.td?rev=198063=198062=198063=diff
>>> 
>>> ==
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Dec 26
>>> 17:38:39 2013
>>> @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess :
>>>"%select{destination|source}2; expected %3 or an explicit length">,
>>>InGroup;
>>>  def warn_strlcpycat_wrong_size : Warning<
>>> -  "size argument in %0 call appears to be size of the source; expected
>>> the size of "
>>> -  "the destination">,
>>> +  "size argument in %0 call appears to be size of the source; "
>>> +  "expected the size of the destination">,
>>>InGroup>;
>>>  def note_strlcpycat_wrong_size : Note<
>>>"change size argument to be the size of the destination">;
>>> +def warn_memsize_comparison : Warning<
>>> +  "size argument in %0 call is a comparison">,
>>> +  InGroup>;
>>> +def warn_memsize_comparison_paren_note : Note<
>>> +  "did you mean to compare the result of %0 instead?">;
>>> +def warn_memsize_comparison_cast_note : Note<
>>> +  "explicitly cast the argument to size_t to silence this warning">;
>>>
>>>  def warn_strncat_large_size : Warning<
>>>"the value of the size argument in 'strncat' is too large, might lead
>>> to a "
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC
>>> hecking.cpp?rev=198063=198062=198063=diff
>>> 
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 26 17:38:39 2013
>>> @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(u
>>>   RHS.get(), AA_Assigning))
>>>return true;
>>>}
>>> -
>>> +
>>>// For NEON intrinsics which take an immediate value as part of the
>>>// instruction, range check them here.
>>>unsigned i = 0, l = 0, u = 0;
>>> @@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const Strin
>>>
>>>  //===--- CHECK: Standard memory functions --
>>> ---===//
>>>
>>> +/// \brief Takes the expression passed to the size_t parameter of
>>> functions
>>> +/// such as memcmp, strncat, etc and warns if it's a comparison.
>>> +///
>>> +/// This is to catch typos like `if (memcmp(, , sizeof(a) > 0))`.
>>> +static bool CheckMemorySizeofForComparison(Sema , const Expr *E,
>>> +   IdentifierInfo *FnName,
>>> +

Re: Fwd: r198063 - Warn on mismatched parentheses in memcmp and friends.

2016-10-18 Thread Nico Weber via cfe-commits 
Did you find real-world code triggering this, or did you happen to see the
implementation of the warning? I'm not sure if omitting just the note or
the whole warning is more useful in practice. Intuitively I'd say that
people "never" will compare the result of memcpy, so I'd guess that it
doesn't matter much what we pick and omitting the warning completely is
fine, but I'm not sure.

On Oct 17, 2016 1:44 PM, "Richard Smith"  wrote:

> [Re-send to correct addresses.]
>
> On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber  wrote:
>
>> Author: nico
>> Date: Thu Dec 26 17:38:39 2013
>> New Revision: 198063
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=198063=rev
>> Log:
>> Warn on mismatched parentheses in memcmp and friends.
>>
>> Thisadds a new warning that warns on code like this:
>>
>>   if (memcmp(a, b, sizeof(a) != 0))
>>
>> The warning looks like:
>>
>> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison
>> [-Wmemsize-comparison]
>>   if (memcmp(a, b, sizeof(a) != 0))
>>~~^~~~
>> test4.cc:5:7: note: did you mean to compare the result of 'memcmp'
>> instead?
>>   if (memcmp(a, b, sizeof(a) != 0))
>>   ^  ~
>> )
>> test4.cc:5:20: note: explicitly cast the argument to size_t to silence
>> this warning
>>   if (memcmp(a, b, sizeof(a) != 0))
>>^
>>(size_t)( )
>> 1 warning generated.
>>
>> This found 2 bugs in chromium and has 0 false positives on both chromium
>> and
>> llvm.
>>
>> The idea of triggering this warning on a binop in the size argument is
>> due to
>> rnk.
>>
>>
>> Added:
>> cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaChecking.cpp
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
>> Basic/DiagnosticSemaKinds.td?rev=198063=198062=198063=diff
>> 
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Dec 26
>> 17:38:39 2013
>> @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess :
>>"%select{destination|source}2; expected %3 or an explicit length">,
>>InGroup;
>>  def warn_strlcpycat_wrong_size : Warning<
>> -  "size argument in %0 call appears to be size of the source; expected
>> the size of "
>> -  "the destination">,
>> +  "size argument in %0 call appears to be size of the source; "
>> +  "expected the size of the destination">,
>>InGroup>;
>>  def note_strlcpycat_wrong_size : Note<
>>"change size argument to be the size of the destination">;
>> +def warn_memsize_comparison : Warning<
>> +  "size argument in %0 call is a comparison">,
>> +  InGroup>;
>> +def warn_memsize_comparison_paren_note : Note<
>> +  "did you mean to compare the result of %0 instead?">;
>> +def warn_memsize_comparison_cast_note : Note<
>> +  "explicitly cast the argument to size_t to silence this warning">;
>>
>>  def warn_strncat_large_size : Warning<
>>"the value of the size argument in 'strncat' is too large, might lead
>> to a "
>>
>> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC
>> hecking.cpp?rev=198063=198062=198063=diff
>> 
>> ==
>> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 26 17:38:39 2013
>> @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(u
>>   RHS.get(), AA_Assigning))
>>return true;
>>}
>> -
>> +
>>// For NEON intrinsics which take an immediate value as part of the
>>// instruction, range check them here.
>>unsigned i = 0, l = 0, u = 0;
>> @@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const Strin
>>
>>  //===--- CHECK: Standard memory functions --
>> ---===//
>>
>> +/// \brief Takes the expression passed to the size_t parameter of
>> functions
>> +/// such as memcmp, strncat, etc and warns if it's a comparison.
>> +///
>> +/// This is to catch typos like `if (memcmp(, , sizeof(a) > 0))`.
>> +static bool CheckMemorySizeofForComparison(Sema , const Expr *E,
>> +   IdentifierInfo *FnName,
>> +   SourceLocation FnLoc,
>> +   SourceLocation RParenLoc) {
>> +  const BinaryOperator *Size = dyn_cast(E);
>> +  if (!Size)
>> +return false;
>> +
>> +  // if E is binop and op is >, <, >=, <=, ==, &&, ||:
>> +  if (!Size->isComparisonOp() && !Size->isEqualityOp()

Fwd: r198063 - Warn on mismatched parentheses in memcmp and friends.

2016-10-17 Thread Richard Smith via cfe-commits 
[Re-send to correct addresses.]

On Thu, Dec 26, 2013 at 3:38 PM, Nico Weber  wrote:

> Author: nico
> Date: Thu Dec 26 17:38:39 2013
> New Revision: 198063
>
> URL: http://llvm.org/viewvc/llvm-project?rev=198063=rev
> Log:
> Warn on mismatched parentheses in memcmp and friends.
>
> Thisadds a new warning that warns on code like this:
>
>   if (memcmp(a, b, sizeof(a) != 0))
>
> The warning looks like:
>
> test4.cc:5:30: warning: size argument in 'memcmp' call is a comparison
> [-Wmemsize-comparison]
>   if (memcmp(a, b, sizeof(a) != 0))
>~~^~~~
> test4.cc:5:7: note: did you mean to compare the result of 'memcmp' instead?
>   if (memcmp(a, b, sizeof(a) != 0))
>   ^  ~
> )
> test4.cc:5:20: note: explicitly cast the argument to size_t to silence
> this warning
>   if (memcmp(a, b, sizeof(a) != 0))
>^
>(size_t)( )
> 1 warning generated.
>
> This found 2 bugs in chromium and has 0 false positives on both chromium
> and
> llvm.
>
> The idea of triggering this warning on a binop in the size argument is due
> to
> rnk.
>
>
> Added:
> cfe/trunk/test/SemaCXX/warn-memsize-comparison.cpp
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaChecking.cpp
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/
> Basic/DiagnosticSemaKinds.td?rev=198063=198062=198063=diff
> 
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Dec 26
> 17:38:39 2013
> @@ -390,11 +390,18 @@ def warn_sizeof_pointer_type_memaccess :
>"%select{destination|source}2; expected %3 or an explicit length">,
>InGroup;
>  def warn_strlcpycat_wrong_size : Warning<
> -  "size argument in %0 call appears to be size of the source; expected
> the size of "
> -  "the destination">,
> +  "size argument in %0 call appears to be size of the source; "
> +  "expected the size of the destination">,
>InGroup>;
>  def note_strlcpycat_wrong_size : Note<
>"change size argument to be the size of the destination">;
> +def warn_memsize_comparison : Warning<
> +  "size argument in %0 call is a comparison">,
> +  InGroup>;
> +def warn_memsize_comparison_paren_note : Note<
> +  "did you mean to compare the result of %0 instead?">;
> +def warn_memsize_comparison_cast_note : Note<
> +  "explicitly cast the argument to size_t to silence this warning">;
>
>  def warn_strncat_large_size : Warning<
>"the value of the size argument in 'strncat' is too large, might lead
> to a "
>
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaC
> hecking.cpp?rev=198063=198062=198063=diff
> 
> ==
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Thu Dec 26 17:38:39 2013
> @@ -622,7 +622,7 @@ bool Sema::CheckARMBuiltinFunctionCall(u
>   RHS.get(), AA_Assigning))
>return true;
>}
> -
> +
>// For NEON intrinsics which take an immediate value as part of the
>// instruction, range check them here.
>unsigned i = 0, l = 0, u = 0;
> @@ -3560,6 +3560,40 @@ void Sema::CheckFormatString(const Strin
>
>  //===--- CHECK: Standard memory functions --
> ---===//
>
> +/// \brief Takes the expression passed to the size_t parameter of
> functions
> +/// such as memcmp, strncat, etc and warns if it's a comparison.
> +///
> +/// This is to catch typos like `if (memcmp(, , sizeof(a) > 0))`.
> +static bool CheckMemorySizeofForComparison(Sema , const Expr *E,
> +   IdentifierInfo *FnName,
> +   SourceLocation FnLoc,
> +   SourceLocation RParenLoc) {
> +  const BinaryOperator *Size = dyn_cast(E);
> +  if (!Size)
> +return false;
> +
> +  // if E is binop and op is >, <, >=, <=, ==, &&, ||:
> +  if (!Size->isComparisonOp() && !Size->isEqualityOp() &&
> !Size->isLogicalOp())
> +return false;
> +
> +  Preprocessor  = S.getPreprocessor();
> +  SourceRange SizeRange = Size->getSourceRange();
> +  S.Diag(Size->getOperatorLoc(), diag::warn_memsize_comparison)
> +  << SizeRange << FnName;
> +  S.Diag(FnLoc, diag::warn_memsize_comparison_paren_note)
> +  << FnName
> +  << FixItHint::CreateInsertion(
> + PP.getLocForEndOfToken(Size->getLHS()->getLocEnd()),
> + ")")
> +  << FixItHint::CreateRemoval(RParenLoc);
> +  S.Diag(SizeRange.getBegin(), diag::warn_memsize_comparison_cast_note)