Re: r368499 - Attempt to reapply "Even more warnings utilizing gsl::Owner/gsl::Pointer annotations"

[Gábor Horváth via cfe-commits]

Indeed!

There pointer is moved later on! Interestingly, I run these warnings on
300+ projects and none of them had this pattern. Will revert or fix the
patch soon.

On Fri, 9 Aug 2019 at 17:13, Nico Weber  wrote:

> http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/14045
>
> FAILED:
> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
>
> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/install/stage1/bin/clang++
>   -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/Frontend
> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend
> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/include
> -Itools/clang/include -Iinclude
> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/include
> -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time
> -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra
> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
> -Wmissing-field-initializers -pedantic -Wno-long-long
> -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type
> -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion
> -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common
> -Woverloaded-virtual -Wno-nested-anon-types -O3-UNDEBUG
>  -fno-exceptions -fno-rtti -MD -MT
> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
> -MF
> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o.d
> -o
> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
> -c
> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp
> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp:40:22:
> error: binding reference member 'Out' to stack allocated parameter 'Out'
> [-Werror,-Wdangling-field]
> : Out(Out ? *Out : llvm::outs()), OwnedOut(std::move(Out)),
>  ^~~
> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp:98:18:
> note: reference member declared here
> raw_ostream &Out;
>  ^
> 1 error generated.
>
> http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/ASTConsumers.cpp#40
>
> That looks like a false positive.
>
> On Fri, Aug 9, 2019 at 7:02 PM Gabor Horvath via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: xazax
>> Date: Fri Aug  9 16:03:50 2019
>> New Revision: 368499
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=368499&view=rev
>> Log:
>> Attempt to reapply "Even more warnings utilizing gsl::Owner/gsl::Pointer
>> annotations"
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaInit.cpp
>> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368499&r1=368498&r2=368499&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 16:03:50 2019
>> @@ -6568,19 +6568,33 @@ static bool shouldTrackImplicitObjectArg
>>if (auto *Conv = dyn_cast_or_null(Callee))
>>  if (isRecordWithAttr(Conv->getConversionType()))
>>return true;
>> -  if (!Callee->getParent()->isInStdNamespace() ||
>> !Callee->getIdentifier())
>> +  if (!Callee->getParent()->isInStdNamespace())
>>  return false;
>>if (!isRecordWithAttr(Callee->getThisObjectType()) &&
>>!isRecordWithAttr(Callee->getThisObjectType()))
>>  return false;
>> -  if (!isRecordWithAttr(Callee->getReturnType()) &&
>> -  !Callee->getReturnType()->isPointerType())
>> -return false;
>> -  return llvm::StringSwitch(Callee->getName())
>> -  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
>> -  .Cases("end", "rend", "cend", "crend", true)
>> -  .Cases("c_str", "data", "get", true)
>> -  .Default(false);
>> +  if (Callee->getReturnType()->isPointerType() ||
>> +  isRecordWithAttr(Callee->getReturnType())) {
>> +if (!Callee->getIdentifier())
>> +  return false;
>> +return llvm::StringSwitch(Callee->getName())
>> +.Cases("begin", "rbegin", "cbegin", "crbegin", true)
>> +.Cases("end", "rend", "cend", "crend", true)
>> +.Cases("c_str", "data", "get", true)
>> +// Map and set types.
>> +.Cases("find", "equal_range", "lower_bound", "upper_bound", true)
>> +.Default(false);
>> +  } else if (Callee->getReturnType()->isReferenceType()) {
>> +if (!Callee->getIdentifier()) {
>> +  auto OO = Callee->getOverloadedOperator();
>> +  return OO == OverloadedOperatorKind::OO_Subscript ||
>> + OO == OverloadedOperatorKind::OO_Star;
>> +}
>> +return llvm::StringSwitch(Callee->getName())
>

Re: r368499 - Attempt to reapply "Even more warnings utilizing gsl::Owner/gsl::Pointer annotations"

[Gábor Horváth via cfe-commits]

It was an easy fix! In case any problem persists I will revert the commit
with the fix itself.

On Fri, 9 Aug 2019 at 17:16, Gábor Horváth  wrote:

> Indeed!
>
> There pointer is moved later on! Interestingly, I run these warnings on
> 300+ projects and none of them had this pattern. Will revert or fix the
> patch soon.
>
> On Fri, 9 Aug 2019 at 17:13, Nico Weber  wrote:
>
>> http://lab.llvm.org:8011/builders/clang-with-lto-ubuntu/builds/14045
>>
>> FAILED:
>> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
>>
>> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/install/stage1/bin/clang++
>>   -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS
>> -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/Frontend
>> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend
>> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/include
>> -Itools/clang/include -Iinclude
>> -I/home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/include
>> -fPIC -fvisibility-inlines-hidden -Werror -Werror=date-time
>> -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra
>> -Wno-unused-parameter -Wwrite-strings -Wcast-qual
>> -Wmissing-field-initializers -pedantic -Wno-long-long
>> -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type
>> -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion
>> -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common
>> -Woverloaded-virtual -Wno-nested-anon-types -O3-UNDEBUG
>>  -fno-exceptions -fno-rtti -MD -MT
>> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
>> -MF
>> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o.d
>> -o
>> tools/clang/lib/Frontend/CMakeFiles/obj.clangFrontend.dir/ASTConsumers.cpp.o
>> -c
>> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp
>> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp:40:22:
>> error: binding reference member 'Out' to stack allocated parameter 'Out'
>> [-Werror,-Wdangling-field]
>> : Out(Out ? *Out : llvm::outs()), OwnedOut(std::move(Out)),
>>  ^~~
>> /home/buildslave/buildslave1a/clang-with-lto-ubuntu/llvm.src/tools/clang/lib/Frontend/ASTConsumers.cpp:98:18:
>> note: reference member declared here
>> raw_ostream &Out;
>>  ^
>> 1 error generated.
>>
>> http://llvm-cs.pcc.me.uk/tools/clang/lib/Frontend/ASTConsumers.cpp#40
>>
>> That looks like a false positive.
>>
>> On Fri, Aug 9, 2019 at 7:02 PM Gabor Horvath via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: xazax
>>> Date: Fri Aug  9 16:03:50 2019
>>> New Revision: 368499
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=368499&view=rev
>>> Log:
>>> Attempt to reapply "Even more warnings utilizing gsl::Owner/gsl::Pointer
>>> annotations"
>>>
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaInit.cpp
>>> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368499&r1=368498&r2=368499&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 16:03:50 2019
>>> @@ -6568,19 +6568,33 @@ static bool shouldTrackImplicitObjectArg
>>>if (auto *Conv = dyn_cast_or_null(Callee))
>>>  if (isRecordWithAttr(Conv->getConversionType()))
>>>return true;
>>> -  if (!Callee->getParent()->isInStdNamespace() ||
>>> !Callee->getIdentifier())
>>> +  if (!Callee->getParent()->isInStdNamespace())
>>>  return false;
>>>if (!isRecordWithAttr(Callee->getThisObjectType()) &&
>>>!isRecordWithAttr(Callee->getThisObjectType()))
>>>  return false;
>>> -  if (!isRecordWithAttr(Callee->getReturnType()) &&
>>> -  !Callee->getReturnType()->isPointerType())
>>> -return false;
>>> -  return llvm::StringSwitch(Callee->getName())
>>> -  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
>>> -  .Cases("end", "rend", "cend", "crend", true)
>>> -  .Cases("c_str", "data", "get", true)
>>> -  .Default(false);
>>> +  if (Callee->getReturnType()->isPointerType() ||
>>> +  isRecordWithAttr(Callee->getReturnType())) {
>>> +if (!Callee->getIdentifier())
>>> +  return false;
>>> +return llvm::StringSwitch(Callee->getName())
>>> +.Cases("begin", "rbegin", "cbegin", "crbegin", true)
>>> +.Cases("end", "rend", "cend", "crend", true)
>>> +.Cases("c_str", "data", "get", true)
>>> +// Map and set types.
>>> +.Cases("find", "equal_range", "lower_bound", "upper_bound",
>>> true)
>>> +.Default(false);
>>> +  } else if (Callee->getReturnType()->isReferenceType()) {
>>> +if (!

Re: r368501 - Fix a false positive warning when initializing members with gsl::Owners.

[Gábor Horváth via cfe-commits]

It does! Do you want me to commit that test as well?

On Fri, 9 Aug 2019 at 17:50, Nico Weber  wrote:

> This fixes `+  X(std::unique_ptr up) : pointee(up.get()),
> pointer(std::move(up)) {}` as well, right?
>
> On Fri, Aug 9, 2019 at 8:31 PM Gabor Horvath via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: xazax
>> Date: Fri Aug  9 17:32:29 2019
>> New Revision: 368501
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=368501&view=rev
>> Log:
>> Fix a false positive warning when initializing members with gsl::Owners.
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaInit.cpp
>> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368501&r1=368500&r2=368501&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 17:32:29 2019
>> @@ -7217,6 +7217,11 @@ void Sema::checkInitializerLifetime(cons
>>  if (pathContainsInit(Path))
>>return false;
>>
>> +// Suppress false positives for code like the below:
>> +//   Ctor(unique_ptr up) : member(*up), member2(move(up)) {}
>> +if (IsLocalGslOwner && pathOnlyInitializesGslPointer(Path))
>> +  return false;
>> +
>>  auto *DRE = dyn_cast(L);
>>  auto *VD = DRE ? dyn_cast(DRE->getDecl()) : nullptr;
>>  if (!VD) {
>>
>> Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368501&r1=368500&r2=368501&view=diff
>>
>> ==
>> --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
>> +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
>> 17:32:29 2019
>> @@ -120,6 +120,13 @@ void initLocalGslPtrWithTempOwner() {
>>  }
>>
>>  namespace std {
>> +template struct remove_reference   { typedef T type; };
>> +template struct remove_reference  { typedef T type; };
>> +template struct remove_reference { typedef T type; };
>> +
>> +template
>> +typename remove_reference::type &&move(T &&t) noexcept;
>> +
>>  template 
>>  struct basic_iterator {
>>basic_iterator operator++();
>> @@ -153,6 +160,7 @@ struct basic_string {
>>
>>  template
>>  struct unique_ptr {
>> +  T &operator*();
>>T *get() const;
>>  };
>>
>> @@ -217,3 +225,10 @@ int &doNotFollowReferencesForLocalOwner(
>>  const char *trackThroughMultiplePointer() {
>>return
>> std::basic_string_view(std::basic_string()).begin(); //
>> expected-warning {{returning address of local temporary object}}
>>  }
>> +
>> +struct X {
>> +  X(std::unique_ptr up) : pointee(*up), pointer(std::move(up)) {}
>> +
>> +  int &pointee;
>> +  std::unique_ptr pointer;
>> +};
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r368446 - More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via cfe-commits]

You are right!

I will look into this tomorrow. If you think this is urgent feel free to
revert the commits.

Cheers,
Gabor

On Sat, Aug 10, 2019, 6:41 PM Nico Weber  wrote:

> Hi Gabor,
>
> this makes clang warn on this program:
>
> $ cat test.cc
> #include 
> #include 
>
> struct S {
>   bool operator==(const S &rhs) const;
> };
>
> const S &f(const std::vector &v) {
>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>   return *it;
> }
>
>
> $ out/gn/bin/clang -c test.cc -isysroot $(xcrun -show-sdk-path) -isystem
> libcxx/include/ -Wall
> test.cc:10:11: warning: returning reference to local temporary object
> [-Wreturn-stack-address]
>   return *it;
>   ^~
> test.cc:9:15: note: binding reference variable 'it' here
>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>   ^
> 1 warning generated.
>
>
> Is the warning correct here? The `const auto &it` is lifetime-extended to
> the end of the block, and *it should return a reference to an element in
> the vector. Since the vector's passed in, this should be fine. Or am I
> missing something?
>
> Thanks,
> Nico
>
> On Fri, Aug 9, 2019 at 11:15 AM Gabor Horvath via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: xazax
>> Date: Fri Aug  9 08:16:35 2019
>> New Revision: 368446
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=368446&view=rev
>> Log:
>> More warnings regarding gsl::Pointer and gsl::Owner attributes
>>
>> Differential Revision: https://reviews.llvm.org/D65120
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaInit.cpp
>> cfe/trunk/test/Analysis/inner-pointer.cpp
>> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>
>> ==
>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 08:16:35 2019
>> @@ -6564,6 +6564,25 @@ template  static bool isReco
>>return false;
>>  }
>>
>> +static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
>> +  if (auto *Conv = dyn_cast_or_null(Callee))
>> +if (isRecordWithAttr(Conv->getConversionType()))
>> +  return true;
>> +  if (!Callee->getParent()->isInStdNamespace() ||
>> !Callee->getIdentifier())
>> +return false;
>> +  if (!isRecordWithAttr(Callee->getThisObjectType()) &&
>> +  !isRecordWithAttr(Callee->getThisObjectType()))
>> +return false;
>> +  if (!isRecordWithAttr(Callee->getReturnType()) &&
>> +  !Callee->getReturnType()->isPointerType())
>> +return false;
>> +  return llvm::StringSwitch(Callee->getName())
>> +  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
>> +  .Cases("end", "rend", "cend", "crend", true)
>> +  .Cases("c_str", "data", "get", true)
>> +  .Default(false);
>> +}
>> +
>>  static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
>>  LocalVisitor Visit) {
>>auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
>> @@ -6577,10 +6596,9 @@ static void handleGslAnnotatedTypes(Indi
>>};
>>
>>if (auto *MCE = dyn_cast(Call)) {
>> -const FunctionDecl *Callee = MCE->getDirectCallee();
>> -if (auto *Conv = dyn_cast_or_null(Callee))
>> -  if (isRecordWithAttr(Conv->getConversionType()))
>> -VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
>> +const auto *MD = cast_or_null(MCE->getDirectCallee());
>> +if (MD && shouldTrackImplicitObjectArg(MD))
>> +  VisitPointerArg(MD, MCE->getImplicitObjectArgument());
>>  return;
>>}
>>
>>
>> Modified: cfe/trunk/test/Analysis/inner-pointer.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inner-pointer.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>
>> ==
>> --- cfe/trunk/test/Analysis/inner-pointer.cpp (original)
>> +++ cfe/trunk/test/Analysis/inner-pointer.cpp Fri Aug  9 08:16:35 2019
>> @@ -1,4 +1,5 @@
>> -// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer \
>> +// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer   \
>> +// RUN:   -Wno-dangling -Wno-dangling-field -Wno-return-stack-address \
>>  // RUN:   %s -analyzer-output=text -verify
>>
>>  #include "Inputs/system-header-simulator-cxx.h"
>>
>> Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>
>> ==
>> --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
>> +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
>> 08:16:35 2019
>> @@ -2,7 +2,6 @@
>>  struct [[gsl::O

Re: r368446 - More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via cfe-commits]

This should be fixed now.

On Sat, Aug 10, 2019, 8:08 PM Gábor Horváth  wrote:

> You are right!
>
> I will look into this tomorrow. If you think this is urgent feel free to
> revert the commits.
>
> Cheers,
> Gabor
>
> On Sat, Aug 10, 2019, 6:41 PM Nico Weber  wrote:
>
>> Hi Gabor,
>>
>> this makes clang warn on this program:
>>
>> $ cat test.cc
>> #include 
>> #include 
>>
>> struct S {
>>   bool operator==(const S &rhs) const;
>> };
>>
>> const S &f(const std::vector &v) {
>>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>>   return *it;
>> }
>>
>>
>> $ out/gn/bin/clang -c test.cc -isysroot $(xcrun -show-sdk-path) -isystem
>> libcxx/include/ -Wall
>> test.cc:10:11: warning: returning reference to local temporary object
>> [-Wreturn-stack-address]
>>   return *it;
>>   ^~
>> test.cc:9:15: note: binding reference variable 'it' here
>>   const auto &it = std::find(v.rbegin(), v.rend(), S());
>>   ^
>> 1 warning generated.
>>
>>
>> Is the warning correct here? The `const auto &it` is lifetime-extended to
>> the end of the block, and *it should return a reference to an element in
>> the vector. Since the vector's passed in, this should be fine. Or am I
>> missing something?
>>
>> Thanks,
>> Nico
>>
>> On Fri, Aug 9, 2019 at 11:15 AM Gabor Horvath via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: xazax
>>> Date: Fri Aug  9 08:16:35 2019
>>> New Revision: 368446
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=368446&view=rev
>>> Log:
>>> More warnings regarding gsl::Pointer and gsl::Owner attributes
>>>
>>> Differential Revision: https://reviews.llvm.org/D65120
>>>
>>> Modified:
>>> cfe/trunk/lib/Sema/SemaInit.cpp
>>> cfe/trunk/test/Analysis/inner-pointer.cpp
>>> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>>
>>> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>
>>> ==
>>> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>>> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 08:16:35 2019
>>> @@ -6564,6 +6564,25 @@ template  static bool isReco
>>>return false;
>>>  }
>>>
>>> +static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
>>> +  if (auto *Conv = dyn_cast_or_null(Callee))
>>> +if (isRecordWithAttr(Conv->getConversionType()))
>>> +  return true;
>>> +  if (!Callee->getParent()->isInStdNamespace() ||
>>> !Callee->getIdentifier())
>>> +return false;
>>> +  if (!isRecordWithAttr(Callee->getThisObjectType()) &&
>>> +  !isRecordWithAttr(Callee->getThisObjectType()))
>>> +return false;
>>> +  if (!isRecordWithAttr(Callee->getReturnType()) &&
>>> +  !Callee->getReturnType()->isPointerType())
>>> +return false;
>>> +  return llvm::StringSwitch(Callee->getName())
>>> +  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
>>> +  .Cases("end", "rend", "cend", "crend", true)
>>> +  .Cases("c_str", "data", "get", true)
>>> +  .Default(false);
>>> +}
>>> +
>>>  static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
>>>  LocalVisitor Visit) {
>>>auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
>>> @@ -6577,10 +6596,9 @@ static void handleGslAnnotatedTypes(Indi
>>>};
>>>
>>>if (auto *MCE = dyn_cast(Call)) {
>>> -const FunctionDecl *Callee = MCE->getDirectCallee();
>>> -if (auto *Conv = dyn_cast_or_null(Callee))
>>> -  if (isRecordWithAttr(Conv->getConversionType()))
>>> -VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
>>> +const auto *MD =
>>> cast_or_null(MCE->getDirectCallee());
>>> +if (MD && shouldTrackImplicitObjectArg(MD))
>>> +  VisitPointerArg(MD, MCE->getImplicitObjectArgument());
>>>  return;
>>>}
>>>
>>>
>>> Modified: cfe/trunk/test/Analysis/inner-pointer.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inner-pointer.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>
>>> ==
>>> --- cfe/trunk/test/Analysis/inner-pointer.cpp (original)
>>> +++ cfe/trunk/test/Analysis/inner-pointer.cpp Fri Aug  9 08:16:35 2019
>>> @@ -1,4 +1,5 @@
>>> -// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer \
>>> +// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer   \
>>> +// RUN:   -Wno-dangling -Wno-dangling-field -Wno-return-stack-address \
>>>  // RUN:   %s -analyzer-output=text -verify
>>>
>>>  #include "Inputs/system-header-simulator-cxx.h"
>>>
>>> Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368446&r1=368445&r2=368446&view=diff
>>>
>>> ===

Re: r368446 - More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via cfe-commits]

Great news!

Thanks for all the feedback and patience! :)

On Sun, Aug 11, 2019, 4:35 PM Nico Weber  wrote:

> Confirmed, after r368534 the Chromium builds looks good again. (r368528
> without r368534 regressed something else, but r368534 fixed things. I'm
> assuming you don't need a repro for the breakage in r368528
> without r368534.)
>
> Your warnings even found a bug:
> https://bugs.chromium.org/p/openscreen/issues/detail?id=63 Thanks! :)
>
> On Sun, Aug 11, 2019 at 3:18 PM Gábor Horváth  wrote:
>
>> This should be fixed now.
>>
>> On Sat, Aug 10, 2019, 8:08 PM Gábor Horváth  wrote:
>>
>>> You are right!
>>>
>>> I will look into this tomorrow. If you think this is urgent feel free to
>>> revert the commits.
>>>
>>> Cheers,
>>> Gabor
>>>
>>> On Sat, Aug 10, 2019, 6:41 PM Nico Weber  wrote:
>>>
 Hi Gabor,

 this makes clang warn on this program:

 $ cat test.cc
 #include 
 #include 

 struct S {
   bool operator==(const S &rhs) const;
 };

 const S &f(const std::vector &v) {
   const auto &it = std::find(v.rbegin(), v.rend(), S());
   return *it;
 }


 $ out/gn/bin/clang -c test.cc -isysroot $(xcrun -show-sdk-path)
 -isystem libcxx/include/ -Wall
 test.cc:10:11: warning: returning reference to local temporary object
 [-Wreturn-stack-address]
   return *it;
   ^~
 test.cc:9:15: note: binding reference variable 'it' here
   const auto &it = std::find(v.rbegin(), v.rend(), S());
   ^
 1 warning generated.


 Is the warning correct here? The `const auto &it` is lifetime-extended
 to the end of the block, and *it should return a reference to an element in
 the vector. Since the vector's passed in, this should be fine. Or am I
 missing something?

 Thanks,
 Nico

 On Fri, Aug 9, 2019 at 11:15 AM Gabor Horvath via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Author: xazax
> Date: Fri Aug  9 08:16:35 2019
> New Revision: 368446
>
> URL: http://llvm.org/viewvc/llvm-project?rev=368446&view=rev
> Log:
> More warnings regarding gsl::Pointer and gsl::Owner attributes
>
> Differential Revision: https://reviews.llvm.org/D65120
>
> Modified:
> cfe/trunk/lib/Sema/SemaInit.cpp
> cfe/trunk/test/Analysis/inner-pointer.cpp
> cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368446&r1=368445&r2=368446&view=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 08:16:35 2019
> @@ -6564,6 +6564,25 @@ template  static bool isReco
>return false;
>  }
>
> +static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee)
> {
> +  if (auto *Conv = dyn_cast_or_null(Callee))
> +if (isRecordWithAttr(Conv->getConversionType()))
> +  return true;
> +  if (!Callee->getParent()->isInStdNamespace() ||
> !Callee->getIdentifier())
> +return false;
> +  if (!isRecordWithAttr(Callee->getThisObjectType()) &&
> +  !isRecordWithAttr(Callee->getThisObjectType()))
> +return false;
> +  if (!isRecordWithAttr(Callee->getReturnType()) &&
> +  !Callee->getReturnType()->isPointerType())
> +return false;
> +  return llvm::StringSwitch(Callee->getName())
> +  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
> +  .Cases("end", "rend", "cend", "crend", true)
> +  .Cases("c_str", "data", "get", true)
> +  .Default(false);
> +}
> +
>  static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr
> *Call,
>  LocalVisitor Visit) {
>auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
> @@ -6577,10 +6596,9 @@ static void handleGslAnnotatedTypes(Indi
>};
>
>if (auto *MCE = dyn_cast(Call)) {
> -const FunctionDecl *Callee = MCE->getDirectCallee();
> -if (auto *Conv = dyn_cast_or_null(Callee))
> -  if (isRecordWithAttr(Conv->getConversionType()))
> -VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
> +const auto *MD =
> cast_or_null(MCE->getDirectCallee());
> +if (MD && shouldTrackImplicitObjectArg(MD))
> +  VisitPointerArg(MD, MCE->getImplicitObjectArgument());
>  return;
>}
>
>
> Modified: cfe/trunk/test/Analysis/inner-pointer.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inner-pointer.cpp?rev=368446&r1=368445&r2=368446&view=diff
>
> ===

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Gábor Horváth via cfe-commits]

Hi Richard,

I'll move these behind a flag today. Moving forward, it would be great to
have a way to dogfood those warnings without blocking you. We do run them
over ~340 open source projects regularly, but clearly that is not enough :)

Thanks,
Gabor

On Fri, 23 Aug 2019 at 13:03, Richard Smith  wrote:

> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Hi Diana,
>> hi Richard,
>>
>> thank you for the feedback!
>>
>> Diana,
>> I remember that some gcc version had issues with raw string literals
>> inside macros. I'll fix that to use normal
>> string literals instead.
>>
>> Richard,
>> We are definitely want the gsl::Pointer-based warnings that are enabled
>> by default to be free of any false-positives.
>> As you expected, this is showing a problem we have in another part of our
>> analysis, and we will fix it before landing
>> this PR again.
>>
>
> It looks like this revert wasn't enough to unblock us. We're currently
> unable to release compilers due to the scale of the new enabled-by-default
> diagnostics produced by these warnings, and we're not happy about turning
> off the existing (zero false positives) warning flags here in order to
> unblock our releases, because they're so valuable in catching errors. I'd
> expect others will hit similar issues when upgrading Clang. Even if there
> were no false positives in the new warning, it appears to be undeployable
> as-is because the new warning is behind the same warning flag as an
> existing high-value warning. So I think we need the new warnings to be
> moved behind different warning flags (a subgroup of ReturnStackAddress
> would be OK, but it needs to be independently controllable).
>
> If this can be done imminently, that would be OK, but otherwise I think we
> should temporarily roll this back until it can be moved to a separate
> warning group.
>
> Both, sorry for the breakage!
>>
>> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
>> rich...@metafoo.co.uk>:
>>
>>> Reverted in r369677.
>>>
>>> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
>>> wrote:
>>>
 Hi Matthias,

 This introduces false positives into -Wreturn-stack-address for an
 example such as:

 #include 

 std::vector::iterator downcast_to(std::vector::iterator
 value) {
   return *&value;
 }

 This breaks an internal build bot for us, so I'm going to revert this
 for now (though I expect this isn't the cause of the problem, but is rather
 exposing a problem elsewhere).

 If we can make the gsl::Pointer diagnostics false-positive-free, that's
 great, but otherwise we should use a different warning flag for the
 warnings that involve these annotations and use -Wreturn-stack-address for
 only the zero-false-positive cases that it historically diagnosed.

 Thanks, and sorry for the trouble.

 On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Author: mgehre
> Date: Wed Aug 21 15:08:59 2019
> New Revision: 369591
>
> URL: http://llvm.org/viewvc/llvm-project?rev=369591&view=rev
> Log:
> [LifetimeAnalysis] Support more STL idioms (template forward
> declaration and DependentNameType)
>
> Summary:
> This fixes inference of gsl::Pointer on std::set::iterator with
> libstdc++ (the typedef for iterator
> on the template is a DependentNameType - we can only put the
> gsl::Pointer attribute
> on the underlaying record after instantiation)
>
> inference of gsl::Pointer on std::vector::iterator with libc++ (the
> class was forward-declared,
> we added the gsl::Pointer on the canonical decl (the forward decl),
> and later when the
> template was instantiated, there was no attribute on the definition so
> it was not instantiated).
>
> and a duplicate gsl::Pointer on some class with libstdc++ (we first
> added an attribute to
> a incomplete instantiation, and then another was copied from the
> template definition
> when the instantiation was completed).
>
> We now add the attributes to all redeclarations to fix thos issues and
> make their usage easier.
>
> Reviewers: gribozavr
>
> Subscribers: Szelethus, xazax.hun, cfe-commits
>
> Tags: #clang
>
> Differential Revision: https://reviews.llvm.org/D66179
>
> Added:
> cfe/trunk/unittests/Sema/GslOwnerPointerInference.cpp
> Modified:
> cfe/trunk/lib/Sema/SemaAttr.cpp
> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
> cfe/trunk/lib/Sema/SemaInit.cpp
> cfe/trunk/lib/Sema/SemaTemplateInstantiateDecl.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer-std.cpp
> cfe/trunk/test/SemaCXX/attr-gsl-owner-pointer.cpp
> cfe/trunk/unittests/Sema/CMakeLists.txt
>
> Modified: cfe/trunk/lib/Sema/SemaAttr.cpp
> URL:
> http://l

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Gábor Horváth via cfe-commits]

Hi Richard,

Sorry for the slow response, unfortunately the compile times are not great
on the machine I have access to at the moment.
Here is a patch, I'll commit it if you agree with the approach:
https://reviews.llvm.org/D66686

Basically, the idea is to not run the new warning related code at all when
it is turned off, so other warning flags like ReturnStackAddress should
never be triggered by the new warnings.
In the future, we can come up with something else but I wanted to go for
the safe solution given the urgency.

Cheers,
Gabor

On Fri, 23 Aug 2019 at 13:31, Gábor Horváth  wrote:

> Hi Richard,
>
> I'll move these behind a flag today. Moving forward, it would be great to
> have a way to dogfood those warnings without blocking you. We do run them
> over ~340 open source projects regularly, but clearly that is not enough :)
>
> Thanks,
> Gabor
>
> On Fri, 23 Aug 2019 at 13:03, Richard Smith  wrote:
>
>> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Hi Diana,
>>> hi Richard,
>>>
>>> thank you for the feedback!
>>>
>>> Diana,
>>> I remember that some gcc version had issues with raw string literals
>>> inside macros. I'll fix that to use normal
>>> string literals instead.
>>>
>>> Richard,
>>> We are definitely want the gsl::Pointer-based warnings that are enabled
>>> by default to be free of any false-positives.
>>> As you expected, this is showing a problem we have in another part of
>>> our analysis, and we will fix it before landing
>>> this PR again.
>>>
>>
>> It looks like this revert wasn't enough to unblock us. We're currently
>> unable to release compilers due to the scale of the new enabled-by-default
>> diagnostics produced by these warnings, and we're not happy about turning
>> off the existing (zero false positives) warning flags here in order to
>> unblock our releases, because they're so valuable in catching errors. I'd
>> expect others will hit similar issues when upgrading Clang. Even if there
>> were no false positives in the new warning, it appears to be undeployable
>> as-is because the new warning is behind the same warning flag as an
>> existing high-value warning. So I think we need the new warnings to be
>> moved behind different warning flags (a subgroup of ReturnStackAddress
>> would be OK, but it needs to be independently controllable).
>>
>> If this can be done imminently, that would be OK, but otherwise I think
>> we should temporarily roll this back until it can be moved to a separate
>> warning group.
>>
>> Both, sorry for the breakage!
>>>
>>> Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
>>> rich...@metafoo.co.uk>:
>>>
 Reverted in r369677.

 On Thu, 22 Aug 2019 at 10:34, Richard Smith 
 wrote:

> Hi Matthias,
>
> This introduces false positives into -Wreturn-stack-address for an
> example such as:
>
> #include 
>
> std::vector::iterator downcast_to(std::vector::iterator
> value) {
>   return *&value;
> }
>
> This breaks an internal build bot for us, so I'm going to revert this
> for now (though I expect this isn't the cause of the problem, but is 
> rather
> exposing a problem elsewhere).
>
> If we can make the gsl::Pointer diagnostics false-positive-free,
> that's great, but otherwise we should use a different warning flag for the
> warnings that involve these annotations and use -Wreturn-stack-address for
> only the zero-false-positive cases that it historically diagnosed.
>
> Thanks, and sorry for the trouble.
>
> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: mgehre
>> Date: Wed Aug 21 15:08:59 2019
>> New Revision: 369591
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=369591&view=rev
>> Log:
>> [LifetimeAnalysis] Support more STL idioms (template forward
>> declaration and DependentNameType)
>>
>> Summary:
>> This fixes inference of gsl::Pointer on std::set::iterator with
>> libstdc++ (the typedef for iterator
>> on the template is a DependentNameType - we can only put the
>> gsl::Pointer attribute
>> on the underlaying record after instantiation)
>>
>> inference of gsl::Pointer on std::vector::iterator with libc++ (the
>> class was forward-declared,
>> we added the gsl::Pointer on the canonical decl (the forward decl),
>> and later when the
>> template was instantiated, there was no attribute on the definition
>> so it was not instantiated).
>>
>> and a duplicate gsl::Pointer on some class with libstdc++ (we first
>> added an attribute to
>> a incomplete instantiation, and then another was copied from the
>> template definition
>> when the instantiation was completed).
>>
>> We now add the attributes to all redeclarations to fix thos issues
>> and make their usage easier.

Re: r369591 - [LifetimeAnalysis] Support more STL idioms (template forward declaration and DependentNameType)

[Gábor Horváth via cfe-commits]

I committed this in r369817 to keep things moving. If you have any
suggestion, complaints let me know and I will do my best to solve it
post-commit ASAP.

On Fri, 23 Aug 2019 at 14:51, Gábor Horváth  wrote:

> Hi Richard,
>
> Sorry for the slow response, unfortunately the compile times are not great
> on the machine I have access to at the moment.
> Here is a patch, I'll commit it if you agree with the approach:
> https://reviews.llvm.org/D66686
>
> Basically, the idea is to not run the new warning related code at all when
> it is turned off, so other warning flags like ReturnStackAddress should
> never be triggered by the new warnings.
> In the future, we can come up with something else but I wanted to go for
> the safe solution given the urgency.
>
> Cheers,
> Gabor
>
> On Fri, 23 Aug 2019 at 13:31, Gábor Horváth  wrote:
>
>> Hi Richard,
>>
>> I'll move these behind a flag today. Moving forward, it would be great to
>> have a way to dogfood those warnings without blocking you. We do run them
>> over ~340 open source projects regularly, but clearly that is not enough :)
>>
>> Thanks,
>> Gabor
>>
>> On Fri, 23 Aug 2019 at 13:03, Richard Smith 
>> wrote:
>>
>>> On Thu, 22 Aug 2019 at 13:05, Matthias Gehre via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Hi Diana,
 hi Richard,

 thank you for the feedback!

 Diana,
 I remember that some gcc version had issues with raw string literals
 inside macros. I'll fix that to use normal
 string literals instead.

 Richard,
 We are definitely want the gsl::Pointer-based warnings that are enabled
 by default to be free of any false-positives.
 As you expected, this is showing a problem we have in another part of
 our analysis, and we will fix it before landing
 this PR again.

>>>
>>> It looks like this revert wasn't enough to unblock us. We're currently
>>> unable to release compilers due to the scale of the new enabled-by-default
>>> diagnostics produced by these warnings, and we're not happy about turning
>>> off the existing (zero false positives) warning flags here in order to
>>> unblock our releases, because they're so valuable in catching errors. I'd
>>> expect others will hit similar issues when upgrading Clang. Even if there
>>> were no false positives in the new warning, it appears to be undeployable
>>> as-is because the new warning is behind the same warning flag as an
>>> existing high-value warning. So I think we need the new warnings to be
>>> moved behind different warning flags (a subgroup of ReturnStackAddress
>>> would be OK, but it needs to be independently controllable).
>>>
>>> If this can be done imminently, that would be OK, but otherwise I think
>>> we should temporarily roll this back until it can be moved to a separate
>>> warning group.
>>>
>>> Both, sorry for the breakage!

 Am Do., 22. Aug. 2019 um 19:47 Uhr schrieb Richard Smith <
 rich...@metafoo.co.uk>:

> Reverted in r369677.
>
> On Thu, 22 Aug 2019 at 10:34, Richard Smith 
> wrote:
>
>> Hi Matthias,
>>
>> This introduces false positives into -Wreturn-stack-address for an
>> example such as:
>>
>> #include 
>>
>> std::vector::iterator downcast_to(std::vector::iterator
>> value) {
>>   return *&value;
>> }
>>
>> This breaks an internal build bot for us, so I'm going to revert this
>> for now (though I expect this isn't the cause of the problem, but is 
>> rather
>> exposing a problem elsewhere).
>>
>> If we can make the gsl::Pointer diagnostics false-positive-free,
>> that's great, but otherwise we should use a different warning flag for 
>> the
>> warnings that involve these annotations and use -Wreturn-stack-address 
>> for
>> only the zero-false-positive cases that it historically diagnosed.
>>
>> Thanks, and sorry for the trouble.
>>
>> On Wed, 21 Aug 2019 at 15:07, Matthias Gehre via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: mgehre
>>> Date: Wed Aug 21 15:08:59 2019
>>> New Revision: 369591
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=369591&view=rev
>>> Log:
>>> [LifetimeAnalysis] Support more STL idioms (template forward
>>> declaration and DependentNameType)
>>>
>>> Summary:
>>> This fixes inference of gsl::Pointer on std::set::iterator with
>>> libstdc++ (the typedef for iterator
>>> on the template is a DependentNameType - we can only put the
>>> gsl::Pointer attribute
>>> on the underlaying record after instantiation)
>>>
>>> inference of gsl::Pointer on std::vector::iterator with libc++ (the
>>> class was forward-declared,
>>> we added the gsl::Pointer on the canonical decl (the forward decl),
>>> and later when the
>>> template was instantiated, there was no attribute on the definition
>>> so it was not instantiated).
>>>

Re: r368459 - Fix a build bot failure and multiple warnings instances for range base for loops

[Gábor Horváth via cfe-commits]

Hmm, strange. Looking into it! If I do not manage to find the root cause in
a few minutes I will revert!

On Fri, 9 Aug 2019 at 11:32, Jonas Devlieghere 
wrote:

> I think this is causing a stage2 failure:
>
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/124/consoleFull#-95886206949ba4694-19c4-4d7e-bec5-911270d8a58c
>
> On Fri, Aug 9, 2019 at 10:41 AM Gabor Horvath via cfe-commits
>  wrote:
> >
> > Author: xazax
> > Date: Fri Aug  9 10:42:41 2019
> > New Revision: 368459
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=368459&view=rev
> > Log:
> > Fix a build bot failure and multiple warnings instances for range base
> for loops
> >
> > Modified:
> > cfe/trunk/lib/Sema/SemaInit.cpp
> > cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
> >
> > Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368459&r1=368458&r2=368459&view=diff
> >
> ==
> > --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> > +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 10:42:41 2019
> > @@ -6616,7 +6616,7 @@ static void handleGslAnnotatedTypes(Indi
> >  return;
> >} else if (auto *OCE = dyn_cast(Call)) {
> >  FunctionDecl *Callee = OCE->getDirectCallee();
> > -if (Callee->isCXXInstanceMember() &&
> > +if (Callee && Callee->isCXXInstanceMember() &&
> >  shouldTrackImplicitObjectArg(cast(Callee)))
> >VisitPointerArg(Callee, OCE->getArg(0));
> >  return;
> > @@ -7070,8 +7070,11 @@ static SourceRange nextPathEntryRange(co
> >// supporting lifetime extension.
> >break;
> >
> > -case IndirectLocalPathEntry::DefaultInit:
> >  case IndirectLocalPathEntry::VarInit:
> > +  if (cast(Path[I].D)->isImplicit())
> > +return SourceRange();
> > +  LLVM_FALLTHROUGH;
> > +case IndirectLocalPathEntry::DefaultInit:
> >return Path[I].E->getSourceRange();
> >  }
> >}
> > @@ -7133,7 +7136,7 @@ void Sema::checkInitializerLifetime(cons
> >  return false;
> >}
> >
> > -  if (IsGslPtrInitWithGslTempOwner) {
> > +  if (IsGslPtrInitWithGslTempOwner && DiagLoc.isValid()) {
> >  Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) <<
> DiagRange;
> >  return false;
> >}
> >
> > Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368459&r1=368458&r2=368459&view=diff
> >
> ==
> > --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
> > +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
> 10:42:41 2019
> > @@ -201,6 +201,13 @@ void danglingReferenceFromTempOwner() {
> >  std::vector getTempVec();
> >  std::optional> getTempOptVec();
> >
> > +void testLoops() {
> > +  for (auto i : getTempVec()) // ok
> > +;
> > +  for (auto i : *getTempOptVec()) // expected-warning {{object backing
> the pointer will be destroyed at the end of the full-expression}}
> > +;
> > +}
> > +
> >  int &usedToBeFalsePositive(std::vector &v) {
> >std::vector::iterator it = v.begin();
> >int& value = *it;
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r368459 - Fix a build bot failure and multiple warnings instances for range base for loops

[Gábor Horváth via cfe-commits]

I reverted but I cannot reproduce this locally on a linux box. Is there any
way to get more information from the build bot (like preprocessed files?)?

On Fri, 9 Aug 2019 at 11:38, Gábor Horváth  wrote:

> Hmm, strange. Looking into it! If I do not manage to find the root cause
> in a few minutes I will revert!
>
> On Fri, 9 Aug 2019 at 11:32, Jonas Devlieghere 
> wrote:
>
>> I think this is causing a stage2 failure:
>>
>> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/124/consoleFull#-95886206949ba4694-19c4-4d7e-bec5-911270d8a58c
>>
>> On Fri, Aug 9, 2019 at 10:41 AM Gabor Horvath via cfe-commits
>>  wrote:
>> >
>> > Author: xazax
>> > Date: Fri Aug  9 10:42:41 2019
>> > New Revision: 368459
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=368459&view=rev
>> > Log:
>> > Fix a build bot failure and multiple warnings instances for range base
>> for loops
>> >
>> > Modified:
>> > cfe/trunk/lib/Sema/SemaInit.cpp
>> > cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> >
>> > Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368459&r1=368458&r2=368459&view=diff
>> >
>> ==
>> > --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>> > +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 10:42:41 2019
>> > @@ -6616,7 +6616,7 @@ static void handleGslAnnotatedTypes(Indi
>> >  return;
>> >} else if (auto *OCE = dyn_cast(Call)) {
>> >  FunctionDecl *Callee = OCE->getDirectCallee();
>> > -if (Callee->isCXXInstanceMember() &&
>> > +if (Callee && Callee->isCXXInstanceMember() &&
>> >  shouldTrackImplicitObjectArg(cast(Callee)))
>> >VisitPointerArg(Callee, OCE->getArg(0));
>> >  return;
>> > @@ -7070,8 +7070,11 @@ static SourceRange nextPathEntryRange(co
>> >// supporting lifetime extension.
>> >break;
>> >
>> > -case IndirectLocalPathEntry::DefaultInit:
>> >  case IndirectLocalPathEntry::VarInit:
>> > +  if (cast(Path[I].D)->isImplicit())
>> > +return SourceRange();
>> > +  LLVM_FALLTHROUGH;
>> > +case IndirectLocalPathEntry::DefaultInit:
>> >return Path[I].E->getSourceRange();
>> >  }
>> >}
>> > @@ -7133,7 +7136,7 @@ void Sema::checkInitializerLifetime(cons
>> >  return false;
>> >}
>> >
>> > -  if (IsGslPtrInitWithGslTempOwner) {
>> > +  if (IsGslPtrInitWithGslTempOwner && DiagLoc.isValid()) {
>> >  Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) <<
>> DiagRange;
>> >  return false;
>> >}
>> >
>> > Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368459&r1=368458&r2=368459&view=diff
>> >
>> ==
>> > --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
>> > +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
>> 10:42:41 2019
>> > @@ -201,6 +201,13 @@ void danglingReferenceFromTempOwner() {
>> >  std::vector getTempVec();
>> >  std::optional> getTempOptVec();
>> >
>> > +void testLoops() {
>> > +  for (auto i : getTempVec()) // ok
>> > +;
>> > +  for (auto i : *getTempOptVec()) // expected-warning {{object backing
>> the pointer will be destroyed at the end of the full-expression}}
>> > +;
>> > +}
>> > +
>> >  int &usedToBeFalsePositive(std::vector &v) {
>> >std::vector::iterator it = v.begin();
>> >int& value = *it;
>> >
>> >
>> > ___
>> > cfe-commits mailing list
>> > cfe-commits@lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r368459 - Fix a build bot failure and multiple warnings instances for range base for loops

[Gábor Horváth via cfe-commits]

I tried both with libc++ instead of libstdc++ and modules but none of them
reproduced in my local machine I would be glad if you could try it locally
as well!

Thanks in advance,
Gabor

On Fri, 9 Aug 2019 at 13:10, Jonas Devlieghere 
wrote:

> The bot is a little special in that it has modules enabled. Maybe that
> explains it? Let me know if that doesn't work and I can try
> reproducing locally.
>
> On Fri, Aug 9, 2019 at 12:02 PM Gábor Horváth  wrote:
> >
> > I reverted but I cannot reproduce this locally on a linux box. Is there
> any way to get more information from the build bot (like preprocessed
> files?)?
> >
> > On Fri, 9 Aug 2019 at 11:38, Gábor Horváth  wrote:
> >>
> >> Hmm, strange. Looking into it! If I do not manage to find the root
> cause in a few minutes I will revert!
> >>
> >> On Fri, 9 Aug 2019 at 11:32, Jonas Devlieghere 
> wrote:
> >>>
> >>> I think this is causing a stage2 failure:
> >>>
> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/124/consoleFull#-95886206949ba4694-19c4-4d7e-bec5-911270d8a58c
> >>>
> >>> On Fri, Aug 9, 2019 at 10:41 AM Gabor Horvath via cfe-commits
> >>>  wrote:
> >>> >
> >>> > Author: xazax
> >>> > Date: Fri Aug  9 10:42:41 2019
> >>> > New Revision: 368459
> >>> >
> >>> > URL: http://llvm.org/viewvc/llvm-project?rev=368459&view=rev
> >>> > Log:
> >>> > Fix a build bot failure and multiple warnings instances for range
> base for loops
> >>> >
> >>> > Modified:
> >>> > cfe/trunk/lib/Sema/SemaInit.cpp
> >>> > cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
> >>> >
> >>> > Modified: cfe/trunk/lib/Sema/SemaInit.cpp
> >>> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368459&r1=368458&r2=368459&view=diff
> >>> >
> ==
> >>> > --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
> >>> > +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 10:42:41 2019
> >>> > @@ -6616,7 +6616,7 @@ static void handleGslAnnotatedTypes(Indi
> >>> >  return;
> >>> >} else if (auto *OCE = dyn_cast(Call)) {
> >>> >  FunctionDecl *Callee = OCE->getDirectCallee();
> >>> > -if (Callee->isCXXInstanceMember() &&
> >>> > +if (Callee && Callee->isCXXInstanceMember() &&
> >>> >  shouldTrackImplicitObjectArg(cast(Callee)))
> >>> >VisitPointerArg(Callee, OCE->getArg(0));
> >>> >  return;
> >>> > @@ -7070,8 +7070,11 @@ static SourceRange nextPathEntryRange(co
> >>> >// supporting lifetime extension.
> >>> >break;
> >>> >
> >>> > -case IndirectLocalPathEntry::DefaultInit:
> >>> >  case IndirectLocalPathEntry::VarInit:
> >>> > +  if (cast(Path[I].D)->isImplicit())
> >>> > +return SourceRange();
> >>> > +  LLVM_FALLTHROUGH;
> >>> > +case IndirectLocalPathEntry::DefaultInit:
> >>> >return Path[I].E->getSourceRange();
> >>> >  }
> >>> >}
> >>> > @@ -7133,7 +7136,7 @@ void Sema::checkInitializerLifetime(cons
> >>> >  return false;
> >>> >}
> >>> >
> >>> > -  if (IsGslPtrInitWithGslTempOwner) {
> >>> > +  if (IsGslPtrInitWithGslTempOwner && DiagLoc.isValid()) {
> >>> >  Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) <<
> DiagRange;
> >>> >  return false;
> >>> >}
> >>> >
> >>> > Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
> >>> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368459&r1=368458&r2=368459&view=diff
> >>> >
> ==
> >>> > --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
> >>> > +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
> 10:42:41 2019
> >>> > @@ -201,6 +201,13 @@ void danglingReferenceFromTempOwner() {
> >>> >  std::vector getTempVec();
> >>> >  std::optional> getTempOptVec();
> >>> >
> >>> > +void testLoops() {
> >>> > +  for (auto i : getTempVec()) // ok
> >>> > +;
> >>> > +  for (auto i : *getTempOptVec()) // expected-warning {{object
> backing the pointer will be destroyed at the end of the full-expression}}
> >>> > +;
> >>> > +}
> >>> > +
> >>> >  int &usedToBeFalsePositive(std::vector &v) {
> >>> >std::vector::iterator it = v.begin();
> >>> >int& value = *it;
> >>> >
> >>> >
> >>> > ___
> >>> > cfe-commits mailing list
> >>> > cfe-commits@lists.llvm.org
> >>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r368459 - Fix a build bot failure and multiple warnings instances for range base for loops

[Gábor Horváth via cfe-commits]

Also, how does this stage2 bot work?
I clearly see which revision is being compiled from the logs but it is not
apparent to me what is the version of the compiler that is used to compiler
it. Is it possible that due to some race condition a compiler before my fix
is picked up and that is causing the failures?

Thanks,
Gabor

On Fri, 9 Aug 2019 at 15:05, Gábor Horváth  wrote:

> I tried both with libc++ instead of libstdc++ and modules but none of them
> reproduced in my local machine I would be glad if you could try it locally
> as well!
>
> Thanks in advance,
> Gabor
>
> On Fri, 9 Aug 2019 at 13:10, Jonas Devlieghere 
> wrote:
>
>> The bot is a little special in that it has modules enabled. Maybe that
>> explains it? Let me know if that doesn't work and I can try
>> reproducing locally.
>>
>> On Fri, Aug 9, 2019 at 12:02 PM Gábor Horváth 
>> wrote:
>> >
>> > I reverted but I cannot reproduce this locally on a linux box. Is there
>> any way to get more information from the build bot (like preprocessed
>> files?)?
>> >
>> > On Fri, 9 Aug 2019 at 11:38, Gábor Horváth  wrote:
>> >>
>> >> Hmm, strange. Looking into it! If I do not manage to find the root
>> cause in a few minutes I will revert!
>> >>
>> >> On Fri, 9 Aug 2019 at 11:32, Jonas Devlieghere 
>> wrote:
>> >>>
>> >>> I think this is causing a stage2 failure:
>> >>>
>> http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/124/consoleFull#-95886206949ba4694-19c4-4d7e-bec5-911270d8a58c
>> >>>
>> >>> On Fri, Aug 9, 2019 at 10:41 AM Gabor Horvath via cfe-commits
>> >>>  wrote:
>> >>> >
>> >>> > Author: xazax
>> >>> > Date: Fri Aug  9 10:42:41 2019
>> >>> > New Revision: 368459
>> >>> >
>> >>> > URL: http://llvm.org/viewvc/llvm-project?rev=368459&view=rev
>> >>> > Log:
>> >>> > Fix a build bot failure and multiple warnings instances for range
>> base for loops
>> >>> >
>> >>> > Modified:
>> >>> > cfe/trunk/lib/Sema/SemaInit.cpp
>> >>> > cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> >>> >
>> >>> > Modified: cfe/trunk/lib/Sema/SemaInit.cpp
>> >>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaInit.cpp?rev=368459&r1=368458&r2=368459&view=diff
>> >>> >
>> ==
>> >>> > --- cfe/trunk/lib/Sema/SemaInit.cpp (original)
>> >>> > +++ cfe/trunk/lib/Sema/SemaInit.cpp Fri Aug  9 10:42:41 2019
>> >>> > @@ -6616,7 +6616,7 @@ static void handleGslAnnotatedTypes(Indi
>> >>> >  return;
>> >>> >} else if (auto *OCE = dyn_cast(Call)) {
>> >>> >  FunctionDecl *Callee = OCE->getDirectCallee();
>> >>> > -if (Callee->isCXXInstanceMember() &&
>> >>> > +if (Callee && Callee->isCXXInstanceMember() &&
>> >>> >  shouldTrackImplicitObjectArg(cast(Callee)))
>> >>> >VisitPointerArg(Callee, OCE->getArg(0));
>> >>> >  return;
>> >>> > @@ -7070,8 +7070,11 @@ static SourceRange nextPathEntryRange(co
>> >>> >// supporting lifetime extension.
>> >>> >break;
>> >>> >
>> >>> > -case IndirectLocalPathEntry::DefaultInit:
>> >>> >  case IndirectLocalPathEntry::VarInit:
>> >>> > +  if (cast(Path[I].D)->isImplicit())
>> >>> > +return SourceRange();
>> >>> > +  LLVM_FALLTHROUGH;
>> >>> > +case IndirectLocalPathEntry::DefaultInit:
>> >>> >return Path[I].E->getSourceRange();
>> >>> >  }
>> >>> >}
>> >>> > @@ -7133,7 +7136,7 @@ void Sema::checkInitializerLifetime(cons
>> >>> >  return false;
>> >>> >}
>> >>> >
>> >>> > -  if (IsGslPtrInitWithGslTempOwner) {
>> >>> > +  if (IsGslPtrInitWithGslTempOwner && DiagLoc.isValid()) {
>> >>> >  Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) <<
>> DiagRange;
>> >>> >  return false;
>> >>> >}
>> >>> >
>> >>> > Modified: cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp
>> >>> > URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp?rev=368459&r1=368458&r2=368459&view=diff
>> >>> >
>> ==
>> >>> > --- cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp (original)
>> >>> > +++ cfe/trunk/test/Sema/warn-lifetime-analysis-nocfg.cpp Fri Aug  9
>> 10:42:41 2019
>> >>> > @@ -201,6 +201,13 @@ void danglingReferenceFromTempOwner() {
>> >>> >  std::vector getTempVec();
>> >>> >  std::optional> getTempOptVec();
>> >>> >
>> >>> > +void testLoops() {
>> >>> > +  for (auto i : getTempVec()) // ok
>> >>> > +;
>> >>> > +  for (auto i : *getTempOptVec()) // expected-warning {{object
>> backing the pointer will be destroyed at the end of the full-expression}}
>> >>> > +;
>> >>> > +}
>> >>> > +
>> >>> >  int &usedToBeFalsePositive(std::vector &v) {
>> >>> >std::vector::iterator it = v.begin();
>> >>> >int& value = *it;
>> >>> >
>> >>> >
>> >>> > ___
>> >>> > cfe-commits mailing list
>> >>> > cfe-commits@lists.llvm.org
>> >>> > https://lists.ll

Re: [PATCH] D11832: [Patch] [Analyzer] false positive: Potential leak connected with memcpy (PR 22954)

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

I reverted the commit until this assertion is fixed.

Steps to reproduce:
Download the following preprocessed file: F804743: clang_crash_7QnDaH.i 

Execute the analyzer on that one: clang -cc1 -analyze -analyzer-checker=core 
-analyzer-checker=unix -fblocks clang_crash_7QnDaH.i


Repository:
  rL LLVM

http://reviews.llvm.org/D11832



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 33704.
xazax.hun added a comment.

Made sure that inlined defensive checks do not generate false positives.


http://reviews.llvm.org/D12445

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,65 @@
   takesNullable(p);
   takesNonnull(p);
 }
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // The first warning was not a sink. The analysis expected to continue.
+  int i = 0;
+  i = 5 / i; // expected-warning {{Division by zero}}
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+  if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+  if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+  if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+  switch (getRandom()) {
+  case 1: inlinedNullable(p); break;
+  case 2: inlinedNonnull(p); break;
+  case 3: inlinedUnspecified(p); break;
+  }
+  if (getRandom())
+takesNonnull(p);
+  return p;
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,16 @@
 const MemRegion *Region;
   };
 
+  /// When any of the nonnull arguments of the analyzed function is null, do not
+  /// report anything and turn off the check.
+  ///
+  /// When \p SuppressPath is set to true, no more bugs will be reported on this
+  /// path by this checker.
+  void reportBugConditionally(ErrorKind Error, ExplodedNode *N,
+  const MemRegion *Region, CheckerContext &C,
+  const Stmt *ValueExpr = nullptr,
+  bool SuppressPath = false) const;
+
   void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
  BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
 if (!BT)
@@ -220,6 +230,13 @@
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
 
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
 enum class NullConstraint { IsNull, IsNotNull, Unknown };
 
 static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +319,76 @@
   return Nullability::Unspecified;
 }
 
+template 
+static ProgramStateRef
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ProgramStateRef State,
+const LocationContext *LocCtxt) {
+  for (const auto *ParamDecl : Params) {
+if (ParamDecl->isParameterPack())
+  break;
+
+if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+  continue;
+
+auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+  .template getAs();
+if (!RegVal)
+  continue;
+
+auto ParamValue = State->getSVal(RegVal->getRegion())
+  .template getAs();
+if (!ParamValue)
+  continue;
+
+if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+  return State->set(true);
+}
+  }
+  return State;
+}
+
+static ProgramStateRef
+checkPreconditionViolation(ProgramStateRef State,
+   const LocationContext *LocCtxt) {
+  const Decl *D = LocCtxt->getDecl();
+  if (!D)
+return State;
+
+  if (const auto *BlockD = dyn_cast(D)) {
+return checkParamsForPreconditionViolation(BlockD->parameters(), State,
+   

Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun marked an inline comment as not done.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:81
@@ +80,3 @@
+initBugType();
+SmallString<64> Buf;
+llvm::raw_svector_ostream OS(Buf);

zaks.anna wrote:
> How do we know that the string is big enough?
When the string is not big enough, there will be an allocation. So this is not 
a correctness issue. However I checked that, and the error messages tend to be 
very long. I could either increase the size of this smallstring to something 
like 150 which should be enough for the common of the cases, or I could just 
make it a string. Which one do you prefer?


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:179
@@ -178,3 @@
-  // We only track dynamic type info for regions.
-  const MemRegion *ToR = C.getSVal(CastE).getAsRegion();
-  if (!ToR)

zaks.anna wrote:
> This line used to be unconditional and now, it's only executed if we are 
> casting between ObjC Types.
It should not be a problem. The code bellow only executes for bitcasts, and 
getBetterObjCType only returns a valid value, when the cast was between Obj-C 
types.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:407
@@ +406,3 @@
+// Clean up the states stored by the generics checker.
+void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
+  CheckerContext &C) const {

zaks.anna wrote:
> Do you know if the info tracked by the DynamicTypeInfo checker gets cleaned 
> up for dead symbols?
That information is stored in DynamicTypeMap which is populated in 
lib/StaticAnalyzer/Core/ProgramState.cpp

I could not find any cleanup code. What do you think, what would be the best 
way to do the cleanup. Exposing a removeDynamicTypeInfo method from the 
ProgramState does not seem to be elegant, but it would work.


http://reviews.llvm.org/D12381



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:806
@@ -690,1 +805,3 @@
 
+  ProgramStateRef State = C.getState();
+  if (State->get())

zaks.anna wrote:
> Maybe we should only check these at the time the bug is about to be 
> reported.. 
> 
> That way the code would be less error prone..
This is checked before error reporting as well.
The purpose of this check at the beginning of the callbacks is optimization 
only.


http://reviews.llvm.org/D12445



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 33728.
xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

Style fixes according to the review.


http://reviews.llvm.org/D12445

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,65 @@
   takesNullable(p);
   takesNonnull(p);
 }
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // The first warning was not a sink. The analysis expected to continue.
+  int i = 0;
+  i = 5 / i; // expected-warning {{Division by zero}}
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+  if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+  if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+  if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+  switch (getRandom()) {
+  case 1: inlinedNullable(p); break;
+  case 2: inlinedNonnull(p); break;
+  case 3: inlinedUnspecified(p); break;
+  }
+  if (getRandom())
+takesNonnull(p);
+  return p;
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,16 @@
 const MemRegion *Region;
   };
 
+  /// When any of the nonnull arguments of the analyzed function is null, do not
+  /// report anything and turn off the check.
+  ///
+  /// When \p SuppressPath is set to true, no more bugs will be reported on this
+  /// path by this checker.
+  void reportBugIfPreconditionHolds(ErrorKind Error, ExplodedNode *N,
+const MemRegion *Region, CheckerContext &C,
+const Stmt *ValueExpr = nullptr,
+bool SuppressPath = false) const;
+
   void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
  BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
 if (!BT)
@@ -220,6 +230,13 @@
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
 
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
 enum class NullConstraint { IsNull, IsNotNull, Unknown };
 
 static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +319,82 @@
   return Nullability::Unspecified;
 }
 
+template 
+static bool
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ProgramStateRef State,
+const LocationContext *LocCtxt) {
+  for (const auto *ParamDecl : Params) {
+if (ParamDecl->isParameterPack())
+  break;
+
+if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+  continue;
+
+auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+  .template getAs();
+if (!RegVal)
+  continue;
+
+auto ParamValue = State->getSVal(RegVal->getRegion())
+  .template getAs();
+if (!ParamValue)
+  continue;
+
+if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+  return true;
+}
+  }
+  return false;
+}
+
+static bool checkPreconditionViolation(ProgramStateRef State, ExplodedNode *N,
+   CheckerContext &C) {
+  if (State->get())
+return true;
+
+  const LocationContext *LocCtxt = C.getLocationContext();
+  const Decl *D = LocCtxt->getDecl();
+  if (!D)
+return false;
+
+  if (const auto *BlockD = dy

[PATCH] D12619: [Static Analyzer] Minor cleanups for the nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: dcoughlin, zaks.anna.
xazax.hun added a subscriber: cfe-commits.

This patch contains minor cleanups and style fixes for the nullability checker. 
NFC.

http://reviews.llvm.org/D12619

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -58,12 +58,12 @@
 /// the nullability of the receiver or the nullability of the return type of the
 /// method, depending on which is more nullable. Contradicted is considered to
 /// be the most nullable, to avoid false positive results.
-static Nullability getMostNullable(Nullability Lhs, Nullability Rhs) {
+Nullability getMostNullable(Nullability Lhs, Nullability Rhs) {
   return static_cast(
   std::min(static_cast(Lhs), static_cast(Rhs)));
 }
 
-static const char *getNullabilityString(Nullability Nullab) {
+const char *getNullabilityString(Nullability Nullab) {
   switch (Nullab) {
   case Nullability::Contradicted:
 return "contradicted";
@@ -74,7 +74,7 @@
   case Nullability::Nonnull:
 return "nonnull";
   }
-  assert(false);
+  llvm_unreachable("Unexpected enumeration.");
   return "";
 }
 
@@ -89,19 +89,19 @@
   NullablePassedToNonnull
 };
 
-const char *ErrorMessages[] = {"Null pointer is assigned to a pointer which "
-   "has _Nonnull type",
-   "Null pointer is passed to a parameter which is "
-   "marked as _Nonnull",
-   "Null pointer is returned from a function that "
-   "has _Nonnull return type",
-   "Nullable pointer is assigned to a pointer "
-   "which has _Nonnull type",
-   "Nullable pointer is returned from a function "
-   "that has _Nonnull return type",
-   "Nullable pointer is dereferenced",
-   "Nullable pointer is passed to a parameter "
-   "which is marked as _Nonnull"};
+const char *const ErrorMessages[] = {"Null pointer is assigned to a pointer "
+ "which has _Nonnull type",
+ "Null pointer is passed to a parameter "
+ "which is marked as _Nonnull",
+ "Null pointer is returned from a function "
+ "that has _Nonnull return type",
+ "Nullable pointer is assigned to a "
+ "pointer which has _Nonnull type",
+ "Nullable pointer is returned from a "
+ "function that has _Nonnull return type",
+ "Nullable pointer is dereferenced",
+ "Nullable pointer is passed to a "
+ "parameter which is marked as _Nonnull"};
 
 class NullabilityChecker
 : public Checker,
@@ -176,7 +176,6 @@
 if (!BT)
   BT.reset(new BugType(this, "Nullability", "Memory error"));
 const char *Msg = ErrorMessages[static_cast(Error)];
-assert(Msg);
 std::unique_ptr R(new BugReport(*BT, Msg, N));
 if (Region) {
   R->markInteresting(Region);
@@ -262,7 +261,7 @@
   if (CheckSuperRegion) {
 if (auto FieldReg = Region->getAs())
   return dyn_cast(FieldReg->getSuperRegion());
-else if (auto ElementReg = Region->getAs())
+if (auto ElementReg = Region->getAs())
   return dyn_cast(ElementReg->getSuperRegion());
   }
 
@@ -272,12 +271,12 @@
 PathDiagnosticPiece *NullabilityChecker::NullabilityBugVisitor::VisitNode(
 const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
 BugReport &BR) {
-  ProgramStateRef state = N->getState();
-  ProgramStateRef statePrev = PrevN->getState();
+  ProgramStateRef State = N->getState();
+  ProgramStateRef StatePrev = PrevN->getState();
 
-  const NullabilityState *TrackedNullab = state->get(Region);
+  const NullabilityState *TrackedNullab = State->get(Region);
   const NullabilityState *TrackedNullabPrev =
-  statePrev->get(Region);
+  StatePrev->get(Region);
   if (!TrackedNullab)
 return nullptr;
 
@@ -645,34 +644,31 @@
 
 static Nullability getReceiverNullability(const ObjCMethodCall &M,
   ProgramStateRef State) {
-  Nullability RetNullability = Nullability::Unspecified;
   if (M.isReceiverSelfOrSuper()) {
 // For super and super class receivers we assume that the receiver is
 // nonnull.
-RetNullability = Nullability::Nonnull;
-  } else {
-

Re: [PATCH] D12619: [Static Analyzer] Minor cleanups for the nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

They are in the anonymous namespace. Maybe it would be cleaner to move them 
outside of the namespace and preserve the static keyword?


http://reviews.llvm.org/D12619



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: dcoughlin, zaks.anna, jordan_rose, ted.
xazax.hun added a subscriber: cfe-commits.

This patch adds lambda support for the static analyzer.
All of my initial tests are passed.

Before this patch each time a LambdaExpr was encountered a sink node was 
generated. This also reduced the coverage of code that is actually not inside a 
lambda operator(). Also the lack of lambda support is a known cause of some 
false positives (for example in the dead store checker).

This is a work in progress version of this patch, I will move this feature 
behind a flag and run it on LLVM to make sure it is also tested with real world 
code.


http://reviews.llvm.org/D12652

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/MemRegion.cpp
  test/Analysis/dead-stores.cpp
  test/Analysis/lambdas.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -299,13 +299,7 @@
   void testRecursiveFramesStart() { testRecursiveFrames(false); }
 
   void testLambdas() {
-// This is the test we would like to write:
-// []() { check(NoReturnDtor()); } != nullptr || check(Dtor());
-// But currently the analyzer stops when it encounters a lambda:
-[] {};
-// The CFG for this now looks correct, but we still do not reach the line
-// below.
-clang_analyzer_warnIfReached(); // FIXME: Should warn.
+[]() { check(NoReturnDtor()); } != nullptr || check(Dtor());
   }
 
   void testGnuExpressionStatements(int v) {
Index: test/Analysis/lambdas.cpp
===
--- test/Analysis/lambdas.cpp
+++ test/Analysis/lambdas.cpp
@@ -1,9 +1,142 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=debug.DumpCFG %s > %t 2>&1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -verify %s 
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.DumpCFG %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s
 
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
 struct X { X(const X&); };
 void f(X x) { (void) [x]{}; }
 
+
+// Lambda semantics tests.
+
+void basicCapture() {
+  int i = 5;
+  [i]() mutable {
+// clang_analyzer_eval does nothing in inlined functions.
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void deferredLambdaCall() {
+  int i = 5;
+  auto l1 = [i]() mutable {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  };
+  auto l2 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  };
+  auto l3 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  };
+  l1();
+  l2();
+  l3();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void multipleCaptures() {
+  int i = 5, j = 5;
+  [i, &j]() mutable {
+if (i != 5 && j != 5)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [=]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [&]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 7); // expected-warning{{TRUE}}
+}
+
+void testReturnValue() {
+  int i = 5;
+  auto l = [i] (int a) {
+return i + a;
+  };
+  int b = l(3);
+  clang_analyzer_eval(b == 8); // expected-warning{{TRUE}}
+}
+
+// Nested lambdas.
+
+void testNestedLambdas() {
+  int i = 5;
+  auto l = [i]() mutable {
+[&i]() {
+  ++i;
+}();
+if (i != 6)
+  clang_analyzer_warnIfReached();
+  };
+  l();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+}
+
+// Captured this.
+
+class RandomClass {
+  int i;
+
+  void captureFields() {
+i = 5;
+[this]() {
+  // clang_analyzer_eval does nothing in inlined functions.
+  if (i != 5)
+clang_analyzer_warnIfReached();
+  ++i;
+}();
+clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  }
+};
+
+// Captured function pointers.
+
+void inc(int &x) {
+  ++x;
+}
+
+void testFunctionPointerCapture() {
+  void (*func)(int &) = inc;
+  int i = 5;
+  [&i, func] {
+func(i);
+  }();

Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

There are several fallouts from this review, so I decided to split this patch 
up the following way:

1. I created a patch to incorporate the result of this review into 
ObjCGenericsChecker: http://reviews.llvm.org/D12701
2. I will created a separate patch to purge the dynamic type information from 
the GDM for dead symbols.
3. Once the former two patch is accepted I will rebase this patch on the top of 
those, so this will only contain minimal changes required for the merge.


http://reviews.llvm.org/D12381



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12701: [Static Analyzer] Objective-C Generics checker improvements.

[Gábor Horváth via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

http://reviews.llvm.org/D12701



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34285.
xazax.hun added a comment.

- Updated to newest trunk.
- Moved the feature behind an option.
- Fixed a crash when an operator() of a lambda is analyzed as a top level 
function, and a ThisExpr is referring to the this in the enclosing scope (this 
can only happen when lambda support is turned off).
- Added a new test case for nested lambdas capturing 'this'.


http://reviews.llvm.org/D12652

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/MemRegion.cpp
  test/Analysis/dead-stores.cpp
  test/Analysis/lambdas.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -299,13 +299,7 @@
   void testRecursiveFramesStart() { testRecursiveFrames(false); }
 
   void testLambdas() {
-// This is the test we would like to write:
-// []() { check(NoReturnDtor()); } != nullptr || check(Dtor());
-// But currently the analyzer stops when it encounters a lambda:
-[] {};
-// The CFG for this now looks correct, but we still do not reach the line
-// below.
-clang_analyzer_warnIfReached(); // FIXME: Should warn.
+[]() { check(NoReturnDtor()); } != nullptr || check(Dtor());
   }
 
   void testGnuExpressionStatements(int v) {
Index: test/Analysis/lambdas.cpp
===
--- test/Analysis/lambdas.cpp
+++ test/Analysis/lambdas.cpp
@@ -1,9 +1,167 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=debug.DumpCFG %s > %t 2>&1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config inline-lambdas=true -verify %s 
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.DumpCFG -analyzer-config inline-lambdas=true %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s
 
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
 struct X { X(const X&); };
 void f(X x) { (void) [x]{}; }
 
+
+// Lambda semantics tests.
+
+void basicCapture() {
+  int i = 5;
+  [i]() mutable {
+// clang_analyzer_eval does nothing in inlined functions.
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void deferredLambdaCall() {
+  int i = 5;
+  auto l1 = [i]() mutable {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  };
+  auto l2 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  };
+  auto l3 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  };
+  l1();
+  l2();
+  l3();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void multipleCaptures() {
+  int i = 5, j = 5;
+  [i, &j]() mutable {
+if (i != 5 && j != 5)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [=]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [&]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 7); // expected-warning{{TRUE}}
+}
+
+void testReturnValue() {
+  int i = 5;
+  auto l = [i] (int a) {
+return i + a;
+  };
+  int b = l(3);
+  clang_analyzer_eval(b == 8); // expected-warning{{TRUE}}
+}
+
+// Nested lambdas.
+
+void testNestedLambdas() {
+  int i = 5;
+  auto l = [i]() mutable {
+[&i]() {
+  ++i;
+}();
+if (i != 6)
+  clang_analyzer_warnIfReached();
+  };
+  l();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+}
+
+// Captured this.
+
+class RandomClass {
+  int i;
+
+  void captureFields() {
+i = 5;
+[this]() {
+  // clang_analyzer_eval does nothing in inlined functions.
+  if (i != 5)
+clang_analyzer_warnIfReached();
+  ++i;
+}();
+clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  }
+};
+
+
+// Nested this capture.
+
+class RandomClass2 {
+  int i;
+
+  void captureFields() {
+i = 5;
+[this]() {
+  // clang_analyzer_eval does nothing in inlined functions.
+  if (i != 5)
+clang_analyzer_warnIfReached();
+  ++i;
+  [this]() {
+// clang_ana

[PATCH] D12767: [Static Analyzer] Properly clean up the dynamic type information for dead regions.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose, krememek.
xazax.hun added a subscriber: cfe-commits.

This patch is intended to clean up the dynamic type information for regions 
that are dead. The behavior should not change.

In the a future patch it might be beneficial to factor out getDynamicTypeInfo 
and setDynamicTypeInfo from program state into free functions. This way the API 
of ProgramState could remain minimal.

http://reviews.llvm.org/D12767

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp

Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h"
@@ -752,12 +753,6 @@
   return Tainted;
 }
 
-/// The GDM component containing the dynamic type info. This is a map from a
-/// symbol to its most likely type.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(DynamicTypeMap,
- CLANG_ENTO_PROGRAMSTATE_MAP(const MemRegion *,
- DynamicTypeInfo))
-
 DynamicTypeInfo ProgramState::getDynamicTypeInfo(const MemRegion *Reg) const {
   Reg = Reg->StripCasts();
 
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 
@@ -27,6 +28,7 @@
 class DynamicTypePropagation:
 public Checker< check::PreCall,
 check::PostCall,
+check::DeadSymbols,
 check::PostStmt,
 check::PostStmt > {
   const ObjCObjectType *getObjectTypeForAllocAndNew(const ObjCMessageExpr *MsgE,
@@ -40,9 +42,23 @@
   void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostStmt(const ImplicitCastExpr *CastE, CheckerContext &C) const;
   void checkPostStmt(const CXXNewExpr *NewE, CheckerContext &C) const;
+  void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const;
 };
 }
 
+void DynamicTypePropagation::checkDeadSymbols(SymbolReaper &SR,
+  CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  DynamicTypeMapImpl TypeMap = State->get();
+  for (DynamicTypeMapImpl::iterator I = TypeMap.begin(), E = TypeMap.end();
+   I != E; ++I) {
+if (!SR.isLiveRegion(I->first)) {
+  State = State->remove(I->first);
+}
+  }
+  C.addTransition(State);
+}
+
 static void recordFixedType(const MemRegion *Region, const CXXMethodDecl *MD,
 CheckerContext &C) {
   assert(Region);
Index: include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
===
--- /dev/null
+++ include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
@@ -0,0 +1,41 @@
+//== DynamicTypeMap.h - Dynamic type map --- -*- C++ -*--=//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file provides APIs for tracking dynamic type information.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICTYPEMAP_H
+#define LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_DYNAMICTYPEMAP_H
+#include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "llvm/ADT/ImmutableMap.h"
+
+namespace clang {
+namespace ento {
+
+/// The GDM component containing the dynamic type info. This is a map from a
+/// symbol to its most likely type.

Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In http://reviews.llvm.org/D12381#241709, @xazax.hun wrote:

> There are several fallouts from this review, so I decided to split this patch 
> up the following way:
>
> 1. I created a patch to incorporate the result of this review into 
> ObjCGenericsChecker: http://reviews.llvm.org/D12701
> 2. I will created a separate patch to purge the dynamic type information from 
> the GDM for dead symbols.


The second patch is available here: http://reviews.llvm.org/D12767

> 3. Once the former two patch is accepted I will rebase this patch on the top 
> of those, so this will only contain minimal changes required for the merge.



http://reviews.llvm.org/D12381



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12619: [Static Analyzer] Minor cleanups for the nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34458.
xazax.hun added a comment.

- Updated to latest trunk
- Reworded the diagnostic messages


http://reviews.llvm.org/D12619

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -58,12 +58,12 @@
 /// the nullability of the receiver or the nullability of the return type of the
 /// method, depending on which is more nullable. Contradicted is considered to
 /// be the most nullable, to avoid false positive results.
-static Nullability getMostNullable(Nullability Lhs, Nullability Rhs) {
+Nullability getMostNullable(Nullability Lhs, Nullability Rhs) {
   return static_cast(
   std::min(static_cast(Lhs), static_cast(Rhs)));
 }
 
-static const char *getNullabilityString(Nullability Nullab) {
+const char *getNullabilityString(Nullability Nullab) {
   switch (Nullab) {
   case Nullability::Contradicted:
 return "contradicted";
@@ -74,7 +74,7 @@
   case Nullability::Nonnull:
 return "nonnull";
   }
-  assert(false);
+  llvm_unreachable("Unexpected enumeration.");
   return "";
 }
 
@@ -89,19 +89,17 @@
   NullablePassedToNonnull
 };
 
-const char *ErrorMessages[] = {"Null pointer is assigned to a pointer which "
-   "has _Nonnull type",
-   "Null pointer is passed to a parameter which is "
-   "marked as _Nonnull",
-   "Null pointer is returned from a function that "
-   "has _Nonnull return type",
-   "Nullable pointer is assigned to a pointer "
-   "which has _Nonnull type",
-   "Nullable pointer is returned from a function "
-   "that has _Nonnull return type",
-   "Nullable pointer is dereferenced",
-   "Nullable pointer is passed to a parameter "
-   "which is marked as _Nonnull"};
+const char *const ErrorMessages[] = {
+"Null is assigned to a pointer which is expected to have non-null value",
+"Null passed to a callee that requires a non-null argument",
+"Null is returned from a function that is expected to return a non-null "
+"value",
+"Nullable pointer is assigned to a pointer which is expected to have "
+"non-null value",
+"Nullable pointer is returned from a function that is expected to return a "
+"non-null value",
+"Nullable pointer is dereferenced",
+"Nullable pointer is passed to a calle that requires a non-null argument"};
 
 class NullabilityChecker
 : public Checker,
@@ -176,7 +174,6 @@
 if (!BT)
   BT.reset(new BugType(this, "Nullability", "Memory error"));
 const char *Msg = ErrorMessages[static_cast(Error)];
-assert(Msg);
 std::unique_ptr R(new BugReport(*BT, Msg, N));
 if (Region) {
   R->markInteresting(Region);
@@ -262,7 +259,7 @@
   if (CheckSuperRegion) {
 if (auto FieldReg = Region->getAs())
   return dyn_cast(FieldReg->getSuperRegion());
-else if (auto ElementReg = Region->getAs())
+if (auto ElementReg = Region->getAs())
   return dyn_cast(ElementReg->getSuperRegion());
   }
 
@@ -272,12 +269,12 @@
 PathDiagnosticPiece *NullabilityChecker::NullabilityBugVisitor::VisitNode(
 const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
 BugReport &BR) {
-  ProgramStateRef state = N->getState();
-  ProgramStateRef statePrev = PrevN->getState();
+  ProgramStateRef State = N->getState();
+  ProgramStateRef StatePrev = PrevN->getState();
 
-  const NullabilityState *TrackedNullab = state->get(Region);
+  const NullabilityState *TrackedNullab = State->get(Region);
   const NullabilityState *TrackedNullabPrev =
-  statePrev->get(Region);
+  StatePrev->get(Region);
   if (!TrackedNullab)
 return nullptr;
 
@@ -645,34 +642,31 @@
 
 static Nullability getReceiverNullability(const ObjCMethodCall &M,
   ProgramStateRef State) {
-  Nullability RetNullability = Nullability::Unspecified;
   if (M.isReceiverSelfOrSuper()) {
 // For super and super class receivers we assume that the receiver is
 // nonnull.
-RetNullability = Nullability::Nonnull;
-  } else {
-// Otherwise look up nullability in the state.
-SVal Receiver = M.getReceiverSVal();
-auto ValueRegionSVal = Receiver.getAs();
-if (ValueRegionSVal) {
-  const MemRegion *SelfRegion = ValueRegionSVal->getRegion();
-  assert(SelfRegion);
-
-  const NullabilityState *TrackedSelfNullability =
-  State->get(SelfRegion);
-  if (TrackedSelfNullability) {
-RetNullability = TrackedSelfNullability->getValue();

Re: [PATCH] D12769: [analyzer] Update SATestBuild.py to set -isysroot for preprocessed files

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: utils/analyzer/SATestBuild.py:277
@@ +276,3 @@
+# For now, we assume the preprocessed files should be analyzed
+# with the OS X SDK.
+SDKPath = getSDKPath("macosx")

I think it might be better to check if the host os is OS X. This way one might 
be able to check preprocessed files on other linux.


http://reviews.llvm.org/D12769



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12769: [analyzer] Update SATestBuild.py to set -isysroot for preprocessed files

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

- on other OS-es like linux.


http://reviews.llvm.org/D12769



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12769: [analyzer] Update SATestBuild.py to set -isysroot for preprocessed files

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: utils/analyzer/SATestBuild.py:277
@@ +276,3 @@
+# For now, we assume the preprocessed files should be analyzed
+# with the OS X SDK.
+SDKPath = getSDKPath("macosx")

dcoughlin wrote:
> xazax.hun wrote:
> > I think it might be better to check if the host os is OS X. This way one 
> > might be able to check preprocessed files on other linux.
> The patch checks for the existence of xcrun in the path and doesn't set 
> -isysroot if that is not present, so this should continue to work on Linux 
> and cygwin.
I see, however -cc1 option is only used when the SDKPath is not None. Is this 
intended?


http://reviews.llvm.org/D12769



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12769: [analyzer] Update SATestBuild.py to set -isysroot for preprocessed files

[Gábor Horváth via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

It looks good to me.


http://reviews.llvm.org/D12769



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34498.
xazax.hun added a comment.

- Updated to latest trunk
- Added test for the defensive inline check heuristic case
- Added test for diagnostic within a lambda body
- Added test for path notes


http://reviews.llvm.org/D12652

Files:
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/AnalyzerOptions.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  lib/StaticAnalyzer/Core/MemRegion.cpp
  test/Analysis/dead-stores.cpp
  test/Analysis/lambda-notes.cpp
  test/Analysis/lambdas.cpp
  test/Analysis/temporaries.cpp

Index: test/Analysis/temporaries.cpp
===
--- test/Analysis/temporaries.cpp
+++ test/Analysis/temporaries.cpp
@@ -299,13 +299,7 @@
   void testRecursiveFramesStart() { testRecursiveFrames(false); }
 
   void testLambdas() {
-// This is the test we would like to write:
-// []() { check(NoReturnDtor()); } != nullptr || check(Dtor());
-// But currently the analyzer stops when it encounters a lambda:
-[] {};
-// The CFG for this now looks correct, but we still do not reach the line
-// below.
-clang_analyzer_warnIfReached(); // FIXME: Should warn.
+[]() { check(NoReturnDtor()); } != nullptr || check(Dtor());
   }
 
   void testGnuExpressionStatements(int v) {
Index: test/Analysis/lambdas.cpp
===
--- test/Analysis/lambdas.cpp
+++ test/Analysis/lambdas.cpp
@@ -1,9 +1,181 @@
-// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=debug.DumpCFG %s > %t 2>&1
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.ExprInspection -analyzer-config inline-lambdas=true -verify %s 
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -analyze -analyzer-checker=core,debug.DumpCFG -analyzer-config inline-lambdas=true %s > %t 2>&1
 // RUN: FileCheck --input-file=%t %s
 
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(int);
+
 struct X { X(const X&); };
 void f(X x) { (void) [x]{}; }
 
+
+// Lambda semantics tests.
+
+void basicCapture() {
+  int i = 5;
+  [i]() mutable {
+// clang_analyzer_eval does nothing in inlined functions.
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  }();
+  [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void deferredLambdaCall() {
+  int i = 5;
+  auto l1 = [i]() mutable {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+++i;
+  };
+  auto l2 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+  };
+  auto l3 = [&i] {
+if (i != 5)
+  clang_analyzer_warnIfReached();
+i++;
+  };
+  l1();
+  l2();
+  l3();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+}
+
+void multipleCaptures() {
+  int i = 5, j = 5;
+  [i, &j]() mutable {
+if (i != 5 && j != 5)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [=]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 6); // expected-warning{{TRUE}}
+  [&]() mutable {
+if (i != 5 && j != 6)
+  clang_analyzer_warnIfReached();
+++i;
+++j;
+  }();
+  clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  clang_analyzer_eval(j == 7); // expected-warning{{TRUE}}
+}
+
+void testReturnValue() {
+  int i = 5;
+  auto l = [i] (int a) {
+return i + a;
+  };
+  int b = l(3);
+  clang_analyzer_eval(b == 8); // expected-warning{{TRUE}}
+}
+
+// Nested lambdas.
+
+void testNestedLambdas() {
+  int i = 5;
+  auto l = [i]() mutable {
+[&i]() {
+  ++i;
+}();
+if (i != 6)
+  clang_analyzer_warnIfReached();
+  };
+  l();
+  clang_analyzer_eval(i == 5); // expected-warning{{TRUE}}
+}
+
+// Captured this.
+
+class RandomClass {
+  int i;
+
+  void captureFields() {
+i = 5;
+[this]() {
+  // clang_analyzer_eval does nothing in inlined functions.
+  if (i != 5)
+clang_analyzer_warnIfReached();
+  ++i;
+}();
+clang_analyzer_eval(i == 6); // expected-warning{{TRUE}}
+  }
+};
+
+
+// Nested this capture.
+
+class RandomClass2 {
+  int i;
+
+  void captureFields() {
+i = 5;
+[this]() {
+  // clang_analyzer_eval does nothing in inlined functions.
+  if (i != 5)
+clang_analyzer_warnIfReached();
+  ++i;
+  [this]() {
+// clang_analyzer_eval does nothing in inlined functions.
+if (i != 6)
+  clang_analyzer_warnIfReached();
+++i;
+  }();
+ 

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In http://reviews.llvm.org/D12652#243762, @zaks.anna wrote:

> Have you tested this on a large codebase that uses lambdas? When do you think 
> we should turn this on by default?


I checked llvm and clang and did not found any failure. There are no obvious 
limitations or problems that I know of at the moment. I think this is a good 
candidate to turn on by default. At least some potential errors might be found 
earlier.

> Please, add test cases that demonstrate what happens when an issue is 
> reported within a lambda and to check if inlined defensive checks work.


I extended the tests with these cases.

> (As a follow up to this patch, we may need to teach LiveVariables.cpp and 
> UninitializedValues.cpp about lambdas. For example, to address issues like 
> this one: https://llvm.org/bugs/show_bug.cgi?id=22834)





http://reviews.llvm.org/D12652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12767: [Static Analyzer] Properly clean up the dynamic type information for dead regions.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34507.
xazax.hun added a comment.

- Updated to latest trunk.
- Separated the set/getDynamicTypeInfo APIs out from ProgramState

Note that it is not advised to put the implementations of 
set/getDynamicTypeInfo into a checker, because in that case there is a circular 
dependency during linking. The checkers use the symbols that are implemented in 
the clangStaticAnalyzerCore module and this way clangStaticAnalyzerCore would 
use symbols from that are implemented in the checkers as well.


http://reviews.llvm.org/D12767

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Core/CMakeLists.txt
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp

Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -14,6 +14,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SubEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/TaintManager.h"
@@ -752,36 +753,3 @@
   return Tainted;
 }
 
-/// The GDM component containing the dynamic type info. This is a map from a
-/// symbol to its most likely type.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(DynamicTypeMap,
- CLANG_ENTO_PROGRAMSTATE_MAP(const MemRegion *,
- DynamicTypeInfo))
-
-DynamicTypeInfo ProgramState::getDynamicTypeInfo(const MemRegion *Reg) const {
-  Reg = Reg->StripCasts();
-
-  // Look up the dynamic type in the GDM.
-  const DynamicTypeInfo *GDMType = get(Reg);
-  if (GDMType)
-return *GDMType;
-
-  // Otherwise, fall back to what we know about the region.
-  if (const TypedRegion *TR = dyn_cast(Reg))
-return DynamicTypeInfo(TR->getLocationType(), /*CanBeSubclass=*/false);
-
-  if (const SymbolicRegion *SR = dyn_cast(Reg)) {
-SymbolRef Sym = SR->getSymbol();
-return DynamicTypeInfo(Sym->getType());
-  }
-
-  return DynamicTypeInfo();
-}
-
-ProgramStateRef ProgramState::setDynamicTypeInfo(const MemRegion *Reg,
- DynamicTypeInfo NewTy) const {
-  Reg = Reg->StripCasts();
-  ProgramStateRef NewState = set(Reg, NewTy);
-  assert(NewState);
-  return NewState;
-}
Index: lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
===
--- /dev/null
+++ lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
@@ -0,0 +1,51 @@
+//==- DynamicTypeMap.cpp - Dynamic Type Info related APIs --*- C++ -*-//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines APIs that are related to track and query dynamic type
+//  information. This information can be used to devirtualize calls during the
+//  symbolic exection or do type checking.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
+
+namespace clang {
+namespace ento {
+
+DynamicTypeInfo getDynamicTypeInfo(ProgramStateRef State,
+   const MemRegion *Reg) {
+  Reg = Reg->StripCasts();
+
+  // Look up the dynamic type in the GDM.
+  const DynamicTypeInfo *GDMType = State->get(Reg);
+  if (GDMType)
+return *GDMType;
+
+  // Otherwise, fall back to what we know about the region.
+  if (const TypedRegion *TR = dyn_cast(Reg))
+return DynamicTypeInfo(TR->getLocationType(), /*CanBeSubclass=*/false);
+
+  if (const SymbolicRegion *SR = dyn_cast(Reg)) {
+SymbolRef Sym = SR->getSymbol();
+return DynamicTypeInfo(Sym->getType());
+  }
+
+  return DynamicTypeInfo();
+}
+
+ProgramStateRef setDynamicTypeInfo(ProgramStateRef State, const MemRegion *Reg,
+   DynamicTypeInfo NewTy) {
+  Reg = Reg->StripCasts();
+  ProgramStateRef NewState = State->set(Reg, NewTy);
+  assert(NewState);
+  return NewState;
+}
+
+} // namespace ento
+} // namespace clang
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -17,6 +17,7 @@
 #include "clang/AST/ParentMap.h"
 #incl

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:515-517
@@ -511,1 +514,5 @@
 
+  /// Returns true if lambdas should be inlined. Otherwise a sink node will be
+  /// generated each time a LambdaExpr is visited.
+  bool shouldInlineLambdas();
+

jordan_rose wrote:
> "inline" is kind of a misnomer, since we may not actually inline lambdas. I 
> would have suggested "model lambdas" or "lambda support".
Even when this configuration option is set to false, the body of the lambda is 
analyzed as a top level function. For this reason I think the "lambda support" 
might be a misnomer too. What do you think?


Comment at: lib/StaticAnalyzer/Core/MemRegion.cpp:740-741
@@ -739,3 +739,4 @@
   const DeclContext *DC,
-  const VarDecl *VD) {
+  const VarDecl *VD,
+  MemRegionManager *Mmgr) {
   while (LC) {

jordan_rose wrote:
> Why the extra parameter?
This is just a leftover from code evolution, thank you for spotting this.


http://reviews.llvm.org/D12652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12767: [Static Analyzer] Properly clean up the dynamic type information for dead regions.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34554.
xazax.hun added a comment.

- Addressed the comments.


http://reviews.llvm.org/D12767

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Core/CMakeLists.txt
  lib/StaticAnalyzer/Core/CallEvent.cpp
  lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp

Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -752,36 +752,3 @@
   return Tainted;
 }
 
-/// The GDM component containing the dynamic type info. This is a map from a
-/// symbol to its most likely type.
-REGISTER_TRAIT_WITH_PROGRAMSTATE(DynamicTypeMap,
- CLANG_ENTO_PROGRAMSTATE_MAP(const MemRegion *,
- DynamicTypeInfo))
-
-DynamicTypeInfo ProgramState::getDynamicTypeInfo(const MemRegion *Reg) const {
-  Reg = Reg->StripCasts();
-
-  // Look up the dynamic type in the GDM.
-  const DynamicTypeInfo *GDMType = get(Reg);
-  if (GDMType)
-return *GDMType;
-
-  // Otherwise, fall back to what we know about the region.
-  if (const TypedRegion *TR = dyn_cast(Reg))
-return DynamicTypeInfo(TR->getLocationType(), /*CanBeSubclass=*/false);
-
-  if (const SymbolicRegion *SR = dyn_cast(Reg)) {
-SymbolRef Sym = SR->getSymbol();
-return DynamicTypeInfo(Sym->getType());
-  }
-
-  return DynamicTypeInfo();
-}
-
-ProgramStateRef ProgramState::setDynamicTypeInfo(const MemRegion *Reg,
- DynamicTypeInfo NewTy) const {
-  Reg = Reg->StripCasts();
-  ProgramStateRef NewState = set(Reg, NewTy);
-  assert(NewState);
-  return NewState;
-}
Index: lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
===
--- /dev/null
+++ lib/StaticAnalyzer/Core/DynamicTypeMap.cpp
@@ -0,0 +1,51 @@
+//==- DynamicTypeMap.cpp - Dynamic Type Info related APIs --*- C++ -*-//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+//  This file defines APIs that track and query dynamic type information. This
+//  information can be used to devirtualize calls during the symbolic exection
+//  or do type checking.
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
+
+namespace clang {
+namespace ento {
+
+DynamicTypeInfo getDynamicTypeInfo(ProgramStateRef State,
+   const MemRegion *Reg) {
+  Reg = Reg->StripCasts();
+
+  // Look up the dynamic type in the GDM.
+  const DynamicTypeInfo *GDMType = State->get(Reg);
+  if (GDMType)
+return *GDMType;
+
+  // Otherwise, fall back to what we know about the region.
+  if (const TypedRegion *TR = dyn_cast(Reg))
+return DynamicTypeInfo(TR->getLocationType(), /*CanBeSubclass=*/false);
+
+  if (const SymbolicRegion *SR = dyn_cast(Reg)) {
+SymbolRef Sym = SR->getSymbol();
+return DynamicTypeInfo(Sym->getType());
+  }
+
+  return DynamicTypeInfo();
+}
+
+ProgramStateRef setDynamicTypeInfo(ProgramStateRef State, const MemRegion *Reg,
+   DynamicTypeInfo NewTy) {
+  Reg = Reg->StripCasts();
+  ProgramStateRef NewState = State->set(Reg, NewTy);
+  assert(NewState);
+  return NewState;
+}
+
+} // namespace ento
+} // namespace clang
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -17,6 +17,7 @@
 #include "clang/AST/ParentMap.h"
 #include "clang/Analysis/ProgramPoint.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
@@ -435,7 +436,7 @@
 return RuntimeDefinition();
 
   // Do we know anything about the type of 'this'?
-  DynamicTypeInfo DynType = getState()->getDynamicTypeInfo(R);
+  DynamicTypeInfo DynType = getDynamicTypeInfo(getState(), R);
   if (!DynType.isValid())
 return RuntimeDefinition();
 
@@ -800,7 +801,7 @@
   if (!Receiver)
 return RuntimeDefinition();
 
-  DynamicTypeInfo DTI = getState()->getDynamicTypeInfo(Receiver);
+  DynamicTypeInfo DTI = getDynamicTypeInfo(getState(), Receiver);
   QualType DynType = DTI.getType();
   CanBeSubClassed = DTI.canBeASubClass();
   Rece

Re: [PATCH] D12767: [Static Analyzer] Properly clean up the dynamic type information for dead regions.

[Gábor Horváth via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

http://reviews.llvm.org/D12767



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: include/clang/StaticAnalyzer/Core/AnalyzerOptions.h:515-517
@@ -511,1 +514,5 @@
 
+  /// Returns true if lambdas should be inlined. Otherwise a sink node will be
+  /// generated each time a LambdaExpr is visited.
+  bool shouldInlineLambdas();
+

xazax.hun wrote:
> jordan_rose wrote:
> > "inline" is kind of a misnomer, since we may not actually inline lambdas. I 
> > would have suggested "model lambdas" or "lambda support".
> Even when this configuration option is set to false, the body of the lambda 
> is analyzed as a top level function. For this reason I think the "lambda 
> support" might be a misnomer too. What do you think?
If we come up with a better name for the option I will address that in a 
separate patch.


Repository:
  rL LLVM

http://reviews.llvm.org/D12652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In general I like this change, the node handling of the checkers are more 
readable and reflects the intent in a clearer way.  I have some comments inline.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:244
@@ +243,3 @@
+  const ProgramPointTag *Tag = nullptr) {
+return generateSink(State, /*Pred=*/nullptr,
+   (Tag ? Tag : Location.getTag()));

zaks.anna wrote:
> Please, use a non-null Pred for clarity
The following workflow is not supported by this API: a checker that generates 
multiple transition in the same callback (the generated nodes are added 
sequentially to the path), and one of the might be an error node.

This also applies to generateNonFatalErrorNode.

In case we would like to improve the documentation it might be useful to give 
some pointers to the users which when should an error node be considered as 
fatal.


Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:322
@@ -290,1 +321,3 @@
+// the same as the predecessor state has made a mistake. We return the
+// predecessor and rather than cache out.
 if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))

As a slightly related note: is it documented anywhere what "cache out" means? 
Maybe it would be great to refer to that document or write it if it is not 
written yet.


http://reviews.llvm.org/D12780



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12818: [Static Analyzer] Relaxing a caching out related assert.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose, krememek.
xazax.hun added a subscriber: cfe-commits.

During the development of the nullability checkers I hit this assert several 
times. However it is very hard to reproduce it with a minimal example, and hard 
to come up with a test case. I could not identify any issue with the checkers 
themselves, and I think it is ok to cache out at this point of execution, when 
the node was created by a checker. 

http://reviews.llvm.org/D12818

Files:
  lib/StaticAnalyzer/Core/ExprEngineObjC.cpp

Index: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -186,8 +186,11 @@
 
 // Generate a transition to non-Nil state.
 if (notNilState != State) {
+  bool HasTag = Pred->getLocation().getTag();
   Pred = Bldr.generateNode(ME, Pred, notNilState);
-  assert(Pred && "Should have cached out already!");
+  assert((Pred || HasTag) && "Should have cached out already!");
+  if (!Pred)
+continue;
 }
   }
 } else {


Index: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -186,8 +186,11 @@
 
 // Generate a transition to non-Nil state.
 if (notNilState != State) {
+  bool HasTag = Pred->getLocation().getTag();
   Pred = Bldr.generateNode(ME, Pred, notNilState);
-  assert(Pred && "Should have cached out already!");
+  assert((Pred || HasTag) && "Should have cached out already!");
+  if (!Pred)
+continue;
 }
   }
 } else {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34606.
xazax.hun added a comment.

- Rebased on the top of latest trunk (which contains patch #1 and patch #2 from 
my previous comments)


http://reviews.llvm.org/D12381

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
  test/Analysis/generics.m

Index: test/Analysis/generics.m
===
--- test/Analysis/generics.m
+++ test/Analysis/generics.m
@@ -435,7 +435,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash2
@@ -545,7 +545,7 @@
 // CHECK:descriptionConversion from value of type 'NSNumber *' to incompatible type 'NSString *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash3
@@ -689,7 +689,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash5
@@ -939,7 +939,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextcrossProceduralErasedTypes
 // CHECK:   issue_hash1
@@ -1049,7 +1049,7 @@
 // CHECK:descriptionConversion from value of type 'NSNumber *' to incompatible type 'NSString *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErasedReverseConversion
 // CHECK:   issue_hash2
@@ -1193,7 +1193,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErasedReverseConversion
 // CHECK:   issue_hash4
@@ -1303,7 +1303,7 @@
 // CHECK:descriptionConversion from value of type 'NSNumber *' to incompatible type 'NSString *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextidErasedIncompatibleTypesReverseConversion
 // CHECK:   issue_hash2
@@ -1447,7 +1447,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextidErasedIncompatibleTypesReverseConversion
 // CHECK:   issue_hash4
@@ -1591,7 +1591,7 @@
 // CHECK:descriptionConversion from value of type 'NSArray *' to incompatible type 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextidErasedIncompatibleTypes
 // CHECK:   issue_hash2
@@ -1701,7 +1701,7 @@
 // CHECK:descriptionConversion from value of type 'NSNumber *' to incompatible type 'NSString *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextidErasedIncompatibleTypes
 // CHEC

Re: [PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun marked 3 inline comments as done.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:121
@@ -54,3 +120,3 @@
I != E; ++I) {
 if (!SR.isLiveRegion(I->first)) {
   State = State->remove(I->first);

zaks.anna wrote:
> It's odd that we are using this API.. Are we keeping track of non-symbolic 
> regions? If not, can't we just check if Region->getSymbol() is dead?
> (Same in the nullability checker.)
The DynamicTypeMap might contain non-symbolic regions. (The tests fails when I 
cast the regions stored in the map to symbolic regions.)

This observation is good however for the nullability checker. 


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:275
@@ +274,3 @@
+///
+/// Precondition: the cast is between ObjCObjectPointers.
+ExplodedNode *DynamicTypePropagation::dynamicTypePropagationOnCasts(

zaks.anna wrote:
> I do not see where you are checking the precondition.
The check is in line 507. It is checked anyways for the generics checker and I 
did not want to add redundant checks to this function.


http://reviews.llvm.org/D12381



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12848: [Static Analyzer] Nullability checker optimization.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: dcoughlin, zaks.anna, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

Right now the nullability checker only tracks symbolic regions. For this reason 
if there are no dead symbols it is safe to skip the precondition checking. I 
also changed to cleanup code to work with symbols directly instead of memory 
regions.

http://reviews.llvm.org/D12848

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -395,12 +395,17 @@
 /// Cleaning up the program state.
 void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
   CheckerContext &C) const {
+  if (!SR.hasDeadSymbols())
+return;
+
   ProgramStateRef State = C.getState();
   NullabilityMapTy Nullabilities = State->get();
   for (NullabilityMapTy::iterator I = Nullabilities.begin(),
   E = Nullabilities.end();
I != E; ++I) {
-if (!SR.isLiveRegion(I->first)) {
+const auto *Region = I->first->getAs();
+assert(Region && "Non-symbolic region is tracked.");
+if (SR.isDead(Region->getSymbol())) {
   State = State->remove(I->first);
 }
   }


Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -395,12 +395,17 @@
 /// Cleaning up the program state.
 void NullabilityChecker::checkDeadSymbols(SymbolReaper &SR,
   CheckerContext &C) const {
+  if (!SR.hasDeadSymbols())
+return;
+
   ProgramStateRef State = C.getState();
   NullabilityMapTy Nullabilities = State->get();
   for (NullabilityMapTy::iterator I = Nullabilities.begin(),
   E = Nullabilities.end();
I != E; ++I) {
-if (!SR.isLiveRegion(I->first)) {
+const auto *Region = I->first->getAs();
+assert(Region && "Non-symbolic region is tracked.");
+if (SR.isDead(Region->getSymbol())) {
   State = State->remove(I->first);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12852: [Static Analyzer] Moving nullability checkers to a top level package.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

This patch is a preparation to enabling some of the nullability checks by 
default.

1. Moved nullability checkers to a top level package. The core package is not a 
great fit, because some of the checks should be turned on by default and some 
of them should be opt-in checks.

2. When the only the default checks turned on, tracking nullability of the 
symbols is not necessary. This minimizes the impact of the default checks on 
memory consumption.

3. Added a separate test file to the configuration when only those checks are 
turned on that will be turned on by default later on. 

http://reviews.llvm.org/D12852

Files:
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm
  test/Analysis/nullability_nullonly.mm

Index: test/Analysis/nullability_nullonly.mm
===
--- /dev/null
+++ test/Analysis/nullability_nullonly.mm
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.nullability.NullPassedToNonnull,alpha.nullability.NullReturnedFromNonnull -verify %s
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+Dummy *_Nullable returnsNullable();
+
+void testBasicRules() {
+  // The tracking of nullable values is turned off.
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // no warning
+  Dummy *q = 0;
+  if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+  }
+}
+
+Dummy *_Nonnull testNullReturn() {
+  Dummy *p = 0;
+  return p; // expected-warning {{}}
+}
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = 0;
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // Passing null to nonnull is a sink. Stop the analysis.
+  int i = 0;
+  i = 5 / i; // no warning
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+  if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+  if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+  if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+  switch (getRandom()) {
+  case 1: inlinedNullable(p); break;
+  case 2: inlinedNonnull(p); break;
+  case 3: inlinedUnspecified(p); break;
+  }
+  if (getRandom())
+takesNonnull(p);
+  return p;
+}
Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.nullability -verify %s
 
 #define nil 0
 #define BOOL int
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -136,6 +136,7 @@
   };
 
   NullabilityChecksFilter Filter;
+  DefaultBool NeedTracking;
 
 private:
   class NullabilityBugVisitor
@@ -188,6 +189,11 @@
 }
 BR.emitReport(std::move(R));
   }
+
+  /// If an SVal wraps a region that should be tracked, it will return a pointer
+  /// to the wrapped region. Otherwise it will return a nullptr.
+  const SymbolicRegion *getTrackRegion(SVal Val,
+   bool CheckSuperRegion = false) const;
 };
 
 class NullabilityState {
@@ -210,10 +216,13 @@
 
 private:
   Nullability Nullab;
-  // Source is the expression which determined the nullability. For example in a
-  // message like [nullable nonnull_returning] has nullable nullability, because
+  // Source is the expression which determined the nullability. For example in
+  // a
+  // message like [nullable nonnull_returning] has nullable nullability,
+  // because
   // the receiver is nullable. Here the receiver will be the source of the
-  // nullability. This is useful information when the diagnostics are generated.
+  // nullability. This is u

Re: [PATCH] D12852: [Static Analyzer] Moving nullability checkers to a top level package.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 34705.
xazax.hun added a comment.

Removed an unintended comment formatting change.


http://reviews.llvm.org/D12852

Files:
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm
  test/Analysis/nullability_nullonly.mm

Index: test/Analysis/nullability_nullonly.mm
===
--- /dev/null
+++ test/Analysis/nullability_nullonly.mm
@@ -0,0 +1,87 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.nullability.NullPassedToNonnull,alpha.nullability.NullReturnedFromNonnull -verify %s
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+Dummy *_Nullable returnsNullable();
+
+void testBasicRules() {
+  // The tracking of nullable values is turned off.
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // no warning
+  Dummy *q = 0;
+  if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+  }
+}
+
+Dummy *_Nonnull testNullReturn() {
+  Dummy *p = 0;
+  return p; // expected-warning {{}}
+}
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = 0;
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // Passing null to nonnull is a sink. Stop the analysis.
+  int i = 0;
+  i = 5 / i; // no warning
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
+
+void inlinedNullable(Dummy *_Nullable p) {
+  if (p) return;
+}
+void inlinedNonnull(Dummy *_Nonnull p) {
+  if (p) return;
+}
+void inlinedUnspecified(Dummy *p) {
+  if (p) return;
+}
+
+Dummy *_Nonnull testDefensiveInlineChecks(Dummy * p) {
+  switch (getRandom()) {
+  case 1: inlinedNullable(p); break;
+  case 2: inlinedNonnull(p); break;
+  case 3: inlinedUnspecified(p); break;
+  }
+  if (getRandom())
+takesNonnull(p);
+  return p;
+}
Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.nullability -verify %s
 
 #define nil 0
 #define BOOL int
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -136,6 +136,7 @@
   };
 
   NullabilityChecksFilter Filter;
+  DefaultBool NeedTracking;
 
 private:
   class NullabilityBugVisitor
@@ -188,6 +189,11 @@
 }
 BR.emitReport(std::move(R));
   }
+
+  /// If an SVal wraps a region that should be tracked, it will return a pointer
+  /// to the wrapped region. Otherwise it will return a nullptr.
+  const SymbolicRegion *getTrackRegion(SVal Val,
+   bool CheckSuperRegion = false) const;
 };
 
 class NullabilityState {
@@ -246,10 +252,11 @@
   return NullConstraint::Unknown;
 }
 
-// If an SVal wraps a region that should be tracked, it will return a pointer
-// to the wrapped region. Otherwise it will return a nullptr.
-static const SymbolicRegion *getTrackRegion(SVal Val,
-bool CheckSuperRegion = false) {
+const SymbolicRegion *
+NullabilityChecker::getTrackRegion(SVal Val, bool CheckSuperRegion) const {
+  if (!NeedTracking)
+return nullptr;
+
   auto RegionSVal = Val.getAs();
   if (!RegionSVal)
 return nullptr;
@@ -941,15 +948,16 @@
   }
 }
 
-#define REGISTER_CHECKER(name) \
+#define REGISTER_CHECKER(name, trackingRequired)   \
   void ento::register##name##Checker(CheckerManager &mgr) {\
 NullabilityChecker *checker = mgr.registerChecker();   \
 checker->Filter.Check##name = true;\
 checker->Filter.CheckName##name = mgr.getCurrentCheckName();   \
+checker->NeedTracking = checker->NeedTracking || trackingRequired; \
   }
 
-REGISTER_CHECKER(NullPassedToNonn

[PATCH] D12858: [Static Analyzer] Turn on some nullability checks by default.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

This patch turns on some of the nullability related patches by default. These 
checks has low false positive rate. 

http://reviews.llvm.org/D12858

Files:
  lib/Driver/Tools.cpp

Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3322,6 +3322,11 @@
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mktemp");
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mkstemp");
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.vfork");
+
+  // Default nullability checks.
+  CmdArgs.push_back("-analyzer-checker=nullability.NullPassedToNonnull");
+  CmdArgs.push_back(
+  "-analyzer-checker=nullability.NullReturnedFromNonnull");
 }
 
 // Set the output format. The default is plist, for (lame) historical


Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -3322,6 +3322,11 @@
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mktemp");
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.mkstemp");
   CmdArgs.push_back("-analyzer-checker=security.insecureAPI.vfork");
+
+  // Default nullability checks.
+  CmdArgs.push_back("-analyzer-checker=nullability.NullPassedToNonnull");
+  CmdArgs.push_back(
+  "-analyzer-checker=nullability.NullReturnedFromNonnull");
 }
 
 // Set the output format. The default is plist, for (lame) historical
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12889: [Static Analyzer] Generics Checker: When an ObjC method returns a specialized object, track it properly.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:675
@@ +674,3 @@
+
+  QualType ResultType = Method->getReturnType().substObjCTypeArgs(
+  C, TypeArgs, ObjCSubstitutionContext::Result);

zaks.anna wrote:
> Could you use StaticResultType here?
In StaticResultType every occurrence of a Type Argument is replaced with id, 
which is not suitable for type checking.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:795
@@ -817,1 +794,3 @@
+  // Checking that the return type is used correctly is done in pre-call to get
+  // cleaner diagnostic paths.
   checkReturnType(MessageExpr, *TrackedType, Sym, Method, *TypeArgs,

zaks.anna wrote:
> The path is longer if the call is inlined, correct? (Not much difference 
> otherwise..)
In fact there is a heuristic that omits the stepping in into the function when 
the diagnostic is generated. I moved this check to the post call callback.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:803
@@ +802,3 @@
+/// initialized with by invoking the 'class' method on a class.
+/// This method is also used to infer the type information for the return
+/// types.

zaks.anna wrote:
> It looks like it's only inferring the type information of the specializations 
> and not for non-generic types..
Added a todo for the missing case.


http://reviews.llvm.org/D12889



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12889: [Static Analyzer] Generics Checker: When an ObjC method returns a specialized object, track it properly.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:680
@@ +679,3 @@
+return nullptr;
+
+  QualType ResultType = Method->getReturnType().substObjCTypeArgs(

zaks.anna wrote:
> From above:
>  QualType StaticResultType = Method->getReturnType();
> 
> You could do 
>  QualType ResultType = StaticResultType.substObjCTypeArgs(
>   C, TypeArgs, ObjCSubstitutionContext::Result);
Oh, I see. Sorry for misunderstanding.


http://reviews.llvm.org/D12889



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12780: [analyzer] Add generateErrorNode() APIs to CheckerContext

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:328
@@ -290,1 +327,3 @@
+// DereferenceChecker, CallAndMessageChecker, and DynamicTypePropagation)
+// rely upon the defensive behavior and would need to be updated.
 if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))

I think the most common form of relying on this branch is adding a transition 
to an unchanged state.


http://reviews.llvm.org/D12780



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12916: [Static Analyzer] Use generics related information to infer dynamic types.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

This patch makes the DynamicTypePropagation checker utilize the information 
about generics. Using this additional information more precise inlining can be 
done. It also fixes an XFAIL test.

The same stored information will be used by a separate general type checker. 
That checker will make it possible to get rid of the isReturnValueMisused 
function.

http://reviews.llvm.org/D12916

Files:
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  test/Analysis/DynamicTypePropagation.m

Index: test/Analysis/DynamicTypePropagation.m
===
--- test/Analysis/DynamicTypePropagation.m
+++ test/Analysis/DynamicTypePropagation.m
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze 
-analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify %s
-// XFAIL: *
 
 #if !__has_feature(objc_generics)
 #  error Compiler does not support Objective-C generics?
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -665,38 +665,36 @@
 /// Get the returned ObjCObjectPointerType by a method based on the tracked 
type
 /// information, or null pointer when the returned type is not an
 /// ObjCObjectPointerType.
-static const ObjCObjectPointerType *getReturnTypeForMethod(
+static QualType getReturnTypeForMethod(
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 const ObjCObjectPointerType *SelfType, ASTContext &C) {
   QualType StaticResultType = Method->getReturnType();
 
   // Is the return type declared as instance type?
   if (StaticResultType == C.getObjCInstanceType())
-return SelfType;
+return QualType(SelfType, 0);
 
   // Check whether the result type depends on a type parameter.
   if (!isObjCTypeParamDependent(StaticResultType))
-return nullptr;
+return QualType();
 
   QualType ResultType = StaticResultType.substObjCTypeArgs(
   C, TypeArgs, ObjCSubstitutionContext::Result);
 
-  return ResultType->getAs();
+  return ResultType;
 }
 
 /// Validate that the return type of a message expression is used correctly.
 /// Returns true in case an error is detected.
 bool DynamicTypePropagation::isReturnValueMisused(
 const ObjCMessageExpr *MessageExpr,
-const ObjCObjectPointerType *SeflType, SymbolRef Sym,
+const ObjCObjectPointerType *ResultPtrType, SymbolRef Sym,
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 bool SubscriptOrProperty, CheckerContext &C) const {
-  ASTContext &ASTCtxt = C.getASTContext();
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, TypeArgs, SeflType, ASTCtxt);
   if (!ResultPtrType)
 return false;
 
+  ASTContext &ASTCtxt = C.getASTContext();
   const Stmt *Parent =
   C.getCurrentAnalysisDeclContext()->getParentMap().getParent(MessageExpr);
   if (SubscriptOrProperty) {
@@ -861,20 +859,34 @@
   if (!TypeArgs)
 return;
 
-  if (isReturnValueMisused(MessageExpr, *TrackedType, RecSym, Method, 
*TypeArgs,
-   M.getMessageKind() != OCM_Message, C))
+  QualType ResultType =
+  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
+  // The static type is the same as the deduced type.
+  if (ResultType.isNull())
+return;
+
+  const MemRegion *RetRegion = M.getReturnValue().getAsRegion();
+  ExplodedNode *Pred = C.getPredecessor();
+  if (RetRegion) {
+State = setDynamicTypeInfo(State, RetRegion, ResultType,
+   /*CanBeSubclass=*/true);
+Pred = C.addTransition(State);
+  }
+
+  const auto *ResultPtrType = ResultType->getAs();
+
+  if (isReturnValueMisused(MessageExpr, ResultPtrType, RecSym, Method,
+   *TypeArgs, M.getMessageKind() != OCM_Message, C))
 return;
 
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
   if (!ResultPtrType || ResultPtrType->isUnspecialized())
 return;
 
   // When the result is a specialized type and it is not tracked yet, track it
   // for the result symbol.
   if (!State->get(RetSym)) {
 State = State->set(RetSym, ResultPtrType);
-C.addTransition(State);
+C.addTransition(State, Pred);
   }
 }
 


Index: test/Analysis/DynamicTypePropagation.m
===
--- test/Analysis/DynamicTypePropagation.m
+++ test/Analysis/DynamicTypePropagation.m
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify %s
-// XFAIL: *
 
 #if !__has_feature(objc_generics)
 #  error Compiler does not support Objective-C generics?
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- 

Re: [PATCH] D12916: [Static Analyzer] Use generics related information to infer dynamic types.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 35027.
xazax.hun added a comment.

Addressed the comments.


http://reviews.llvm.org/D12916

Files:
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  test/Analysis/DynamicTypePropagation.m

Index: test/Analysis/DynamicTypePropagation.m
===
--- test/Analysis/DynamicTypePropagation.m
+++ test/Analysis/DynamicTypePropagation.m
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify %s
-// XFAIL: *
 
 #if !__has_feature(objc_generics)
 #  error Compiler does not support Objective-C generics?
Index: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
===
--- lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -665,38 +665,36 @@
 /// Get the returned ObjCObjectPointerType by a method based on the tracked type
 /// information, or null pointer when the returned type is not an
 /// ObjCObjectPointerType.
-static const ObjCObjectPointerType *getReturnTypeForMethod(
+static QualType getReturnTypeForMethod(
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 const ObjCObjectPointerType *SelfType, ASTContext &C) {
   QualType StaticResultType = Method->getReturnType();
 
   // Is the return type declared as instance type?
   if (StaticResultType == C.getObjCInstanceType())
-return SelfType;
+return QualType(SelfType, 0);
 
   // Check whether the result type depends on a type parameter.
   if (!isObjCTypeParamDependent(StaticResultType))
-return nullptr;
+return QualType();
 
   QualType ResultType = StaticResultType.substObjCTypeArgs(
   C, TypeArgs, ObjCSubstitutionContext::Result);
 
-  return ResultType->getAs();
+  return ResultType;
 }
 
 /// Validate that the return type of a message expression is used correctly.
 /// Returns true in case an error is detected.
 bool DynamicTypePropagation::isReturnValueMisused(
 const ObjCMessageExpr *MessageExpr,
-const ObjCObjectPointerType *SeflType, SymbolRef Sym,
+const ObjCObjectPointerType *ResultPtrType, SymbolRef Sym,
 const ObjCMethodDecl *Method, ArrayRef TypeArgs,
 bool SubscriptOrProperty, CheckerContext &C) const {
-  ASTContext &ASTCtxt = C.getASTContext();
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, TypeArgs, SeflType, ASTCtxt);
   if (!ResultPtrType)
 return false;
 
+  ASTContext &ASTCtxt = C.getASTContext();
   const Stmt *Parent =
   C.getCurrentAnalysisDeclContext()->getParentMap().getParent(MessageExpr);
   if (SubscriptOrProperty) {
@@ -861,20 +859,37 @@
   if (!TypeArgs)
 return;
 
-  if (isReturnValueMisused(MessageExpr, *TrackedType, RecSym, Method, *TypeArgs,
-   M.getMessageKind() != OCM_Message, C))
+  QualType ResultType =
+  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
+  // The static type is the same as the deduced type.
+  if (ResultType.isNull())
+return;
+
+  const MemRegion *RetRegion = M.getReturnValue().getAsRegion();
+  ExplodedNode *Pred = C.getPredecessor();
+  if (RetRegion && !State->get(RetRegion)) {
+// TODO: we have duplicated information in DynamicTypeMap and
+// MostSpecializedTypeArgsMap. We should only store anything in the later if
+// the stored data differs from the one stored in the former.
+State = setDynamicTypeInfo(State, RetRegion, ResultType,
+   /*CanBeSubclass=*/true);
+Pred = C.addTransition(State);
+  }
+
+  const auto *ResultPtrType = ResultType->getAs();
+
+  if (isReturnValueMisused(MessageExpr, ResultPtrType, RecSym, Method,
+   *TypeArgs, M.getMessageKind() != OCM_Message, C))
 return;
 
-  const auto *ResultPtrType =
-  getReturnTypeForMethod(Method, *TypeArgs, *TrackedType, ASTCtxt);
   if (!ResultPtrType || ResultPtrType->isUnspecialized())
 return;
 
   // When the result is a specialized type and it is not tracked yet, track it
   // for the result symbol.
   if (!State->get(RetSym)) {
 State = State->set(RetSym, ResultPtrType);
-C.addTransition(State);
+C.addTransition(State, Pred);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12916: [Static Analyzer] Use generics related information to infer dynamic types.

[Gábor Horváth via cfe-commits]

xazax.hun marked an inline comment as done.


Comment at: lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:871
@@ +870,3 @@
+  if (RetRegion && !State->get(RetRegion)) {
+// TODO: we have duplicated information in DynamicTypeMap and
+// MostSpecializedTypeArgsMap. We should only store anything in the later 
if

You are right, I am added the check and the comment.
The dynamicTypePropagationOnCasts should not cause any trouble, it only updates 
the dynamic type map, when there is new and better information available. See 
getBetterObjCType function.

We do not need to call getBetterObjCType here, because if there were no 
information yet, we will populate the table, if a function was inlined and 
there is a cast before return getBetterObjCType was already called there, so it 
is safe to just skip updating the DynamicTypeMap.


http://reviews.llvm.org/D12916



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

I can see two very differen directions on the two version of this patch. I 
think this is bad and we should pick one.

In the other version we started to exclude some of the stuff (like filename) 
from the hash, since it is available already in the plist and gives the 
processing scripts more freedom. While this patch adds bug type to the hash.


http://reviews.llvm.org/D12906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

I was thinking a bit more, what would be the best way to determine what should 
we include in the hash.

In order to determine that first we need to define the scope of the hash 
itself. There are several sensible choices such as:

- identify bugs that were generated by a specific checker in a translation 
unit. (i.e. check name is not included in the hash)
- identify bugs that has the same bugtype in the same translation unit.
- identify bugs in a translation unit.
- identify bug within a project.
- identify bugs globally.
- etc...

Note that the scope of the hash does not necesserily affects the scope of 
diffing results, since the information omitted from the hash can be processed 
separately.

If we exclude something from the hash that is available in the plist we give 
flexibility to the tools that consume plists, but also give more possibility to 
diverge in the way bugs are identified.


http://reviews.llvm.org/D12906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12906: [RFC] Bug identification("issue_hash") change for CmpRuns.py

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In http://reviews.llvm.org/D12906#248418, @honggyu.kim wrote:

> (3) bug type (bug message)


As far as I remember the other version of this patch exclude any message that 
is displayed to the user deliberately, to make the hash more resilient to 
changes to those strings. As always, there is a trade off here, and in order to 
decide we need to have the exact scope of the hash. If we expect to only 
compare hashes that was generated with the same version of clang, it is great 
to include that in the hash. If we want to compare hashes that was generated by 
different versions of clang, this might not be a good idea, depending on how 
often are those messages changes. The original patch included the check_name in 
the hash. Unfortunately that might change too.


http://reviews.llvm.org/D12906



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12973: [Static Analyzer] General type checker based on dynamic type information.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

This patch adds a checker that utilizes the information that was collected by 
DynamicTypePropagation and warns when the static type contradicts the dynamic 
type.

This checker also replaces the AST matching logic in ObjCGenericsChecker's 
return type checking which was error prone and fragile. As a positive side 
effect this change also reduced the false negative cases.

http://reviews.llvm.org/D12973

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DynamicTypeChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  test/Analysis/dynamic_type_check.m
  test/Analysis/generics.m

Index: test/Analysis/generics.m
===
--- test/Analysis/generics.m
+++ test/Analysis/generics.m
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify -Wno-objc-method-access %s
-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics -verify -Wno-objc-method-access %s -analyzer-output=plist -o %t.plist
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics,alpha.core.DynamicTypeChecker -verify -Wno-objc-method-access %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.osx.cocoa.ObjCGenerics,alpha.core.DynamicTypeChecker -verify -Wno-objc-method-access %s -analyzer-output=plist -o %t.plist
 // RUN: FileCheck --input-file %t.plist %s
 
 #if !__has_feature(objc_generics)
@@ -236,13 +236,13 @@
 
 void workWithProperties(NSArray *a) {
   NSArray *b = a;
-  NSString *str = [b getObjAtIndex: 0]; // expected-warning {{Conversion}}
+  NSString *str = [b getObjAtIndex: 0]; // expected-warning {{Object}}
   NSNumber *num = [b getObjAtIndex: 0];
-  str = [b firstObject]; // expected-warning {{Conversion}}
+  str = [b firstObject]; // expected-warning {{Object}}
   num = [b firstObject];
-  str = b.firstObject; // expected-warning {{Conversion}}
+  str = b.firstObject; // expected-warning {{Object}}
   num = b.firstObject;
-  str = b[0]; // expected-warning {{Conversion}}
+  str = b[0]; // expected-warning {{Object}}
   num = b[0];
 }
 
@@ -318,15 +318,14 @@
 
 void returnToUnrelatedType(NSArray *> *arr) {
   NSArray *erased = arr;
-  NSSet* a = [erased firstObject]; // expected-warning {{Conversion}}
+  NSSet* a = [erased firstObject]; // expected-warning {{Object}}
   (void)a;
 }
 
 void returnToIdVariable(NSArray *arr) {
   NSArray *erased = arr;
   id a = [erased firstObject];
-  // TODO: Warn in this case. Possibly in a separate checker.
-  NSNumber *res = a;
+  NSNumber *res = a; // expected-warning {{Object}}
 }
 
 // CHECK:  
@@ -4428,35 +4427,6 @@
 // CHECK:path
 // CHECK:
 // CHECK: 
-// CHECK:  kindevent
-// CHECK:  location
-// CHECK:  
-// CHECK:   line238
-// CHECK:   col16
-// CHECK:   file0
-// CHECK:  
-// CHECK:  ranges
-// CHECK:  
-// CHECK:
-// CHECK: 
-// CHECK:  line238
-// CHECK:  col16
-// CHECK:  file0
-// CHECK: 
-// CHECK: 
-// CHECK:  line238
-// CHECK:  col16
-// CHECK:  file0
-// CHECK: 
-// CHECK:
-// CHECK:  
-// CHECK:  depth0
-// CHECK:  extended_message
-// CHECK:  Type 'NSArray *' is inferred from implicit cast (from 'NSArray *' to 'NSArray *')
-// CHECK:  message
-// CHECK:  Type 'NSArray *' is inferred from implicit cast (from 'NSArray *' to 'NSArray *')
-// CHECK: 
-// CHECK: 
 // CHECK:  kindcontrol
 // CHECK:  edges
 // CHECK:   
@@ -4549,57 +4519,57 @@
 // CHECK:  
 // CHECK:  depth0
 // CHECK:  extended_message
-// CHECK:  Conversion from value of type 'NSNumber *' to incompatible type 'NSString *'
+// CHECK:  Type 'NSNumber *' is inferred from this context
 // CHECK:  message
-// CHECK:  Conversion from value of type 'NSNumber *' to incompatible type 'NSString *'
+// CHECK:  Type 'NSNumber *' is inferred from this context
 // CHECK: 
-// CHECK:
-// CHECK:descriptionConversion from value of type 'NSNumber *' to incompatible type 'NSString *'
-// CHECK:categoryCore Foundation/Objective-C
-// CHECK:typeGenerics
-// CHECK:check_namecore.DynamicTypePropagation
-// CHECK:   issue_context_kindfunction
-// CHECK:   issue_contextworkWithProperties
-// CHECK:   issue_hash2
-// CHECK:   location
-// CHECK:   
-// CHECK:line239
-// CHECK:col19
-// CHECK:file0
-// CHECK:   
-// CHECK:   
-// CHECK:   
-// CHECK:path
-// CHECK:
 // CHECK: 
 // CHECK:  kindevent
 // CHECK:  location
 // CHECK:  
-// CHECK:   line238
-// CHECK:   col16
+// CHECK:   line239
+// CHECK:   col19
 // CHECK

Re: [PATCH] D12901: [Static Analyzer] Intersecting ranges and 64 bit to 32 bit truncations causing "System is over constrained." assertions.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Hi!

Thank you for the patch!

What happens if you factor the "index + 1" expression out into a separate 
variable?
E.g.: unsigned temp = index + 1; and use temp in the condition?

My impression is that, the ranges does not model the overflow behavior 
correctly (which is well defined for unsigned values). I wondering why do you 
think that, the right way to solve this is to modify assumeSymNE and 
assumeSymEQ? Wouldn't it be better to actually handle the ranges properly on 
assignments and other operations (such as +), so assumeSymNE and assumeSymEQ 
can remain unmodified?


http://reviews.llvm.org/D12901



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In http://reviews.llvm.org/D12725#243150, @NoQ wrote:

> Thanks, fixed :)


Can you commit this or do you need someone to commit this for you?


http://reviews.llvm.org/D12725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12725: [analyzer] A fix for substraction of an integer from a pointer.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

In http://reviews.llvm.org/D12725#248931, @NoQ wrote:

> I've got no commit access yet, sorry, that's my first patch here actually :)


No problem, I committed it in r248021. Thank you for your contribution!


Repository:
  rL LLVM

http://reviews.llvm.org/D12725



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Even if the analzyer no longer generates new hashes it should be easy to 
maintain old hash algorithms out of tree.


http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13099: [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Hi!

Thanks for this patch! I think this would be a great addition! I have some 
comments inline.



Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:409
@@ +408,3 @@
+  if (const CXXMethodDecl *D = cast_or_null(getDecl())) {
+if(D->getCanonicalDecl()->isConst()) {
+  // Check if the containing class/struct has mutable members

Do we need to get the cannonical decl here? When one declaration is const, all 
of them supposed to be const.


Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:412
@@ +411,3 @@
+  const MemRegion *ThisRegion = getCXXThisVal().getAsRegion();
+  if (ThisRegion) {
+MemRegionManager *MemMgr = ThisRegion->getMemRegionManager();

Is it possible to fail to get ThisRegion? Should this be an assert?


Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:415
@@ +414,3 @@
+const CXXRecordDecl *Parent = D->getParent();
+for (const auto *I : Parent->fields()) {
+  if (I->isMutable()) {

What about the mutable fields of base classes? Are they covered here?


http://reviews.llvm.org/D13099



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13099: [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

One more note. Do we want to support const_cast for this? A possible way to do 
that is to invalidate this, when a const cast appears in the body of the 
function. (However the body might not be available. It is only my opinion, but 
I would be ok to accept this patch without const cast support, but it is 
something that we might want to document somewhere as a caveat.


http://reviews.llvm.org/D13099



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13099: [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

A general style comment: you could decrease the level of indentation using 
early returns. I have one more comment inline, otherwise it looks good to me.



Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:422
@@ +421,3 @@
+  // Check if this is a call to a const method.
+  if (const CXXMethodDecl *D = cast_or_null(getDecl())) {
+if(D->isConst()) {

Does this check work for member operators? I wonder what is the reason that 
getDecl returns a FunctionDecl instead of CXXMethodDecl. 


http://reviews.llvm.org/D13099



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12652: [Static Analyzer] Lambda support.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

I will look into it. Do you prefer to revert the patch this it is fixed?


Repository:
  rL LLVM

http://reviews.llvm.org/D12652



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13313: [clang-tidy] new check misc-no-reinterpret-cast

[Gábor Horváth via cfe-commits]

xazax.hun added a subscriber: xazax.hun.
xazax.hun added a comment.

If you check AvoidCStyleCastsCheck.cpp, it also suggest what kind of cast 
should you use to replace a C style cast. Probably, in some cases, the 
developers might be able to change the reinterpret_cast to something more type 
safe. It would be nice to make a suggestion in those cases.


http://reviews.llvm.org/D13313



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

xazax.hun marked 9 inline comments as done.


Comment at: lib/StaticAnalyzer/Core/BugId.cpp:29
@@ +28,3 @@
+
+static std::string GetSignature(const FunctionDecl *Target) {
+  if (!Target)

zaks.anna wrote:
> Can/Should we use some existing machinery for this? For example, mangleName().
Generating mangled names requires ASTContext which is not available during the 
error reporting. BugReporter does have the ASTContext, so it would not be a big 
change to add it to the DiagnosticConsumers though. And I think the mangled 
name might contain too much redundant information. The only relevant 
information here is the fully qualified name and the parts of the signature 
that can be ocerloaded on e.g.: constness. Using this method might also be 
easier to debug, since the IssueString is more readable.


Comment at: lib/StaticAnalyzer/Core/BugId.cpp:38
@@ +37,3 @@
+
+  switch (Target->getStorageClass()) {
+  case SC_Extern:

zaks.anna wrote:
> Why are these necessary? 
You are right, it is not possible to overload on these properties, so it is 
safe (and even beneficial) to remove them from the hash. 


Comment at: lib/StaticAnalyzer/Core/BugId.cpp:52
@@ +51,3 @@
+
+  if (Target->isInlineSpecified())
+Signature.append("inline ");

zaks.anna wrote:
> Why do we need these in the hash?
We do not need these information (see my previous comment). I removed them from 
the hash.


Comment at: lib/StaticAnalyzer/Core/BugId.cpp:198
@@ +197,3 @@
+std::string clang::GetIssueString(const SourceManager &SM, FullSourceLoc &L,
+  StringRef CheckerName, StringRef HashField,
+  const Decl *D) {

zaks.anna wrote:
> Including the checker name here is not perfect because checker name can 
> easily change from one release of clang to another. For example, when a 
> checker moves to another package.
> 
> I think the best approach here would be to give checkers some unique 
> identifiers and using those here. We can discuss the more if you are 
> interested in solving this problem.
I definitely agree. It would be great to have a unique identifier for checkers 
that is independent from the name/package. It is not a trivial problem however, 
since we probably want to support plugins too. I can think of a similar 
solution like git commit ids, but I think this problem might be out of the 
scope of this patch. But I am happy to start a discussion on the mailing list 
and create a new patch to solve this. 


Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:20
@@ -19,2 +19,3 @@
 #include "clang/Lex/Preprocessor.h"
+#include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h"

zaks.anna wrote:
> Is this needed?
Removed.


Comment at: test/Analysis/model-file.cpp:43
@@ -42,7 +42,3 @@
 // CHECK-NEXT:  
-// CHECK-NEXT:  
-// CHECK-NEXT:   path
-// CHECK-NEXT:   
-// CHECK-NEXT:
-// CHECK-NEXT: kindcontrol
-// CHECK-NEXT: edges
+// CHECK-NEXT:   
+// CHECK-NEXT:path

zaks.anna wrote:
> Extra space in the whole file.
Whatever I do it looks like the whitespaces are not alligned well for this 
file. My guess s that this plist might be generated with an old version of the 
static analyzer. I think it is better to actually update it to the old 
formatting than doing a diff by hand, or writing a script to do just that.


http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D22374: [analyzer] Copy and move constructors - ExprEngine extended for "almost trivial" copy and move constructors

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Note that, once a constructor is not available, we will conservatively treat it 
as nontrivial. This is not the case however for most of the templated code. 
Since the STL use templates heavily I think this patch is a great step forward 
in improving the modeling of C++ code.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:37
@@ -36,1 +36,3 @@
 
+bool isCopyOrMoveLike(const CXXConstructorDecl *Constr) {
+  if (Constr->isCopyOrMoveConstructor())

These two functions only used in this translation unit right? Maybe it would be 
better to make them static.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:57
@@ +56,3 @@
+
+  if(ParamRecDecl!=ThisRecDecl)
+return false;

nit: need spaced around the operator.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:63
@@ +62,3 @@
+
+bool isAlmostTrivial(const CXXMethodDecl *Met) {
+  if (Met->isTrivial())

Isn't this only applicable to ctors? Maybe you should reflect this in the name 
or change the type of the parameter accordingly.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:67
@@ +66,3 @@
+
+  if(!Met->hasTrivialBody())
+return false;

Please document that, in case we do not see the body of a ctor, we treat it 
conservatively as non trivial. (hasTrivialBody returns false when the body is 
not available)


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:88
@@ +87,3 @@
+  if(Initzer->isBaseInitializer() &&
+ Initzer->getBaseClass() == &*Base.getType()) {
+if(const auto *CtrCall = 
dyn_cast(Initzer->getInit()->IgnoreParenImpCasts())) {

Maybe you could reduce the indentation if you use "early continue" here.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:109
@@ +108,3 @@
+if(!Field->getType()->isScalarType() &&
+   !Field->getType()->isRecordType())
+  return false;

What types do we want to exclude and why? It might be a good idea to document 
them.


Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:112
@@ +111,3 @@
+bool found = false;
+for(const auto *Initzer: Constr->inits()) {
+  if(Initzer->isMemberInitializer() && Initzer->getMember() == Field) {

Instead of the O(n^2) algorithm, it might be better to just iterate through the 
initializers, and put each field into a llvm small pointer set, or something 
like that. Afterwards you can iterate through the fields of the struct and 
check whether each field is inside the set. This might be both more efficient 
and cleaner.


https://reviews.llvm.org/D22374



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D19311: [analyzer] Self Assignment Checker

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1738
@@ +1737,3 @@
+
+  const auto Msg = "Assuming " + Met->getParamDecl(0)->getName() +
+   ((Param == This) ? " == " : " != ") + "*this";

getName will return a StringRef here. Contatenating const char * and StringRef 
will give you a Twine. So Msg will be a twine which refers to temporary 
objects. This will result in a use after free. You shoud convert the result of 
the concatenation (the Twine) to a std::string, to copy the data and avoid use 
after free.


Repository:
  rL LLVM

https://reviews.llvm.org/D19311



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15227: [analyzer] Valist checkers.

[Gábor Horváth via cfe-commits]

xazax.hun marked 10 inline comments as done.
xazax.hun added a comment.

https://reviews.llvm.org/D15227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23014: [analyzer] Model base to derived casts more precisely.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, dergachev.a, a.sidorin.
xazax.hun added a subscriber: cfe-commits.

Dynamic casts are handled relatively well by the static analyzer. BaseToDerived 
casts however are handled conservatively. This can cause some false positives 
with the NewDeleteLeaks checker.

This patch alters the behavior of BaseToDerived casts. In case a dynamic cast 
would succeed use the same semantics. Otherwise fall back to the conservative 
approach.

A slightly related question: in case a cast on a pointer can not be modelled 
precisely, maybe that should be handled as a pointer escape to avoid false 
positives in some cases?

https://reviews.llvm.org/D23014

Files:
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  test/Analysis/NewDelete-checker-test.cpp

Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -377,3 +377,19 @@
   delete foo;
   delete foo;  // expected-warning {{Attempt to delete released memory}}
 }
+
+struct Base {
+  virtual ~Base() {}
+};
+
+struct Derived : Base {
+};
+
+Base *allocate() {
+  return new Derived;
+}
+
+void shouldNotReportLeak() {
+  Derived *p = (Derived *)allocate();
+  delete p;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -412,6 +412,28 @@
 Bldr.generateNode(CastE, Pred, state);
 continue;
   }
+  case CK_BaseToDerived: {
+SVal val = state->getSVal(Ex, LCtx);
+QualType resultType = CastE->getType();
+if (CastE->isGLValue())
+  resultType = getContext().getPointerType(resultType);
+
+bool Failed = false;
+
+if (!val.isZeroConstant()) {
+  val = getStoreManager().evalDynamicCast(val, T, Failed);
+}
+
+// Failed to cast or the result is unknown, fall back to conservative.
+if (Failed || val.isUnknown()) {
+  val =
+svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx, resultType,
+ currBldrCtx->blockCount());
+}
+state = state->BindExpr(CastE, LCtx, val);
+Bldr.generateNode(CastE, Pred, state);
+continue;
+  }
   case CK_NullToMemberPointer: {
 // FIXME: For now, member pointers are represented by void *.
 SVal V = svalBuilder.makeNull();
@@ -421,7 +443,6 @@
   }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
-  case CK_BaseToDerived:
   case CK_BaseToDerivedMemberPointer:
   case CK_DerivedToBaseMemberPointer:
   case CK_ReinterpretMemberPointer:


Index: test/Analysis/NewDelete-checker-test.cpp
===
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -377,3 +377,19 @@
   delete foo;
   delete foo;  // expected-warning {{Attempt to delete released memory}}
 }
+
+struct Base {
+  virtual ~Base() {}
+};
+
+struct Derived : Base {
+};
+
+Base *allocate() {
+  return new Derived;
+}
+
+void shouldNotReportLeak() {
+  Derived *p = (Derived *)allocate();
+  delete p;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -412,6 +412,28 @@
 Bldr.generateNode(CastE, Pred, state);
 continue;
   }
+  case CK_BaseToDerived: {
+SVal val = state->getSVal(Ex, LCtx);
+QualType resultType = CastE->getType();
+if (CastE->isGLValue())
+  resultType = getContext().getPointerType(resultType);
+
+bool Failed = false;
+
+if (!val.isZeroConstant()) {
+  val = getStoreManager().evalDynamicCast(val, T, Failed);
+}
+
+// Failed to cast or the result is unknown, fall back to conservative.
+if (Failed || val.isUnknown()) {
+  val =
+svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx, resultType,
+ currBldrCtx->blockCount());
+}
+state = state->BindExpr(CastE, LCtx, val);
+Bldr.generateNode(CastE, Pred, state);
+continue;
+  }
   case CK_NullToMemberPointer: {
 // FIXME: For now, member pointers are represented by void *.
 SVal V = svalBuilder.makeNull();
@@ -421,7 +443,6 @@
   }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
-  case CK_BaseToDerived:
   case CK_BaseToDerivedMemberPointer:
   case CK_DerivedToBaseMemberPointer:
   case CK_ReinterpretMemberPointer:
___
cfe-commits mailing list
cfe-commits@lis

Re: [PATCH] D23014: [analyzer] Model base to derived casts more precisely.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: test/Analysis/NewDelete-checker-test.cpp:394
@@ +393,3 @@
+  Derived *p = (Derived *)allocate();
+  delete p;
+}

Before the modification the analyzer reports a leak here, since the symbol 
returned by the BaseToDerived cast is independent of the original symbol.


https://reviews.llvm.org/D23014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23014: [analyzer] Model base to derived casts more precisely.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:423
@@ +422,3 @@
+
+if (!val.isZeroConstant()) {
+  val = getStoreManager().evalDynamicCast(val, T, Failed);

NoQ wrote:
> I guess if `val` is a //non-zero// constant, it wouldn't make much difference.
I might be wrong, but isn't the only valid constant value for a pointer the 
zero constant?


https://reviews.llvm.org/D23014



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23060: [analyzer] Show enabled checker list

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, NoQ.
xazax.hun added a subscriber: cfe-commits.

This patch adds a command line option to list the checkers that were enabled by 
analyzer-checker and not disabled by -analyzer-disable-checker.

It can be very useful to debug long command lines when it is not immediately 
apparent which checkers are turned on and which checkers are turned off.

Clang tidy already has a similar feature. 

https://reviews.llvm.org/D23060

Files:
  include/clang/Driver/CC1Options.td
  include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
  include/clang/StaticAnalyzer/Core/CheckerRegistry.h
  include/clang/StaticAnalyzer/Frontend/FrontendActions.h
  lib/Frontend/CompilerInvocation.cpp
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/StaticAnalyzer/Core/CheckerRegistry.cpp
  lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
  test/Analysis/analyzer-enabled-checkers.c

Index: test/Analysis/analyzer-enabled-checkers.c
===
--- /dev/null
+++ test/Analysis/analyzer-enabled-checkers.c
@@ -0,0 +1,29 @@
+// RUN: %clang -target x86_64-apple-darwin10 --analyze %s -o /dev/null -Xclang -analyzer-checker=core -Xclang -analyzer-checker-list-enabled > %t 2>&1
+// RUN: FileCheck --input-file=%t %s
+
+void bar() {}
+void foo() {
+  // Call bar 33 times so max-times-inline-large is met and
+  // min-blocks-for-inline-large is checked
+  for (int i = 0; i < 34; ++i) {
+bar();
+  }
+}
+
+// CHECK: OVERVIEW: Clang Static Analyzer Enabled Checkers List
+// CHECK: core.CallAndMessage
+// CHECK: core.DivideZero
+// CHECK: core.DynamicTypePropagation
+// CHECK: core.NonNullParamChecker
+// CHECK: core.NullDereference
+// CHECK: core.StackAddressEscape
+// CHECK: core.UndefinedBinaryOperatorResult
+// CHECK: core.VLASize
+// CHECK: core.builtin.BuiltinFunctions
+// CHECK: core.builtin.NoReturnFunctions
+// CHECK: core.uninitialized.ArraySubscript
+// CHECK: core.uninitialized.Assign
+// CHECK: core.uninitialized.Branch
+// CHECK: core.uninitialized.CapturedBlockVariable
+// CHECK: core.uninitialized.UndefReturn
+
Index: lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
===
--- lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
+++ lib/StaticAnalyzer/Frontend/CheckerRegistration.cpp
@@ -101,18 +101,24 @@
   << pluginAPIVersion;
 }
 
+static SmallVector
+getCheckerOptList(const AnalyzerOptions &opts) {
+  SmallVector checkerOpts;
+  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
+const std::pair &opt = opts.CheckersControlList[i];
+checkerOpts.push_back(CheckerOptInfo(opt.first.c_str(), opt.second));
+  }
+  return checkerOpts;
+}
+
 std::unique_ptr
 ento::createCheckerManager(AnalyzerOptions &opts, const LangOptions &langOpts,
ArrayRef plugins,
DiagnosticsEngine &diags) {
   std::unique_ptr checkerMgr(
   new CheckerManager(langOpts, &opts));
 
-  SmallVector checkerOpts;
-  for (unsigned i = 0, e = opts.CheckersControlList.size(); i != e; ++i) {
-const std::pair &opt = opts.CheckersControlList[i];
-checkerOpts.push_back(CheckerOptInfo(opt.first.c_str(), opt.second));
-  }
+  SmallVector checkerOpts = getCheckerOptList(opts);
 
   ClangCheckerRegistry allCheckers(plugins, &diags);
   allCheckers.initializeManager(*checkerMgr, checkerOpts);
@@ -137,3 +143,12 @@
 
   ClangCheckerRegistry(plugins).printHelp(out);
 }
+
+void ento::printEnabledCheckerList(raw_ostream &out,
+   ArrayRef plugins,
+   const AnalyzerOptions &opts) {
+  out << "OVERVIEW: Clang Static Analyzer Enabled Checkers List\n\n";
+
+  SmallVector checkerOpts = getCheckerOptList(opts);
+  ClangCheckerRegistry(plugins).printList(out, checkerOpts);
+}
Index: lib/StaticAnalyzer/Core/CheckerRegistry.cpp
===
--- lib/StaticAnalyzer/Core/CheckerRegistry.cpp
+++ lib/StaticAnalyzer/Core/CheckerRegistry.cpp
@@ -175,3 +175,22 @@
 out << '\n';
   }
 }
+
+void CheckerRegistry::printList(
+raw_ostream &out, SmallVectorImpl &opts) const {
+  std::sort(Checkers.begin(), Checkers.end(), checkerNameLT);
+
+  // Collect checkers enabled by the options.
+  CheckerInfoSet enabledCheckers;
+  for (SmallVectorImpl::iterator i = opts.begin(),
+   e = opts.end();
+   i != e; ++i) {
+collectCheckers(Checkers, Packages, *i, enabledCheckers);
+  }
+
+  for (CheckerInfoSet::const_iterator i = enabledCheckers.begin(),
+  e = enabledCheckers.end();
+   i != e; ++i) {
+out << (*i)->FullName << '\n';
+  }
+}
Index: lib/FrontendTool/ExecuteCompilerInvocation.cpp
===
--- lib/FrontendTool/ExecuteCompilerI

Re: r310449 - [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements

[Gábor Horváth via cfe-commits]

Sure!

There was a follow-up patch that excluded anonymous enums. Is it still that
bad to introduce it under a new flag?

Regards,
Gábor

On 9 August 2017 at 16:07, Nico Weber  wrote:

> Any way we could put this behind a different flag (say,
> -Wenum-compare-switch, in the same group as -Wenum-compare, so that
> -W(no-)enum-compare affects both)? Our codebase was -Wenum-compare clean
> before this commit but is very not clean now, so we'd now have to disable
> -Wenum-compare altogether, but then we won't catch regressions in
> non-switch statements either.
>
> On Wed, Aug 9, 2017 at 4:57 AM, Gabor Horvath via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: xazax
>> Date: Wed Aug  9 01:57:09 2017
>> New Revision: 310449
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310449&view=rev
>> Log:
>> [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch
>> statements
>>
>> Patch by: Reka Nikolett Kovacs
>>
>> Differential Revision: https://reviews.llvm.org/D36407
>>
>> Modified:
>> cfe/trunk/lib/Sema/SemaStmt.cpp
>> cfe/trunk/test/Sema/switch.c
>> cfe/trunk/test/SemaCXX/warn-enum-compare.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaStmt.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaS
>> tmt.cpp?rev=310449&r1=310448&r2=310449&view=diff
>> 
>> ==
>> --- cfe/trunk/lib/Sema/SemaStmt.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaStmt.cpp Wed Aug  9 01:57:09 2017
>> @@ -602,14 +602,14 @@ static bool EqEnumVals(const std::pair>
>>  /// GetTypeBeforeIntegralPromotion - Returns the pre-promotion type of
>>  /// potentially integral-promoted expression @p expr.
>> -static QualType GetTypeBeforeIntegralPromotion(Expr *&expr) {
>> -  if (ExprWithCleanups *cleanups = dyn_cast(expr))
>> -expr = cleanups->getSubExpr();
>> -  while (ImplicitCastExpr *impcast = dyn_cast(expr)) {
>> -if (impcast->getCastKind() != CK_IntegralCast) break;
>> -expr = impcast->getSubExpr();
>> +static QualType GetTypeBeforeIntegralPromotion(const Expr *&E) {
>> +  if (const auto *CleanUps = dyn_cast(E))
>> +E = CleanUps->getSubExpr();
>> +  while (const auto *ImpCast = dyn_cast(E)) {
>> +if (ImpCast->getCastKind() != CK_IntegralCast) break;
>> +E = ImpCast->getSubExpr();
>>}
>> -  return expr->getType();
>> +  return E->getType();
>>  }
>>
>>  ExprResult Sema::CheckSwitchCondition(SourceLocation SwitchLoc, Expr
>> *Cond) {
>> @@ -743,6 +743,24 @@ static bool ShouldDiagnoseSwitchCaseNotI
>>return true;
>>  }
>>
>> +static void checkEnumTypesInSwitchStmt(Sema &S, const Expr *Cond,
>> +   const Expr *Case) {
>> +  QualType CondType = GetTypeBeforeIntegralPromotion(Cond);
>> +  QualType CaseType = Case->getType();
>> +
>> +  const EnumType *CondEnumType = CondType->getAs();
>> +  const EnumType *CaseEnumType = CaseType->getAs();
>> +  if (!CondEnumType || !CaseEnumType)
>> +return;
>> +
>> +  if (S.Context.hasSameUnqualifiedType(CondType, CaseType))
>> +return;
>> +
>> +  S.Diag(Case->getExprLoc(), diag::warn_comparison_of_mixed_enum_types)
>> +  << CondType << CaseType << Cond->getSourceRange()
>> +  << Case->getSourceRange();
>> +}
>> +
>>  StmtResult
>>  Sema::ActOnFinishSwitchStmt(SourceLocation SwitchLoc, Stmt *Switch,
>>  Stmt *BodyStmt) {
>> @@ -760,7 +778,7 @@ Sema::ActOnFinishSwitchStmt(SourceLocati
>>
>>QualType CondType = CondExpr->getType();
>>
>> -  Expr *CondExprBeforePromotion = CondExpr;
>> +  const Expr *CondExprBeforePromotion = CondExpr;
>>QualType CondTypeBeforePromotion =
>>GetTypeBeforeIntegralPromotion(CondExprBeforePromotion);
>>
>> @@ -843,6 +861,8 @@ Sema::ActOnFinishSwitchStmt(SourceLocati
>>  break;
>>}
>>
>> +  checkEnumTypesInSwitchStmt(*this, CondExpr, Lo);
>> +
>>llvm::APSInt LoVal;
>>
>>if (getLangOpts().CPlusPlus11) {
>>
>> Modified: cfe/trunk/test/Sema/switch.c
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/swit
>> ch.c?rev=310449&r1=310448&r2=310449&view=diff
>> 
>> ==
>> --- cfe/trunk/test/Sema/switch.c (original)
>> +++ cfe/trunk/test/Sema/switch.c Wed Aug  9 01:57:09 2017
>> @@ -372,6 +372,7 @@ void switch_on_ExtendedEnum1(enum Extend
>>case EE1_b: break;
>>case EE1_c: break; // no-warning
>>case EE1_d: break; // expected-warning {{case value not in enumerated
>> type 'enum ExtendedEnum1'}}
>> +  // expected-warning@-1 {{comparison of two values with different
>> enumeration types ('enum ExtendedEnum1' and 'const enum
>> ExtendedEnum1_unrelated')}}
>>}
>>  }
>>
>>
>> Modified: cfe/trunk/test/SemaCXX/warn-enum-compare.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/
>> warn-enum-compare.cpp?rev=310449&r1=310448&r2=310449&view=diff
>> ==

Re: [clang-tools-extra] r310559 - [clang-tidy] Minor documentation improvement

[Gábor Horváth via cfe-commits]

On 10 August 2017 at 17:40, Alexander Kornienko  wrote:

>
>
> On Thu, Aug 10, 2017 at 11:13 AM, Gabor Horvath via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: xazax
>> Date: Thu Aug 10 02:13:26 2017
>> New Revision: 310559
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=310559&view=rev
>> Log:
>> [clang-tidy] Minor documentation improvement
>>
>> Patch by: Lilla Barancsuk
>>
>> Modified:
>> clang-tools-extra/trunk/docs/clang-tidy/checks/readability-s
>> tatic-accessed-through-instance.rst
>> clang-tools-extra/trunk/test/clang-tidy/readability-static-a
>> ccessed-through-instance.cpp
>>
>> Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/readability-s
>> tatic-accessed-through-instance.rst
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>> docs/clang-tidy/checks/readability-static-accessed-through-
>> instance.rst?rev=310559&r1=310558&r2=310559&view=diff
>> 
>> ==
>> --- clang-tools-extra/trunk/docs/clang-tidy/checks/readability-s
>> tatic-accessed-through-instance.rst (original)
>> +++ clang-tools-extra/trunk/docs/clang-tidy/checks/readability-s
>> tatic-accessed-through-instance.rst Thu Aug 10 02:13:26 2017
>> @@ -25,6 +25,7 @@ is changed to:
>>
>>  .. code-block:: c++
>>
>> +  C *c1 = new C();
>>C::foo();
>>C::x;
>>
>>
>> Modified: clang-tools-extra/trunk/test/clang-tidy/readability-static-a
>> ccessed-through-instance.cpp
>> URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/
>> test/clang-tidy/readability-static-accessed-through-
>> instance.cpp?rev=310559&r1=310558&r2=310559&view=diff
>> 
>> ==
>> --- 
>> clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
>> (original)
>> +++ 
>> clang-tools-extra/trunk/test/clang-tidy/readability-static-accessed-through-instance.cpp
>> Thu Aug 10 02:13:26 2017
>> @@ -116,7 +116,7 @@ using E = D;
>>
>>  template  void f(T t, C c) {
>>t.x; // OK, t is a template parameter.
>> -  c.x; // 1
>> +  c.x;
>>
>
> This comment was there to make the CHECK-FIXES line unique so that it
> doesn't match wrong line in the test. Any specific reason to remove it?
>

There was no specific reason, this seemed to be redundant, and reducing
readability. But I just noticed there is a C::x; line far above, so it
looks like this was not redundant at all. Should I add this back or you are
fine with leaving it as is for now?


>
>
>>// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member
>>// CHECK-FIXES: {{^}}  C::x; // 1{{$}}
>>  }
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Gábor Horváth via cfe-commits]

2018. márc. 1. 14:58 ezt írta ("Ilya Biryukov" ):

I replied to a commit in the wrong thread (https://reviews.llvm.org/rL326323),
sorry.
Here are the important bits:

This change introduced the following cyclic dependency in the build system:
StaticAnalyzerCore -> CrossTU -> Frontend -> StaticAnalyzerCore.


I do not see the Frontend -> StaticAnalyzerCore dependency upstream. See:
https://github.com/llvm-mirror/clang/blob/master/lib/Frontend/CMakeLists.txt

Do I miss something here?

Thanks in advance,
Gábor


I'm sorry, but I had to revert the commit in r326432. Cyclic dependency
broke our internal integrate (we use a different buildsystem, which breaks
on cyclic deps) and it's really messy to workaround it since CrossTU
depends on both Index and Frontend.
Moving the code that uses CrossTU from StaticAnalyzerCore to
StaticAnalyzerFrontend should probably fix it, but I don't have enough
context to come up with a fix.



On Thu, Mar 1, 2018 at 2:01 PM Aleksei Sidorin via Phabricator via
cfe-commits  wrote:

> a.sidorin reopened this revision.
> a.sidorin added a comment.
>
> The changes were reverted: http://llvm.org/viewvc/llvm-
> project?rev=326432&view=rev
> Gabor, could you take a look?
>
>
> Repository:
>   rC Clang
>
> https://reviews.llvm.org/D30691
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>


-- 
Regards,
Ilya Biryukov
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Gábor Horváth via cfe-commits]

I am away from my workstation so I would really appreciate if you could
recommit.

Thanks in advance,
Gábor

2018. márc. 1. 15:28 ezt írta ("Ilya Biryukov" ):

> You're right. We have this extra dependency in our internal build files
> for some reason and I missed that.
> It's totally my fault.
>
> Should I resubmit the patch that I reverted or you would rather do it
> yourself?
>
>
>
> On Thu, Mar 1, 2018 at 3:07 PM Gábor Horváth  wrote:
>
>>
>>
>> 2018. márc. 1. 14:58 ezt írta ("Ilya Biryukov" ):
>>
>> I replied to a commit in the wrong thread (https://reviews.llvm.org/
>> rL326323), sorry.
>> Here are the important bits:
>>
>> This change introduced the following cyclic dependency in the build
>> system: StaticAnalyzerCore -> CrossTU -> Frontend -> StaticAnalyzerCore.
>>
>>
>> I do not see the Frontend -> StaticAnalyzerCore dependency upstream. See:
>> https://github.com/llvm-mirror/clang/blob/master/lib/
>> Frontend/CMakeLists.txt
>>
>> Do I miss something here?
>>
>> Thanks in advance,
>> Gábor
>>
>>
>> I'm sorry, but I had to revert the commit in r326432. Cyclic dependency
>> broke our internal integrate (we use a different buildsystem, which breaks
>> on cyclic deps) and it's really messy to workaround it since CrossTU
>> depends on both Index and Frontend.
>> Moving the code that uses CrossTU from StaticAnalyzerCore to
>> StaticAnalyzerFrontend should probably fix it, but I don't have enough
>> context to come up with a fix.
>>
>>
>>
>> On Thu, Mar 1, 2018 at 2:01 PM Aleksei Sidorin via Phabricator via
>> cfe-commits  wrote:
>>
>>> a.sidorin reopened this revision.
>>> a.sidorin added a comment.
>>>
>>> The changes were reverted: http://llvm.org/viewvc/llvm-
>>> project?rev=326432&view=rev
>>> Gabor, could you take a look?
>>>
>>>
>>> Repository:
>>>   rC Clang
>>>
>>> https://reviews.llvm.org/D30691
>>>
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11427: [Static Analyzer] Checker for Obj-C generics

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Addressed the suggestions.



Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:257
@@ +256,3 @@
+// However a downcast may also lose information. E. g.:
+//   MutableMap : Map
+// The downcast to mutable map loses the information about the types of the

zaks.anna wrote:
> Should the Map be specialized in this example?
No, this case handles buggy implementations, when the type parameters are not 
forwarded. I reworded the comments to make this more clear.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:297
@@ +296,3 @@
+reportBug(*TrackedType, DestObjectPtrType, N, Sym, C);
+return;
+  }

zaks.anna wrote:
> This will not get caught by the check below?
You are right, I refactored the code.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:326
@@ +325,3 @@
+static const Expr *stripImplicitIdCast(const Expr *E, ASTContext &ASTCtxt) {
+  const ImplicitCastExpr *CE = dyn_cast(E);
+  if (CE && CE->getCastKind() == CK_BitCast &&

zaks.anna wrote:
> Shouldn't we strip more than id casts?
We should. And we also should strip some sugar. I rewrote that function.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:425
@@ +424,3 @@
+ ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType,
+ *TrackedType))) {
+  const ObjCInterfaceDecl *InterfaceDecl =

zaks.anna wrote:
> What if ASTCtxt.canAssignObjCInterfaces is false here? Shouldn't we warn?
> Will we warn elsewhere? Let's make sure we have a test case. And add a 
> comment.
The tracked type might be the super class of the static type (for example when 
the super type is specialized but the subtype is not). We should not warn in 
that case. When the tracked type is not in subtyping relationship with the 
static type, there was a cast somewhere, and the checker issued the warning 
there. I already have testcase for these cases. I updated the names of the 
tests to reflect this. 


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:444
@@ +443,3 @@
+  Optional> TypeArgs =
+  (*TrackedType)->getObjCSubstitutions(Method->getDeclContext());
+  // This case might happen when there is an unspecialized override of a

zaks.anna wrote:
> Should we use the type on which this is defined and not the tracked type?
The type arguments might be only available for the tracked type.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:453
@@ +452,3 @@
+// We can't do any type-checking on a type-dependent argument.
+if (Arg->isTypeDependent())
+  continue;

zaks.anna wrote:
> Why are we not checking other parameter types? Will those bugs get caught by 
> casts? I guess yes..
The analyzer does not visit template definitions, only the instantiations. I 
removed this redundant check.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:495
@@ +494,3 @@
+// accepted.
+if (!ASTCtxt.canAssignObjCInterfaces(ParamObjectPtrType,
+ ArgObjectPtrType) &&

zaks.anna wrote:
> This would be simplified if you pull out the negation; you essentially, list 
> the cases where we do not warn on parameters:
>  1) arg can be assigned to (subtype of) param
>  2) covariant parameter types and param is a subtype of arg
The check there was overcomplicated. Passing a subtype of a parameter as 
argument should always be safe.


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:513
@@ +512,3 @@
+  ASTCtxt, *TypeArgs, ObjCSubstitutionContext::Result);
+  if (IsInstanceType)
+ResultType = QualType(*TrackedType, 0);

zaks.anna wrote:
> We should not use TrackedType in the case lookup failed.
> Can the TrackedType be less specialized that the return type?
> Maybe rename as IsDeclaredAsInstanceType?
The lookup might fail when the tracked type is the super of the dynamic type 
(which might be the subtype of the static type). In some cases the tracked type 
might even end up be the super type of the static type (when that super type 
has more information about the type parameters.) 


Comment at: lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp:532
@@ +531,3 @@
+
+  if (!ExprTypeAboveCast || !ResultPtrType)
+return;

zaks.anna wrote:
> What are we doing here?
> I prefer not to have any AST pattern matching.
Right now in order to keep the state as low as possible, only generic types are 
tracked. If it is not a problem to have bigger state, a future improvement 
would be to track the dynamic type of every object, and utilize that info in 
this checker. Alternatively that could utilize the get/setDynamicTypeInfo API 
of ProgramState.

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

The problem here is that, this checker can emit a warning for two cases:
1, Null was bound to a reference which should be reported as a dereference
2, Null was passed to parameter that should be non-null (marked by the 
attribute, not the qualifier) which should be reported appropriately

Right now exactly the same event will be triggered for both cases, so the 
checkers that process this event has no information whether it is a dereference 
or a value passed to a nonnull parameter, so it can not provide appropriate 
diagnostic for both cases.

One possibility would be to have two separate events, the other is to have an 
argument to an event that determines its origin. Probably it would be better to 
have two separate events, since it might be confusing to have dereference in 
the name of the event in the second case.

What do you think?


http://reviews.llvm.org/D11433



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r244416 - [modules] PR22534: Load files specified by -fmodule-file= eagerly. In particular, this avoids the need to re-parse module map files when using such a module.

[Gábor Horváth via cfe-commits]

Sorry, I sent the original message to the wrong list.

-- Forwarded message --
From: Gábor Horváth 
Date: 18 August 2015 at 12:59
Subject: Re: r244416 - [modules] PR22534: Load files specified by
-fmodule-file= eagerly. In particular, this avoids the need to re-parse
module map files when using such a module.
To: Richard Smith 
Cc: "cfe-comm...@cs.uiuc.edu" 


Hi!

In r244416 you made createModuleManager to call the Initialize method of
the ASTConsumer.
Because of this change the ASTConsumer::Initialize might be called twice in
some scenarios.

This change makes the Static Analyzer crash (use after free) in those
scenarios. I think most of the ASTConsumers was not written with that in
mind Initialize might be called twice for the object. This fact is also not
covered by the documentation.

In my opinion we should either guarantee that the Initialize method will be
called only once during the lifetime of an ASTConsumer, or document the
fact that, it might be called multiple times and notify the authors of the
different consumers to update their classes accordingly (announcement on
the mailing list?).

What do you think?

Regards,
Gabor Horvath
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r244416 - [modules] PR22534: Load files specified by -fmodule-file= eagerly. In particular, this avoids the need to re-parse module map files when using such a module.

[Gábor Horváth via cfe-commits]

On 18 August 2015 at 14:46, Richard Smith  wrote:

> On Tue, Aug 18, 2015 at 1:52 PM, Gábor Horváth 
> wrote:
>
>> On 18 August 2015 at 13:41, Richard Smith  wrote:
>>
>>> On Tue, Aug 18, 2015 at 12:59 PM, Gábor Horváth 
>>> wrote:
>>>
 Hi!

 In r244416 you made createModuleManager to call the Initialize method
 of the ASTConsumer.
 Because of this change the ASTConsumer::Initialize might be called
 twice in some scenarios.

 This change makes the Static Analyzer crash (use after free) in those
 scenarios. I think most of the ASTConsumers was not written with that in
 mind Initialize might be called twice for the object. This fact is also not
 covered by the documentation.

 In my opinion we should either guarantee that the Initialize method
 will be called only once during the lifetime of an ASTConsumer, or document
 the fact that, it might be called multiple times and notify the authors of
 the different consumers to update their classes accordingly (announcement
 on the mailing list?).

 What do you think?

>>>
>>> Fixed in r245346.
>>>
>>> It seems like an indicator of poor design that we have the initialize
>>> function at all. I don't think there is any situation where we need or want
>>> to create an ASTConsumer before we have enough information to create the
>>> ASTContext, so perhaps we can just get rid of this function.
>>>
>>
>> Thank you for fixing this!
>>
>> I agree that it would be awesome to get rid of that function.
>>
>
> CCing the right list again :)
>
> Are you interested in looking into this? (If not, it's going to near the
> bottom of a long list of TODO items for me...)
>

Hope to CC the right list this time :)

Well, it would end up on the bottom of my TODO list as well, so maybe we
should both add it, and the one who gets there earlier should do this.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12123: [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Looks good to me. There are some minor nits inline.



Comment at: include/clang/StaticAnalyzer/Core/CheckerManager.h:96
@@ -95,1 +95,3 @@
 
+enum class ObjCCheckerKind {
+  PreVisit,

I do not really like the name ObjCCheckerKind, because it is not kind of an 
Obj-C related checker. It is the kind of the visit of the message expression. 
Maybe ObjCMessageVisitKind? Or just MessageVisitKind to be a bit shorter?


Comment at: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp:153
@@ +152,3 @@
+  ProgramStateRef notNilState, nilState;
+  std::tie(notNilState, nilState) = State->assume(receiverVal);
+  if (nilState && !notNilState) {

The old code had a comment about merging two cases and a reference to a rdar. 
Is that rdar already fixed? Maybe it would be good to preserve the at least the 
first part of the commend?


http://reviews.llvm.org/D12123



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12123: [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Core/CheckerManager.cpp:237
@@ +236,3 @@
+return PreObjCMessageCheckers;
+break;
+  case ObjCMessageVisitKind::Post:

nit: remove the break after the return.


Comment at: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp:197
@@ +196,3 @@
+  // Generate a transition to non-Nil state, dropping any potential
+  // non-nil flow.
+  if (notNilState != State) {

Aren't we dropping the nil flow here instead of the non-nil? If that's the 
case, the comment should reflect that.


http://reviews.llvm.org/D12123



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 32854.
xazax.hun added a comment.

Only send implicit dereference events, when the null pointer was bound to a 
reference.


http://reviews.llvm.org/D11433

Files:
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp

Index: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -28,7 +28,7 @@
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall > {
+  : public Checker< check::PreCall, EventDispatcher > {
   mutable std::unique_ptr BTAttrNonNull;
   mutable std::unique_ptr BTNullRefArg;
 
@@ -139,26 +139,33 @@
 ProgramStateRef stateNotNull, stateNull;
 std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-if (stateNull && !stateNotNull) {
-  // Generate an error node.  Check for a null node in case
-  // we cache out.
-  if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
+if (stateNull) {
+  if (!stateNotNull){
+// Generate an error node.  Check for a null node in case
+// we cache out.
+if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
 
-std::unique_ptr R;
-if (haveAttrNonNull)
-  R = genReportNullAttrNonNull(errorNode, ArgE);
-else if (haveRefTypeParam)
-  R = genReportReferenceToNullPointer(errorNode, ArgE);
+  std::unique_ptr R;
+  if (haveAttrNonNull)
+R = genReportNullAttrNonNull(errorNode, ArgE);
+  else if (haveRefTypeParam)
+R = genReportReferenceToNullPointer(errorNode, ArgE);
 
-// Highlight the range of the argument that was null.
-R->addRange(Call.getArgSourceRange(idx));
+  // Highlight the range of the argument that was null.
+  R->addRange(Call.getArgSourceRange(idx));
 
-// Emit the bug report.
-C.emitReport(std::move(R));
-  }
+  // Emit the bug report.
+  C.emitReport(std::move(R));
+}
 
-  // Always return.  Either we cached out or we just emitted an error.
-  return;
+// Always return.  Either we cached out or we just emitted an error.
+return;
+  }
+  ExplodedNode *N = C.generateSink(stateNull);
+  if (haveRefTypeParam && N) {
+ImplicitNullDerefEvent event = { V, false, N, &C.getBugReporter() };
+dispatchEvent(event);
+  }
 }
 
 // If a pointer value passed the check we should assume that it is


Index: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -28,7 +28,7 @@
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall > {
+  : public Checker< check::PreCall, EventDispatcher > {
   mutable std::unique_ptr BTAttrNonNull;
   mutable std::unique_ptr BTNullRefArg;
 
@@ -139,26 +139,33 @@
 ProgramStateRef stateNotNull, stateNull;
 std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-if (stateNull && !stateNotNull) {
-  // Generate an error node.  Check for a null node in case
-  // we cache out.
-  if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
+if (stateNull) {
+  if (!stateNotNull){
+// Generate an error node.  Check for a null node in case
+// we cache out.
+if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
 
-std::unique_ptr R;
-if (haveAttrNonNull)
-  R = genReportNullAttrNonNull(errorNode, ArgE);
-else if (haveRefTypeParam)
-  R = genReportReferenceToNullPointer(errorNode, ArgE);
+  std::unique_ptr R;
+  if (haveAttrNonNull)
+R = genReportNullAttrNonNull(errorNode, ArgE);
+  else if (haveRefTypeParam)
+R = genReportReferenceToNullPointer(errorNode, ArgE);
 
-// Highlight the range of the argument that was null.
-R->addRange(Call.getArgSourceRange(idx));
+  // Highlight the range of the argument that was null.
+  R->addRange(Call.getArgSourceRange(idx));
 
-// Emit the bug report.
-C.emitReport(std::move(R));
-  }
+  // Emit the bug report.
+  C.emitReport(std::move(R));
+}
 
-  // Always return.  Either we cached out or we just emitted an error.
-  return;
+// Always return.  Either we cached out or we just emitted an error.
+return;
+  }
+  ExplodedNode *N = C.generateSink(stateNull);
+  if (haveRefTypeParam && N) {
+ImplicitNullDerefEvent event = { V, false, N, &C.getBugReporter() };
+dispatchEvent(event);
+  }
 }
 
 // If a pointer value passed the check we should assume that it is

Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 32869.
xazax.hun added a comment.

- Updated to the latest trunk.
- Relaxed an assert in ExprEngine which turned out to be unsound.
- The individual checks can be turned on or off.
- Added some framework specific heuristic to reduce the number of false 
positive results.
- Refactoring.


http://reviews.llvm.org/D11468

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- /dev/null
+++ test/Analysis/nullability.mm
@@ -0,0 +1,170 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+
+#define nil 0
+#define BOOL int
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+@end
+
+@protocol NSCopying
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObject
+@end
+
+@interface NSString : NSObject
+- (BOOL)isEqualToString : (NSString *_Nonnull)aString;
+- (NSString *)stringByAppendingString:(NSString *_Nonnull)aString;
+@end
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template  T *eraseNullab(T *p) { return p; }
+
+void testBasicRules() {
+  Dummy *p = returnsNullable();
+  int *ptr = returnsNullableInt();
+  // Make every dereference a different path to avoid sinks after errors.
+  switch (getRandom()) {
+  case 0: {
+Dummy &r = *p; // expected-warning {{}}
+  } break;
+  case 1: {
+int b = p->val; // expected-warning {{}}
+  } break;
+  case 2: {
+int stuff = *ptr; // expected-warning {{}}
+  } break;
+  case 3:
+takesNonnull(p); // expected-warning {{}}
+break;
+  case 4: {
+Dummy d;
+takesNullable(&d);
+Dummy dd(d);
+break;
+  }
+  // Here the copy constructor is called, so a reference is initialized with the
+  // value of p. No ImplicitNullDereference event will be dispatched for this
+  // case. A followup patch is expected to fix this in NonNullParamChecker.
+  default: { Dummy d = *p; } break; // No warning.
+  }
+  if (p) {
+takesNonnull(p);
+if (getRandom()) {
+  Dummy &r = *p;
+} else {
+  int b = p->val;
+}
+  }
+  Dummy *q = 0;
+  if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+  }
+  Dummy a;
+  Dummy *_Nonnull nonnull = &a;
+  nonnull = q; // expected-warning {{}}
+  q = &a;
+  takesNullable(q);
+  takesNonnull(q);
+}
+
+void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  Dummy *p = nullable;
+  nonnull = p; // expected-warning {{}}
+  p = 0;
+  Dummy *q = nonnull;
+  q = p;
+}
+
+Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
+  Dummy *p = a;
+  return p; // expected-warning {{}}
+}
+
+Dummy *_Nonnull testNullReturn() {
+  Dummy *p = 0;
+  return p; // expected-warning {{}}
+}
+
+void testObjCMessageResultNullability() {
+  // The expected result: the most nullable of self and method return type.
+  TestObject *o = getUnspecifiedTestObject();
+  int *shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNonnull];
+  switch (getRandom()) {
+  case 0:
+// The core analyzer assumes that the receiver is non-null after a message
+// send. This is to avoid some false positives, and increase performance
+// but it also reduces the coverage and makes this checker unable to reason
+// about the nullness of the receiver. 
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+  case 1:
+shouldBeNullable =
+[eraseNullab(getNullableTestObject()) returnsUnspecified];
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+  case 3:
+shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNullable];
+[o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+break;
+  case 4:
+shouldBeNullable = [eraseNullab(getNonnullTestObject()) returnsNullable];
+[o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+break;
+  case 5:
+shouldBeNullable =
+[eraseNullab(getUnspecifiedTestObject()) returnsNullable];
+[o takesNonnull:shouldBeNullable]; // expected-warning {{}}
+break;
+  case

Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun marked 11 inline comments as done.
xazax.hun added a comment.

Thank you for the review. I will upload the new version of the patch once the 
rest of it was reviewed.



Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:12
@@ +11,3 @@
+// checker is that, the user is running this checker after all the nullability
+// warnings that is emitted by the compiler was fixed.
+//

zaks.anna wrote:
> "is" -> "are"
> 
> How are we relying on this assumption? What happens if they are not fixed?
> 
> Also we should describe what we mean by nullability warnings, maybe refer to 
> the annotations themselves here? It would be great to give a high level 
> overflow of the algorithm and maybe even talk about the difference between 
> the checks?
There are no assumptions like that in the code of the checker right now. I 
updated the comment to reflect that.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:54
@@ +53,3 @@
+  return static_cast(
+  std::min(static_cast(Lhs), static_cast(Rhs)));
+}

zaks.anna wrote:
> ??
Added a comment to clarify what it is and how it is used.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:192
@@ +191,3 @@
+
+static bool shouldTrackRegion(const MemRegion *Region,
+  AnalysisDeclContext *DeclContext) {

zaks.anna wrote:
> Is this used for optimization purposes? Can you describe the rules we use 
> here?
> 
> What's the benefit of not tracking self?
Initially it was not just an optimization. It was also intended to deal with 
the caching out issue, but eventually it turned out that caching out was ok, 
and not an error in the checker. I did not erase the code afterwards.

There are several possibilities now:
* Leave this part as it is.
* Only track symbolic regions.
* Delete this function.

Which one do you prefer?



Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:267
@@ +266,3 @@
+return Nullability::Nonnull;
+  return Nullability::Unspecified;
+}

zaks.anna wrote:
> shouldn't this be an llvm_unreachable?
There are several AttrKind.
Source: 
http://clang.llvm.org/doxygen/classclang_1_1AttributedType.html#ab4901f7a37f5539698cb5ebd706245ed

So that code is not unreachable.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:298
@@ +297,3 @@
+TrackedNullability = State->get(
+Region->getAs()->getSuperRegion());
+if (!TrackedNullability)

zaks.anna wrote:
> Are we sure that if "!TrackedNullability", the event complains about a 
> dereference on the parent (like field access)?
Sometimes when there is an expression like p[0] or p->field, the event contains 
the region for the element or the field region. However the tracked information 
is only available for p. For this reason I also check whether the super region 
has some tracked nullability and complain about that region in this case.


http://reviews.llvm.org/D11468



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D12123: [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

[Gábor Horváth via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

Looks good to me.


http://reviews.llvm.org/D12123



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 33151.
xazax.hun marked 30 inline comments as done.
xazax.hun added a comment.

Addressed the comments.
Added some more tests.
Added a design document.


http://reviews.llvm.org/D11468

Files:
  docs/analyzer/nullability.rst
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- /dev/null
+++ test/Analysis/nullability.mm
@@ -0,0 +1,181 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+
+#define nil 0
+#define BOOL int
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+@end
+
+@protocol NSCopying
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObject
+@end
+
+@interface NSString : NSObject
+- (BOOL)isEqualToString : (NSString *_Nonnull)aString;
+- (NSString *)stringByAppendingString:(NSString *_Nonnull)aString;
+@end
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template  T *eraseNullab(T *p) { return p; }
+
+void testBasicRules() {
+  Dummy *p = returnsNullable();
+  int *ptr = returnsNullableInt();
+  // Make every dereference a different path to avoid sinks after errors.
+  switch (getRandom()) {
+  case 0: {
+Dummy &r = *p; // expected-warning {{}}
+  } break;
+  case 1: {
+int b = p->val; // expected-warning {{}}
+  } break;
+  case 2: {
+int stuff = *ptr; // expected-warning {{}}
+  } break;
+  case 3:
+takesNonnull(p); // expected-warning {{}}
+break;
+  case 4: {
+Dummy d;
+takesNullable(&d);
+Dummy dd(d);
+break;
+  }
+  // Here the copy constructor is called, so a reference is initialized with the
+  // value of p. No ImplicitNullDereference event will be dispatched for this
+  // case. A followup patch is expected to fix this in NonNullParamChecker.
+  default: { Dummy d = *p; } break; // No warning.
+  }
+  if (p) {
+takesNonnull(p);
+if (getRandom()) {
+  Dummy &r = *p;
+} else {
+  int b = p->val;
+}
+  }
+  Dummy *q = 0;
+  if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+  }
+  Dummy a;
+  Dummy *_Nonnull nonnull = &a;
+  nonnull = q; // expected-warning {{}}
+  q = &a;
+  takesNullable(q);
+  takesNonnull(q);
+}
+
+void testMultiParamChecking(Dummy *_Nonnull a, Dummy *_Nullable b,
+Dummy *_Nonnull c);
+
+void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  Dummy *p = nullable;
+  Dummy *q = nonnull;
+  switch(getRandom()) {
+  case 1: nonnull = p; break; // expected-warning {{}}
+  case 2: p = 0; break;
+  case 3: q = p; break;
+  case 4: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}}
+  case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}}
+  case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}}
+  case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
+Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
+  Dummy *p = a;
+  return p; // expected-warning {{}}
+}
+
+Dummy *_Nonnull testNullReturn() {
+  Dummy *p = 0;
+  return p; // expected-warning {{}}
+}
+
+void testObjCMessageResultNullability() {
+  // The expected result: the most nullable of self and method return type.
+  TestObject *o = getUnspecifiedTestObject();
+  int *shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNonnull];
+  switch (getRandom()) {
+  case 0:
+// The core analyzer assumes that the receiver is non-null after a message
+// send. This is to avoid some false positives, and increase performance
+// but it also reduces the coverage and makes this checker unable to reason
+// about the nullness of the receiver. 
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+  case 1:
+shouldBeNullable =
+[eraseNullab(getNullableTestObject()) returnsUnspecified];
+[o takesNonnull:shouldBeNullable]; // No warning exp

Re: [PATCH] D11468: [Static Analyzer] The first implementation of nullability checker.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 33239.
xazax.hun added a comment.

- Fine tuned the heuristics for cocoa APIs.
- Only track "nullable" and "contradicted" nullability for symbols. Use the 
rest statically.
- Cleanups.


http://reviews.llvm.org/D11468

Files:
  docs/analyzer/nullability.rst
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- /dev/null
+++ test/Analysis/nullability.mm
@@ -0,0 +1,181 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.nullability -verify %s
+
+#define nil 0
+#define BOOL int
+
+@protocol NSObject
++ (id)alloc;
+- (id)init;
+@end
+
+@protocol NSCopying
+@end
+
+__attribute__((objc_root_class))
+@interface
+NSObject
+@end
+
+@interface NSString : NSObject
+- (BOOL)isEqualToString : (NSString *_Nonnull)aString;
+- (NSString *)stringByAppendingString:(NSString *_Nonnull)aString;
+@end
+
+@interface TestObject : NSObject
+- (int *_Nonnull)returnsNonnull;
+- (int *_Nullable)returnsNullable;
+- (int *)returnsUnspecified;
+- (void)takesNonnull:(int *_Nonnull)p;
+- (void)takesNullable:(int *_Nullable)p;
+- (void)takesUnspecified:(int *)p;
+@property(readonly, strong) NSString *stuff;
+@end
+
+TestObject * getUnspecifiedTestObject();
+TestObject *_Nonnull getNonnullTestObject();
+TestObject *_Nullable getNullableTestObject();
+
+int getRandom();
+
+typedef struct Dummy { int val; } Dummy;
+
+void takesNullable(Dummy *_Nullable);
+void takesNonnull(Dummy *_Nonnull);
+void takesUnspecified(Dummy *);
+
+Dummy *_Nullable returnsNullable();
+Dummy *_Nonnull returnsNonnull();
+Dummy *returnsUnspecified();
+int *_Nullable returnsNullableInt();
+
+template  T *eraseNullab(T *p) { return p; }
+
+void testBasicRules() {
+  Dummy *p = returnsNullable();
+  int *ptr = returnsNullableInt();
+  // Make every dereference a different path to avoid sinks after errors.
+  switch (getRandom()) {
+  case 0: {
+Dummy &r = *p; // expected-warning {{}}
+  } break;
+  case 1: {
+int b = p->val; // expected-warning {{}}
+  } break;
+  case 2: {
+int stuff = *ptr; // expected-warning {{}}
+  } break;
+  case 3:
+takesNonnull(p); // expected-warning {{}}
+break;
+  case 4: {
+Dummy d;
+takesNullable(&d);
+Dummy dd(d);
+break;
+  }
+  // Here the copy constructor is called, so a reference is initialized with the
+  // value of p. No ImplicitNullDereference event will be dispatched for this
+  // case. A followup patch is expected to fix this in NonNullParamChecker.
+  default: { Dummy d = *p; } break; // No warning.
+  }
+  if (p) {
+takesNonnull(p);
+if (getRandom()) {
+  Dummy &r = *p;
+} else {
+  int b = p->val;
+}
+  }
+  Dummy *q = 0;
+  if (getRandom()) {
+takesNullable(q);
+takesNonnull(q); // expected-warning {{}}
+  }
+  Dummy a;
+  Dummy *_Nonnull nonnull = &a;
+  nonnull = q; // expected-warning {{}}
+  q = &a;
+  takesNullable(q);
+  takesNonnull(q);
+}
+
+void testMultiParamChecking(Dummy *_Nonnull a, Dummy *_Nullable b,
+Dummy *_Nonnull c);
+
+void testArgumentTracking(Dummy *_Nonnull nonnull, Dummy *_Nullable nullable) {
+  Dummy *p = nullable;
+  Dummy *q = nonnull;
+  switch(getRandom()) {
+  case 1: nonnull = p; break; // expected-warning {{}}
+  case 2: p = 0; break;
+  case 3: q = p; break;
+  case 4: testMultiParamChecking(nonnull, nullable, nonnull); break;
+  case 5: testMultiParamChecking(nonnull, nonnull, nonnull); break;
+  case 6: testMultiParamChecking(nonnull, nullable, nullable); break; // expected-warning {{}}
+  case 7: testMultiParamChecking(nullable, nullable, nonnull); // expected-warning {{}}
+  case 8: testMultiParamChecking(nullable, nullable, nullable); // expected-warning {{}}
+  case 9: testMultiParamChecking((Dummy *_Nonnull)0, nullable, nonnull); break;
+  }
+}
+
+Dummy *_Nonnull testNullableReturn(Dummy *_Nullable a) {
+  Dummy *p = a;
+  return p; // expected-warning {{}}
+}
+
+Dummy *_Nonnull testNullReturn() {
+  Dummy *p = 0;
+  return p; // expected-warning {{}}
+}
+
+void testObjCMessageResultNullability() {
+  // The expected result: the most nullable of self and method return type.
+  TestObject *o = getUnspecifiedTestObject();
+  int *shouldBeNullable = [eraseNullab(getNullableTestObject()) returnsNonnull];
+  switch (getRandom()) {
+  case 0:
+// The core analyzer assumes that the receiver is non-null after a message
+// send. This is to avoid some false positives, and increase performance
+// but it also reduces the coverage and makes this checker unable to reason
+// about the nullness of the receiver. 
+[o takesNonnull:shouldBeNullable]; // No warning expected.
+break;
+  case 1:
+shouldBeNullable =
+[eraseNullab(getNullableTestObject()) returnsUnspecified];
+[o takesNonnull:sho

[PATCH] D12381: [Static Analyzer] Merge the Objective-C Generics Checker into Dynamic Type Propagation checker.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: krememek, zaks.anna, jordan_rose, dcoughlin.
xazax.hun added a subscriber: cfe-commits.

This patch merged the functionality from ObjCGenericsChecker into 
DynamicTypePropagation checker.
Note that the Generics Checker can still be turned on or off individually but 
this only affects whether diagnostics are generated or not.
There is no intended functional change in this patch.

Rationale:
This is a refactoring that makes some collaboration between the 
ObjCGenericsChecker and DynamicTypePropagation checker possible.
The final goal (which will be achieved by some followup patches) is to use the 
reasoning of ObjCGenericsChecker about generics to infer dynamic type for 
objects.

Lets consider the following scenario:
id object = arrayOfStrings.firstObject;

Here the DynamicTypePropagation checker can not infer any dynamic type 
information because the static type of the arrayOfStrings.firstObject 
expression is id when arrayOfStrings is not annotated. However the generics 
checker might be able to infer an upper bound (NSString *) for the same 
expression from the usage of the symbol. 

In a follow up patch when the DynamicTypePropagation checker fails to infer the 
dynamic type for an object, we would fall back to the inference of the generics 
checker, because it may have additional information.

Impact:

When an exact type is inferred as a dynamic type (this happens when the 
allocation was visible by the analyzer), method calls on that object will be 
"devirtualized" (inlined).
When the allocation is not visible but an upper bound can be inferred, there 
will be a path split on method calls. On one path the method will be inlined 
(when a body is available) on the other, there will be no inlining. 
There are some heuristic cases, where an upper bound is treated as an exact 
type.

The expected effect of the follow up patch is that, upper bound can be inferred 
more frequently. Due to cross translation unit limits, this might not change 
the inlining behavior significantly. However there are other advantages. 
Utilizing this dynamic type information, a generic type checker could be 
implemented trivially which could catch errors like:

id object = myNSNumber;
NSString *myString = object;

Why not keep the Generics checker and the dynamic type propagation as 
completely separate checks?

One of the problem is that, right now both checkers infer type information from 
casts. In order to be able to fallback to the type inference of Generics 
checker when the type inference of dynamic type propagation failed, the 
Generics checker should assume that the dynamic type propagation is done 
already by the time its callback is invoked. Since there is no way to specify 
ordering between the checkers right now, the only way to do it correctly is to 
merge two checks into one checker.

What do you think?

http://reviews.llvm.org/D12381

Files:
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/ObjCGenericsChecker.cpp
  test/Analysis/generics.m

Index: test/Analysis/generics.m
===
--- test/Analysis/generics.m
+++ test/Analysis/generics.m
@@ -418,7 +418,7 @@
 // CHECK:descriptionIncompatible pointer types assigning to 'NSArray *' from 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash2
@@ -528,7 +528,7 @@
 // CHECK:descriptionIncompatible pointer types assigning to 'NSString *' from 'NSNumber *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash3
@@ -672,7 +672,7 @@
 // CHECK:descriptionIncompatible pointer types assigning to 'NSArray *' from 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_contextincompatibleTypesErased
 // CHECK:   issue_hash5
@@ -922,7 +922,7 @@
 // CHECK:descriptionIncompatible pointer types assigning to 'NSArray *' from 'NSArray *'
 // CHECK:categoryCore Foundation/Objective-C
 // CHECK:typeGenerics
-// CHECK:check_namealpha.osx.cocoa.ObjCGenerics
+// CHECK:check_namecore.DynamicTypePropagation
 // CHECK:   issue_context_kindfunction
 // CHECK:   issue_context

Re: [PATCH] D12358: [Analyzer] Handling constant bound loops

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

A conservative solution should work.

But if we want to preserve some precision I think it should be possible to not 
to invalidate those values on the stack that are not effected by the loop. The 
hard problem here is that, it is impossible to reason about the heap without 
global knowledge, so if there is a pointer on the heap to a local variable that 
should be invalidated as well.

What do you think?


http://reviews.llvm.org/D12358



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11433: [Static Analyzer] Make NonNullParamChecker emit implicit null dereference events.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 5.
xazax.hun added a comment.

- Updated to latest trunk.
- Added a new field to the event with documentation.
- Rebased the patch on the top of nullability checker.
- Added covered cases to the nullability checker tests.


http://reviews.llvm.org/D11433

Files:
  include/clang/StaticAnalyzer/Core/Checker.h
  lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -50,6 +50,8 @@
 
 template  T *eraseNullab(T *p) { return p; }
 
+void takesAttrNonnull(Dummy *p) __attribute((nonnull(1)));
+
 void testBasicRules() {
   Dummy *p = returnsNullable();
   int *ptr = returnsNullableInt();
@@ -73,10 +75,8 @@
 Dummy dd(d);
 break;
   }
-  // Here the copy constructor is called, so a reference is initialized with the
-  // value of p. No ImplicitNullDereference event will be dispatched for this
-  // case. A followup patch is expected to fix this in NonNullParamChecker.
-  default: { Dummy d = *p; } break; // No warning.
+  case 5: takesAttrNonnull(p); break; // expected-warning {{Nullable pointer is passed to}}
+  default: { Dummy d = *p; } break; // expected-warning {{Nullable pointer is dereferenced}}
   }
   if (p) {
 takesNonnull(p);
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -335,7 +335,10 @@
   if (Filter.CheckNullableDereferenced &&
   TrackedNullability->getValue() == Nullability::Nullable) {
 BugReporter &BR = *Event.BR;
-reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+if (Event.IsDirectDereference)
+  reportBug(ErrorKind::NullableDereferenced, Event.SinkNode, Region, BR);
+else
+  reportBug(ErrorKind::NullablePassedToNonnull, Event.SinkNode, Region, BR);
   }
 }
 
Index: lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -28,7 +28,7 @@
 
 namespace {
 class NonNullParamChecker
-  : public Checker< check::PreCall > {
+  : public Checker< check::PreCall, EventDispatcher > {
   mutable std::unique_ptr BTAttrNonNull;
   mutable std::unique_ptr BTNullRefArg;
 
@@ -139,26 +139,34 @@
 ProgramStateRef stateNotNull, stateNull;
 std::tie(stateNotNull, stateNull) = CM.assumeDual(state, *DV);
 
-if (stateNull && !stateNotNull) {
-  // Generate an error node.  Check for a null node in case
-  // we cache out.
-  if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
+if (stateNull) {
+  if (!stateNotNull) {
+// Generate an error node.  Check for a null node in case
+// we cache out.
+if (ExplodedNode *errorNode = C.generateSink(stateNull)) {
 
-std::unique_ptr R;
-if (haveAttrNonNull)
-  R = genReportNullAttrNonNull(errorNode, ArgE);
-else if (haveRefTypeParam)
-  R = genReportReferenceToNullPointer(errorNode, ArgE);
+  std::unique_ptr R;
+  if (haveAttrNonNull)
+R = genReportNullAttrNonNull(errorNode, ArgE);
+  else if (haveRefTypeParam)
+R = genReportReferenceToNullPointer(errorNode, ArgE);
 
-// Highlight the range of the argument that was null.
-R->addRange(Call.getArgSourceRange(idx));
+  // Highlight the range of the argument that was null.
+  R->addRange(Call.getArgSourceRange(idx));
 
-// Emit the bug report.
-C.emitReport(std::move(R));
-  }
+  // Emit the bug report.
+  C.emitReport(std::move(R));
+}
 
-  // Always return.  Either we cached out or we just emitted an error.
-  return;
+// Always return.  Either we cached out or we just emitted an error.
+return;
+  }
+  if (ExplodedNode *N = C.generateSink(stateNull)) {
+ImplicitNullDerefEvent event = {
+V, false, N, &C.getBugReporter(),
+/*IsDirectDereference=*/haveRefTypeParam};
+dispatchEvent(event);
+  }
 }
 
 // If a pointer value passed the check we should assume that it is
Index: lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -220,7 +220,8 @@
 // null or not-null.  Record the error node as an "implicit" null
 // dereference.
 if (ExplodedNode *N = C.generateSink(nullState)) {
-

Re: [PATCH] D12163: [Patch] [Analyzer] BugReporter.cpp:2869: Assertion failed: !RemainingNodes.empty() && "No error node found in the trimmed graph" (PR 24184)

[Gábor Horváth via cfe-commits]

xazax.hun added a subscriber: xazax.hun.
xazax.hun closed this revision.
xazax.hun added a comment.

Committed in http://reviews.llvm.org/rL246188.

Thanks for the patch.


http://reviews.llvm.org/D12163



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun created this revision.
xazax.hun added reviewers: zaks.anna, dcoughlin, krememek, jordan_rose.
xazax.hun added a subscriber: cfe-commits.

With this patch a nullability violation no longer generates a sink node, so it 
does not cause any coverage loss for other checkers.
>From now on, it also do not warns, when a nullability precondition is 
>violated. These changes are related, because in order to be able to remove the 
>sinks (without generating duplicate reports for the same symbol), there should 
>be a mechanism to turn off this checker for a path.

http://reviews.llvm.org/D12445

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,44 @@
   takesNullable(p);
   takesNonnull(p);
 }
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // The first warning was not a sink. The analysis expected to continue.
+  int i = 0;
+  i = 5 / i; // expected-warning {{Division by zero}}
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,11 @@
 const MemRegion *Region;
   };
 
+  // This overload of report bug reports the bugs conditionally. When a
+  // nullability precondition is violated it will not report any bugs at all.
+  void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+ CheckerContext &C, const Stmt *ValueExpr = nullptr) const;
+
   void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
  BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
 if (!BT)
@@ -220,6 +225,13 @@
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
 
+// If the nullability preconditions of a function is violated, we should not
+// report that, a postcondition is violated. For this reason once a precondition
+// is not met on a path, this checker will be esentially turned of for the rest
+// of the analysis. We do not want to generate a sink node however, so this
+// check does not reduce the coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
 enum class NullConstraint { IsNull, IsNotNull, Unknown };
 
 static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +314,68 @@
   return Nullability::Unspecified;
 }
 
+template 
+static ProgramStateRef
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ProgramStateRef State,
+const LocationContext *LocCtxt) {
+  for (const auto *ParamDecl : Params) {
+if (ParamDecl->isParameterPack())
+  break;
+
+if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+  continue;
+
+auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+  .template getAs();
+if (!RegVal)
+  continue;
+
+auto ParamValue = State->getSVal(RegVal->getRegion())
+  .template getAs();
+if (!ParamValue)
+  continue;
+
+if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+  return State->set(true);
+}
+  }
+  return State;
+}
+
+static ProgramStateRef
+checkPreconditionViolation(ProgramStateRef State,
+   const LocationContext *LocCtxt) {
+  const Decl *D = LocCtxt->getDecl();
+  if (!D)
+return State;
+
+  if (const auto *BlockD = dyn_cast(D)) {
+return checkParamsForPreconditionViolation(BlockD->parameters(), State,
+   LocCtxt);
+  }
+
+  if (const auto *FuncDecl = dyn_cast(D)) {
+return checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+   LocCtxt);
+  }
+  return State;
+}
+
+void

Re: [PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun updated this revision to Diff 33501.
xazax.hun marked 6 inline comments as done.
xazax.hun added a comment.

Addressed the comments.


http://reviews.llvm.org/D12445

Files:
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  test/Analysis/nullability.mm

Index: test/Analysis/nullability.mm
===
--- test/Analysis/nullability.mm
+++ test/Analysis/nullability.mm
@@ -179,3 +179,44 @@
   takesNullable(p);
   takesNonnull(p);
 }
+
+void onlyReportFirstPreconditionViolationOnPath() {
+  Dummy *p = returnsNullable();
+  takesNonnull(p); // expected-warning {{}}
+  takesNonnull(p); // No warning.
+  // The first warning was not a sink. The analysis expected to continue.
+  int i = 0;
+  i = 5 / i; // expected-warning {{Division by zero}}
+  (void)i;
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolatedInTopFunc(
+Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+Dummy *_Nonnull doNotWarnWhenPreconditionIsViolated(Dummy *_Nonnull p) {
+  if (!p) {
+Dummy *ret =
+0; // avoid compiler warning (which is not generated by the analyzer)
+if (getRandom())
+  return ret; // no warning
+else
+  return p; // no warning
+  } else {
+return p;
+  }
+}
+
+void testPreconditionViolationInInlinedFunction(Dummy *p) {
+  doNotWarnWhenPreconditionIsViolated(p);
+}
Index: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -161,6 +161,16 @@
 const MemRegion *Region;
   };
 
+  /// When any of the nonnull arguments of the analyzed function is null, do not
+  /// report anything and turn off the check.
+  ///
+  /// When \p SuppressPath is set to true, no more bugs will be reported on this
+  /// path by this checker.
+  void reportBugConditionally(ErrorKind Error, ExplodedNode *N,
+  const MemRegion *Region, CheckerContext &C,
+  const Stmt *ValueExpr = nullptr,
+  bool SuppressPath = false) const;
+
   void reportBug(ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
  BugReporter &BR, const Stmt *ValueExpr = nullptr) const {
 if (!BT)
@@ -220,6 +230,13 @@
 REGISTER_MAP_WITH_PROGRAMSTATE(NullabilityMap, const MemRegion *,
NullabilityState)
 
+// If the nullability precondition of a function is violated, we should not
+// report nullability related issues on that path. For this reason once a
+// precondition is not met on a path, this checker will be esentially turned off
+// for the rest of the analysis. We do not want to generate a sink node however,
+// so this checker would not lead to reduced coverage.
+REGISTER_TRAIT_WITH_PROGRAMSTATE(PreconditionViolated, bool)
+
 enum class NullConstraint { IsNull, IsNotNull, Unknown };
 
 static NullConstraint getNullConstraint(DefinedOrUnknownSVal Val,
@@ -302,6 +319,75 @@
   return Nullability::Unspecified;
 }
 
+template 
+static ProgramStateRef
+checkParamsForPreconditionViolation(const ParamVarDeclRange &Params,
+ProgramStateRef State,
+const LocationContext *LocCtxt) {
+  for (const auto *ParamDecl : Params) {
+if (ParamDecl->isParameterPack())
+  break;
+
+if (getNullabilityAnnotation(ParamDecl->getType()) != Nullability::Nonnull)
+  continue;
+
+auto RegVal = State->getLValue(ParamDecl, LocCtxt)
+  .template getAs();
+if (!RegVal)
+  continue;
+
+auto ParamValue = State->getSVal(RegVal->getRegion())
+  .template getAs();
+if (!ParamValue)
+  continue;
+
+if (getNullConstraint(*ParamValue, State) == NullConstraint::IsNull) {
+  return State->set(true);
+}
+  }
+  return State;
+}
+
+static ProgramStateRef
+checkPreconditionViolation(ProgramStateRef State,
+   const LocationContext *LocCtxt) {
+  const Decl *D = LocCtxt->getDecl();
+  if (!D)
+return State;
+
+  if (const auto *BlockD = dyn_cast(D)) {
+return checkParamsForPreconditionViolation(BlockD->parameters(), State,
+   LocCtxt);
+  }
+
+  if (const auto *FuncDecl = dyn_cast(D)) {
+return checkParamsForPreconditionViolation(FuncDecl->parameters(), State,
+   LocCtxt);
+  }
+  return State;
+}
+
+void NullabilityChecker::reportBugConditionally(
+ErrorKind Error, ExplodedNode *N, const MemRegion *Region,
+CheckerContext &C, const Stmt *ValueExpr, bool SuppressPath) const {
+  ProgramStateRef

Re: [PATCH] D12445: [Static Analyzer] Remove sinks from nullability checks.

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:395
@@ +394,3 @@
+  // reaped.
+  if (!State->get()) {
+ProgramStateRef NewState =

zaks.anna wrote:
> Maybe you should only do this if there was at least one symbol with 
> nullability info removed in the loop above? Would that work?
That would not work.
Only "nullable" nullability and contradictions are tracked for the symbols 
right now. If nothing is marked nullable, this branch would never be taken.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:430
@@ -338,1 +429,3 @@
+// Do not suppress errors on defensive code paths, because dereferencing
+// null is alwazs an error.
 if (Event.IsDirectDereference)

zaks.anna wrote:
> Please, spell check your comments.
> 
> We are not dereferencing null.. we are dereferencing a nullable pointer..
> Am I wrong?
You are right.


Comment at: lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp:565
@@ -468,5 +564,3 @@
   ParamNullability == Nullability::Nonnull) {
-static CheckerProgramPointTag Tag(this, "NullablePassedToNonnull");
-ExplodedNode *N = C.generateSink(State, C.getPredecessor(), &Tag);
-reportBug(ErrorKind::NullablePassedToNonnull, N, Region,
-  C.getBugReporter(), ArgExpr);
+State = State->set(true);
+ExplodedNode *N = C.addTransition(State);

zaks.anna wrote:
> I do not understand how this works..
> 
> Does this ever report an issue? I thought this version of the bugReport will 
> only report an issue if PreconditionViolation is not set. (Same for the code 
> below and above..)
> 
> Is this tested?
> 
> 
Yes, it works.

The reason is that, reportBug does report bugs, when PreconditionViolated is 
set to true. It does not report bugs, when one of the nonnull argument is 
constrained to be null. PreconditionViolated is used at the beginning of some 
callbacks to return early.

However I think this behavior might be confusing, so I refactored the code. 
From now on reportBugConditionally does not report any bug, when 
PreconditionViolated is set to true, and it does set it to true when a reported 
bug should suppress other bugs along the path.


http://reviews.llvm.org/D12445



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11832: [Patch] [Analyzer] false positive: Potential leak connected with memcpy (PR 22954)

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Hi!

With this patch committed I noticed a regression in the static analyzer.

I analyzed openssl-1.0.0d (using the test suite in 
utils/analyzer/SATestBuild.py).
I got the following assertion error:
(lldb) bt

- thread #1: tid = 0xa1fcb, 0x7fff943e50ae 
libsystem_kernel.dylib`__pthread_kill + 10, queue = 'com.apple.main-thread', 
stop reason = signal SIGABRT
  - frame #0: 0x7fff943e50ae libsystem_kernel.dylib`__pthread_kill + 10 
frame #1: 0x7fff943f25fd libsystem_pthread.dylib`pthread_kill + 90 frame 
#2: 0x000100960106 clang`::abort() [inlined] raise(sig=6) + 18 at 
Signals.inc:504 frame #3: 0x0001009600f4 clang`::abort() + 4 at 
Signals.inc:521 frame #4: 0x0001009600e1 
clang`::__assert_rtn(func=, file=, 
line=, expr=) + 81 at Signals.inc:517 frame #5: 
0x0001018fc418 clang`(anonymous 
namespace)::CStringChecker::InvalidateBuffer(clang::ento::CheckerContext&, 
llvm::IntrusiveRefCntPtr, clang::Expr const*, 
clang::ento::SVal, bool, clang::Expr const*) [inlined] clang::ento::NonLoc 
clang::ento::SVal::castAs() const + 1448 at SVals.h:76 
frame #6: 0x0001018fc3f9 clang`(anonymous 
namespace)::CStringChecker::InvalidateBuffer(clang::ento::CheckerContext&, 
llvm::IntrusiveRefCntPtr, clang::Expr const*, 
clang::ento::SVal, bool, clang::Expr const*) [inlined] (anonymous 
namespace)::CStringChecker::IsFirstBufInBound(state=clang::ento::ProgramStateRef
 @ 0x000103bf2080, FirstBuf=0x000103a86768) at CStringChecker.cpp:842 
frame #7: 0x0001018fc3f9 clang`(anonymous 
namespace)::CStringChecker::InvalidateBuffer(C=, 
state=, E=0x000103a86768, V=, 
IsSourceBuffer=, Size=) + 1417 at 
CStringChecker.cpp:920 frame #8: 0x0001018fadf7 clang`(anonymous 
namespace)::CStringChecker::evalCopyCommon(this=0x000103212fb0, 
C=0x7fff5fbfc1a0, CE=, state=clang::ento::ProgramStateRef @ 
0x7fff5fbfc0c0, Size=0x000103a867b0, Dest=0x000103a86768, 
Source=, Restricted=, IsMempcpy=) const 
+ 3991 at CStringChecker.cpp:1079 frame #9: 0x0001018f8ad8 clang`(anonymous 
namespace)::CStringChecker::evalMemcpy(this=0x000103212fb0, 
C=0x7fff5fbfc1a0, CE=0x000103a86720) const + 248 at 
CStringChecker.cpp:1101 frame #10: 0x0001018f89b6 clang`bool 
clang::ento::eval::Call::_evalCall<(anonymous 
namespace)::CStringChecker>(void*, clang::CallExpr const*, 
clang::ento::CheckerContext&) [inlined] (anonymous 
namespace)::CStringChecker::evalCall(CE=0x000103a86720, 
C=0x7fff5fbfc1a0) const + 655 at CStringChecker.cpp:2002 frame #11: 
0x0001018f8727 clang`bool clang::ento::eval::Call::_evalCall<(anonymous 
namespace)::CStringChecker>(checker=0x000103212fb0, CE=0x000103a86720, 
C=0x7fff5fbfc1a0) + 23 at Checker.h:438 frame #12: 0x000101a0417d 
clang`clang::ento::CheckerManager::runCheckersForEvalCall(clang::ento::ExplodedNodeSet&,
 clang::ento::ExplodedNodeSet const&, clang::ento::CallEvent const&, 
clang::ento::ExprEngine&) [inlined] clang::ento::CheckerFn::operator(this=, 
ps=)(clang::CallExpr const*, clang::ento::CheckerContext&) const + 
653 at CheckerManager.h:58 frame #13: 0x000101a0416b 
clang`clang::ento::CheckerManager::runCheckersForEvalCall(this=0x000103211950,
 Dst=0x7fff5fbfc2d8, Src=, Call=0x000103ac2070, 
Eng=0x7fff5fbfcd90) + 635 at CheckerManager.cpp:549 frame #14: 
0x000101a361af 
clang`clang::ento::ExprEngine::evalCall(this=0x7fff5fbfcd90, 
Dst=0x7fff5fbfc448, Pred=, Call=0x000103ac2070) + 383 at 
ExprEngineCallAndReturn.cpp:527 frame #15: 0x000101a35ee0 
clang`clang::ento::ExprEngine::VisitCallExpr(this=0x7fff5fbfcd90, 
CE=0x000103a86720, Pred=, dst=0x7fff5fbfc9b8) + 528 at 
ExprEngineCallAndReturn.cpp:499 frame #16: 0x000101a1b4a0 
clang`clang::ento::ExprEngine::Visit(this=0x7fff5fbfcd90, 
S=0x000103a86720, Pred=, DstTop=) + 12224 at 
ExprEngine.cpp:1075 frame #17: 0x000101a16c30 
clang`clang::ento::ExprEngine::ProcessStmt(this=0x7fff5fbfcd90, 
S=, Pred=) + 880 at ExprEngine.cpp:446 frame #18: 
0x000101a1681e 
clang`clang::ento::ExprEngine::processCFGElement(this=, 
E=, Pred=0x000103bf1be0, StmtIdx=, 
Ctx=0x7fff5fbfcc98) + 190 at ExprEngine.cpp:295 frame #19: 
0x000101a0c128 
clang`clang::ento::CoreEngine::HandlePostStmt(this=, 
B=, StmtIdx=, Pred=) + 136 at 
CoreEngine.cpp:503 frame #20: 0x000101a0b71b 
clang`clang::ento::CoreEngine::ExecuteWorkList(this=0x7fff5fbfcda8, 
L=, Steps=15, InitState=clang::ento::ProgramStateRef @ 
0x7fff5fbfd120) + 491 at CoreEngine.cpp:223 frame #21: 0x0001012698a0 
clang`(anonymous namespace)::AnalysisConsumer::ActionExprEngine(clang::Decl*, 
bool, clang::ento::ExprEngine::InliningModes, llvm::DenseSet >*) [inlined] 
clang::ento::ExprEngine::ExecuteWorkList(L=0x0001032c84a0, 
Steps=) + 35 at ExprEngine.h:109 frame #22: 0x00010126987d 
clang`(anonymous 
namespace)::AnalysisConsumer::ActionExprEngine(this=0x000103211090, 
D=0x0001039b8418

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

xazax.hun marked 2 inline comments as done.
xazax.hun added a comment.

In http://reviews.llvm.org/D10305#258671, @zaks.anna wrote:

> > Generating mangled names requires ASTContext which is not available during 
> > the error reporting. BugReporter does have the ASTContext, so it would not 
>
> >  be a big change to add it to the DiagnosticConsumers though. And I think 
> > the mangled name might contain too much redundant information. The only 
>
> >  relevant information here is the fully qualified name and the parts of the 
> > signature that can be ocerloaded on e.g.: constness. Using this method 
> > might also 
>
> >  be easier to debug, since the IssueString is more readable.
>
>
> I think it'd be fine to pass ASTContext to the DiagnosticConsumers. It would 
> be useful to find out exactly what extra information the mangled names have 
> that we do not want to see in the issue_hash.


I looked into the name mangling and found the following downsides:

- The name mangling is different when clang is running in msvc compatible mode. 
If we want to consistent hashes on all platforms, we should not rely on mangled 
names.
- The name mangling contains the calling convention which is redundant 
information.
- Some attributes have effect on name mangling.
- Linkage is included in mangled name.

In general using mangled name might cause unexpected behaviour for users. E.g. 
a user might  not expect that making a function extern "C" changes the hashes 
in the function.

> One issue that I see with the current approach is that we do not 
> differentiate methods from different classes/namespaces. Is this by design?


The implementation do differentiate. It uses qualified names whenever a name is 
queried, which contains all of the enclosing namespaces and classes.

> 

> 

> > I definitely agree. It would be great to have a unique identifier for 
> > checkers that is independent from the name/package. It is not a trivial 
> > problem however,

> 

> >  since we probably want to support plugins too. I can think of a similar 
> > solution like git commit ids, but I think this problem might be out of the 
> > scope of this

> 

> >  patch. But I am happy to start a discussion on the mailing list and create 
> > a new patch to solve this.

> 

> 

> Sounds good to me. I agree that this is out of scope for this patch.





http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13099: [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[Gábor Horváth via cfe-commits]

xazax.hun added inline comments.


Comment at: test/Analysis/const-method-call.cpp:26
@@ +25,3 @@
+
+struct Derived : Base {
+  mutable int mut;

Please add a  test case, where only base have a mutable field.


http://reviews.llvm.org/D13099



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D10305: [Clang Static Analyzer] Bug identification

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Thank you for the review!

Before committing this I would like to have a policy regarding future changes 
and document it inside the IssueHash header.
My proposed policy is the following:

- Do not change the calculation of issue hash unless we have a very good reason 
to do so.
- Every time the way of calculation changes the name of the hash in the plist 
should be changed (this makes it possible for users to patch clang to generate 
both old and new hash for those who are relying on the old hash).

I have some questions though:

- Should we require the generation of old hashes once a change is introduced, 
or should we expect users who rely on old hash to maintain the old hash 
generation as an out of tree patch?
- The hash calculation WILL change in the near future once we figured out how 
to identify checkers properly (but I think it will not make sense to rename the 
hash for this change). For this reason I think we should mark this feature as 
experimental, until that change is introduced. What is the recommended way, to 
do that? Generating a comment to the plist? Just adding a comment to the 
headers? Only mention it in the commit log?

What do you think?


http://reviews.llvm.org/D10305



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits