date:20190616

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D62883#1545343 , @Charusso wrote:

> In D62883#1545341 , @Szelethus wrote:
>
> > In D62883#1545324 , @Charusso 
> > wrote:
> >
> > > As @NoQ pointed out we have some problem with that function. We are 
> > > tracking *values* without using the `redecl()`-chain (see in 
> > > https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or 
> > > without tracking conditions. You have solved(?) the latter, but the 
> > > former is equally important to solve.
> >
> >
> > I'm a little confused, which is the "former" and "latter" you're referring 
> > to?
>
>
> I believe you have solved the condition tracking as you move in-place what is 
> going on.

I'm still not sure I follow :) What are the two distinct things to be solved 
here?

>>> I believe this patch should be on by default, but it requires to fix the 
>>> "In which range do we track?" problem with D62978 
>>> .
>> 
>> I disagree with this. The reason why I'd like to make this an off-by-default 
>> option is to implement my followup improvements incrementally (not only 
>> that, but a whole family of conditions is going to be added), and allow us 
>> to observe the changes those make in relation to this patch -- besides, I 
>> don't really see this patch changing even if I manage to fix those issues. 
>> Would you like to see a change being made to this specific patch?
> 
> As you moving stuff around it just bypasses the usefulness of the mentioned 
> two visitors, which is a problem. Also you have mentioned some very crazy bad 
> side-effects which is yes, related to in which range do we operate.

Could you please elaborate? The particular thing I'm missing, I guess, is the 
need to get those issues fixed before this patch, and making this 
on-by-default. If anything, enabling a flag like this could really demonstrate 
changes on those problems. I also like to think that tracking a condition 
(especially a condition of a control dependency of yet another condition) is a 
drastically different case that tracking a "regular" variable (in particular, I 
think more aggressive pruning could be used), and such a change would 
definitely be out of scope for this patch.

I like to think that most of the unimportant notes can be easily categorized, 
as seen above. With some, admittedly crude heuristics, we could cut these down 
greatly, and leave some breathing room for fine-tuning it.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883

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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204980.
Szelethus added a comment.

Add another `RUN:` line to see more clearly the effects of this patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,164 @@
+// RUN: %clang_analyze_cc1 %s -verify -DTRACKING_CONDITIONS \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+//
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+#endif // TRACKING_CONDITIONS
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Value assigned to 'flag'}}
+  // expected-note@-3{{Value assigned to 'bar'}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Calling 'foo'}}
+  // expected-note@-3{{Returning from 'foo'}}
+#endif // TRACKING_CONDITIONS
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin();
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo())
+#ifdef TRACKING_CONDITIONS
+// expected-note@-2{{Calling 'foo'}}
+// expected-note@-3{{Returning from 'foo'}}
+// expected-note@-4{{'flag' initialized here}}
+#endif // TRACKING_CONDITIONS
+// expected-note@-6{{Assuming 'flag' is not equal to 0}}
+// expected-note@-7{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+  operator bool() const { return coin(); }
+#ifdef TRACKING_CONDITIONS
+  // expected-note@-2{{Returning value}}
+#endif // TRACKING_CONDITIONS
+};
+
+void test() {
+  in

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso added a comment.

In D62883#1545341 , @Szelethus wrote:

> In D62883#1545324 , @Charusso wrote:
>
> > As @NoQ pointed out we have some problem with that function. We are 
> > tracking *values* without using the `redecl()`-chain (see in 
> > https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
> > tracking conditions. You have solved(?) the latter, but the former is 
> > equally important to solve.
>
>
> I'm a little confused, which is the "former" and "latter" you're referring to?


I believe you have solved the condition tracking as you move in-place what is 
going on.

>> I believe this patch should be on by default, but it requires to fix the "In 
>> which range do we track?" problem with D62978 
>> .
> 
> I disagree with this. The reason why I'd like to make this an off-by-default 
> option is to implement my followup improvements incrementally (not only that, 
> but a whole family of conditions is going to be added), and allow us to 
> observe the changes those make in relation to this patch -- besides, I don't 
> really see this patch changing even if I manage to fix those issues. Would 
> you like to see a change being made to this specific patch?

As you moving stuff around it just bypasses the usefulness of the mentioned two 
visitors, which is a problem. Also you have mentioned some very crazy bad 
side-effects which is yes, related to in which range do we operate.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus marked an inline comment as done.
Szelethus added a comment.

In D62883#1545324 , @Charusso wrote:

> Hey! It is a cool patch as the smallest the scope the better to understand 
> what is going on. I like the direction, but we saw with @NoQ, we have to 
> think about "In which range do we track?".
>
> In D62883#1545282 , @Szelethus wrote:
>
> > - Hide the current implementation behind the off-by-default analyzer config 
> > `"track-conditions"` Do you think that this patch looks good enough now 
> > that it's off by default? Maybe it'd be easier to develop/review followup 
> > changes separately.
>
>
> It would be helpful to see what is exactly new, what trackExpressionValue() 
> cannot provide. Could you adjust the test case with a second run, where you 
> switch off that new feature, please? (I highly recommend that for future 
> developments also.)


You can see how this would look like by checking the history of this revision: 
https://reviews.llvm.org/D62883?vs=204554&id=204973&whitespace=ignore-most#toc. 
Adding a second `RUN:` line might be a good idea though.

> In D62883#1544609 , @Szelethus wrote:
> 
>> In D62883#1544302 , @NoQ wrote:
>>
>> > On the other hand, all of these problems seem to be examples of the 
>> > problem of D62978 . Might it be that it's 
>> > the only thing that we're missing? Like, we need to formulate a rule 
>> > that'll give us an understanding of when do we track the value past the 
>> > point where it has been constrained to true or false - do we care about 
>> > the origins of the value or do we care about its constraints only? In case 
>> > of `flag` in the test examples, we clearly do. In case of these bools that 
>> > come from boolean conversions and assertions and such, we clearly don't. 
>> > What's the difference?
>> >
>> > How about we track the condition value past its collapse point only if it 
>> > involves an overwrite of a variable (or other region) from which the value 
>> > is loaded? Like, if there are no overwrites, stop at the collapse point. 
>> > If there are overwrites, stop at the oldest overwrite point (i.e., the 
>> > value was loaded from 'x' which was previously overwritten by the value of 
>> > value 'y' which was previously overwritten by the initial value of 
>> > variable 'z' => stop at the overwrite point of 'y').
>> >
>> > (then, again, not sure what happens if more nested stack frames are around)
>>
>>
>> For now, I'd like to restrict my efforts to simply "we should track this"  
>> or "we shouldn't track this". I think simply not tracking cases where the 
>> condition is a single variable assignment is a good initial approach. Let 
>> how the tracking is done be a worry for another day. Also, I'm a little 
>> confused, isn't this comment about how D62978 
>>  could be solved?
> 
> 
> As @NoQ pointed out we have some problem with that function. We are tracking 
> *values* without using the `redecl()`-chain (see in 
> https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
> tracking conditions. You have solved(?) the latter, but the former is equally 
> important to solve.

I'm a little confused, which is the "formet" and "latter" you're referring to?

> As I see that is the current situation:
> 
> - ReturnVisitor: give us too much unrelated information as we go out of scope.
> - FindLastStoreBRVisitor: do not use `redecls()` so gets confused with all 
> the hackish SVal conversions.
> - TrackControlDependencyCondBRVisitor: swaps notes which is questionable if I 
> want to see what the above two kindly provides for me, or I would remove them 
> instead.
> 
>   I believe this patch should be on by default, but it requires to fix the 
> "In which range do we track?" problem with D62978 
> .

I disagree with this. The reason why I'd like to make this an off-by-default 
option is to implement my followup improvements incrementally (not only that, 
but a whole family of conditions is going to be added), and allow us to observe 
the changes those make in relation to this patch -- besides, I don't really see 
this patch changing even if I manage to fix those issues. Would you like to see 
a change being made to this specific patch?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883



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

[PATCH] D60974: Clang IFSO driver action.

2019-06-16 Thread Puyan Lotfi via Phabricator via cfe-commits

plotfi updated this revision to Diff 204978.
plotfi marked 17 inline comments as done.
plotfi added a comment.

Addressing @compnerd's feedback


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Driver/Options.td
  clang/include/clang/Driver/Types.def
  clang/include/clang/Frontend/FrontendActions.h
  clang/include/clang/Frontend/FrontendOptions.h
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CMakeLists.txt
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp
  clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  clang/test/InterfaceStubs/bad-format.cpp
  clang/test/InterfaceStubs/class-template-specialization.cpp
  clang/test/InterfaceStubs/externstatic.c
  clang/test/InterfaceStubs/function-template-specialization.cpp
  clang/test/InterfaceStubs/hidden-class-inheritance.cpp
  clang/test/InterfaceStubs/inline.c
  clang/test/InterfaceStubs/inline.h
  clang/test/InterfaceStubs/object.cpp
  clang/test/InterfaceStubs/template-namespace-function.cpp
  clang/test/InterfaceStubs/virtual.cpp
  clang/test/InterfaceStubs/visibility.cpp
  clang/test/InterfaceStubs/weak.cpp

Index: clang/test/InterfaceStubs/weak.cpp
===
--- /dev/null
+++ clang/test/InterfaceStubs/weak.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang -target x86_64-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-tapi-elf-v1 %s | \
+// RUN: FileCheck %s
+
+// RUN: %clang -target x86_64-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-yaml-elf-v1 %s | \
+// RUN: FileCheck --check-prefix=CHECK-YAML %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -c %s | llvm-nm - 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-SYMBOLS %s
+
+// CHECK: Symbols:
+// CHECK-DAG:  _Z8weakFuncv: { Type: Func, Weak: true }
+// CHECK-DAG:  _Z10strongFuncv: { Type: Func }
+
+// CHECK-YAML: Symbols:
+// CHECK-YAML-DAG:   - Name:_Z8weakFuncv
+// CHECK-YAML-DAG: Type:STT_FUNC
+// CHECK-YAML-DAG: Binding: STB_WEAK
+// CHECK-YAML-DAG:   - Name:_Z10strongFuncv
+// CHECK-YAML-DAG: Type:STT_FUNC
+// CHECK-YAML-DAG: Binding: STB_GLOBAL
+
+// CHECK-SYMBOLS-DAG: _Z10strongFuncv
+// CHECK-SYMBOLS-DAG: _Z8weakFuncv
+__attribute__((weak)) void weakFunc() {}
+int strongFunc() { return 42; }
Index: clang/test/InterfaceStubs/visibility.cpp
===
--- /dev/null
+++ clang/test/InterfaceStubs/visibility.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-tapi-elf-v1 -fvisibility=hidden \
+// RUN: %s | FileCheck --check-prefix=CHECK-CMD-HIDDEN %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-yaml-elf-v1 -fvisibility=hidden \
+// RUN: %s | FileCheck --check-prefix=CHECK-CMD-HIDDEN %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-tapi-elf-v1 %s | FileCheck %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-yaml-elf-v1 %s | FileCheck %s
+
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -c %s | llvm-readelf -s - 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-SYMBOLS %s
+
+// Always Be Hidden:
+// CHECK-CMD-HIDDEN-NOT: _Z6hiddenv
+// CHECK-NOT: _Z6hiddenv
+__attribute__((visibility("hidden"))) void hidden() {}
+
+// Always Be Visible:
+// CHECK-CMD-HIDDEN: _Z9nothiddenv
+// CHECK-DAG: _Z9nothiddenv
+__attribute__((visibility("default"))) void nothidden() {}
+
+// Do Whatever -fvisibility says:
+// CHECK-CMD-HIDDEN-NOT: _Z10cmdVisiblev
+// CHECK-DAG: _Z10cmdVisiblev
+void cmdVisible() {}
+
+// CHECK-SYMBOLS-DAG: DEFAULT{{.*}} _Z10cmdVisiblev
+// CHECK-SYMBOLS-DAG: HIDDEN {{.*}} _Z6hiddenv
+// CHECK-SYMBOLS-DAG: DEFAULT{{.*}} _Z9nothiddenv
Index: clang/test/InterfaceStubs/virtual.cpp
===
--- /dev/null
+++ clang/test/InterfaceStubs/virtual.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -emit-interface-stubs \
+// RUN: -interface-stub-version=experimental-tapi-elf-v1 %s | \
+// RUN: FileCheck -check-prefix=CHECK-TAPI %s
+// RUN: %clang -target x86_64-unknown-linux-gnu -o - -c %s | \
+// RUN: llvm-readelf -s - 2>&1 | FileCheck -check-prefix=CHECK-SYMBOLS %s
+
+#define HIDDEN  __attribute__((__visibility__(("hidden"
+#define DEFAULT __attribute__((__visibility__(("default"
+
+// CHECK-TAPI-NOT: _ZNK1Q5func1Ev
+// CHECK-TAPI-NOT: _ZNK1Q5f

[PATCH] D60974: Clang IFSO driver action.

2019-06-16 Thread Puyan Lotfi via Phabricator via cfe-commits

plotfi added inline comments.



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:21
+  StringRef InFile;
+  StringRef Format;
+  std::set ParsedTemplates;

compnerd wrote:
> An enumeration may be nicer.
For now the format goes into the first line of the yaml (in both formats) and 
ends up as "--- ! \n".
I'd like to keep it this way for now. 



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:39
+  bool WriteNamedDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) {
+if (!(RDO & FromTU))
+  return true;

compnerd wrote:
> I tend to prefer `if (RDO & ~FromTU)`
0 != (RDO & 0x1) vs (RDO & 0xE)

I'm not sure if these are logically equivalent



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:72
+ VD->getParentFunctionOrMethod() == nullptr))
+  return true;
+  if (const FunctionDecl *FD = dyn_cast(ND)) {

compnerd wrote:
> If it is a `VarDecl`, it cannot be a `FunctionDecl`.  I think that this can 
> be simplified a bit as:
> 
> ```
> if (const auto *VD = dyn_cast(VD))
>   return VD->getStorageClass() == StorageClass::SC_Extern ||
>   VD->getStorageClass() == StorageClass::SC_Static ||
>   VD->getParentFunctionOrMethod() == nullptr;
> ```
I don't understand. Here I remember wanting to bail if 
VD->getParentFunctionOrMethod() == nullptr and the vardecl was marked static. 



Comment at: clang/lib/Frontend/InterfaceStubFunctionsConsumer.cpp:77
+  return true;
+if (const CXXMethodDecl *MD = dyn_cast(FD)) {
+  if (const auto *RC = dyn_cast(MD->getParent())) {

compnerd wrote:
> I think that flipping this around would be nicer.
> 
> ```
> if (const auto *MD = dyn_cast(FD)) {
>   ...
> }
> 
> return FD->isInlined() && !Instance.getLangOpts().GNUInline;
> ```
I think this still causes the isInlined + !GNUInlined code to trigger on 
CXXMethodDecls. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60974/new/

https://reviews.llvm.org/D60974



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso requested changes to this revision.
Charusso added a comment.
This revision now requires changes to proceed.

Hey! It is a cool patch as the smallest the scope the better to understand what 
is going on. I like the direction, but we saw with @NoQ, we have to think about 
"In which range do we track?".

In D62883#1545282 , @Szelethus wrote:

> - Hide the current implementation behind the off-by-default analyzer config 
> `"track-conditions"` Do you think that this patch looks good enough now that 
> it's off by default? Maybe it'd be easier to develop/review followup changes 
> separately.


It would be helpful to see what is exactly new, what trackExpressionValue() 
cannot provide. Could you adjust the test case with a second run, where you 
switch off that new feature, please? (I highly recommend that for future 
developments also.)

In D62883#1544609 , @Szelethus wrote:

> In D62883#1544302 , @NoQ wrote:
>
> > On the other hand, all of these problems seem to be examples of the problem 
> > of D62978 . Might it be that it's the only 
> > thing that we're missing? Like, we need to formulate a rule that'll give us 
> > an understanding of when do we track the value past the point where it has 
> > been constrained to true or false - do we care about the origins of the 
> > value or do we care about its constraints only? In case of `flag` in the 
> > test examples, we clearly do. In case of these bools that come from boolean 
> > conversions and assertions and such, we clearly don't. What's the 
> > difference?
> >
> > How about we track the condition value past its collapse point only if it 
> > involves an overwrite of a variable (or other region) from which the value 
> > is loaded? Like, if there are no overwrites, stop at the collapse point. If 
> > there are overwrites, stop at the oldest overwrite point (i.e., the value 
> > was loaded from 'x' which was previously overwritten by the value of value 
> > 'y' which was previously overwritten by the initial value of variable 'z' 
> > => stop at the overwrite point of 'y').
> >
> > (then, again, not sure what happens if more nested stack frames are around)
>
>
> For now, I'd like to restrict my efforts to simply "we should track this"  or 
> "we shouldn't track this". I think simply not tracking cases where the 
> condition is a single variable assignment is a good initial approach. Let how 
> the tracking is done be a worry for another day. Also, I'm a little confused, 
> isn't this comment about how D62978  could 
> be solved?


As @NoQ pointed out we have some problem with that function. We are tracking 
*values* without using the `redecl()`-chain (see in 
https://clang.llvm.org/doxygen/DeclBase_8h_source.html#l00948), or without 
tracking conditions. You have solved(?) the latter, but the former is equally 
important to solve.

As I see that is the current situation:

- ReturnVisitor: give us too much unrelated information as we go out of scope.
- FindLastStoreBRVisitor: do not use `redecls()` so gets confused with all the 
hackish SVal conversions.
- TrackControlDependencyCondBRVisitor: swaps notes which is questionable if I 
want to see what the above two kindly provides for me, or I would remove them 
instead.

I believe this patch should be on by default, but it requires to fix the "In 
which range do we track?" problem with D62978 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus marked 8 inline comments as done.
Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1850
+
+return const_cast(Node->getLocationContext()
+->getAnalysisDeclContext()->getCFGStmtMap()->getBlock(S));

xazax.hun wrote:
> Why do we need a ptr to non-const CFGBlock?
`llvm::IDFCalculator` takes the blocks as a non-const pointer.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204973.
Szelethus added a comment.

- Rebase on the rest of the patches
- Make `TrackControlDependencyCondBRVisitor` local to `BugReporterVisitors.cpp`
- Hide the current implementation behind the off-by-default analyzer config 
`"track-conditions"`
- Add two more testcases I'd like to improve on in followup patches

Do you think that this patch looks good enough now that it's off by default? 
Maybe it'd be easier to develop/review followup changes separately.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- /dev/null
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-config track-conditions=true \
+// RUN:   -analyzer-output=text \
+// RUN:   -analyzer-checker=core
+
+namespace example_1 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // TODO: Add nodes here about flag's value being invalidated.
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_1
+
+namespace example_2 {
+int flag;
+bool coin();
+
+void foo() {
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+}
+
+void test() {
+  int *x = 0;
+  flag = 1;
+
+  foo();
+  if (flag) // expected-note   {{Assuming 'flag' is 0}}
+// expected-note@-1{{Taking false branch}}
+x = new int;
+
+  x = 0; // expected-note{{Null pointer value stored to 'x'}}
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_2
+
+namespace example_3 {
+int flag;
+bool coin();
+
+void foo() {
+  // coin() could write bar, do it's invalidated.
+  flag = coin(); // expected-note {{Value assigned to 'flag'}}
+ // expected-note@-1 {{Value assigned to 'bar'}}
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+  flag = 1;
+
+  foo(); // expected-note   {{Calling 'foo'}}
+ // expected-note@-1{{Returning from 'foo'}}
+
+  if (bar) // expected-note   {{Assuming 'bar' is not equal to 0}}
+   // expected-note@-1{{Taking true branch}}
+if (flag) // expected-note   {{Assuming 'flag' is not equal to 0}}
+  // expected-note@-1{{Taking true branch}}
+  *x = 5; // expected-warning{{Dereference of null pointer}}
+  // expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace example_3
+
+namespace variable_declaration_in_condition {
+bool coin();
+
+bool foo() {
+  return coin(); // expected-note {{Returning value}}
+}
+
+int bar;
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (int flag = foo()) // expected-note {{Calling 'foo'}}
+// expected-note@-1{{Returning from 'foo'}}
+// expected-note@-2{{'flag' initialized here}}
+// expected-note@-3{{Assuming 'flag' is not equal to 0}}
+// expected-note@-4{{Taking true branch}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+} // end of namespace variable_declaration_in_condition
+
+namespace conversion_to_bool {
+bool coin();
+
+struct ConvertsToBool {
+  operator bool() const { return coin(); } // expected-note {{Returning value}}
+};
+
+void test() {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  if (ConvertsToBool()) // expected-note {{Calling 'ConvertsToBool::operator bool'}}
+// expected-note@-1{{Returning from 'ConvertsToBool::operator bool'}}
+// expected-note@-2{{Assumi

[PATCH] D62619: [analyzer][Dominators] Add a control dependency calculator + a new debug checker

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus updated this revision to Diff 204972.
Szelethus retitled this revision from "[analyzer][Dominators] Add a control 
dependency tree builder + a new debug checker" to "[analyzer][Dominators] Add a 
control dependency calculator + a new debug checker".
Szelethus edited the summary of this revision.
Szelethus added a comment.

Now using `llvm::IDFCalculator`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62619/new/

https://reviews.llvm.org/D62619

Files:
  clang/include/clang/Analysis/Analyses/Dominators.h
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/test/Analysis/domtest.c
  clang/test/Analysis/domtest.cpp
  clang/unittests/Analysis/CFGDominatorTree.cpp

Index: clang/unittests/Analysis/CFGDominatorTree.cpp
===
--- clang/unittests/Analysis/CFGDominatorTree.cpp
+++ clang/unittests/Analysis/CFGDominatorTree.cpp
@@ -117,6 +117,99 @@
   EXPECT_TRUE(PostDom.dominates(nullptr, ExitBlock));
 }
 
+TEST(CFGDominatorTree, ControlDependency) {
+  const char *Code = R"(bool coin();
+
+void funcWithBranch() {
+  int x = 0;
+  if (coin()) {
+if (coin()) {
+  x = 5;
+}
+int j = 10 / x;
+(void)j;
+  }
+};)";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //  1st if  2nd if
+  //  [B5 (ENTRY)]  -> [B4] -> [B3] -> [B2] -> [B1] -> [B0 (EXIT)]
+  //\\  / /
+  // \-> /
+  //  -->
+
+  CFG *cfg = Result.getCFG();
+
+  // Sanity checks.
+  EXPECT_EQ(cfg->size(), 6u);
+
+  CFGBlock *ExitBlock = *cfg->begin();
+  EXPECT_EQ(ExitBlock, &cfg->getExit());
+
+  CFGBlock *NullDerefBlock = *(cfg->begin() + 1);
+
+  CFGBlock *SecondThenBlock = *(cfg->begin() + 2);
+
+  CFGBlock *SecondIfBlock = *(cfg->begin() + 3);
+  EXPECT_TRUE(hasStmtType(SecondIfBlock));
+
+  CFGBlock *FirstIfBlock = *(cfg->begin() + 4);
+  EXPECT_TRUE(hasStmtType(FirstIfBlock));
+
+  CFGBlock *EntryBlock = *(cfg->begin() + 5);
+  EXPECT_EQ(EntryBlock, &cfg->getEntry());
+
+  ControlDependencyCalculator Control(cfg);
+
+  EXPECT_TRUE(Control.isControlDependent(SecondIfBlock, SecondThenBlock));
+  EXPECT_TRUE(Control.isControlDependent(FirstIfBlock, SecondIfBlock));
+  EXPECT_FALSE(Control.isControlDependent(SecondIfBlock, NullDerefBlock));
+}
+
+TEST(CFGDominatorTree, ControlDependencyWithLoops) {
+  const char *Code = R"(int test3() {
+  int x,y,z;
+
+  x = y = z = 1;
+  if (x > 0) {
+while (x >= 0){
+  while (y >= x) {
+x = x-1;
+y = y/2;
+  }
+}
+  }
+  z = y;
+
+  return 0;
+})";
+  BuildResult Result = BuildCFG(Code);
+  EXPECT_EQ(BuildResult::BuiltCFG, Result.getStatus());
+
+  //   <- [B2] <-
+  //  /  \
+  // [B8 (ENTRY)] -> [B7] -> [B6] ---> [B5] -> [B4] -> [B3]
+  //   \   | \  /
+  //\  |  <-
+  // \  \
+  //  > [B1] -> [B0 (EXIT)]
+
+  CFG *cfg = Result.getCFG();
+
+  ControlDependencyCalculator Control(cfg);
+
+  auto GetBlock = [cfg] (unsigned Index) -> CFGBlock * {
+assert(Index < cfg->size());
+return *(cfg->begin() + Index);
+  };
+
+  // While not immediately obvious, the second block in fact post dominates the
+  // fifth, hence B5 is not a control dependency of 2.
+  EXPECT_FALSE(Control.isControlDependent(GetBlock(5), GetBlock(2)));
+}
+
+
 } // namespace
 } // namespace analysis
 } // namespace clang
Index: clang/test/Analysis/domtest.cpp
===
--- clang/test/Analysis/domtest.cpp
+++ clang/test/Analysis/domtest.cpp
@@ -1,6 +1,7 @@
 // RUN: %clang_analyze_cc1 %s \
 // RUN:   -analyzer-checker=debug.DumpDominators \
 // RUN:   -analyzer-checker=debug.DumpPostDominators \
+// RUN:   -analyzer-checker=debug.DumpControlDependencies \
 // RUN:   2>&1 | FileCheck %s
 
 bool coin();
@@ -20,7 +21,8 @@
 
 //  [B3 (ENTRY)]  -> [B1] -> [B2] -> [B0 (EXIT)]
 
-// CHECK:  Immediate dominance tree (Node#,IDom#):
+// CHECK:  Control dependencies (Node#,Dependency#):
+// CHECK-NEXT: Immediate dominance tree (Node#,IDom#):
 // CHECK-NEXT: (0,2)

[PATCH] D63088: [clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

2019-06-16 Thread Matthias Gehre via Phabricator via cfe-commits

mgehre marked an inline comment as done.
mgehre added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/UnusedParametersCheck.cpp:141-156
+  // Cannot remove parameter for non-local functions.
   if (Function->isExternallyVisible() ||
   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
   !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) {
-SourceRange RemovalRange(Param->getLocation());
-// Note: We always add a space before the '/*' to not accidentally create a
-// '*/*' for pointer types, which doesn't start a comment. clang-format 
will
-// clean this up afterwards.
-MyDiag << FixItHint::CreateReplacement(
-RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
+
+// Comment out parameter name.
+if (Result.Context->getLangOpts().CPlusPlus) {

lebedev.ri wrote:
> I'd recommend to instead do less confusing
> ```
>   // Cannot remove parameter for non-local functions.
>   if (Function->isExternallyVisible() ||
>   !Result.SourceManager->isInMainFile(Function->getLocation()) ||
>   !Indexer->getOtherRefs(Function).empty() || isOverrideMethod(Function)) 
> {
> // It is illegal to omit parameter name here in C code, so early-out.
> if (!Result.Context->getLangOpts().CPlusPlus)
>   return;
> 
> SourceRange RemovalRange(Param->getLocation());
> // Note: We always add a space before the '/*' to not accidentally create
> // a '*/*' for pointer types, which doesn't start a comment. clang-format
> // will clean this up afterwards.
> MyDiag << FixItHint::CreateReplacement(
> RemovalRange, (Twine(" /*") + Param->getName() + "*/").str());
> return;
>   }
> ```
Good idea, will do!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63088/new/

https://reviews.llvm.org/D63088



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

[PATCH] D63088: [clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

2019-06-16 Thread Matthias Gehre via Phabricator via cfe-commits

mgehre marked an inline comment as done.
mgehre added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/misc-unused-parameters.c:1-7
 // RUN: %check_clang_tidy %s misc-unused-parameters %t -- -- -xc
 
 // Basic removal
 // =
 void a(int i) {;}
 // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: parameter 'i' is unused 
[misc-unused-parameters]
-// CHECK-FIXES: {{^}}void a(int  /*i*/) {;}{{$}}

lebedev.ri wrote:
> This screams a separate bug to me.
> Does `%check_clang_tidy` not check that sources, after applying fix-it's, 
> still parse?
Unfortunately, no :-(


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63088/new/

https://reviews.llvm.org/D63088



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

r363521 - [docs] Fix another bot error by setting highlight language of objc code-block to objc instead of c++.

2019-06-16 Thread Don Hinton via cfe-commits

Author: dhinton
Date: Sun Jun 16 12:15:04 2019
New Revision: 363521

URL: http://llvm.org/viewvc/llvm-project?rev=363521&view=rev
Log:
[docs] Fix another bot error by setting highlight language of objc code-block 
to objc instead of c++.

Modified:
cfe/trunk/docs/ReleaseNotes.rst

Modified: cfe/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ReleaseNotes.rst?rev=363521&r1=363520&r2=363521&view=diff
==
--- cfe/trunk/docs/ReleaseNotes.rst (original)
+++ cfe/trunk/docs/ReleaseNotes.rst Sun Jun 16 12:15:04 2019
@@ -127,7 +127,7 @@ Objective-C Language Changes in Clang
 
 - Fixed encoding of ObjC pointer types that are pointers to typedefs.
 
-.. code-block:: c++
+.. code-block:: objc
 
   typedef NSArray MyArray;
 


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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet TrackedConditions;
+

xazax.hun wrote:
> Do we need this? I wonder if marking the result of the condition as 
> interesting is sufficient.
Later on, when I'm covering the "should-not-have-happened" case, it's possible 
that a condition would be added that wasn't even explored by the analyzer, so 
this is kinda necessary.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62883/new/

https://reviews.llvm.org/D62883



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

[clang-tools-extra] r363520 - [docs] Fix another bot warning by adding a blank line to separate the `option::` command from the text below.

2019-06-16 Thread Don Hinton via cfe-commits

Author: dhinton
Date: Sun Jun 16 11:41:31 2019
New Revision: 363520

URL: http://llvm.org/viewvc/llvm-project?rev=363520&view=rev
Log:
[docs] Fix another bot warning by adding a blank line to separate the 
`option::` command from the text below.

Modified:

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst?rev=363520&r1=363519&r2=363520&view=diff
==
--- 
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst
 (original)
+++ 
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-pro-type-member-init.rst
 Sun Jun 16 11:41:31 2019
@@ -34,6 +34,7 @@ Options
be important to not initialize fixed-size array members. Default is `0`.
 
 .. option:: UseAssignment
+
If set to non-zero, the check will provide fix-its with literal initializers
\( ``int i = 0;`` \) instead of curly braces \( ``int i{};`` \).
 


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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso marked 3 inline comments as done.
Charusso added a comment.

@NoQ, thanks for the review! Now everything is working by rL363515 
.




Comment at: cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp:16
+
+typedef unsigned long uintptr_t;
+

Szelethus wrote:
> Szelethus wrote:
> > hintonda wrote:
> > > I don't believe this is really ever portable, but definitely not on 64 
> > > bit Windows where a long is 32 bits.
> > > 
> > > Can't you just `#include ` instead?
> > In the test, I'm pretty sure that he can't. 
> > 
> > I grepped `test/Analysis`, and found that `malloc.cpp` does the following:
> > 
> > ```
> > typedef unsigned __INTPTR_TYPE__ uintptr_t
> > ```
> > 
> > Maybe that's worth a shot?
> I seem to have been proven wrong. :)
I was a little-bit surprised as well. Thanks for the idea! 


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

2019-06-16 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso marked an inline comment as done.
Charusso added a comment.

In D62926#1545163 , @hintonda wrote:

> This test fails to compile on Windows 64 bit builds.  Please see 
> http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/77

Thanks you! I have not got a mail, now it passes on my side: 
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/80/steps/stage%201%20check/logs/stdio
But some unrelated error occurs: 
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/80/steps/stage%202%20build/logs/stdio

Sorry for all the inconveniences it caused.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926

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

[clang-tools-extra] r363518 - [docs] Fix a few problems with clang-tool docs to get the bots green again.

2019-06-16 Thread Don Hinton via cfe-commits

Author: dhinton
Date: Sun Jun 16 10:57:37 2019
New Revision: 363518

URL: http://llvm.org/viewvc/llvm-project?rev=363518&view=rev
Log:
[docs] Fix a few problems with clang-tool docs to get the bots green again.

Modified:
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst

Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=363518&r1=363517&r2=363518&view=diff
==
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Sun Jun 16 10:57:37 2019
@@ -192,11 +192,12 @@ Improvements to clang-tidy
   Rewrites function signatures to use a trailing return type.
 
 - The :doc:`misc-throw-by-value-catch-by-reference
-  ` now supports
+  ` now supports
   `WarnOnLargeObject` and `MaxSize` options to warn on any large trivial
   object caught by value.
 
-- Added `UseAssignment` option to :doc:`cppcoreguidelines-pro-type-member-init`
+- Added `UseAssignment` option to :doc:`cppcoreguidelines-pro-type-member-init
+  `
 
   If set to true, the check will provide fix-its with literal initializers
   (``int i = 0;``) instead of curly braces (``int i{};``).

Modified: 
clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst?rev=363518&r1=363517&r2=363518&view=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst 
(original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst Sun 
Jun 16 10:57:37 2019
@@ -17,4 +17,5 @@ Examples:
 Suggested replacement:
 
 .. code-block:: c++
+
   pipe2(pipefd, O_CLOEXEC);


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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added inline comments.



Comment at: cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp:16
+
+typedef unsigned long uintptr_t;
+

hintonda wrote:
> I don't believe this is really ever portable, but definitely not on 64 bit 
> Windows where a long is 32 bits.
> 
> Can't you just `#include ` instead?
In the test, I'm pretty sure that he can't. 

I grepped `test/Analysis`, and found that `malloc.cpp` does the following:

```
typedef unsigned __INTPTR_TYPE__ uintptr_t
```

Maybe that's worth a shot?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added inline comments.



Comment at: cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp:16
+
+typedef unsigned long uintptr_t;
+

Szelethus wrote:
> hintonda wrote:
> > I don't believe this is really ever portable, but definitely not on 64 bit 
> > Windows where a long is 32 bits.
> > 
> > Can't you just `#include ` instead?
> In the test, I'm pretty sure that he can't. 
> 
> I grepped `test/Analysis`, and found that `malloc.cpp` does the following:
> 
> ```
> typedef unsigned __INTPTR_TYPE__ uintptr_t
> ```
> 
> Maybe that's worth a shot?
I seem to have been proven wrong. :)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926



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

[PATCH] D63009: [OpenMP] Add target task alloc function with device ID

2019-06-16 Thread Alexey Bataev via Phabricator via cfe-commits

ABataev added a comment.

In D63009#1545152 , @gtbercea wrote:

> In D63009#1544984 , @ABataev wrote:
>
> > In D63009#1544900 , @gtbercea 
> > wrote:
> >
> > > In D63009#1544758 , @Hahnfeld 
> > > wrote:
> > >
> > > > Am I correct that the second to last revision ("- Fix tests.") removed 
> > > > all checks for the actual `device_id` argument from the tests? From my 
> > > > point of view that's not fixing but weakening the tests! Can you 
> > > > explain why they needed "fixing"?
> > >
> > >
> > > When I was just passing the default value the LLVM-IR was: i64 -1 i.e. 
> > > constant, easy to check.
> > >
> > > With the latest change the emitted code is: i64 %123 i.e. where %123 is a 
> > > local derived from the expression of the device ID.
> >
> >
> > If the value is constant, check for the constant.
>
>
> All the tests here use expressions so the value is never constant. If you'd 
> like me to add a test with constant I can.


Yes, there must be at least one test with the default value.

> 
> 
>> And at least several tests with the expressions should check for the correct 
>> value of the expression.
> 
> I'll have to check how to do this. There's nothing that distinguishes an 
> expression that represents the device ID from any other expression in the 
> code.




Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63009/new/

https://reviews.llvm.org/D63009



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

r363515 - [analyzer] ReturnVisitor: more portable test case

2019-06-16 Thread Csaba Dabis via cfe-commits

Author: charusso
Date: Sun Jun 16 10:29:37 2019
New Revision: 363515

URL: http://llvm.org/viewvc/llvm-project?rev=363515&view=rev
Log:
[analyzer] ReturnVisitor: more portable test case

Modified:
cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp

Modified: cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp?rev=363515&r1=363514&r2=363515&view=diff
==
--- cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp (original)
+++ cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp Sun Jun 
16 10:29:37 2019
@@ -11,10 +11,9 @@
 // expected-no-diagnostics
 #endif
 
+#include 
 #include "../Inputs/system-header-simulator-cxx.h"
 
-typedef unsigned long uintptr_t;
-
 void error();
 void *malloc(size_t);
 


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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

2019-06-16 Thread Don Hinton via Phabricator via cfe-commits

hintonda added a comment.

This test fails to compile on Windows 64 bit builds.  Please see 
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/77




Comment at: cfe/trunk/test/Analysis/inlining/placement-new-fp-suppression.cpp:16
+
+typedef unsigned long uintptr_t;
+

I don't believe this is really ever portable, but definitely not on 64 bit 
Windows where a long is 32 bits.

Can't you just `#include ` instead?


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62926/new/

https://reviews.llvm.org/D62926



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

[PATCH] D62899: [analyzer][UninitializedObjectChecker] Mark uninitialized regions as interesting.

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

Szelethus added a comment.

In D62899#1544630 , @NoQ wrote:

> I don't remember what exactly does `markInteresting()` do and these tests 
> don't really convince me that it does anything at all. Halp? >.<

Damnit, forgot my actual test file in the office (did not git add it).

I wanted to just say "Well `markInteresting()` is about the same as 
`trackExpressionValue()`!", and while I suspect that there's some truth to 
that, honestly, it's explained extremely poorly in the code. As I understand 
it, interesting symbols and expressions are used in the pruning of the bug 
path, namely, if a you'd like to prune out B->C->D from bugpath A->B->C->D->E, 
but C is interesting, no pruning will take place.

I particularly liked your description of BugReporter.cpp as a "military grade 
portion of spaghetti", because thats pretty much all I could figure out. I'll 
try harder though, because it seems to be an important part of the bug report 
construction. @xazax.hun, you seem to be a lot more confident with this, is 
what I'm saying more or less correct?

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62899/new/

https://reviews.llvm.org/D62899

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

[PATCH] D63009: [OpenMP] Add target task alloc function with device ID

2019-06-16 Thread Gheorghe-Teodor Bercea via Phabricator via cfe-commits

gtbercea added a comment.

In D63009#1544984 , @ABataev wrote:

> In D63009#1544900 , @gtbercea wrote:
>
> > In D63009#1544758 , @Hahnfeld 
> > wrote:
> >
> > > Am I correct that the second to last revision ("- Fix tests.") removed 
> > > all checks for the actual `device_id` argument from the tests? From my 
> > > point of view that's not fixing but weakening the tests! Can you explain 
> > > why they needed "fixing"?
> >
> >
> > When I was just passing the default value the LLVM-IR was: i64 -1 i.e. 
> > constant, easy to check.
> >
> > With the latest change the emitted code is: i64 %123 i.e. where %123 is a 
> > local derived from the expression of the device ID.
>
>
> If the value is constant, check for the constant.


All the tests here use expressions so the value is never constant. If you'd 
like me to add a test with constant I can.

> And at least several tests with the expressions should check for the correct 
> value of the expression.

I'll have to check how to do this. There's nothing that distinguishes an 
expression that represents the device ID from any other expression in the code.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63009/new/

https://reviews.llvm.org/D63009



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

[PATCH] D63387: clang: Promote -fdebug-compilation-dir from cc1 flag to driver-level flag

2019-06-16 Thread Nico Weber via Phabricator via cfe-commits

thakis created this revision.
thakis added reviewers: hans, rnk.
Herald added a subscriber: aprantl.

The flag is useful when wanting to create .o files that are independent
from the absolute path to the build directory. -fdebug-prefix-map= can
be used to the same effect, but it requires putting the absolute path
to the build directory on the build command line, so it still requires
the build command line to be dependent on the absolute path of the build
directory. With this flag, "-fdebug-compilation-dir ." makes it so that
both debug info and the compile command itself are independent of the
absolute path of the build directory, which is good for build
determinism (in the sense that the build is independent of which
directory it happens in) and for caching compile results.


https://reviews.llvm.org/D63387

Files:
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/clang_f_opts.c


Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -525,11 +525,15 @@
 // CHECK-CF-PROTECTION-BRANCH: -fcf-protection=branch
 // CHECK-NO-CF-PROTECTION-BRANCH-NOT: -fcf-protection=branch
 
+// RUN: %clang -### -S -fdebug-compilation-dir . %s 2>&1 | FileCheck 
-check-prefix=CHECK-DEBUG-COMPILATION-DIR %s
+// RUN: %clang -### -fdebug-compilation-dir . -x assembler %s 2>&1 | FileCheck 
-check-prefix=CHECK-DEBUG-COMPILATION-DIR %s
+// CHECK-DEBUG-COMPILATION-DIR: "-fdebug-compilation-dir" "."
+
 // RUN: %clang -### -S -fdiscard-value-names %s 2>&1 | FileCheck 
-check-prefix=CHECK-DISCARD-NAMES %s
 // RUN: %clang -### -S -fno-discard-value-names %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-DISCARD-NAMES %s
 // CHECK-DISCARD-NAMES: "-discard-value-names"
 // CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names"
-//
+
 // RUN: %clang -### -S -fmerge-all-constants %s 2>&1 | FileCheck 
-check-prefix=CHECK-MERGE-ALL-CONSTANTS %s
 // RUN: %clang -### -S -fno-merge-all-constants %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
 // RUN: %clang -### -S -fmerge-all-constants -fno-merge-all-constants %s 2>&1 
| FileCheck -check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -619,6 +619,7 @@
 // RUN: -fno-coverage-mapping \
 // RUN: -fdiagnostics-color \
 // RUN: -fno-diagnostics-color \
+// RUN: -fdebug-compilation-dir . \
 // RUN: -fdiagnostics-parseable-fixits \
 // RUN: -fdiagnostics-absolute-paths \
 // RUN: -ferror-limit=10 \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -618,7 +618,11 @@
 /// Add a CC1 option to specify the debug compilation directory.
 static void addDebugCompDirArg(const ArgList &Args, ArgStringList &CmdArgs,
const llvm::vfs::FileSystem &VFS) {
-  if (llvm::ErrorOr CWD = VFS.getCurrentWorkingDirectory()) {
+  if (Arg *A = Args.getLastArg(options::OPT_fdebug_compilation_dir)) {
+CmdArgs.push_back("-fdebug-compilation-dir");
+CmdArgs.push_back(A->getValue());
+  } else if (llvm::ErrorOr CWD =
+ VFS.getCurrentWorkingDirectory()) {
 CmdArgs.push_back("-fdebug-compilation-dir");
 CmdArgs.push_back(Args.MakeArgString(*CWD));
   }
@@ -637,7 +641,8 @@
 }
 
 /// Vectorize at all optimization levels greater than 1 except for -Oz.
-/// For -Oz the loop vectorizer is disable, while the slp vectorizer is 
enabled.
+/// For -Oz the loop vectorizer is disabled, while the slp vectorizer is
+/// enabled.
 static bool shouldEnableVectorizerAtOLevel(const ArgList &Args, bool isSlpVec) 
{
   if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {
 if (A->getOption().matches(options::OPT_O4) ||
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -713,11 +713,14 @@
 Group, Alias;
 def fno_auto_profile_accurate : Flag<["-"], "fno-auto-profile-accurate">,
 Group, Alias;
-def fdebug_info_for_profiling : Flag<["-"], "fdebug-info-for-profiling">, 
Group,
-Flags<[CC1Option]>,
+def fdebug_compilation_dir : Separate<["-"], "fdebug-compilation-dir">,
+Group, Flags<[CC1Option, CC1AsOption, CoreOption]>,
+HelpText<"The compilation directory to embed in the debug info.">;
+def fdebug_info_for_profiling : Flag<["-"], "fdebug-info-for-profiling">,
+Group, Flags<[CC1Option]>,
 HelpText<"Emit extra debug info to make sample profile more accurate.">;
-def fno_debug_info_for_profiling : Flag<["-"], 
"fno-de

r363512 - [analyzer] Push correct version of 'Track indices of arrays'

2019-06-16 Thread Kristof Umann via cfe-commits

Author: szelethus
Date: Sun Jun 16 08:41:25 2019
New Revision: 363512

URL: http://llvm.org/viewvc/llvm-project?rev=363512&view=rev
Log:
[analyzer] Push correct version of 'Track indices of arrays'

Messed up the commit, oops.

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=363512&r1=363511&r2=363512&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Sun Jun 16 
08:41:25 2019
@@ -1740,9 +1740,10 @@ bool bugreporter::trackExpressionValue(c
   if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, 
LVNode))
 trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
 
+  // Track the index if this is an array subscript.
   if (const auto *Arr = dyn_cast(Inner))
 trackExpressionValue(
-LVNode, Arr->getIdx(), report, EnableNullFPSuppression);
+LVNode, Arr->getIdx(), report, /*EnableNullFPSuppression*/ false);
 
   // See if the expression we're interested refers to a variable.
   // If so, we can track both its contents and constraints on its value.

Modified: cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp?rev=363512&r1=363511&r2=363512&view=diff
==
--- cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp Sun Jun 16 
08:41:25 2019
@@ -23,11 +23,12 @@ void consume(int);
 
 int getIndex(int x) {
   int a;
-  if (x > 0)
-a = 3;
+  if (x > 0) // expected-note {{Assuming 'x' is > 0}}
+ // expected-note@-1 {{Taking true branch}}
+a = 3; // expected-note {{The value 3 is assigned to 'a'}}
   else
 a = 2;
-  return a;
+  return a; // expected-note {{Returning the value 3 (loaded from 'a')}}
 }
 
 int getInt();
@@ -36,9 +37,47 @@ void testArrayIndexTracking() {
   int arr[10];
 
   for (int i = 0; i < 3; ++i)
+// expected-note@-1 3{{Loop condition is true.  Entering loop body}}
+// expected-note@-2 {{Loop condition is false. Execution continues on line 
43}}
 arr[i] = 0;
   int x = getInt();
-  int n = getIndex(x);
+  int n = getIndex(x); // expected-note {{Calling 'getIndex'}}
+   // expected-note@-1 {{Returning from 'getIndex'}}
+   // expected-note@-2 {{'n' initialized to 3}}
   consume(arr[n]);
+  // expected-note@-1 {{1st function call argument is an uninitialized value}}
+  // expected-warning@-2{{1st function call argument is an uninitialized 
value}}
 }
 } // end of namespace array_index_tracking
+
+namespace multi_array_index_tracking {
+void consume(int);
+
+int getIndex(int x) {
+  int a;
+  if (x > 0) // expected-note {{Assuming 'x' is > 0}}
+ // expected-note@-1 {{Taking true branch}}
+a = 3; // expected-note {{The value 3 is assigned to 'a'}}
+  else
+a = 2;
+  return a; // expected-note {{Returning the value 3 (loaded from 'a')}}
+}
+
+int getInt();
+
+void testArrayIndexTracking() {
+  int arr[2][10];
+
+  for (int i = 0; i < 3; ++i)
+// expected-note@-1 3{{Loop condition is true.  Entering loop body}}
+// expected-note@-2 {{Loop condition is false. Execution continues on line 
75}}
+arr[1][i] = 0;
+  int x = getInt();
+  int n = getIndex(x); // expected-note {{Calling 'getIndex'}}
+   // expected-note@-1 {{Returning from 'getIndex'}}
+   // expected-note@-2 {{'n' initialized to 3}}
+  consume(arr[1][n]);
+  // expected-note@-1 {{1st function call argument is an uninitialized value}}
+  // expected-warning@-2{{1st function call argument is an uninitialized 
value}}
+}
+} // end of namespace mulit_array_index_tracking


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

[PATCH] D63080: [analyzer] Track indices of arrays

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363510: [analyzer] Track indices of arrays (authored by 
Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63080?vs=204888&id=204947#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63080/new/

https://reviews.llvm.org/D63080

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp


Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1740,6 +1740,10 @@
   if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, 
LVNode))
 trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
 
+  if (const auto *Arr = dyn_cast(Inner))
+trackExpressionValue(
+LVNode, Arr->getIdx(), report, EnableNullFPSuppression);
+
   // See if the expression we're interested refers to a variable.
   // If so, we can track both its contents and constraints on its value.
   if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
Index: cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
+++ cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -17,3 +17,28 @@
   (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the 
left shift is undefined due to shifting by '255', which is greater or equal to 
the width of type 'int'}}
   // expected-note@-1{{The result of the 
left shift is undefined due to shifting by '255', which is greater or equal to 
the width of type 'int'}}
 }
+
+namespace array_index_tracking {
+void consume(int);
+
+int getIndex(int x) {
+  int a;
+  if (x > 0)
+a = 3;
+  else
+a = 2;
+  return a;
+}
+
+int getInt();
+
+void testArrayIndexTracking() {
+  int arr[10];
+
+  for (int i = 0; i < 3; ++i)
+arr[i] = 0;
+  int x = getInt();
+  int n = getIndex(x);
+  consume(arr[n]);
+}
+} // end of namespace array_index_tracking


Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1740,6 +1740,10 @@
   if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode))
 trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
 
+  if (const auto *Arr = dyn_cast(Inner))
+trackExpressionValue(
+LVNode, Arr->getIdx(), report, EnableNullFPSuppression);
+
   // See if the expression we're interested refers to a variable.
   // If so, we can track both its contents and constraints on its value.
   if (ExplodedGraph::isInterestingLValueExpr(Inner)) {
Index: cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
+++ cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -17,3 +17,28 @@
   (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
   // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
 }
+
+namespace array_index_tracking {
+void consume(int);
+
+int getIndex(int x) {
+  int a;
+  if (x > 0)
+a = 3;
+  else
+a = 2;
+  return a;
+}
+
+int getInt();
+
+void testArrayIndexTracking() {
+  int arr[10];
+
+  for (int i = 0; i < 3; ++i)
+arr[i] = 0;
+  int x = getInt();
+  int n = getIndex(x);
+  consume(arr[n]);
+}
+} // end of namespace array_index_tracking
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r363510 - [analyzer] Track indices of arrays

2019-06-16 Thread Kristof Umann via cfe-commits

Author: szelethus
Date: Sun Jun 16 07:52:56 2019
New Revision: 363510

URL: http://llvm.org/viewvc/llvm-project?rev=363510&view=rev
Log:
[analyzer] Track indices of arrays

Often times, when an ArraySubscriptExpr was reported as null or
undefined, the bug report was difficult to understand, because the
analyzer explained why arr[i] has that value, but didn't realize that in
fact i's value is very important as well. This patch fixes this by
tracking the indices of arrays.

Differential Revision: https://reviews.llvm.org/D63080

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=363510&r1=363509&r2=363510&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Sun Jun 16 
07:52:56 2019
@@ -1740,6 +1740,10 @@ bool bugreporter::trackExpressionValue(c
   if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, 
LVNode))
 trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression);
 
+  if (const auto *Arr = dyn_cast(Inner))
+trackExpressionValue(
+LVNode, Arr->getIdx(), report, EnableNullFPSuppression);
+
   // See if the expression we're interested refers to a variable.
   // If so, we can track both its contents and constraints on its value.
   if (ExplodedGraph::isInterestingLValueExpr(Inner)) {

Modified: cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp?rev=363510&r1=363509&r2=363510&view=diff
==
--- cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp (original)
+++ cfe/trunk/test/Analysis/diagnostics/track_subexpressions.cpp Sun Jun 16 
07:52:56 2019
@@ -17,3 +17,28 @@ void shift_by_undefined_value() {
   (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the 
left shift is undefined due to shifting by '255', which is greater or equal to 
the width of type 'int'}}
   // expected-note@-1{{The result of the 
left shift is undefined due to shifting by '255', which is greater or equal to 
the width of type 'int'}}
 }
+
+namespace array_index_tracking {
+void consume(int);
+
+int getIndex(int x) {
+  int a;
+  if (x > 0)
+a = 3;
+  else
+a = 2;
+  return a;
+}
+
+int getInt();
+
+void testArrayIndexTracking() {
+  int arr[10];
+
+  for (int i = 0; i < 3; ++i)
+arr[i] = 0;
+  int x = getInt();
+  int n = getIndex(x);
+  consume(arr[n]);
+}
+} // end of namespace array_index_tracking


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

[PATCH] D63086: [analyzer][NoStoreFuncVisitor][NFC] Move methods out-of-line, turn some to static functions

2019-06-16 Thread Kristóf Umann via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363509: [analyzer][NFC] Tease apart and clang-format 
NoStoreFuncVisitor (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D63086?vs=203865&id=204946#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63086/new/

https://reviews.llvm.org/D63086

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -196,37 +196,6 @@
   return {};
 }
 
-//===--===//
-// Implementation of BugReporterVisitor.
-//===--===//
-
-std::shared_ptr
-BugReporterVisitor::getEndPath(BugReporterContext &,
-   const ExplodedNode *, BugReport &) {
-  return nullptr;
-}
-
-void
-BugReporterVisitor::finalizeVisitor(BugReporterContext &,
-const ExplodedNode *, BugReport &) {}
-
-std::shared_ptr BugReporterVisitor::getDefaultEndPath(
-BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
-  PathDiagnosticLocation L =
-PathDiagnosticLocation::createEndOfPath(EndPathNode,BRC.getSourceManager());
-
-  const auto &Ranges = BR.getRanges();
-
-  // Only add the statement itself as a range if we didn't specify any
-  // special ranges for this report.
-  auto P = std::make_shared(
-  L, BR.getDescription(), Ranges.begin() == Ranges.end());
-  for (SourceRange Range : Ranges)
-P->addRange(Range);
-
-  return P;
-}
-
 /// \return name of the macro inside the location \p Loc.
 static StringRef getMacroName(SourceLocation Loc,
 BugReporterContext &BRC) {
@@ -251,36 +220,70 @@
 }
 
 /// \return Whether \c RegionOfInterest was modified at \p N,
-/// where \p ReturnState is a state associated with the return
-/// from the current frame.
-static bool wasRegionOfInterestModifiedAt(
-const SubRegion *RegionOfInterest,
-const ExplodedNode *N,
-SVal ValueAfter) {
+/// where \p ValueAfter is \c RegionOfInterest's value at the end of the
+/// stack frame.
+static bool wasRegionOfInterestModifiedAt(const SubRegion *RegionOfInterest,
+  const ExplodedNode *N,
+  SVal ValueAfter) {
   ProgramStateRef State = N->getState();
   ProgramStateManager &Mgr = N->getState()->getStateManager();
 
-  if (!N->getLocationAs()
-  && !N->getLocationAs()
-  && !N->getLocationAs())
+  if (!N->getLocationAs() && !N->getLocationAs() &&
+  !N->getLocationAs())
 return false;
 
   // Writing into region of interest.
   if (auto PS = N->getLocationAs())
 if (auto *BO = PS->getStmtAs())
   if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(
-N->getSVal(BO->getLHS()).getAsRegion()))
+  N->getSVal(BO->getLHS()).getAsRegion()))
 return true;
 
   // SVal after the state is possibly different.
   SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
-  if (!Mgr.getSValBuilder().areEqual(State, ValueAtN, ValueAfter).isConstrainedTrue() &&
+  if (!Mgr.getSValBuilder()
+   .areEqual(State, ValueAtN, ValueAfter)
+   .isConstrainedTrue() &&
   (!ValueAtN.isUndef() || !ValueAfter.isUndef()))
 return true;
 
   return false;
 }
 
+//===--===//
+// Implementation of BugReporterVisitor.
+//===--===//
+
+std::shared_ptr
+BugReporterVisitor::getEndPath(BugReporterContext &,
+   const ExplodedNode *, BugReport &) {
+  return nullptr;
+}
+
+void
+BugReporterVisitor::finalizeVisitor(BugReporterContext &,
+const ExplodedNode *, BugReport &) {}
+
+std::shared_ptr BugReporterVisitor::getDefaultEndPath(
+BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
+  PathDiagnosticLocation L =
+PathDiagnosticLocation::createEndOfPath(EndPathNode,BRC.getSourceManager());
+
+  const auto &Ranges = BR.getRanges();
+
+  // Only add the statement itself as a range if we didn't specify any
+  // special ranges for this report.
+  auto P = std::make_shared(
+  L, BR.getDescription(), Ranges.begin() == Ranges.end());
+  for (SourceRange Range : Ranges)
+P->addRange(Range);
+
+  return P;
+}
+
+//===--===//
+// Implementation of NoStoreFuncVisitor.
+//===

r363509 - [analyzer][NFC] Tease apart and clang-format NoStoreFuncVisitor

2019-06-16 Thread Kristof Umann via cfe-commits

Author: szelethus
Date: Sun Jun 16 07:09:11 2019
New Revision: 363509

URL: http://llvm.org/viewvc/llvm-project?rev=363509&view=rev
Log:
[analyzer][NFC] Tease apart and clang-format NoStoreFuncVisitor

Make several methods static functions
Move non-trivial methods out-of-line
Add a divider
Turn non-obvious autos into Optional
clang-format affected lines

Differential Revision: https://reviews.llvm.org/D63086

Modified:
cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp?rev=363509&r1=363508&r2=363509&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp Sun Jun 16 
07:09:11 2019
@@ -196,37 +196,6 @@ getConcreteIntegerValue(const Expr *Cond
   return {};
 }
 
-//===--===//
-// Implementation of BugReporterVisitor.
-//===--===//
-
-std::shared_ptr
-BugReporterVisitor::getEndPath(BugReporterContext &,
-   const ExplodedNode *, BugReport &) {
-  return nullptr;
-}
-
-void
-BugReporterVisitor::finalizeVisitor(BugReporterContext &,
-const ExplodedNode *, BugReport &) {}
-
-std::shared_ptr BugReporterVisitor::getDefaultEndPath(
-BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
-  PathDiagnosticLocation L =
-
PathDiagnosticLocation::createEndOfPath(EndPathNode,BRC.getSourceManager());
-
-  const auto &Ranges = BR.getRanges();
-
-  // Only add the statement itself as a range if we didn't specify any
-  // special ranges for this report.
-  auto P = std::make_shared(
-  L, BR.getDescription(), Ranges.begin() == Ranges.end());
-  for (SourceRange Range : Ranges)
-P->addRange(Range);
-
-  return P;
-}
-
 /// \return name of the macro inside the location \p Loc.
 static StringRef getMacroName(SourceLocation Loc,
 BugReporterContext &BRC) {
@@ -251,36 +220,70 @@ static bool isFunctionMacroExpansion(Sou
 }
 
 /// \return Whether \c RegionOfInterest was modified at \p N,
-/// where \p ReturnState is a state associated with the return
-/// from the current frame.
-static bool wasRegionOfInterestModifiedAt(
-const SubRegion *RegionOfInterest,
-const ExplodedNode *N,
-SVal ValueAfter) {
+/// where \p ValueAfter is \c RegionOfInterest's value at the end of the
+/// stack frame.
+static bool wasRegionOfInterestModifiedAt(const SubRegion *RegionOfInterest,
+  const ExplodedNode *N,
+  SVal ValueAfter) {
   ProgramStateRef State = N->getState();
   ProgramStateManager &Mgr = N->getState()->getStateManager();
 
-  if (!N->getLocationAs()
-  && !N->getLocationAs()
-  && !N->getLocationAs())
+  if (!N->getLocationAs() && !N->getLocationAs() &&
+  !N->getLocationAs())
 return false;
 
   // Writing into region of interest.
   if (auto PS = N->getLocationAs())
 if (auto *BO = PS->getStmtAs())
   if (BO->isAssignmentOp() && RegionOfInterest->isSubRegionOf(
-N->getSVal(BO->getLHS()).getAsRegion()))
+  N->getSVal(BO->getLHS()).getAsRegion()))
 return true;
 
   // SVal after the state is possibly different.
   SVal ValueAtN = N->getState()->getSVal(RegionOfInterest);
-  if (!Mgr.getSValBuilder().areEqual(State, ValueAtN, 
ValueAfter).isConstrainedTrue() &&
+  if (!Mgr.getSValBuilder()
+   .areEqual(State, ValueAtN, ValueAfter)
+   .isConstrainedTrue() &&
   (!ValueAtN.isUndef() || !ValueAfter.isUndef()))
 return true;
 
   return false;
 }
 
+//===--===//
+// Implementation of BugReporterVisitor.
+//===--===//
+
+std::shared_ptr
+BugReporterVisitor::getEndPath(BugReporterContext &,
+   const ExplodedNode *, BugReport &) {
+  return nullptr;
+}
+
+void
+BugReporterVisitor::finalizeVisitor(BugReporterContext &,
+const ExplodedNode *, BugReport &) {}
+
+std::shared_ptr BugReporterVisitor::getDefaultEndPath(
+BugReporterContext &BRC, const ExplodedNode *EndPathNode, BugReport &BR) {
+  PathDiagnosticLocation L =
+
PathDiagnosticLocation::createEndOfPath(EndPathNode,BRC.getSourceManager());
+
+  const auto &Ranges = BR.getRanges();
+
+  // Only add the statement itself as a range if we didn't specify any
+  // special ranges for this report.
+  auto P = std::make_shared(
+  L, BR.getDescription(), Ranges.begin() == Ranges.end());
+  for (SourceRa

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

2019-06-16 Thread Dávid Bolvanský via Phabricator via cfe-commits

xbolva00 added a comment.

Ping


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63082/new/

https://reviews.llvm.org/D63082



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

[PATCH] D63222: [Clangd] Fixed clangd diagnostics priority

2019-06-16 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

Code gets shorter, tests get longer, very nice :-)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63222/new/

https://reviews.llvm.org/D63222



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

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D60974: Clang IFSO driver action.

[PATCH] D60974: Clang IFSO driver action.

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[PATCH] D62619: [analyzer][Dominators] Add a control dependency calculator + a new debug checker

[PATCH] D63088: [clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

[PATCH] D63088: [clang-tidy] misc-unused-parameters: don't comment out parameter name for C code

r363521 - [docs] Fix another bot error by setting highlight language of objc code-block to objc instead of c++.

[PATCH] D62883: [analyzer] Track conditions of terminator statements on which the reported node depends on

[clang-tools-extra] r363520 - [docs] Fix another bot warning by adding a blank line to separate the `option::` command from the text below.

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[clang-tools-extra] r363518 - [docs] Fix a few problems with clang-tool docs to get the bots green again.

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[PATCH] D63009: [OpenMP] Add target task alloc function with device ID

r363515 - [analyzer] ReturnVisitor: more portable test case

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[PATCH] D62899: [analyzer][UninitializedObjectChecker] Mark uninitialized regions as interesting.

[PATCH] D63009: [OpenMP] Add target task alloc function with device ID

[PATCH] D63387: clang: Promote -fdebug-compilation-dir from cc1 flag to driver-level flag

r363512 - [analyzer] Push correct version of 'Track indices of arrays'

[PATCH] D63080: [analyzer] Track indices of arrays

r363510 - [analyzer] Track indices of arrays

[PATCH] D63086: [analyzer][NoStoreFuncVisitor][NFC] Move methods out-of-line, turn some to static functions

r363509 - [analyzer][NFC] Tease apart and clang-format NoStoreFuncVisitor

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

[PATCH] D63222: [Clangd] Fixed clangd diagnostics priority

33 matches

Site Navigation

Mail list logo

Footer information