[PATCH] D156192: [-Wunsafe-buffer-usage] Stop generating incorrect fix-its for variable declarations with unsupported specifiers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:1387
+// with the source range that is being replaced using fix-its.  Especially when
+// we often cannot obtain accruate source ranges of cv-qualified type
+// specifiers.

typo: "accurate"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156192

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

[PATCH] D153059: [-Wunsafe-buffer-usage] Group parameter fix-its

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I vote for splitting the patch to make both the review and any future debugging 
or git archeology easier.


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

https://reviews.llvm.org/D153059

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

[PATCH] D146669: [-Wunsafe-buffer-usage] Hide fixits/suggestions behind an extra flag, -fsafe-buffer-usage-suggestions.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11792
   "change type of '%0' to '%select{std::span|std::array|std::span::iterator}1' 
to preserve bounds information">;
+def note_safe_buffer_usage_suggestions_disabled : Note<
+  "pass -fsafe-buffer-usage-suggestions to receive code hardening 
suggestions">;

I like this idea!

Thinking about the questions you raised:
> Do we really need that? Maybe we should somehow throttle it to reduce noise?

The only case I can see value in not emitting this note is when the user passes 
`-fno-safe-buffer-usage-suggestions`. In that case it can be argued that the 
user is well aware of the flag since they explicitly turned it off.

That being said I am fine with the current behavior and in either case expect 
we will tweak this once we get feedback from the users.


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

https://reviews.llvm.org/D146669

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

[PATCH] D145993: [-Wunsafe-buffer-usage] Reducing non-determinism in diagnostics output stream

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.

+1 to moving the comparator to the .cpp file; otherwise LGTM!
Thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145993

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

[PATCH] D143128: [-Wunsafe-buffer-usage] Fix-Its transforming `[any]` to `(DRE.data() + any)`

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

This is an interesting topic. In the abstract I see the question as should the 
Fix-Its prioritize how the code will fit the desired end state (presumably 
modern idiomatic C++) or carefully respect the state of the code as is now.

The only thing I feel pretty strongly about is that no matter what philosophy 
we decide to use here we should apply it consistently to all our Fix-Its (which 
might or might not already be the case).

And FWIW I can also imagine at some point in the future we might either have 
two dialects of the Fix-Its or that a separate modernizer tool (completely 
independent of Safe Buffers) could suggest transformations like:
"Would you like to change `()[any]` to `(DRE.data() + any)`?"


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

https://reviews.llvm.org/D143128

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

[PATCH] D143455: [-Wunsafe-buffer-usage] Emit Fix-Its only for C++20 and later standards

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

We actually have only commits with warnings in `release/16.x` - no Fix-Its. So 
no need to cherry-pick.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143455

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

[PATCH] D143455: [-Wunsafe-buffer-usage] Emit Fix-Its only for C++20 and later standards

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG8b6ae9bd7466: [-Wunsafe-buffer-usage] Emit Fix-Its only for 
C++20 and later standards (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D143455?vs=495345=496291#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143455

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-no-fixits.cpp
@@ -0,0 +1,82 @@
+// RUN: %clang_cc1 -x c -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c -std=c89 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=gnu89 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=iso9899:1990 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c -std=c17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=gnu17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=iso9899:2017 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -std=c2x -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x c++ -std=c++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=gnu++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=c++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=gnu++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// RUN: %clang_cc1 -x objective-c++ -std=c++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=gnu++98 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=c++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x objective-c++ -std=gnu++17 -Wunsafe-buffer-usage -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// CHECK-NOT: fix-it:
+
+typedef int * Int_ptr_t;
+typedef int Int_t;
+
+void local_array_subscript_simple() {
+  int tmp;
+  int *p;
+  const int *q;
+  tmp = p[5];
+  tmp = q[5];
+
+  Int_ptr_t x;
+  Int_ptr_t y;
+  Int_t * z;
+  Int_t * w;
+
+  tmp = x[5];
+  tmp = y[5];
+  tmp = z[5];
+  tmp = w[5];
+}
+
+void local_ptr_to_array() {
+  int tmp;
+  int n = 10;
+  int a[10];
+  int b[n];
+  int *p = a;
+  int *q = b;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void local_ptr_addrof_init() {
+  int var;
+  int * q = 
+  var = q[5];
+}
+
+void decl_without_init() {
+  int tmp;
+  int * p;
+  Int_ptr_t q;
+  tmp = p[5];
+  tmp = q[5];
+}
+
+void explict_cast() {
+  int tmp;
+  int * p;
+  tmp = p[5];
+
+  int a;
+  char * q = (char *)
+  tmp = (int) q[5];
+
+  void * r = 
+  char * s = (char *) r;
+  tmp = (int) s[5];
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -2516,7 +2516,9 @@
   if (!Diags.isIgnored(diag::warn_unsafe_buffer_operation, D->getBeginLoc()) ||
   !Diags.isIgnored(diag::warn_unsafe_buffer_variable, D->getBeginLoc())) {
 UnsafeBufferUsageReporter R(S);
-checkUnsafeBufferUsage(D, R);
+checkUnsafeBufferUsage(
+D, R,
+/*EmitFixits=*/S.getLangOpts().CPlusPlus20);
   }
 
   // If none of the previous checks caused a CFG build, trigger one here
Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -1008,7 +1008,8 @@
 }
 
 void clang::checkUnsafeBufferUsage(const Decl *D,
-   UnsafeBufferUsageHandler ) {
+   UnsafeBufferUsageHandler ,
+   bool EmitFixits) {
   assert(D && D->getBody());
 
   WarningGadgetSets UnsafeOps;
@@ -1016,40 +1017,46 @@
   DeclUseTracker Tracker;
 
   {
+// FIXME: We could skip even matching Fixables' matchers if EmitFixits ==
+// 

[PATCH] D143128: [-Wunsafe-buffer-usage][WIP] Fix-Its transforming `[any]` to `(DRE.data() + any)`

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:162
+   InnerMatcher)),
+   unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
+  auto CastOperandMatcher =

ziqingluo-90 wrote:
> jkorous wrote:
> > I am just wondering how does the callee matcher work in situation with 
> > multiple re-declarations 樂 
> > 
> > Something like this:
> > ```
> > void foo(int* ptr);
> > [[clang::unsafe_buffer_usage]] void foo(int* ptr);
> > void foo(int* ptr);
> > 
> > void bar(int* ptr) {
> >   foo(ptr);
> > }
> > ```
> I think we are fine.  According to the doc of `FunctionDecl`:
> ```
> /// Represents a function declaration or definition.
> ///
> /// Since a given function can be declared several times in a program,
> /// there may be several FunctionDecls that correspond to that
> /// function. Only one of those FunctionDecls will be found when
> /// traversing the list of declarations in the context of the
> /// FunctionDecl (e.g., the translation unit); this FunctionDecl
> /// contains all of the information known about the function. Other,
> /// previous declarations of the function are available via the
> /// getPreviousDecl() chain.
> ```
I see! Sound like we should be fine indeed and the test seems to confirm.
Thank you!



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:508
+  hasUnaryOperand(arraySubscriptExpr(
+  hasBase(ignoringParenImpCasts(declRefExpr())
+.bind(UPCAddressofArraySubscriptTag);

ziqingluo-90 wrote:
> jkorous wrote:
> > I am wondering what will happen in the weird corner-case of `&5[ptr]` - I 
> > feel the Fix-It we produce would be incorrect.
> > 
> > Here's a suggestion - we could use `hasLHS` instead of `hasBase` here and 
> > add a FIXME that when we find the time we should also properly support the 
> > corner-case. That would be a pretty low-priority though - we definitely 
> > have more important patterns to support first.
> > 
> > WDYT?
> > 
> I'm not sure if I understand your concern.  For `&5[ptr]`, we will generate a 
> fix-it `ptr.data() + 5` in cases `ptr` is assigned a `span` strategy.   It is 
> same as the case of `[5]`.
Oh, my bad! I assumed (AKA didn't check) that we're just replacing the parts of 
the code around the DRE and index.
You're right. Please ignore me :)


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

https://reviews.llvm.org/D143128

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.

LGTM
Thank you!


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

https://reviews.llvm.org/D139737

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

[PATCH] D143128: [-Wunsafe-buffer-usage][WIP] Fix-Its transforming `[any]` to `DRE.data() + any`

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:162
+   InnerMatcher)),
+   unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage);
+  auto CastOperandMatcher =

I am just wondering how does the callee matcher work in situation with multiple 
re-declarations 樂 

Something like this:
```
void foo(int* ptr);
[[clang::unsafe_buffer_usage]] void foo(int* ptr);
void foo(int* ptr);

void bar(int* ptr) {
  foo(ptr);
}
```



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:508
+  hasUnaryOperand(arraySubscriptExpr(
+  hasBase(ignoringParenImpCasts(declRefExpr())
+.bind(UPCAddressofArraySubscriptTag);

I am wondering what will happen in the weird corner-case of `&5[ptr]` - I feel 
the Fix-It we produce would be incorrect.

Here's a suggestion - we could use `hasLHS` instead of `hasBase` here and add a 
FIXME that when we find the time we should also properly support the 
corner-case. That would be a pretty low-priority though - we definitely have 
more important patterns to support first.

WDYT?




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:810
+  case Strategy::Kind::Span:
+return fixUPCAddressofArraySubscriptWithSpan(Node);
+  case Strategy::Kind::Wontfix:

Since we use `std::nullopt` in `getFixits` to signal errors - we should either 
use the same strategy in `fixUPCAddressofArraySubscriptWithSpan` or translate 
the empty return value from it to `nullopt` here.
(FWIWI I am leaning towards the former.)
Forwarding the empty Fix-It would be incorrect.




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

https://reviews.llvm.org/D143128

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

jkorous wrote:
> jkorous wrote:
> > I am afraid I might have found one more problem :(
> > I believe that for `span` strategy we have to make sure the index is > 0. 
> > Otherwise 
> > That means either an unsigned integer or signed or unsigned literal that is 
> > greater than 0.
> > For the literal you can take inspiration here:
> > https://reviews.llvm.org/D142795
> > 
> @ziqingluo-90 Sorry, looks like I wasn't clear here.
> One case (that you've already addressed) is `ptr[-5]` - for that we can't use 
> `std::span::operator[]` as it would immediately trap.
> But there's the other case of:
> ```
> uint8_t foo(uint8_t *ptr, int idx) {
>   return ptr[idx]
> }
> ```
> If anyone uses a value that's both signed and not a compile-time constant 
> then our compile-time analysis can not prove that the index is always >= 0 
> and consequently we can't use `std::span::operator[]` as a replacement.
> That's why I think we really need to make sure that the index is ether a) 
> positive literal or b) unsigned.
> WDYT?
> 
> 
And yes ... I was wrong - literal `0` is totally fine. Thanks for spotting that!



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:894
+if (VD->getType()->isPointerType())
+  return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+return {};

NoQ wrote:
> jkorous wrote:
> > I believe we should add another condition here: `VD->isLocalVarDecl()` as 
> > we don't support globals (yet?).
> > We run the matcher with `any_ds` tag only on function bodies so we won't 
> > discover globals anyway and the `assert(It != Defs.end() && "Definition 
> > never discovered!");` would fail.
> I think this check should happen much earlier. Like, we shouldn't define a 
> strategy for globals, and we shouldn't build fixables out of them. And then 
> `assert()` here, just to double-check.
Fair enough, we can do that. Maybe a follow-up patch?


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

jkorous wrote:
> I am afraid I might have found one more problem :(
> I believe that for `span` strategy we have to make sure the index is > 0. 
> Otherwise 
> That means either an unsigned integer or signed or unsigned literal that is 
> greater than 0.
> For the literal you can take inspiration here:
> https://reviews.llvm.org/D142795
> 
@ziqingluo-90 Sorry, looks like I wasn't clear here.
One case (that you've already addressed) is `ptr[-5]` - for that we can't use 
`std::span::operator[]` as it would immediately trap.
But there's the other case of:
```
uint8_t foo(uint8_t *ptr, int idx) {
  return ptr[idx]
}
```
If anyone uses a value that's both signed and not a compile-time constant then 
our compile-time analysis can not prove that the index is always >= 0 and 
consequently we can't use `std::span::operator[]` as a replacement.
That's why I think we really need to make sure that the index is ether a) 
positive literal or b) unsigned.
WDYT?




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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:703
+  case Strategy::Kind::Span:
+return FixItList{};
+  case Strategy::Kind::Wontfix:

I am afraid I might have found one more problem :(
I believe that for `span` strategy we have to make sure the index is > 0. 
Otherwise 
That means either an unsigned integer or signed or unsigned literal that is 
greater than 0.
For the literal you can take inspiration here:
https://reviews.llvm.org/D142795



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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:894
+if (VD->getType()->isPointerType())
+  return fixVariableWithSpan(VD, Tracker, Ctx, Handler);
+return {};

I believe we should add another condition here: `VD->isLocalVarDecl()` as we 
don't support globals (yet?).
We run the matcher with `any_ds` tag only on function bodies so we won't 
discover globals anyway and the `assert(It != Defs.end() && "Definition never 
discovered!");` would fail.


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I got a test failure in 
`SemaCXX/warn-unsafe-buffer-usage-fixits-local-var-span.cpp` which I believe is 
caused solely by the fact that we emit the diagnostics in different order.
I am not sure it matters and since the Fix-Its clearly specify what source 
lines they apply to I suggest we simply replace every `// CHECK:` with `// 
CHECK-DAG:`.
That fixed the problem for me.


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I am sorry I haven't notice this earlier - let's fix this before we land the 
patch.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:690
+  Val.toString(Txt, 10, true);
+  return Txt.data();
+}

We either need a zero to terminate the string or pass the size of `Txt` to the 
`std::string` constructor here. (While `toString`'s name might sound like it'll 
take care of that it does not.)

Simplified testcase:
```
void local_ptr_to_array() {
  int tmp;
  int a[10];
  int *p = a;
  tmp = p[5];
}
```
what I get is (something like this):
```
void local_ptr_to_array() {
  int tmp;
  int a[10];
  std::span p {a, 10�o
};
  tmp = p[5];
}
```
The problem is that `APInt::toString` stores '1' and '0' to `Txt` but is 
missing the terminating `\0` character that `std::string` constructor expects.




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:690
+  Val.toString(Txt, 10, true);
+  return Txt.data();
+}

jkorous wrote:
> We either need a zero to terminate the string or pass the size of `Txt` to 
> the `std::string` constructor here. (While `toString`'s name might sound like 
> it'll take care of that it does not.)
> 
> Simplified testcase:
> ```
> void local_ptr_to_array() {
>   int tmp;
>   int a[10];
>   int *p = a;
>   tmp = p[5];
> }
> ```
> what I get is (something like this):
> ```
> void local_ptr_to_array() {
>   int tmp;
>   int a[10];
>   std::span p {a, 10�o
};
>   tmp = p[5];
> }
> ```
> The problem is that `APInt::toString` stores '1' and '0' to `Txt` but is 
> missing the terminating `\0` character that `std::string` constructor expects.
> 



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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:127
+static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) 
{
+  auto isLvalueToRvalueCast = [](internal::Matcher M) {
+return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),

Nit: I think we could simplify this to just

```
static auto isInUnspecifiedLvalueContext(internal::Matcher innerMatcher) {
  return implicitCastExpr(hasCastKind(CastKind::CK_LValueToRValue),
  castSubExpr(innerMatcher));
}
```


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

https://reviews.llvm.org/D139737

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

[PATCH] D141356: [-Wunsafe-buffer-usage] Group diagnostics by variable

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG237ca436adf4: [-Wunsafe-buffer-usage] Group diagnostics by 
variable (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D141356?vs=489936=490308#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141356

Files:
  clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Analysis/UnsafeBufferUsage.cpp
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/unsafe-buffer-usage-diag-type.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
===
--- clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp
@@ -1,4 +1,10 @@
-// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+// RUN: %clang_cc1 -std=c++20 -Wno-all -Wunsafe-buffer-usage -fblocks -include %s -verify %s
+
+// RUN: %clang -x c++ -fsyntax-only -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// RUN: %clang_cc1 -std=c++11 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// RUN: %clang_cc1 -std=c++20 -fblocks -include %s %s 2>&1 | FileCheck --allow-empty %s
+// CHECK-NOT: [-Wunsafe-buffer-usage]
+
 #ifndef INCLUDED
 #define INCLUDED
 #pragma clang system_header
@@ -18,41 +24,50 @@
 
 #else
 
-void testIncrement(char *p) {
-  ++p; // expected-warning{{unchecked operation on raw buffer in expression}}
-  p++; // expected-warning{{unchecked operation on raw buffer in expression}}
-  --p; // expected-warning{{unchecked operation on raw buffer in expression}}
-  p--; // expected-warning{{unchecked operation on raw buffer in expression}}
+void testIncrement(char *p) { // expected-warning{{'p' is an unsafe pointer used for buffer access}}
+  ++p; // expected-note{{used in pointer arithmetic here}}
+  p++; // expected-note{{used in pointer arithmetic here}}
+  --p; // expected-note{{used in pointer arithmetic here}}
+  p--; // expected-note{{used in pointer arithmetic here}}
 }
 
 void * voidPtrCall(void);
 char * charPtrCall(void);
 
 void testArraySubscripts(int *p, int **pp) {
-  foo(p[1], // expected-warning{{unchecked operation on raw buffer in expression}}
-  pp[1][1], // expected-warning2{{unchecked operation on raw buffer in expression}}
-  1[1[pp]], // expected-warning2{{unchecked operation on raw buffer in expression}}
-  1[pp][1]  // expected-warning2{{unchecked operation on raw buffer in expression}}
+// expected-warning@-1{{'p' is an unsafe pointer used for buffer access}}
+// expected-warning@-2{{'pp' is an unsafe pointer used for buffer access}}
+  foo(p[1], // expected-note{{used in buffer access here}}
+  pp[1][1], // expected-note{{used in buffer access here}}
+// expected-warning@-1{{unsafe buffer access}}
+  1[1[pp]], // expected-note{{used in buffer access here}}
+// expected-warning@-1{{unsafe buffer access}}
+  1[pp][1]  // expected-note{{used in buffer access here}}
+// expected-warning@-1{{unsafe buffer access}}
   );
 
-  if (p[3]) {   // expected-warning{{unchecked operation on raw buffer in expression}}
+  if (p[3]) {   // expected-note{{used in buffer access here}}
 void * q = p;
 
-foo(((int*)q)[10]); // expected-warning{{unchecked operation on raw buffer in expression}}
+foo(((int*)q)[10]); // expected-warning{{unsafe buffer access}}
   }
 
-  foo(((int*)voidPtrCall())[3], // expected-warning{{unchecked operation on raw buffer in expression}}
-  3[(int*)voidPtrCall()],   // expected-warning{{unchecked operation on raw buffer in expression}}
-  charPtrCall()[3], // expected-warning{{unchecked operation on raw buffer in expression}}
-  3[charPtrCall()]  // expected-warning{{unchecked operation on raw buffer in expression}}
+  foo(((int*)voidPtrCall())[3], // expected-warning{{unsafe buffer access}}
+  3[(int*)voidPtrCall()],   // expected-warning{{unsafe buffer access}}
+  charPtrCall()[3], // expected-warning{{unsafe buffer access}}
+  3[charPtrCall()]  // expected-warning{{unsafe buffer access}}
   );
 
-  int a[10], b[10][10];
+int a[10];  // expected-warning{{'a' is an unsafe buffer that does not perform bounds checks}}
+int b[10][10];  // expected-warning{{'b' is an unsafe buffer that does not perform bounds checks}}
 
-  foo(a[1], 1[a], // expected-warning2{{unchecked operation on 

[PATCH] D141340: [-Wunsafe-buffer-usage] Use relevant source locations for warnings

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG39a63fc7fe98: [-Wunsafe-buffer-usage] Use relevant source 
locations for warnings (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141340

Files:
  clang/lib/Sema/AnalysisBasedWarnings.cpp
  clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp

Index: clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unsafe-buffer-usage-source-ranges.cpp
@@ -0,0 +1,49 @@
+// RUN: %clang_cc1 -Wno-everything -Wunsafe-buffer-usage -fdiagnostics-print-source-range-info %s 2>&1 | FileCheck %s
+
+void foo(int i) {
+  int * ptr;
+
+
+  ptr++;
+  // CHECK-DAG: {7:3-7:6}{{.*}}[-Wunsafe-buffer-usage]
+  ptr--;
+  // CHECK-DAG: {9:3-9:6}{{.*}}[-Wunsafe-buffer-usage]
+  ++ptr;
+  // CHECK-DAG: {11:5-11:8}{{.*}}[-Wunsafe-buffer-usage]
+  --ptr;
+  // CHECK-DAG: {13:5-13:8}{{.*}}[-Wunsafe-buffer-usage]
+
+
+  ptr + 1;
+  // CHECK-DAG: {17:3-17:6}{{.*}}[-Wunsafe-buffer-usage]
+  2 + ptr;
+  // CHECK-DAG: {19:7-19:10}{{.*}}[-Wunsafe-buffer-usage]
+  ptr + i;
+  // CHECK-DAG: {21:3-21:6}{{.*}}[-Wunsafe-buffer-usage]
+  i + ptr;
+  // CHECK-DAG: {23:7-23:10}{{.*}}[-Wunsafe-buffer-usage]
+
+
+  ptr - 3;
+  // CHECK-DAG: {27:3-27:6}{{.*}}[-Wunsafe-buffer-usage]
+  ptr - i;
+  // CHECK-DAG: {29:3-29:6}{{.*}}[-Wunsafe-buffer-usage]
+
+
+  ptr += 4;
+  // CHECK-DAG: {33:3-33:6}{{.*}}[-Wunsafe-buffer-usage]
+  ptr += i;
+  // CHECK-DAG: {35:3-35:6}{{.*}}[-Wunsafe-buffer-usage]
+
+
+  ptr -= 5;
+  // CHECK-DAG: {39:3-39:6}{{.*}}[-Wunsafe-buffer-usage]
+  ptr -= i;
+  // CHECK-DAG: {41:3-41:6}{{.*}}[-Wunsafe-buffer-usage]
+
+
+  ptr[5];
+  // CHECK-DAG: {45:3-45:6}{{.*}}[-Wunsafe-buffer-usage]
+  5[ptr];
+  // CHECK-DAG: {47:5-47:8}{{.*}}[-Wunsafe-buffer-usage]
+}
Index: clang/lib/Sema/AnalysisBasedWarnings.cpp
===
--- clang/lib/Sema/AnalysisBasedWarnings.cpp
+++ clang/lib/Sema/AnalysisBasedWarnings.cpp
@@ -16,8 +16,10 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
+#include "clang/AST/OperationKinds.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/StmtCXX.h"
@@ -2152,8 +2154,35 @@
   UnsafeBufferUsageReporter(Sema ) : S(S) {}
 
   void handleUnsafeOperation(const Stmt *Operation) override {
-S.Diag(Operation->getBeginLoc(), diag::warn_unsafe_buffer_expression)
-<< Operation->getSourceRange();
+SourceLocation Loc;
+SourceRange Range;
+if (const auto *ASE = dyn_cast(Operation)) {
+  Loc = ASE->getBase()->getExprLoc();
+  Range = ASE->getBase()->getSourceRange();
+} else if (const auto *BO = dyn_cast(Operation)) {
+  BinaryOperator::Opcode Op = BO->getOpcode();
+  if (Op == BO_Add || Op == BO_AddAssign || Op == BO_Sub ||
+  Op == BO_SubAssign) {
+if (BO->getRHS()->getType()->isIntegerType()) {
+  Loc = BO->getLHS()->getExprLoc();
+  Range = BO->getLHS()->getSourceRange();
+} else {
+  Loc = BO->getRHS()->getExprLoc();
+  Range = BO->getRHS()->getSourceRange();
+}
+  }
+} else if (const auto *UO = dyn_cast(Operation)) {
+  UnaryOperator::Opcode Op = UO->getOpcode();
+  if (Op == UO_PreInc || Op == UO_PreDec || Op == UO_PostInc ||
+  Op == UO_PostDec) {
+Loc = UO->getSubExpr()->getExprLoc();
+Range = UO->getSubExpr()->getSourceRange();
+  }
+} else {
+  Loc = Operation->getBeginLoc();
+  Range = Operation->getSourceRange();
+}
+S.Diag(Loc, diag::warn_unsafe_buffer_expression) << Range;
   }
 
   void handleFixableVariable(const VarDecl *Variable,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141333: [-Wunsafe-buffer-usage][NFC] Refactor checkUnsafeBufferUsage

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG214312ef7ee4: [-Wunsafe-buffer-usage][NFC] Refactor 
checkUnsafeBufferUsage (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D141333?vs=488387=490008#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141333

Files:
  clang/lib/Analysis/UnsafeBufferUsage.cpp

Index: clang/lib/Analysis/UnsafeBufferUsage.cpp
===
--- clang/lib/Analysis/UnsafeBufferUsage.cpp
+++ clang/lib/Analysis/UnsafeBufferUsage.cpp
@@ -7,9 +7,10 @@
 //===--===//
 
 #include "clang/Analysis/Analyses/UnsafeBufferUsage.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "llvm/ADT/SmallVector.h"
+#include 
 #include 
 
 using namespace llvm;
@@ -383,6 +384,7 @@
   DeclUseTracker() = default;
   DeclUseTracker(const DeclUseTracker &) = delete; // Let's avoid copies.
   DeclUseTracker(DeclUseTracker &&) = default;
+  DeclUseTracker =(DeclUseTracker &&) = default;
 
   // Start tracking a freshly discovered DRE.
   void discoverUse(const DeclRefExpr *DRE) { Uses->insert(DRE); }
@@ -556,97 +558,137 @@
   return {std::move(CB.FixableGadgets), std::move(CB.WarningGadgets), std::move(CB.Tracker)};
 }
 
-void clang::checkUnsafeBufferUsage(const Decl *D,
-   UnsafeBufferUsageHandler ) {
-  assert(D && D->getBody());
-
-  SmallSet WarnedDecls;
-
-  auto [FixableGadgets, WarningGadgets, Tracker] = findGadgets(D);
-
-  DenseMap,
-std::vector>> Map;
+struct WarningGadgetSets {
+  std::map>> byVar;
+  // These Gadgets are not related to pointer variables (e. g. temporaries).
+  llvm::SmallVector, 16> noVar;
+};
 
-  // First, let's sort gadgets by variables. If some gadgets cover more than one
+static WarningGadgetSets
+groupWarningGadgetsByVar(WarningGadgetList &) {
+  WarningGadgetSets result;
+  // If some gadgets cover more than one
   // variable, they'll appear more than once in the map.
-  for (const auto  : FixableGadgets) {
-DeclUseList DREs = G->getClaimedVarUseSites();
+  for (auto  : AllUnsafeOperations) {
+DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites();
 
-// Populate the map.
-for (const DeclRefExpr *DRE : DREs) {
+bool AssociatedWithVarDecl = false;
+for (const DeclRefExpr *DRE : ClaimedVarUseSites) {
   if (const auto *VD = dyn_cast(DRE->getDecl())) {
-Map[VD].first.push_back(G.get());
+result.byVar[VD].emplace(std::move(G));
+AssociatedWithVarDecl = true;
   }
 }
+
+if (!AssociatedWithVarDecl) {
+  result.noVar.emplace_back(std::move(G));
+  continue;
+}
   }
+  return result;
+}
 
-  for (const auto  : WarningGadgets) {
-DeclUseList ClaimedVarUseSites = G->getClaimedVarUseSites();
+struct FixableGadgetSets {
+  std::map>> byVar;
+};
 
-// Populate the map.
-bool Pushed = false;
-for (const DeclRefExpr *DRE : ClaimedVarUseSites) {
+static FixableGadgetSets
+groupFixablesByVar(FixableGadgetList &) {
+  FixableGadgetSets FixablesForUnsafeVars;
+  for (auto  : AllFixableOperations) {
+DeclUseList DREs = F->getClaimedVarUseSites();
+
+for (const DeclRefExpr *DRE : DREs) {
   if (const auto *VD = dyn_cast(DRE->getDecl())) {
-Map[VD].second.push_back(G.get());
-Pushed = true;
+FixablesForUnsafeVars.byVar[VD].emplace(std::move(F));
   }
 }
+  }
+  return FixablesForUnsafeVars;
+}
 
-if (!Pushed) {
-  // We won't return to this gadget later. Emit the warning right away.
-  Handler.handleUnsafeOperation(G->getBaseStmt());
-  continue;
+static std::map
+getFixIts(FixableGadgetSets , const Strategy ) {
+  std::map FixItsForVariable;
+  for (const auto &[VD, Fixables] : FixablesForUnsafeVars.byVar) {
+// TODO fixVariable - fixit for the variable itself
+bool ImpossibleToFix = false;
+llvm::SmallVector FixItsForVD;
+for (const auto  : Fixables) {
+  llvm::Optional Fixits = F->getFixits(S);
+  if (!Fixits) {
+ImpossibleToFix = true;
+break;
+  } else {
+const FixItList CorrectFixes = Fixits.value();
+FixItsForVD.insert(FixItsForVD.end(), CorrectFixes.begin(),
+   CorrectFixes.end());
+  }
 }
+if (ImpossibleToFix)
+  FixItsForVariable.erase(VD);
+else
+  FixItsForVariable[VD].insert(FixItsForVariable[VD].end(),
+   FixItsForVD.begin(), FixItsForVD.end());
   }
+  return FixItsForVariable;
+}
 
+static Strategy
+getNaiveStrategy(const 

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:46
+/// The text indicating that the user needs to provide input there:
+constexpr static const char *const UserFillPlaceHolder = "...";
 } // end namespace clang

jkorous wrote:
> Should we rather pick something that is syntactically incorrect in C++ in 
> order to prevent accidental silent corruption of the sources?
> FWIW Xcode uses `<#placeholder#>` syntax.
Correction - it is not Xcode, it is clang itself.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CodeCompleteConsumer.cpp#L323


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D139737#4042093 , @NoQ wrote:

> It sounds to me as if by landing the patch we'll temporarily make the 
> compiler emit incorrect fixes. I think we should avoid doing that. Is it 
> possible to wait until our first proof-of-concept `FixableGadget` lands 
> before landing this patch? I think there shouldn't be too much conflict 
> between such patches.

Any patch with `FixableGadget` will face the same problem - i. e. we still 
won't emit the fixit until this patch has landed.
We decided to include a simple `Fixable` in this patch to unlock testing.


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h:46
+/// The text indicating that the user needs to provide input there:
+constexpr static const char *const UserFillPlaceHolder = "...";
 } // end namespace clang

Should we rather pick something that is syntactically incorrect in C++ in order 
to prevent accidental silent corruption of the sources?
FWIW Xcode uses `<#placeholder#>` syntax.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp:1
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp

I am starting to think we should split up our tests to allow for less conflicts 
between patches.
Could we please rename the file to a more specific name e. g. 
`warn-unsafe-buffer-usage-fixits-local-var-span.cpp`?



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage-fixits.cpp:1
+// RUN: %clang_cc1 -std=c++20 -Wunsafe-buffer-usage -verify %s
+// RUN: cp %s %t.cpp

jkorous wrote:
> I am starting to think we should split up our tests to allow for less 
> conflicts between patches.
> Could we please rename the file to a more specific name e. g. 
> `warn-unsafe-buffer-usage-fixits-local-var-span.cpp`?
Also, let's check only correctness of the FixIts in this test.
I would remove `RUN` lines with `-verify` and `expected-warnings`.


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

https://reviews.llvm.org/D139737

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

[PATCH] D139737: [-Wunsafe-buffer-usage] Initiate Fix-it generation for local variable declarations

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for the rebase!

Nit: I'd just replace `std::function` with `auto` as it saves us repeating the 
parameter types (and `#include `).




Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:617
+  // Printers that print extent into OS and sets ExtKnown to true:
+  std::function PrintExpr = [, , ](const Expr *Size) {
+Size->printPretty(OS, nullptr, PP);





Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:621
+  };
+  std::function PrintAPInt = [, ](const APInt ) {
+Size.print(OS, false);





Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:625
+  };
+  std::function PrintOne = [, ](void) {
+OS << "1";




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

https://reviews.llvm.org/D139737

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

[PATCH] D140179: [WIP][-Wunsafe-buffer-usage] Add unsafe buffer checking opt-out pragmas

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2379
+for (auto UnsafeUse : UnsafeUses)
+  if (!DE.isSafeBufferOptOut(SM, UnsafeUse->getBeginLoc()))
+UnsafeUsesToReport.push_back(UnsafeUse);

NoQ wrote:
> I believe this check should be performed *much earlier*. It's not about how 
> we display unsafe usages to the user; we can exclude variables from analysis 
> entirely when all their unsafe uses are guarded by the pragma. I suspect that 
> we can drop these gadgets as early as in `findGadgets()` phase (assuming that 
> D140062 causes us to never rely on unsafe gadgets for fixits).
Let's addres this in a follow-up patch.


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

https://reviews.llvm.org/D140179

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

[PATCH] D138253: [-Wunsafe-buffer-usage] NFC: Implement fix-strategies and variable-use-claiming.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D138253

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

[PATCH] D137348: [-Wunsafe-buffer-usage] Introduce an abstraction for fixable code patterns.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D137348

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

[PATCH] D137379: [-Wunsafe-buffer-usage] Add warnings for unsafe buffer accesses by array subscript operations

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/test/SemaCXX/warn-unsafe-buffer-usage.cpp:10-13
+void foo(...);
+
+void * bar(void);
+char * baz(void);

NoQ wrote:
> ziqingluo-90 wrote:
> > steakhal wrote:
> > > NoQ wrote:
> > > > ziqingluo-90 wrote:
> > > > > steakhal wrote:
> > > > > > I would expect this test file to grow quite a bit.
> > > > > > As such, I think we should have more self-descriptive names for 
> > > > > > these functions.
> > > > > > 
> > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > Thanks for the comment.  I agree that they should have better names 
> > > > > or at least explaining comments.
> > > > > 
> > > > > > I'm also curious what's the purpose of `foo()`in the examples.
> > > > > 
> > > > > I make all testing expressions arguments of `foo` so that I do not 
> > > > > have to create statements to use these expressions while avoiding 
> > > > > irrelevant warnings.
> > > > That's pretty cool but please note that when `foo()` is declared this 
> > > > way, it becomes a "C-style variadic function" - a very exotic construct 
> > > > that you don't normally see in code (the only practical example is the 
> > > > `printf`/`scanf` family of functions). So it may be good that we cover 
> > > > this exotic case from the start, but it may also be really bad that we 
> > > > don't cover the *basic* case.
> > > > 
> > > > C++ offers a different way to declare variadic functions: //variadic 
> > > > templates// 
> > > > (https://en.cppreference.com/w/cpp/language/parameter_pack). These are 
> > > > less valuable to test because they expand to AST that's very similar to 
> > > > the basic case, but that also allows you to cover the basic case better.
> > > > 
> > > > Or you can always make yourself happy with a few overloads that cover 
> > > > all your needs, it's not like we're worried about code duplication in 
> > > > tests:
> > > > ```lang=c++
> > > > void foo(int);
> > > > void foo(int, int);
> > > > void foo(int, int, int);
> > > > void foo(int, int, int, int);
> > > > void foo(int, int, int, int, int);
> > > > void foo(int, int, int, int, int, int);
> > > > ```
> > > IMO its fine. I would probably call it `sink()` though. Ive used the same 
> > > construct for the same reason in CSA tests with this name.
> > I don't quite get what "basic case" refers to.  Could you explain it to me 
> > a little more?
> By "basic case" I mean the ordinary, non-variadic function call with 
> predefined set of arguments in the prototype.
@ziqingluo-90 If the only purpose of `foo()` is to suppress warnings about 
unused values then you might also consider just suppressing those warnings with 
relevant `-Wno...` flags.


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

https://reviews.llvm.org/D137379

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

[PATCH] D137346: -Wunsafe-buffer-usage: Initial commit - Transition away from raw buffer accesses.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM.

@aaron.ballman Do you have any objection to us landing this?


Repository:
  rC Clang

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

https://reviews.llvm.org/D137346

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

[PATCH] D136811: -Wunsafe-buffer-usage: WIP: RFC: NFC: User documentation.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@aaron.ballman We'd like to start making progress on the implementation in 
parallel to iterating on the documentation and this is our first patch:
https://reviews.llvm.org/D137346

Since we'll have about 4 people working full-time on this it isn't reasonable 
to expect you to review all our patches.
My understanding is that you are interested mostly in the user model and the 
general user interface.
We can organize reviews for most patches among ourselves (with any extra 
feedback more than welcome!) and would reach out to you specifically with the 
things that are likely to interest you and/or where a broader consensus is 
desirable.
WDYT?


Repository:
  rC Clang

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

https://reviews.llvm.org/D136811

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

[PATCH] D136811: -Wunsafe-buffer-usage: WIP: RFC: NFC: User documentation.

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/docs/SafeBuffers.rst:173
+  #pragma unsafe_buffer_usage begin
+
+Such pragmas not only enable incremental adoption with much smaller 
granularity,

aaron.ballman wrote:
> jkorous wrote:
> > aaron.ballman wrote:
> > > Have you considered allowing a short-hand form of the pragmas that are 
> > > scoped to the current block scope (or file scope if applied at global 
> > > scope)? e.g.,
> > > ```
> > > void func() {
> > >   {
> > > #pragma only_safe_buffers
> > > int stuff[10] = {};
> > > int x = stuff[1]; // Warning: you're a bad person who should feel bad
> > >   }
> > >   int array[10] = {};
> > >   int y = array[1]; // this_is_fine.png
> > > }
> > > ```
> > We discussed other options but generally prefer simple pragmas for their 
> > flexibility.
> > Our current thinking is that for readability purposes we won't allow 
> > nesting of pragmas and would have end of scope to be explicit.
> > While this might be more verbose it would be dead simple to reason about.
> > We discussed other options but generally prefer simple pragmas for their 
> > flexibility.
> 
> A single pragma with lexical scoping behaves like RAII, which is often a 
> replacement for more complex acquire/release paired operations and is a 
> pretty well-understood pattern in C++.
> 
> > Our current thinking is that for readability purposes we won't allow 
> > nesting of pragmas and would have end of scope to be explicit.
> > While this might be more verbose it would be dead simple to reason about.
> 
> True, it is easy to reason about. But not allowing nesting for readability 
> purposes removes some utility, especially for folks in C where macros are 
> much more common. e.g.,
> ```
> #define ONLY_SAFE_BUFFERS _Pragma("only_safe_buffers")
> 
> #define AWESOME_BUFFER_THING(ptr, size) ({ ONLY_SAFE_BUFFERS; (interesting 
> work goes here) })
> 
> void func(int *ptr, size_t size) {
>   ONLY_SAFE_BUFFERS;
>   ... interesting code lives here ...
>   AWESOME_BUFFER_THING(ptr, size); // Oops, this is nested.
>  .. more interesting code lives here ...
> }
> ```
> (You can hit the same situation with paired pragmas.) Because C doesn't have 
> constexpr or consteval support for functions or templates, function-like 
> macros are a pretty common interface folks use in practice and macro 
> expansion makes it somewhat hard to avoid potential surprising conflicts.
It is desirable to minimize the extent of code labeled as unsafe - if a single 
line of code is unsafe then it'd be suboptimal do label a whole scope as unsafe.
Since scopes affect objects lifetime in C++ we won't be able to generally 
recommend adding a scope around the unsafe line either as that might change 
behavior of the code.
In this sense associating the pragmas' semantics with scopes reduces their 
flexibility. Is there any benefit of doing that which would outweigh this?

Separately from that - in theory the users could just end the scope before the 
macro:
```
  ONLY_SAFE_BUFFERS_BEGIN;
  ... interesting code lives here ...
  ONLY_SAFE_BUFFERS_END;
  AWESOME_BUFFER_THING(ptr, size); // Oops, this is nested.
```

But point taken - there might be options for better ergonomics.



Comment at: clang/docs/SafeBuffers.rst:213
+
+The attribute is NOT warranted when the function has runtime protection against
+overflows, assuming hardened libc++, assuming that containers constructed

NoQ wrote:
> aaron.ballman wrote:
> > jkorous wrote:
> > > aaron.ballman wrote:
> > > > One thing I think is worth pointing out about these docs is that the 
> > > > first example effectively says "do add the attribute because the size 
> > > > passed in to the function could be wrong" and the second example says 
> > > > "don't add the attribute on the assumption that the container has the 
> > > > correct size information". The advice feels a bit conflicting to me 
> > > > because in one case we're assuming callers pass in incorrect values and 
> > > > in the other case we're assuming callers pass in correct values and the 
> > > > only distinction between them is a "container was used".  But a 
> > > > significant portion of our users don't use libc++ (they use libstdc++ 
> > > > or MS STL for example).
> > > > 
> > > > I think we should have more details on why the STL used at runtime 
> > > > doesn't matter, or if it still really does matter, we may want to 
> > > > reconsider the advice we give.
> > > > 
> > > > Also, we don't give similar advice for use of the pragmas. Should we? 
> > > > (Maybe split the advice as to when to use markings in general out into 
> > > > its own section and reference it from both the pragma and attribute 
> > > > sections?)
> > > > One thing I think is worth pointing out about these docs is that the 
> > > > first example effectively says "do add the attribute because the size 
> > > > passed in to the function could be wrong" and the second 

[PATCH] D136811: -Wunsafe-buffer-usage: WIP: RFC: NFC: User documentation.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thank you for the feedback Gábor!




Comment at: clang/docs/SafeBuffers.rst:36-37
+hardened mode where C++ classes such as ``std::vector`` and ``std::span``,
+together with their respective ``iterator`` classes, are protected
+at runtime against buffer overflows. This changes buffer overflows from
+extremely dangerous undefined behavior to predictable runtime crash.

xazax.hun wrote:
> Only buffer overflows, so comparing cases like:
> ```
> auto it1 = v1.begin();
> auto it2 = v2.begin();
> 
> if (it1 == it2) { ... }
> ```
> 
> are out of scope, right? 
> 
> The main reason I'm asking, because I am not sure how safe iterators are 
> implemented and whether the same implementation would give opportunities for 
> other safety checks without significant additional cost. 
Yes, this is out of scope for our diagnostics and code transformations. 
@ldionne Do you want to cover the safe iterator design part?



Comment at: clang/docs/SafeBuffers.rst:40-41
+  - Finally, in order to avoid bugs in newly converted code, the
+Clang static analyzer provides a checker to find misconstructed
+``std::span`` objects.
+

xazax.hun wrote:
> Isn't this overly specific? What about `string_view`s? 
This is based on our current plans for near future.
While we are positive we will use `std::span` in the FixIts it is very unlikely 
we'd use `string_view`. That's why having a checker for `std::span` is more 
important for us now.



Comment at: clang/docs/SafeBuffers.rst:287-288
+
+The automatic fixit associated with the warning would transform this array
+into an ``std::array``:
+

xazax.hun wrote:
> I anticipate that doing these fixits for multi-dimensional arrays will be 
> confusing. Is that in scope?
Do you mean that the dimensions get reversed if we replace `int 
arr_3d[3][4][5]` with `array, 4>,  3> arr_3d_r`?



Comment at: clang/docs/SafeBuffers.rst:324-326
+and binary compatibility. In order to avoid that, the fix will provide
+a compatibility overload to preserve the old functionality. The updated code
+produced by the fixit will look like this:

xazax.hun wrote:
> While this might be desirable for some projects, I can imagine other projects 
> want to avoid generating the overload (just generate the fixes in one pass, 
> and apply all of them in a second to avoid a "fixed" header render another TU 
> invalid). But that would require fixits for the call sites. Do you plan to 
> support those batch migration scenarios or is that out of scope?
We currently don't have a machinery that would allow us to analyze the whole 
project at once.

Two passes are generally not enough - that number is something like number of 
unique functions in the deepest call-stack.

Part of our fixits is attributing functions as unsafe and application of such 
fixits can create new warnings in callers.

Let's take an example:

```
foo(int* ptr) {
  ptr[5] = 5;
}

bar(int* ptr) {
  foo(ptr);
}

baz(int* ptr) {
  bar(ptr);
}
```

Initially it is not obvious that `bar` or `baz` might get transformed.

In the first iteration a fixit for `foo` is emitted and when applied we get:
```
foo(span ptr) {
  ptr[5] = 5;
}

[[clang::unsafe_buffer_usage]] foo(int* ptr);

bar(int* ptr) {
  foo(ptr);
}

baz(int* ptr) {
  bar(ptr);
}
```

Only now we learn that `bar` (which can live in a diferent TU) should get 
transformed as well because it calls to unsafe function `foo`. But our analysis 
still won't see that `baz` will eventually have to be transformed too and it'll 
take another iteration.



Comment at: clang/docs/SafeBuffers.rst:352
+
+  - The compatibility overload contains a ``__placeholder__`` which needs
+to be populated manually. In this case ``size`` is a good candidate.

aaron.ballman wrote:
> xazax.hun wrote:
> > Just a small concern. While users are not allowed to use identifiers like 
> > this, there is always a chance someone has a global variable of this name 
> > and the code ends up compiling. So, if we want to be absolutely safe, we 
> > should either have logic to pick a unique name, or not generate fixits in 
> > that case. Although, this is probably rare enough that we could just 
> > hand-wave.
> Agreed; we should not have a fixit that suggests use of a reserved identifier.
Our thinking is to actually use syntactically incorrect placeholder to make 
sure that users can't accidentally miss it.
This is related to:
https://reviews.llvm.org/D136811#inline-1324542



Comment at: clang/docs/SafeBuffers.rst:354-357
+  - Despite accepting a ``std::span`` which carries size information,
+the fixed function still accepts ``size`` separately. It can be removed
+manually, or it can be preserved, if ``size`` and ``buf.size()``
+actually need to be different in your case.

xazax.hun wrote:
> I see 

[PATCH] D136811: WIP: RFC: NFC: C++ Buffer Hardening user documentation.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thank you for the feedback Aaron! We really appreciate the effort you put into 
this!




Comment at: clang/docs/SafeBuffers.rst:69-70
+containers such as ``std::span``, you can achieve bounds safety by
+*simply writing good modern C++ code*. This is what this solution is designed
+to help your codebase with.
+

aaron.ballman wrote:
> This makes it sound like the proposal doesn't cover C because there is no 
> version of `std::span` there.
I read this as a question as: "Could an imaginary codebase benefit from the 
warnings if there were no fixits?".

I can imagine projects where it would be very useful to have 90% of the code 
guaranteed to be free of "potentially dangerous pointer arithmetic" and have 
such operations contained to the remaining 10% of the code. I can imagine a 
principled solution to bounds-safety in C but even just splitting the code into 
safe and 9x smaller "unsafe" part could be extremely useful when it comes to 
debugging, audits, automated and manual testing and similar. Having clang 
guarantee this property reduces the scope of code that needs to be processed 
and a deliberate use of this model by projects can unlock new possibilities.

That is why I feel that the proposal can benefit also C projects.



Comment at: clang/docs/SafeBuffers.rst:115
+   (TODO: Will automatic fixits be able to suggest custom containers or views?)
+   (TODO: Explain how to implement such checks in a custom container?)
+

aaron.ballman wrote:
> I think this would be a good idea, especially if you can use an example of a 
> more involved (but still common) container. Like, how does someone harden a 
> ring buffer?
I believe that we should stick to hardening low-level primitives - all that a 
ring buffer implementation has to do is to store data in `std::array` from 
hardened libc++ and it's all set.



Comment at: clang/docs/SafeBuffers.rst:128
+  ``+``, ``-``, ``+=``, ``-=``,
+  - unless that number is a compile time constant ``0``;
+  - subtraction between two pointers is also fine;

aaron.ballman wrote:
> One case I think this is going to trip up is: `this + 1` (which is not 
> uncommon in practice: 
> https://sourcegraph.com/search?q=context:global+-file:.*test.*+file:.*%5C.cpp+this+%2B+1=standard).
>  How do users fix up that pointer arithmetic and do they ever get any safety 
> or security benefits from it? (Note, this doesn't apply to `+=`, just `+`.)
That's a fair point and if we don't find a good solution we should ignore 
arithmetic with `this` and a constant.



Comment at: clang/docs/SafeBuffers.rst:133
+  - unless the pointer is a compile time constant ``0`` or ``nullptr``;
+  - a number of C/C++ standard library buffer manipulation functions
+(such as std::memcpy() or std::next()) are hardcoded to act

aaron.ballman wrote:
> Eventually we should extend this to have an exhaustive list.
Absolutely! Is it ok if we populate the list as we land the implementation?



Comment at: clang/docs/SafeBuffers.rst:173
+  #pragma unsafe_buffer_usage begin
+
+Such pragmas not only enable incremental adoption with much smaller 
granularity,

aaron.ballman wrote:
> Have you considered allowing a short-hand form of the pragmas that are scoped 
> to the current block scope (or file scope if applied at global scope)? e.g.,
> ```
> void func() {
>   {
> #pragma only_safe_buffers
> int stuff[10] = {};
> int x = stuff[1]; // Warning: you're a bad person who should feel bad
>   }
>   int array[10] = {};
>   int y = array[1]; // this_is_fine.png
> }
> ```
We discussed other options but generally prefer simple pragmas for their 
flexibility.
Our current thinking is that for readability purposes we won't allow nesting of 
pragmas and would have end of scope to be explicit.
While this might be more verbose it would be dead simple to reason about.



Comment at: clang/docs/SafeBuffers.rst:181-188
+Even though similar functionality can be achieved with generic
+``#pragma clang diagnostic``, a custom pragma is preferable because
+the generic solution’s push/pop mechanism may be confusing. In particular,
+it’s not immediately obvious from the code which mode gets enabled after
+a “pop”, which makes it harder to reason about safety both for you and for
+the purposes of security audit. It is also generally valuable for such
+annotation to describe the property of the annotated code,

aaron.ballman wrote:
> > a custom pragma is preferable because the generic solution’s push/pop 
> > mechanism may be confusing.
> 
> Er, I'm not certain how much I agree with that. push/pop semantics on this 
> have been around for a *long* time. I do agree that the user can write code 
> such that another user may struggle to understand what the state of the 
> system is after a pop operation, 

[PATCH] D126908: [VerifyDiagnosticConsumer] Fix last line being discarded when parsing newline

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thank you for the patch!




Comment at: clang/test/SemaCXX/references.cpp:93
 
-struct C : B, A { }; // expected-warning {{direct base 'A' is inaccessible due 
to ambiguity:\nstruct C -> struct B -> struct A\nstruct C -> struct A}}
+struct C : B, A { }; // expected-warning {{direct base 'A' is inaccessible due 
to ambiguity:\nstruct C -> struct B -> struct A\nstruct C -> struct A}}
 

Can you please explain in detail what bug are you fixing?
In my understanding if we stop parsing after the last newline then the existing 
test would have failed. The difference seems to be only the white-space.
Am I missing something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126908

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

[PATCH] D133815: [analyzer] Support implicit parameters in path note

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG85d97aac80b8: [analyzer] Support implicit parameter 
self in path note (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133815

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/path-notes-impl-param.m


Index: clang/test/Analysis/path-notes-impl-param.m
===
--- /dev/null
+++ clang/test/Analysis/path-notes-impl-param.m
@@ -0,0 +1,31 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.RetainCount 
-analyzer-output=text -verify %s
+
+@protocol NSObject
+@end
+
+@interface NSObject  {}
+- (id)init;
++ (id)alloc;
+- (id)autorelease;
+@end
+
+@interface Foo : NSObject
+@property(nonatomic) int bar;
+@end
+
+@implementation Foo
+-(int)bar {
+  return 0;
+}
+@end
+
+int baz() {
+  Foo *f = [Foo alloc];
+  // expected-note@-1 {{'f' initialized here}}
+  // expected-note@-2 {{Method returns an instance of Foo with a +1 retain 
count}}
+
+  return f.bar;
+  // expected-warning@-1 {{Potential leak of an object stored into 'self' 
[osx.cocoa.RetainCount]}}
+  // expected-note@-2 {{Passing value via implicit parameter 'self'}}
+  // expected-note@-3 {{Object leaked: object allocated and stored into 'self' 
is not referenced later in this execution path and has a retain count of +1}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1340,13 +1340,12 @@
 static void showBRParamDiagnostics(llvm::raw_svector_ostream ,
StoreInfo SI) {
   const auto *VR = cast(SI.Dest);
-  const auto *Param = cast(VR->getDecl());
+  const auto *D = VR->getDecl();
 
   OS << "Passing ";
 
   if (isa(SI.Value)) {
-OS << (isObjCPointer(Param) ? "nil object reference"
-: "null pointer value");
+OS << (isObjCPointer(D) ? "nil object reference" : "null pointer value");
 
   } else if (SI.Value.isUndef()) {
 OS << "uninitialized value";
@@ -1361,12 +1360,19 @@
 OS << "value";
   }
 
-  // Printed parameter indexes are 1-based, not 0-based.
-  unsigned Idx = Param->getFunctionScopeIndex() + 1;
-  OS << " via " << Idx << llvm::getOrdinalSuffix(Idx) << " parameter";
-  if (VR->canPrintPretty()) {
-OS << " ";
-VR->printPretty(OS);
+  if (const auto *Param = dyn_cast(VR->getDecl())) {
+// Printed parameter indexes are 1-based, not 0-based.
+unsigned Idx = Param->getFunctionScopeIndex() + 1;
+OS << " via " << Idx << llvm::getOrdinalSuffix(Idx) << " parameter";
+if (VR->canPrintPretty()) {
+  OS << " ";
+  VR->printPretty(OS);
+}
+  } else if (const auto *ImplParam = dyn_cast(D)) {
+if (ImplParam->getParameterKind() ==
+ImplicitParamDecl::ImplicitParamKind::ObjCSelf) {
+  OS << " via implicit parameter 'self'";
+}
   }
 }
 


Index: clang/test/Analysis/path-notes-impl-param.m
===
--- /dev/null
+++ clang/test/Analysis/path-notes-impl-param.m
@@ -0,0 +1,31 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,osx.cocoa.RetainCount -analyzer-output=text -verify %s
+
+@protocol NSObject
+@end
+
+@interface NSObject  {}
+- (id)init;
++ (id)alloc;
+- (id)autorelease;
+@end
+
+@interface Foo : NSObject
+@property(nonatomic) int bar;
+@end
+
+@implementation Foo
+-(int)bar {
+  return 0;
+}
+@end
+
+int baz() {
+  Foo *f = [Foo alloc];
+  // expected-note@-1 {{'f' initialized here}}
+  // expected-note@-2 {{Method returns an instance of Foo with a +1 retain count}}
+
+  return f.bar;
+  // expected-warning@-1 {{Potential leak of an object stored into 'self' [osx.cocoa.RetainCount]}}
+  // expected-note@-2 {{Passing value via implicit parameter 'self'}}
+  // expected-note@-3 {{Object leaked: object allocated and stored into 'self' is not referenced later in this execution path and has a retain count of +1}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1340,13 +1340,12 @@
 static void showBRParamDiagnostics(llvm::raw_svector_ostream ,
StoreInfo SI) {
   const auto *VR = cast(SI.Dest);
-  const auto *Param = cast(VR->getDecl());
+  const auto *D = VR->getDecl();
 
   OS << "Passing ";
 
   if (isa(SI.Value)) {
-OS << (isObjCPointer(Param) ? "nil object reference"
-   

[PATCH] D130055: Clang extensions yolo, woot & kaboom

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

One more thing - the "attributes as a cross-TU metadata" idea might be 
(possibly with some squinting) conceptually similar to `enforce_tcb` attribute 
that @NoQ made me aware of some time ago.
https://clang.llvm.org/docs/AttributeReference.html#enforce-tcb


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130055

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

[PATCH] D130055: Clang extensions yolo, woot & kaboom

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D130055#3683279 , @beanz wrote:

> I'm wondering if there could be a generalization of the attribute like:

I can't imagine it is possible to design a reasonable and practically usable 
system of attributes to model anything but very limited subset of interface 
contracts.
FWIW I would consider the state space of `{initialized, uninitialized}` as a 
great first step. Generalizing this idea to per-abstract-property should be 
entirely possible as a subsequent improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130055

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

[PATCH] D130055: Clang extensions yolo, woot & kaboom

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D130055#3683173 , @aaron.ballman 
wrote:

> Are there circumstances where we cannot "simply" infer this from the 
> constructor itself? (After instantiation) we know the class hierarchy, we 
> know the data members, and we know the ctor init list/in-class initializers, 
> so it seems like we should be able to recursively mark a constructor as yolo 
> for the user rather than making them write it themselves.

The init list can be in a different translation unit (TU) which means we can't 
compute all properties just from the code in current TU. (Unless we decide to 
pessimistically consider such unanalyzable constructors as non-initializing.)
Some variation of proposed attributes could work as the cross-TU communication 
channel that would provide the missing information based on analysis of other 
TUs.

Example - 1 header, 1 implementation file, 1 file with client code:

- foo.hpp
- foo.cpp - implements interface in foo.hpp
- client_of_foo.cpp - uses the interface from foo.hpp.

We can have per-TU analysis that results in correct attributes in the header:

- foo.cpp - Analysis of the available implementation can verify correctness of 
`[[yolo]]` attributes on related declarations in foo.hpp.
- client_of_foo.cpp - Analysis of the implementation can take into the account 
attributes in foo.hpp. (And inductively we should also verify correctness of 
attributes on related declarations.)

That way we could infer properties based on code in current implementation file 
and attributes on declarations it uses from headers.
I believe we should be able to have one-TU-at-a-time analysis that could 
guarantee correctness of the attributes on declarations (or warn otherwise) and 
that could iteratively give us full-project (global) correctness.
The order in which we analyze TUs shouldn't matter as long as we analyze all of 
them.

BTW Even for the constructor analysis - the base class initializer list can be 
in another TU again.

> It seems like inferring this would reduce false positives and false negatives

I strongly support the idea that we should not put the burden of not making 
mistakes on the user if we can make a tool that guarantees or verifies 
correctness.

> ... we might only need this attribute in very rare circumstances ...

I think this is orthogonal to the above but agree that changing the default for 
constructors to be implicitly `[[yolo]]` could be a win for ergonomics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130055

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

[PATCH] D130055: Clang extensions yolo, woot & kaboom

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Chris! This is a very interesting idea!

I do have couple thoughts - mostly that this could lead to something great and 
I would love it to apply to as many relevant cases as possible.

It looks like there is a possibility that a free function, static method or a 
method of another class (a `friend`?) should be `woot` for a specific 
pointer/reference parameter.

  struct Foo {int a};
  void init_Foo(Foo& f) { f.a = 42; }

In the same spirit as the above I think that `kaboom` should be applicable to 
functions in general.

BTW - if we generalize `woot` and `kaboom` - won't we get support for built-in 
types as a side-effect and won't that give us C support as a side-effect?

Last but not least - it would be really interesting to have the ability to 
check and diagnose improper use of these attributes.
One specific scenario that seems interesting to me is the ability to verify 
also absence of an attribute. (That could in-turn potentially unlock more 
aggressive analysis at call-sites.)
Example:

  struct Bar {
int a, b;
Bar() : a(0) { } // <--- warning: constructor should be [[yolo]]
  };

...and if we had that - would might potentially even collapse `woot` and 
!`yolo` together?
I can imagine that some parts of C++ would be tricky - e. g. templates so not 
sure how far could this idea be stretched.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130055

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

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D125349#3509073 , @aaron.ballman 
wrote:

> It's interesting to note that `an_atomic_uint = an_atomic_uint + 
> an_enum_value` works correctly: https://godbolt.org/z/cvP9e6nh7. I was trying 
> to figure out whether the atomic qualifier is properly stripped for the 
> compound operator. When I run under a debugger and dump the AST for 
> `Args[0]`, I get: `DeclRefExpr 0x26efcf87f88 '_Atomic(unsigned int)' lvalue 
> Var 0x26efcf44cb8 'an_atomic_uint' '_Atomic(unsigned int)'` which seems like 
> it may be the root cause of the problem here (tough to say given that this is 
> a C extension in C++ though). The lvalue conversion that takes place for 
> `an_atomic_uint` should drop the atomic qualifier per C2x 6.3.2.1p2.

Thank you for the suggestion! I'm looking into this.


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

https://reviews.llvm.org/D125349

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

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D125349#3509546 , @ahatanak wrote:

> Is it not possible to handle this similarly to `volatile unsigned`? If I 
> replace `_Atomic unsigned` with `volatile unsigned`, I see 
> `LookupOverloadedBinOp` succeed without having to strip volatile because 
> `addAssignmentArithmeticOverloads` adds candidates with volatile types.

Possibly? I am just generally apprehensive about changing the C++ side of 
things as I can't imagine all the consequences.
One difference that I see is the existence of `volatile`-qualified versions of 
these operators being prescribed by C++ standard while the operators you 
suggest definitely aren't:

  // C++ [over.built]p18:
  //
  //   For every triple (L, VQ, R), where L is an arithmetic type,
  //   VQ is either volatile or empty, and R is a promoted
  //   arithmetic type, there exist candidate operator functions of
  //   the form

https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOverload.cpp#L8926


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

https://reviews.llvm.org/D125349

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

[PATCH] D123273: [utils] Avoid hardcoding metadata ids in update_cc_test_checks

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Reverting for now. Will take a look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123273

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

[PATCH] D123273: [utils] Avoid hardcoding metadata ids in update_cc_test_checks

[Jan Korous via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGce583b14b2ec: [utils] Avoid hardcoding metadata ids in 
update_cc_test_checks (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123273

Files:
  clang/test/utils/update_cc_test_checks/Inputs/annotation-id.c
  clang/test/utils/update_cc_test_checks/Inputs/annotation-id.expected
  clang/test/utils/update_cc_test_checks/Inputs/nosanitize-id.c
  clang/test/utils/update_cc_test_checks/Inputs/nosanitize-id.expected
  clang/test/utils/update_cc_test_checks/Inputs/srcloc-id.c
  clang/test/utils/update_cc_test_checks/Inputs/srcloc-id.expected
  clang/test/utils/update_cc_test_checks/Inputs/tbaa-id.c
  clang/test/utils/update_cc_test_checks/Inputs/tbaa-id.expected
  clang/test/utils/update_cc_test_checks/Inputs/tbaa-struct-id.c
  clang/test/utils/update_cc_test_checks/Inputs/tbaa-struct-id.expected
  clang/test/utils/update_cc_test_checks/annotation-id.test
  clang/test/utils/update_cc_test_checks/nosanitize-id.test
  clang/test/utils/update_cc_test_checks/srcloc-id.test
  clang/test/utils/update_cc_test_checks/tbaa-id.test
  clang/test/utils/update_cc_test_checks/tbaa-struct-id.test
  llvm/utils/UpdateTestChecks/common.py

Index: llvm/utils/UpdateTestChecks/common.py
===
--- llvm/utils/UpdateTestChecks/common.py
+++ llvm/utils/UpdateTestChecks/common.py
@@ -616,11 +616,15 @@
 NamelessValue(r'GLOB' , '@' , None   , r'@', r'[a-zA-Z0-9_$"\\.-]+' , None , r'.+', True)  ,
 NamelessValue(r'DBG'  , '!' , r'!dbg '   , None, None   , r'![0-9]+'   , None , False) ,
 NamelessValue(r'PROF' , '!' , r'!prof '  , None, None   , r'![0-9]+'   , None , False) ,
-NamelessValue(r'TBAA' , '!' , r'!tbaa '  , None, None   , r'![0-9]+'   , None , False) ,
 NamelessValue(r'RNG'  , '!' , r'!range ' , None, None   , r'![0-9]+'   , None , False) ,
 NamelessValue(r'LOOP' , '!' , r'!llvm.loop ' , None, None   , r'![0-9]+'   , None , False) ,
 NamelessValue(r'META' , '!' , r'metadata '   , None, None   , r'![0-9]+'   , None , False) ,
 NamelessValue(r'META' , '!' , None   , r'' , r'![0-9]+' , None , r'(?:distinct |)!.*' , False) ,
+NamelessValue(r'TBAA' , '!' , r'!tbaa '  , None, None   , r'![0-9]+'   , None , False) ,
+NamelessValue(r'TBAAST' , '!' , r'!tbaa.struct ', None, None   , r'![0-9]+'   , None , False) ,
+NamelessValue(r'ANNT'   , '!' , r'!annotation ' , None, None   , r'![0-9]+'   , None , False) ,
+NamelessValue(r'SRCLOC' , '!' , r'!srcloc ' , None, None   , r'![0-9]+'   , None , False) ,
+NamelessValue(r'NOSAN'  , '!' , r'!nosanitize ' , None, None   , r'![0-9]+'   , None , False) ,
 ]
 
 def createOrRegexp(old, new):
Index: clang/test/utils/update_cc_test_checks/tbaa-struct-id.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/tbaa-struct-id.test
@@ -0,0 +1,7 @@
+## Test that CHECK lines are generated before the definion and not the declaration
+
+# RUN: cp %S/Inputs/tbaa-struct-id.c %t.c && %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/tbaa-struct-id.expected %t.c
+## Check that re-running update_cc_test_checks doesn't change the output
+# RUN: %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/tbaa-struct-id.expected %t.c
Index: clang/test/utils/update_cc_test_checks/tbaa-id.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/tbaa-id.test
@@ -0,0 +1,7 @@
+## Test that CHECK lines are generated before the definion and not the declaration
+
+# RUN: cp %S/Inputs/tbaa-id.c %t.c && %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/tbaa-id.expected %t.c
+## Check that re-running update_cc_test_checks doesn't change the output
+# RUN: %update_cc_test_checks %t.c
+# RUN: diff -u %S/Inputs/tbaa-id.expected %t.c
Index: clang/test/utils/update_cc_test_checks/srcloc-id.test
===
--- /dev/null
+++ clang/test/utils/update_cc_test_checks/srcloc-id.test
@@ -0,0 +1,7 @@
+## Test that CHECK lines are 

[PATCH] D117238: [C2x] Add BITINT_MAXWIDTH support

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

I verified locally that reverting this patch fixes the build.
Reverted in fad7e491a0770ac4336934030ac67d77e7af5520 
 to 
unblock Green Dragon, etc.
@aaron.ballman Please take a look when you get a chance.


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

https://reviews.llvm.org/D117238

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

[PATCH] D117238: [C2x] Add BITINT_MAXWIDTH support

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@aaron.ballman I believe this change broke the build starting with:

https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/26915/

  
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/include/clang/Basic/Diagnostic.h:1352:8:
 error: use of overloaded operator '<<' is ambiguous (with operand types 'const 
clang::StreamingDiagnostic' and 'typename remove_reference::type' (aka 'unsigned long'))
  ...
  
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/lib/Sema/SemaType.cpp:2274:23:
 note: in instantiation of function template specialization 
'clang::Sema::SemaDiagnosticBuilder::operator<<' requested 
here
  << IsUnsigned << TI.getMaxBitIntWidth();


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

https://reviews.llvm.org/D117238

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

[PATCH] D117938: [clang][dataflow] Avoid MaxIterations overflow

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdd01d971aa2c: [clang][dataflow] Avoid MaxIterations overflow 
(authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D117938?vs=402141=402707#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117938

Files:
  clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp


Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -210,8 +210,8 @@
   // FIXME: Consider making the maximum number of iterations configurable.
   // FIXME: Set up statistics (see llvm/ADT/Statistic.h) to count average 
number
   // of iterations, number of functions that time out, etc.
-  unsigned Iterations = 0;
-  static constexpr unsigned MaxIterations = 1 << 16;
+  uint32_t Iterations = 0;
+  static constexpr uint32_t MaxIterations = 1 << 16;
   while (const CFGBlock *Block = Worklist.dequeue()) {
 if (++Iterations > MaxIterations) {
   llvm::errs() << "Maximum number of iterations reached, giving up.\n";


Index: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
===
--- clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -210,8 +210,8 @@
   // FIXME: Consider making the maximum number of iterations configurable.
   // FIXME: Set up statistics (see llvm/ADT/Statistic.h) to count average number
   // of iterations, number of functions that time out, etc.
-  unsigned Iterations = 0;
-  static constexpr unsigned MaxIterations = 1 << 16;
+  uint32_t Iterations = 0;
+  static constexpr uint32_t MaxIterations = 1 << 16;
   while (const CFGBlock *Block = Worklist.dequeue()) {
 if (++Iterations > MaxIterations) {
   llvm::errs() << "Maximum number of iterations reached, giving up.\n";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D113211: [NFC][clang] Inclusive terms: replace uses of blacklist in clang/test/

[Jan Korous via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGen/address-safety-attr.cpp:9
 
-// RUN: echo "fun:*BlacklistedFunction*" > %t.func.blacklist
-// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -disable-O0-optnone 
-emit-llvm -o - %s -include %t.extra-source.cpp -fsanitize=address 
-fsanitize-blacklist=%t.func.blacklist | FileCheck -check-prefix=BLFUNC %s
+// RUN: echo "fun:*IgnorelistedFunction*" > %t.func.ignorelist
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -disable-O0-optnone 
-emit-llvm -o - %s -include %t.extra-source.cpp -fsanitize=address 
-fsanitize-ignorelist=%t.func.ignorelist | FileCheck -check-prefix=BLFUNC %s

jkorous wrote:
> Shouldn't we teach ASan about the new syntax of ignorelist as well 
> `fun:*IgnorelistedFunction*`?
Nvm - that's just name of the test function below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113211

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

[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous added a comment.

LGTM if we have test coverage for all the cases.




Comment at: clang/test/CodeGenObjC/category-class-empty.m:1
+// RUN: %clang_cc1 -triple i386-apple-darwin9 -O3 -emit-llvm -o - %s | 
FileCheck %s
+// PR7431

zoecarver wrote:
> Is `-O3` needed here?
I would even think `-O0` is better and consider also `-disable-llvm-passes`. 
Ultimately the goal is to make sure frontend codegen doesn't emit the metadata 
so the less processing of the IR in the backend the more sensitive the test 
will be.



Comment at: clang/test/CodeGenObjC/category-class-empty.m:10
+@interface A(foo)
+- (void)foo_myStuff;
+@end

I assume all the cases when we want to emit the metadata (at least one instance 
method, at least one class method, ...) are covered by other tests, right?
If there's a case that's missing we should add a test for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113455

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

[PATCH] D113211: [NFC][clang] Inclusive terms: replace uses of blacklist in clang/test/

[Jan Korous via Phabricator via cfe-commits]

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

Hi! Thank you for the clean-up :)

I feel there might be a bit of work still left. While renaming filenames and 
function names should be mostly inconsequential renaming command line options 
and sanitizer ignorelist content (and by that changing the syntax) most likely 
need accompanying changes in clang itself.

I noticed you have at least one other similar patch but I don't see the changes 
I'd expect there - please let me know if I'm just missing something!

Do the tests actually pass with this patch?




Comment at: clang/test/CodeGen/address-safety-attr.cpp:9
 
-// RUN: echo "fun:*BlacklistedFunction*" > %t.func.blacklist
-// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -disable-O0-optnone 
-emit-llvm -o - %s -include %t.extra-source.cpp -fsanitize=address 
-fsanitize-blacklist=%t.func.blacklist | FileCheck -check-prefix=BLFUNC %s
+// RUN: echo "fun:*IgnorelistedFunction*" > %t.func.ignorelist
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -disable-O0-optnone 
-emit-llvm -o - %s -include %t.extra-source.cpp -fsanitize=address 
-fsanitize-ignorelist=%t.func.ignorelist | FileCheck -check-prefix=BLFUNC %s

Shouldn't we teach ASan about the new syntax of ignorelist as well 
`fun:*IgnorelistedFunction*`?



Comment at: clang/test/CodeGen/address-safety-attr.cpp:10
+// RUN: echo "fun:*IgnorelistedFunction*" > %t.func.ignorelist
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-apple-darwin -disable-O0-optnone 
-emit-llvm -o - %s -include %t.extra-source.cpp -fsanitize=address 
-fsanitize-ignorelist=%t.func.ignorelist | FileCheck -check-prefix=BLFUNC %s
 

Shouldn't the change of the option name to `-fsanitize-ignorelist` in tests be 
accompanied with analogous change in driver and/or cc1 options definition?



Comment at: clang/test/CodeGenCXX/cfi-ignorelist.cpp:3
 
-// Check that blacklisting cfi and cfi-vcall work correctly
+// Check that ignore listing cfi and cfi-vcall work correctly
 // RUN: echo "[cfi-vcall]" > %t.vcall.txt

We should use the verb "to ignorelist smth" (without the space) consistently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113211

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

[PATCH] D97878: [DirectoryWatcher] Increase timeout to make test less flaky

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

LGTM.

Adding the comment would be great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97878

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

[PATCH] D93110: [analyzer] Implement a first version of suppressions via attributes

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Thanks for working on this!




Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2907
+// start with it.
+llvm::SmallVector Stack{DynTypedNode::create(*BugStmt)};
+

Since IIUC a node can have multiple parents - does that mean we could end up 
traversing a node multiple times?
BTW do you have an example of a node that have multiple parents?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2936
+  // Let's continue with the current node's parent(s).
+  for (const auto  : AC.getParents(Current)) {
+Stack.push_back(Parent);

There's a note for `ASTContext::getParents`:

"New callers should use ParentMapContext::getParents() directly."

https://clang.llvm.org/doxygen/classclang_1_1ASTContext.html#a32d11844fdb82310b9059784fd4ceb6b

Shall we do that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93110

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

[PATCH] D89959: UBSAN: emit distinctive traps in trapping mode

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Friendly ping


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

https://reviews.llvm.org/D89959

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

[PATCH] D86230: [SourceManager] Skip module maps when searching files for macro arguments

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe7870223d8b5: [SourceManager] Skip module maps when 
searching files for macro arguments (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D86230?vs=292928=300076#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86230

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1761,7 +1761,12 @@
 if (Invalid)
   return;
 if (Entry.isFile()) {
-  SourceLocation IncludeLoc = Entry.getFile().getIncludeLoc();
+  auto File = Entry.getFile();
+  if (File.getFileCharacteristic() == C_User_ModuleMap ||
+  File.getFileCharacteristic() == C_System_ModuleMap)
+continue;
+
+  SourceLocation IncludeLoc = File.getIncludeLoc();
   bool IncludedInFID =
   (IncludeLoc.isValid() && isInFileID(IncludeLoc, FID)) ||
   // Predefined header doesn't have a valid include location in main


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1761,7 +1761,12 @@
 if (Invalid)
   return;
 if (Entry.isFile()) {
-  SourceLocation IncludeLoc = Entry.getFile().getIncludeLoc();
+  auto File = Entry.getFile();
+  if (File.getFileCharacteristic() == C_User_ModuleMap ||
+  File.getFileCharacteristic() == C_System_ModuleMap)
+continue;
+
+  SourceLocation IncludeLoc = File.getIncludeLoc();
   bool IncludedInFID =
   (IncludeLoc.isValid() && isInFileID(IncludeLoc, FID)) ||
   // Predefined header doesn't have a valid include location in main
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87176: [clang] Enable selectively turning on/off format-insufficient-args warning

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Had to fix the order of expected warnings in warning-wall.c
6fd8c69049a8 
 [clang] 
Update warning-wall.c test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87176

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

[PATCH] D87176: [clang] Enable selectively turning on/off format-insufficient-args warning

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1e86d637eb4f: [clang] Selectively ena/disa-ble 
format-insufficient-args warning (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87176

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-printf-insufficient-data-args.c


Index: clang/test/Sema/warn-printf-insufficient-data-args.c
===
--- /dev/null
+++ clang/test/Sema/warn-printf-insufficient-data-args.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=WARNING-ON %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-format-insufficient-args 
-verify=WARNING-OFF %s
+
+
+int printf(const char * format, ...);
+
+int main(void) {
+  int patatino = 42;
+  printf("%i %i", patatino); // WARNING-ON-warning {{more '%' conversions than 
data arguments}}
+  // WARNING-OFF-no-diagnostics
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -9,6 +9,7 @@
 CHECK-NEXT:  -Wdelete-non-abstract-non-virtual-dtor
 CHECK-NEXT:  -Wdelete-abstract-non-virtual-dtor
 CHECK-NEXT:-Wformat
+CHECK-NEXT:  -Wformat-insufficient-args
 CHECK-NEXT:  -Wformat-extra-args
 CHECK-NEXT:  -Wformat-zero-length
 CHECK-NEXT:  -Wnonnull
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8921,7 +8921,7 @@
   "array %0 declared here">;
 
 def warn_printf_insufficient_data_args : Warning<
-  "more '%%' conversions than data arguments">, InGroup;
+  "more '%%' conversions than data arguments">, 
InGroup;
 def warn_printf_data_arg_not_used : Warning<
   "data argument not used by format string">, InGroup;
 def warn_format_invalid_conversion : Warning<
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -239,6 +239,7 @@
 def GNUFlexibleArrayInitializer : DiagGroup<"gnu-flexible-array-initializer">;
 def GNUFlexibleArrayUnionMember : DiagGroup<"gnu-flexible-array-union-member">;
 def GNUFoldingConstant : DiagGroup<"gnu-folding-constant">;
+def FormatInsufficientArgs : DiagGroup<"format-insufficient-args">;
 def FormatExtraArgs : DiagGroup<"format-extra-args">;
 def FormatZeroLength : DiagGroup<"format-zero-length">;
 
@@ -849,7 +850,8 @@
 def FormatTypeConfusion : DiagGroup<"format-type-confusion">;
 def Format : DiagGroup<"format",
[FormatExtraArgs, FormatZeroLength, NonNull,
-FormatSecurity, FormatY2K, FormatInvalidSpecifier]>,
+FormatSecurity, FormatY2K, FormatInvalidSpecifier,
+FormatInsufficientArgs]>,
  DiagCategory<"Format String Issue">;
 def FormatNonLiteral : DiagGroup<"format-nonliteral">;
 def Format2 : DiagGroup<"format=2",


Index: clang/test/Sema/warn-printf-insufficient-data-args.c
===
--- /dev/null
+++ clang/test/Sema/warn-printf-insufficient-data-args.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -fsyntax-only -verify=WARNING-ON %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-format-insufficient-args -verify=WARNING-OFF %s
+
+
+int printf(const char * format, ...);
+
+int main(void) {
+  int patatino = 42;
+  printf("%i %i", patatino); // WARNING-ON-warning {{more '%' conversions than data arguments}}
+  // WARNING-OFF-no-diagnostics
+}
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -9,6 +9,7 @@
 CHECK-NEXT:  -Wdelete-non-abstract-non-virtual-dtor
 CHECK-NEXT:  -Wdelete-abstract-non-virtual-dtor
 CHECK-NEXT:-Wformat
+CHECK-NEXT:  -Wformat-insufficient-args
 CHECK-NEXT:  -Wformat-extra-args
 CHECK-NEXT:  -Wformat-zero-length
 CHECK-NEXT:  -Wnonnull
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8921,7 +8921,7 @@
   "array %0 declared here">;
 
 def warn_printf_insufficient_data_args : Warning<
-  "more '%%' conversions than data arguments">, InGroup;
+  "more '%%' conversions than data arguments">, InGroup;
 def warn_printf_data_arg_not_used : Warning<
   

[PATCH] D88133: [Analyzer][WebKit] Use tri-state types for relevant predicates

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Created this for eventual post-commit review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88133

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

[PATCH] D88133: [Analyzer][WebKit] Use tri-state types for relevant predicates

[Jan Korous via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG47e6851423fd: [Analyzer][WebKit] Use tri-state types for 
relevant predicates (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88133

Files:
  clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLocalVarsChecker.cpp
@@ -169,7 +169,8 @@
 if (!ArgType)
   return;
 
-if (isUncountedPtr(ArgType)) {
+Optional IsUncountedPtr = isUncountedPtr(ArgType);
+if (IsUncountedPtr && *IsUncountedPtr) {
   const Expr *const InitExpr = V->getInit();
   if (!InitExpr)
 return; // FIXME: later on we might warn on uninitialized vars too
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -59,7 +59,8 @@
   if (C.capturesVariable()) {
 VarDecl *CapturedVar = C.getCapturedVar();
 if (auto *CapturedVarType = CapturedVar->getType().getTypePtrOrNull()) {
-  if (isUncountedPtr(CapturedVarType)) {
+  Optional IsUncountedPtr = isUncountedPtr(CapturedVarType);
+  if (IsUncountedPtr && *IsUncountedPtr) {
 reportBug(C, CapturedVar, CapturedVarType);
   }
 }
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
@@ -86,7 +86,8 @@
   continue; // FIXME? Should we bail?
 
 // FIXME: more complex types (arrays, references to raw pointers, etc)
-if (!isUncountedPtr(ArgType))
+Optional IsUncounted = isUncountedPtr(ArgType);
+if (!IsUncounted || !(*IsUncounted))
   continue;
 
 const auto *Arg = CE->getArg(ArgIdx);
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
@@ -76,19 +76,15 @@
   (AccSpec == AS_none && RD->isClass()))
 return false;
 
-  llvm::Optional MaybeRefCntblBaseRD =
+  llvm::Optional RefCntblBaseRD =
   isRefCountable(Base);
-  if (!MaybeRefCntblBaseRD.hasValue())
+  if (!RefCntblBaseRD || !(*RefCntblBaseRD))
 return false;
 
-  const CXXRecordDecl *RefCntblBaseRD = MaybeRefCntblBaseRD.getValue();
-  if (!RefCntblBaseRD)
-return false;
-
-  const auto *Dtor = RefCntblBaseRD->getDestructor();
+  const auto *Dtor = (*RefCntblBaseRD)->getDestructor();
   if (!Dtor || !Dtor->isVirtual()) {
 ProblematicBaseSpecifier = Base;
-ProblematicBaseClass = RefCntblBaseRD;
+ProblematicBaseClass = *RefCntblBaseRD;
 return true;
   }
 
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
@@ -9,6 +9,8 @@
 #ifndef LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
 #define LLVM_CLANG_ANALYZER_WEBKIT_PTRTYPESEMANTICS_H
 
+#include "llvm/ADT/APInt.h"
+
 namespace clang {
 class CXXBaseSpecifier;
 class CXXMethodDecl;
@@ -25,30 +27,31 @@
 // Ref.
 
 /// \returns CXXRecordDecl of the base if the type is ref-countable, nullptr if
-/// not.
-const clang::CXXRecordDecl *isRefCountable(const clang::CXXBaseSpecifier *Base);
+/// not, None if 

[PATCH] D86231: [SourceManager] Explicitly check for potential iterator underflow

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGae726fecae9a: [SourceManager] Explicitly check for potential 
iterator underflow (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86231

Files:
  clang/lib/Basic/SourceManager.cpp


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1936,6 +1936,11 @@
 
   assert(!MacroArgsCache->empty());
   MacroArgsMap::iterator I = MacroArgsCache->upper_bound(Offset);
+  // In case every element in MacroArgsCache is greater than Offset we can't
+  // decrement the iterator.
+  if (I == MacroArgsCache->begin())
+return Loc;
+
   --I;
 
   unsigned MacroArgBeginOffs = I->first;


Index: clang/lib/Basic/SourceManager.cpp
===
--- clang/lib/Basic/SourceManager.cpp
+++ clang/lib/Basic/SourceManager.cpp
@@ -1936,6 +1936,11 @@
 
   assert(!MacroArgsCache->empty());
   MacroArgsMap::iterator I = MacroArgsCache->upper_bound(Offset);
+  // In case every element in MacroArgsCache is greater than Offset we can't
+  // decrement the iterator.
+  if (I == MacroArgsCache->begin())
+return Loc;
+
   --I;
 
   unsigned MacroArgBeginOffs = I->first;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83814: [clangd] Add Random Forest runtime for code completion.

[Jan Korous via Phabricator via cfe-commits]

jkorous added subscribers: dexonsmith, akyrtzi.
jkorous added a comment.

Hi @usaxena95 and @sammccall,

I am wondering about couple high-level things.

Do you guys intend to open-source also the training part of the model pipeline 
or publish a model trained on generic-enough training set so it could be 
reasonably used on "any" codebase?

Do you still intend to support the heuristic that is currently powering clangd 
in the future?

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83814

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

[PATCH] D86992: [libclang] Expose Rewriter in libclang API

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG69e5abb57b70: [libclang] Add CXRewriter to libclang API 
(authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D86992?vs=289837=290027#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86992

Files:
  clang/include/clang-c/Rewrite.h
  clang/tools/libclang/CMakeLists.txt
  clang/tools/libclang/Rewrite.cpp
  clang/tools/libclang/libclang.exports
  clang/unittests/libclang/LibclangTest.cpp

Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "clang-c/Index.h"
+#include "clang-c/Rewrite.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
@@ -842,3 +843,90 @@
   },
   nullptr);
 }
+class LibclangRewriteTest : public LibclangParseTest {
+public:
+  CXRewriter Rew = nullptr;
+  std::string Filename;
+  CXFile File = nullptr;
+
+  void SetUp() override {
+LibclangParseTest::SetUp();
+Filename = "file.cpp";
+WriteFile(Filename, "int main() { return 0; }");
+ClangTU = clang_parseTranslationUnit(Index, Filename.c_str(), nullptr, 0,
+ nullptr, 0, TUFlags);
+Rew = clang_CXRewriter_create(ClangTU);
+File = clang_getFile(ClangTU, Filename.c_str());
+  }
+  void TearDown() override {
+clang_CXRewriter_dispose(Rew);
+LibclangParseTest::TearDown();
+  }
+};
+
+static std::string getFileContent(const std::string& Filename) {
+  std::ifstream RewrittenFile(Filename);
+  std::string RewrittenFileContent;
+  std::string Line;
+  while (std::getline(RewrittenFile, Line)) {
+if (RewrittenFileContent.empty())
+  RewrittenFileContent = Line;
+else {
+  RewrittenFileContent += "\n" + Line;
+}
+  }
+  return RewrittenFileContent;
+}
+
+TEST_F(LibclangRewriteTest, RewriteReplace) {
+  CXSourceLocation B = clang_getLocation(ClangTU, File, 1, 5);
+  CXSourceLocation E = clang_getLocation(ClangTU, File, 1, 9);
+  CXSourceRange Rng	= clang_getRange(B, E);
+
+  clang_CXRewriter_replaceText(Rew, Rng, "MAIN");
+
+  ASSERT_EQ(clang_CXRewriter_overwriteChangedFiles(Rew), 0);
+  EXPECT_EQ(getFileContent(Filename), "int MAIN() { return 0; }");
+}
+
+TEST_F(LibclangRewriteTest, RewriteReplaceShorter) {
+  CXSourceLocation B = clang_getLocation(ClangTU, File, 1, 5);
+  CXSourceLocation E = clang_getLocation(ClangTU, File, 1, 9);
+  CXSourceRange Rng	= clang_getRange(B, E);
+
+  clang_CXRewriter_replaceText(Rew, Rng, "foo");
+
+  ASSERT_EQ(clang_CXRewriter_overwriteChangedFiles(Rew), 0);
+  EXPECT_EQ(getFileContent(Filename), "int foo() { return 0; }");
+}
+
+TEST_F(LibclangRewriteTest, RewriteReplaceLonger) {
+  CXSourceLocation B = clang_getLocation(ClangTU, File, 1, 5);
+  CXSourceLocation E = clang_getLocation(ClangTU, File, 1, 9);
+  CXSourceRange Rng	= clang_getRange(B, E);
+
+  clang_CXRewriter_replaceText(Rew, Rng, "patatino");
+
+  ASSERT_EQ(clang_CXRewriter_overwriteChangedFiles(Rew), 0);
+  EXPECT_EQ(getFileContent(Filename), "int patatino() { return 0; }");
+}
+
+TEST_F(LibclangRewriteTest, RewriteInsert) {
+  CXSourceLocation Loc = clang_getLocation(ClangTU, File, 1, 5);
+
+  clang_CXRewriter_insertTextBefore(Rew, Loc, "ro");
+
+  ASSERT_EQ(clang_CXRewriter_overwriteChangedFiles(Rew), 0);
+  EXPECT_EQ(getFileContent(Filename), "int romain() { return 0; }");
+}
+
+TEST_F(LibclangRewriteTest, RewriteRemove) {
+  CXSourceLocation B = clang_getLocation(ClangTU, File, 1, 5);
+  CXSourceLocation E = clang_getLocation(ClangTU, File, 1, 9);
+  CXSourceRange Rng	= clang_getRange(B, E);
+
+  clang_CXRewriter_removeText(Rew, Rng);
+
+  ASSERT_EQ(clang_CXRewriter_overwriteChangedFiles(Rew), 0);
+  EXPECT_EQ(getFileContent(Filename), "int () { return 0; }");
+}
Index: clang/tools/libclang/libclang.exports
===
--- clang/tools/libclang/libclang.exports
+++ clang/tools/libclang/libclang.exports
@@ -385,3 +385,10 @@
 clang_Cursor_getVarDeclInitializer
 clang_Cursor_hasVarDeclGlobalStorage
 clang_Cursor_hasVarDeclExternalStorage
+clang_CXRewriter_create
+clang_CXRewriter_insertTextBefore
+clang_CXRewriter_replaceText
+clang_CXRewriter_removeText
+clang_CXRewriter_overwriteChangedFiles
+clang_CXRewriter_writeMainFileToStdOut
+clang_CXRewriter_dispose
Index: clang/tools/libclang/Rewrite.cpp
===
--- /dev/null
+++ clang/tools/libclang/Rewrite.cpp
@@ -0,0 +1,63 @@
+//===- Rewrite.cpp ===//
+//

[PATCH] D86991: [libclang] Expose couple AST details

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG052f83890349: [libclang] Expose couple more AST details via 
cursors (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86991

Files:
  clang/include/clang-c/Index.h
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/libclang.exports
  clang/unittests/libclang/LibclangTest.cpp

Index: clang/unittests/libclang/LibclangTest.cpp
===
--- clang/unittests/libclang/LibclangTest.cpp
+++ clang/unittests/libclang/LibclangTest.cpp
@@ -736,3 +736,109 @@
 
   CheckTokenKinds();
 }
+
+TEST_F(LibclangParseTest, clang_getVarDeclInitializer) {
+  std::string Main = "main.cpp";
+  WriteFile(Main, "int foo() { return 5; }; const int a = foo();");
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0, nullptr,
+   0, TUFlags);
+
+  CXCursor C = clang_getTranslationUnitCursor(ClangTU);
+  clang_visitChildren(
+  C,
+  [](CXCursor cursor, CXCursor parent,
+ CXClientData client_data) -> CXChildVisitResult {
+if (clang_getCursorKind(cursor) == CXCursor_VarDecl) {
+  const CXCursor Initializer = clang_Cursor_getVarDeclInitializer(cursor);
+  EXPECT_FALSE(clang_Cursor_isNull(Initializer));
+  CXString Spelling = clang_getCursorSpelling(Initializer);
+  const char* const SpellingCSstr = clang_getCString(Spelling);
+  EXPECT_TRUE(SpellingCSstr);
+  EXPECT_EQ(std::string(SpellingCSstr), std::string("foo"));
+  clang_disposeString(Spelling);
+  return CXChildVisit_Break;
+}
+return CXChildVisit_Continue;
+  },
+  nullptr);
+}
+
+TEST_F(LibclangParseTest, clang_hasVarDeclGlobalStorageFalse) {
+  std::string Main = "main.cpp";
+  WriteFile(Main, "void foo() { int a; }");
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0, nullptr,
+   0, TUFlags);
+
+  CXCursor C = clang_getTranslationUnitCursor(ClangTU);
+  clang_visitChildren(
+  C,
+  [](CXCursor cursor, CXCursor parent,
+ CXClientData client_data) -> CXChildVisitResult {
+if (clang_getCursorKind(cursor) == CXCursor_VarDecl) {
+  EXPECT_FALSE(clang_Cursor_hasVarDeclGlobalStorage(cursor));
+  return CXChildVisit_Break;
+}
+return CXChildVisit_Continue;
+  },
+  nullptr);
+}
+
+TEST_F(LibclangParseTest, clang_Cursor_hasVarDeclGlobalStorageTrue) {
+  std::string Main = "main.cpp";
+  WriteFile(Main, "int a;");
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0, nullptr,
+   0, TUFlags);
+
+  CXCursor C = clang_getTranslationUnitCursor(ClangTU);
+  clang_visitChildren(
+  C,
+  [](CXCursor cursor, CXCursor parent,
+ CXClientData client_data) -> CXChildVisitResult {
+if (clang_getCursorKind(cursor) == CXCursor_VarDecl) {
+  EXPECT_TRUE(clang_Cursor_hasVarDeclGlobalStorage(cursor));
+  return CXChildVisit_Break;
+}
+return CXChildVisit_Continue;
+  },
+  nullptr);
+}
+
+TEST_F(LibclangParseTest, clang_Cursor_hasVarDeclExternalStorageFalse) {
+  std::string Main = "main.cpp";
+  WriteFile(Main, "int a;");
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0, nullptr,
+   0, TUFlags);
+
+  CXCursor C = clang_getTranslationUnitCursor(ClangTU);
+  clang_visitChildren(
+  C,
+  [](CXCursor cursor, CXCursor parent,
+ CXClientData client_data) -> CXChildVisitResult {
+if (clang_getCursorKind(cursor) == CXCursor_VarDecl) {
+  EXPECT_FALSE(clang_Cursor_hasVarDeclExternalStorage(cursor));
+  return CXChildVisit_Break;
+}
+return CXChildVisit_Continue;
+  },
+  nullptr);
+}
+
+TEST_F(LibclangParseTest, clang_Cursor_hasVarDeclExternalStorageTrue) {
+  std::string Main = "main.cpp";
+  WriteFile(Main, "extern int a;");
+  ClangTU = clang_parseTranslationUnit(Index, Main.c_str(), nullptr, 0, nullptr,
+   0, TUFlags);
+
+  CXCursor C = clang_getTranslationUnitCursor(ClangTU);
+  clang_visitChildren(
+  C,
+  [](CXCursor cursor, CXCursor parent,
+ CXClientData client_data) -> CXChildVisitResult {
+if (clang_getCursorKind(cursor) == CXCursor_VarDecl) {
+  EXPECT_TRUE(clang_Cursor_hasVarDeclExternalStorage(cursor));
+  return CXChildVisit_Break;
+}
+return CXChildVisit_Continue;
+  },
+  nullptr);
+}
Index: clang/tools/libclang/libclang.exports
===
--- clang/tools/libclang/libclang.exports
+++ 

[PATCH] D66854: [index-while-building] PathIndexer

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4da126c3748f: [index-while-building] PathIndexer (authored 
by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66854?vs=283111=286619#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66854

Files:
  clang/include/clang/IndexSerialization/SerializablePathCollection.h
  clang/lib/CMakeLists.txt
  clang/lib/IndexSerialization/CMakeLists.txt
  clang/lib/IndexSerialization/SerializablePathCollection.cpp

Index: clang/lib/IndexSerialization/SerializablePathCollection.cpp
===
--- /dev/null
+++ clang/lib/IndexSerialization/SerializablePathCollection.cpp
@@ -0,0 +1,91 @@
+//===--- SerializablePathCollection.cpp -- Index of paths ---*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/IndexSerialization/SerializablePathCollection.h"
+#include "llvm/Support/Path.h"
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::index;
+
+StringPool::StringOffsetSize StringPool::add(StringRef Str) {
+  const std::size_t Offset = Buffer.size();
+  Buffer += Str;
+  return StringPool::StringOffsetSize(Offset, Str.size());
+}
+
+size_t PathPool::addFilePath(RootDirKind Root,
+ const StringPool::StringOffsetSize ,
+ StringRef Filename) {
+  FilePaths.emplace_back(DirPath(Root, Dir), Paths.add(Filename));
+  return FilePaths.size() - 1;
+}
+
+StringPool::StringOffsetSize PathPool::addDirPath(StringRef Dir) {
+  return Paths.add(Dir);
+}
+
+llvm::ArrayRef PathPool::getFilePaths() const {
+  return FilePaths;
+}
+
+StringRef PathPool::getPaths() const { return Paths.getBuffer(); }
+
+SerializablePathCollection::SerializablePathCollection(
+StringRef CurrentWorkDir, StringRef SysRoot, llvm::StringRef OutputFile)
+: WorkDir(CurrentWorkDir),
+  SysRoot(llvm::sys::path::parent_path(SysRoot).empty() ? StringRef()
+: SysRoot),
+  WorkDirPath(Paths.addDirPath(WorkDir)),
+  SysRootPath(Paths.addDirPath(SysRoot)),
+  OutputFilePath(Paths.addDirPath(OutputFile)) {}
+
+size_t SerializablePathCollection::tryStoreFilePath(const FileEntry ) {
+  auto FileIt = UniqueFiles.find();
+  if (FileIt != UniqueFiles.end())
+return FileIt->second;
+
+  const auto Dir = tryStoreDirPath(sys::path::parent_path(FE.getName()));
+  const auto FileIdx =
+  Paths.addFilePath(Dir.Root, Dir.Path, sys::path::filename(FE.getName()));
+
+  UniqueFiles.try_emplace(, FileIdx);
+  return FileIdx;
+}
+
+PathPool::DirPath SerializablePathCollection::tryStoreDirPath(StringRef Dir) {
+  // We don't want to strip separator if Dir is "/" - so we check size > 1.
+  while (Dir.size() > 1 && llvm::sys::path::is_separator(Dir.back()))
+Dir = Dir.drop_back();
+
+  auto DirIt = UniqueDirs.find(Dir);
+  if (DirIt != UniqueDirs.end())
+return DirIt->second;
+
+  const std::string OrigDir = Dir.str();
+
+  PathPool::RootDirKind Root = PathPool::RootDirKind::Regular;
+  if (!SysRoot.empty() && Dir.startswith(SysRoot) &&
+  llvm::sys::path::is_separator(Dir[SysRoot.size()])) {
+Root = PathPool::RootDirKind::SysRoot;
+Dir = Dir.drop_front(SysRoot.size());
+  } else if (!WorkDir.empty() && Dir.startswith(WorkDir) &&
+ llvm::sys::path::is_separator(Dir[WorkDir.size()])) {
+Root = PathPool::RootDirKind::CurrentWorkDir;
+Dir = Dir.drop_front(WorkDir.size());
+  }
+
+  if (Root != PathPool::RootDirKind::Regular) {
+while (!Dir.empty() && llvm::sys::path::is_separator(Dir.front()))
+  Dir = Dir.drop_front();
+  }
+
+  PathPool::DirPath Result(Root, Paths.addDirPath(Dir));
+  UniqueDirs.try_emplace(OrigDir, Result);
+  return Result;
+}
Index: clang/lib/IndexSerialization/CMakeLists.txt
===
--- /dev/null
+++ clang/lib/IndexSerialization/CMakeLists.txt
@@ -0,0 +1,6 @@
+add_clang_library(clangIndexSerialization
+  SerializablePathCollection.cpp
+
+  LINK_LIBS
+  clangBasic
+  )
Index: clang/lib/CMakeLists.txt
===
--- clang/lib/CMakeLists.txt
+++ clang/lib/CMakeLists.txt
@@ -20,6 +20,7 @@
 add_subdirectory(Tooling)
 add_subdirectory(DirectoryWatcher)
 add_subdirectory(Index)
+add_subdirectory(IndexSerialization)
 if(CLANG_ENABLE_STATIC_ANALYZER)
   add_subdirectory(StaticAnalyzer)
 endif()
Index: clang/include/clang/IndexSerialization/SerializablePathCollection.h

[PATCH] D82837: [Analyzer][WebKit] UncountedLambdaCaptureChecker

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG820e8d8656ec: [Analyzer][WebKit] 
UncountedLambdaCaptureChecker (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82837

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
  clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp

Index: clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/uncounted-lambda-captures.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.UncountedLambdaCapturesChecker %s 2>&1 | FileCheck %s --strict-whitespace
+#include "mock-types.h"
+
+void raw_ptr() {
+  RefCountable* ref_countable = nullptr;
+  auto foo1 = [ref_countable](){};
+  // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  // CHECK-NEXT:{{^}}  auto foo1 = [ref_countable](){};
+  // CHECK-NEXT:{{^}}   ^
+  auto foo2 = [_countable](){};
+  // CHECK: warning: Captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  auto foo3 = [&](){ ref_countable = nullptr; };
+  // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  // CHECK-NEXT:{{^}}  auto foo3 = [&](){ ref_countable = nullptr; };
+  // CHECK-NEXT:{{^}} ^
+  auto foo4 = [=](){ (void) ref_countable; };
+  // CHECK: warning: Implicitly captured raw-pointer 'ref_countable' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+}
+
+void references() {
+  RefCountable automatic;
+  RefCountable& ref_countable_ref = automatic;
+
+  auto foo1 = [ref_countable_ref](){};
+  // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  auto foo2 = [_countable_ref](){};
+  // CHECK: warning: Captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  auto foo3 = [&](){ (void) ref_countable_ref; };
+  // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+  auto foo4 = [=](){ (void) ref_countable_ref; };
+  // CHECK: warning: Implicitly captured reference 'ref_countable_ref' to uncounted type is unsafe [webkit.UncountedLambdaCapturesChecker]
+}
+
+void quiet() {
+// This code is not expected to trigger any warnings.
+  {
+RefCountable automatic;
+RefCountable _countable_ref = automatic;
+  }
+
+  auto foo3 = [&]() {};
+  auto foo4 = [=]() {};
+  RefCountable *ref_countable = nullptr;
+}
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedLambdaCapturesChecker.cpp
@@ -0,0 +1,106 @@
+//===- UncountedLambdaCapturesChecker.cpp *- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class UncountedLambdaCapturesChecker
+: public Checker> {
+private:
+  BugType Bug{this, "Lambda capture of uncounted variable",
+  "WebKit coding guidelines"};
+  mutable BugReporter *BR;
+
+public:
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager ,
+BugReporter ) const {
+BR = 
+
+// The calls to checkAST* from AnalysisConsumer don't
+// visit template instantiations or lambda classes. We
+// want to visit those, so we make our own RecursiveASTVisitor.
+struct LocalVisitor : public RecursiveASTVisitor {
+  const UncountedLambdaCapturesChecker *Checker;
+  explicit LocalVisitor(const UncountedLambdaCapturesChecker *Checker)
+  : Checker(Checker) {
+assert(Checker);

[PATCH] D83438: [AST] Fix potential nullptr dereference in Expr::HasSideEffects

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Sounds like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83438



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

[PATCH] D83438: [AST] Fix potential nullptr dereference in Expr::HasSideEffects

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfdb69539bcd2: [AST] Fix potential nullptr dereference in 
Expr::HasSideEffects (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83438

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/Expr.cpp
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/HasSideEffectsTest.cpp

Index: clang/unittests/AST/HasSideEffectsTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/HasSideEffectsTest.cpp
@@ -0,0 +1,86 @@
+//===- unittest/AST/HasSideEffectsTest.cpp ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+using namespace clang;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function Process)
+  : Process(std::move(Process)) {
+assert(this->Process);
+  }
+
+  std::unique_ptr CreateASTConsumer(CompilerInstance ,
+ StringRef InFile) {
+class Consumer : public ASTConsumer {
+public:
+  Consumer(llvm::function_ref Process)
+  : Process(Process) {}
+
+  void HandleTranslationUnit(ASTContext ) override { Process(Ctx); }
+
+private:
+  llvm::function_ref Process;
+};
+
+return std::make_unique(Process);
+  }
+
+private:
+  llvm::unique_function Process;
+};
+
+class RunHasSideEffects
+: public RecursiveASTVisitor {
+public:
+  RunHasSideEffects(ASTContext& Ctx)
+  : Ctx(Ctx) {}
+
+  bool VisitLambdaExpr(LambdaExpr *LE) {
+LE->HasSideEffects(Ctx);
+return true;
+  }
+
+  ASTContext& Ctx;
+};
+} // namespace
+
+TEST(HasSideEffectsTest, All) {
+  llvm::StringRef Code = R"cpp(
+void Test() {
+  int msize = 4;
+  float arr[msize];
+  [] {};
+}
+  )cpp";
+
+  ASSERT_NO_FATAL_FAILURE(
+clang::tooling::runToolOnCode(
+  std::make_unique(
+  [&](clang::ASTContext ) {
+  RunHasSideEffects Visitor(Ctx);
+  Visitor.TraverseAST(Ctx);
+  }
+  ),
+  Code)
+  );
+
+}
Index: clang/unittests/AST/CMakeLists.txt
===
--- clang/unittests/AST/CMakeLists.txt
+++ clang/unittests/AST/CMakeLists.txt
@@ -26,6 +26,7 @@
   DeclTest.cpp
   EvaluateAsRValueTest.cpp
   ExternalASTSourceTest.cpp
+  HasSideEffectsTest.cpp
   NamedDeclPrinterTest.cpp
   RecursiveASTVisitorTest.cpp
   SizelessTypesTest.cpp
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -3629,7 +3629,7 @@
   case LambdaExprClass: {
 const LambdaExpr *LE = cast(this);
 for (Expr *E : LE->capture_inits())
-  if (E->HasSideEffects(Ctx, IncludePossibleEffects))
+  if (E && E->HasSideEffects(Ctx, IncludePossibleEffects))
 return true;
 return false;
   }
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -1931,6 +1931,7 @@
 
   /// Const iterator that walks over the capture initialization
   /// arguments.
+  /// FIXME: This interface is prone to being used incorrectly.
   using const_capture_init_iterator = Expr *const *;
 
   /// Retrieve the initialization expressions for this lambda's captures.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82629: [libclang] Fix crash when visiting a captured VLA.

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e089e98a9d5: [libclang] Fix crash when visiting a captured 
VLA (authored by ckandeler, committed by jkorous).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82629

Files:
  clang/test/Index/evaluate-cursor.cpp
  clang/tools/libclang/CIndex.cpp


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3272,7 +3272,7 @@
   }
   // Visit init captures
   for (auto InitExpr : E->capture_inits()) {
-if (Visit(InitExpr))
+if (InitExpr && Visit(InitExpr))
   return true;
   }
 
Index: clang/test/Index/evaluate-cursor.cpp
===
--- clang/test/Index/evaluate-cursor.cpp
+++ clang/test/Index/evaluate-cursor.cpp
@@ -29,6 +29,12 @@
 constexpr static int calc_val() { return 1 + 2; }
 const auto the_value = calc_val() + sizeof(char);
 
+void vlaTest() {
+  int msize = 4;
+  float arr[msize];
+  [] {};
+}
+
 // RUN: c-index-test -evaluate-cursor-at=%s:4:7 \
 // RUN:-evaluate-cursor-at=%s:8:7 \
 // RUN:-evaluate-cursor-at=%s:8:11 -std=c++11 %s | FileCheck %s
@@ -65,3 +71,7 @@
 // CHECK-EXPR: Value: 3
 // CHECK-EXPR: unsigned, Value: 4
 // CHECK-EXPR: unsigned, Value: 1
+
+// RUN: c-index-test -evaluate-cursor-at=%s:35:5 \
+// RUN:-std=c++11 %s | FileCheck -check-prefix=VLA %s
+// VLA: Not Evaluatable


Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3272,7 +3272,7 @@
   }
   // Visit init captures
   for (auto InitExpr : E->capture_inits()) {
-if (Visit(InitExpr))
+if (InitExpr && Visit(InitExpr))
   return true;
   }
 
Index: clang/test/Index/evaluate-cursor.cpp
===
--- clang/test/Index/evaluate-cursor.cpp
+++ clang/test/Index/evaluate-cursor.cpp
@@ -29,6 +29,12 @@
 constexpr static int calc_val() { return 1 + 2; }
 const auto the_value = calc_val() + sizeof(char);
 
+void vlaTest() {
+  int msize = 4;
+  float arr[msize];
+  [] {};
+}
+
 // RUN: c-index-test -evaluate-cursor-at=%s:4:7 \
 // RUN:-evaluate-cursor-at=%s:8:7 \
 // RUN:-evaluate-cursor-at=%s:8:11 -std=c++11 %s | FileCheck %s
@@ -65,3 +71,7 @@
 // CHECK-EXPR: Value: 3
 // CHECK-EXPR: unsigned, Value: 4
 // CHECK-EXPR: unsigned, Value: 1
+
+// RUN: c-index-test -evaluate-cursor-at=%s:35:5 \
+// RUN:-std=c++11 %s | FileCheck -check-prefix=VLA %s
+// VLA: Not Evaluatable
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82629: [libclang] Fix crash when visiting a captured VLA.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@ckandeler do you have commit access or do you want me to land the patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82629



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

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@milianw I just approved https://reviews.llvm.org/D82629 - I feel like that 
patch is addressing the core issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82740



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

[PATCH] D82629: [libclang] Fix crash when visiting a captured VLA.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM! Thanks for fixing this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82629



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

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Your patch definitely fixes the symptoms of the bug. I just want to make sure 
that we aren't covering some logic error with a bandaid as it would be harder 
to find out the root cause once we land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82740



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

[PATCH] D82740: [libclang]: check validity before visiting Stmt node

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@milianw could you try to reduce the reproducer you have? Maybe take lldb and 
see where does the `nullptr` come from?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82740



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

[PATCH] D77179: [Analyzer][WebKit] UncountedCallArgsChecker

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa7eb3692e762: [Analyzer][WebKit] UncountedCallArgsChecker 
(authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77179

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/UncountedCallArgsChecker.cpp
  clang/test/Analysis/Checkers/WebKit/call-args.cpp

Index: clang/test/Analysis/Checkers/WebKit/call-args.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/call-args.cpp
@@ -0,0 +1,344 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.UncountedCallArgsChecker -verify %s
+
+#include "mock-types.h"
+
+RefCountable* provide() { return nullptr; }
+void consume_refcntbl(RefCountable*) {}
+
+namespace simple {
+  void foo() {
+consume_refcntbl(provide());
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace multi_arg {
+  void consume_refcntbl(int, RefCountable* foo, bool) {}
+  void foo() {
+consume_refcntbl(42, provide(), true);
+// expected-warning@-1{{Call argument for parameter 'foo' is uncounted and unsafe}}
+  }
+}
+
+namespace ref_counted {
+  Ref provide_ref_counted() { return Ref{}; }
+  void consume_ref_counted(Ref) {}
+
+  void foo() {
+consume_refcntbl(provide_ref_counted().get());
+// no warning
+  }
+}
+
+namespace methods {
+  struct Consumer {
+void consume_ptr(RefCountable* ptr) {}
+void consume_ref(const RefCountable& ref) {}
+  };
+
+  void foo() {
+Consumer c;
+
+c.consume_ptr(provide());
+// expected-warning@-1{{Call argument for parameter 'ptr' is uncounted and unsafe}}
+c.consume_ref(*provide());
+// expected-warning@-1{{Call argument for parameter 'ref' is uncounted and unsafe}}
+  }
+
+  void foo2() {
+struct Consumer {
+  void consume(RefCountable*) { }
+  void whatever() {
+consume(provide());
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+};
+  }
+
+  void foo3() {
+struct Consumer {
+  void consume(RefCountable*) { }
+  void whatever() {
+this->consume(provide());
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+};
+  }
+}
+
+namespace casts {
+  RefCountable* downcast(RefCountable*) { return nullptr; }
+
+  void foo() {
+consume_refcntbl(provide());
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(static_cast(provide()));
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(dynamic_cast(provide()));
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(const_cast(provide()));
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(reinterpret_cast(provide()));
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(downcast(provide()));
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+
+consume_refcntbl(
+  static_cast(
+downcast(
+  static_cast(
+provide()
+  )
+)
+  )
+);
+// expected-warning@-8{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace null_ptr {
+  void foo_ref() {
+consume_refcntbl(nullptr);
+consume_refcntbl(0);
+  }
+}
+
+namespace ref_counted_lookalike {
+  struct Decoy {
+RefCountable* get() { return nullptr; }
+  };
+
+  void foo() {
+Decoy D;
+
+consume_refcntbl(D.get());
+// expected-warning@-1{{Call argument is uncounted and unsafe}}
+  }
+}
+
+namespace Ref_to_reference_conversion_operator {
+  template struct Ref {
+Ref() = default;
+Ref(T*) { }
+T* get() { return nullptr; }
+operator T& () { return t; }
+T t;
+  };
+
+  void consume_ref(RefCountable&) {}
+
+  void foo() {
+Ref bar;
+consume_ref(bar);
+  }
+}
+
+namespace param_formarding_function {
+  void consume_ref_countable_ref(RefCountable&) {}
+  void consume_ref_countable_ptr(RefCountable*) {}
+
+  namespace ptr {
+void foo(RefCountable* param) {
+  consume_ref_countable_ptr(param);
+}
+  }
+
+  namespace ref {
+void foo(RefCountable& param) {
+  consume_ref_countable_ref(param);
+}
+  }
+
+  namespace ref_deref_operators {
+void foo_ref(RefCountable& param) {
+  consume_ref_countable_ptr();
+}
+
+void foo_ptr(RefCountable* param) {
+  consume_ref_countable_ref(*param);
+}
+  }
+
+  namespace casts {
+
+  RefCountable* 

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

aaron.ballman wrote:
> njames93 wrote:
> > jkorous wrote:
> > > aaron.ballman wrote:
> > > > jkorous wrote:
> > > > > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > > > > reason `hasClass` doesn't. English is not my first language though.
> > > > I agree that `hasClass` seems unnatural here. Out of curiosity, could 
> > > > we modify the `hasName` matcher to work on base specifiers so you can 
> > > > write: `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for 
> > > > the more wordy version 
> > > > `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
> > > Wouldn't it be strange to treat `hasName` differently than all the other 
> > > narrowing matchers? Honest question - I feel that `hasName` might be the 
> > > most commonly used, just don't know if that's enough to justify this.
> > > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> > Repurposing `hasName` would be a pain especially considering 99% of its use 
> > cases wont be for base class matching.
> > Wouldn't it be strange to treat hasName differently than all the other 
> > narrowing matchers? Honest question - I feel that hasName might be the most 
> > commonly used, just don't know if that's enough to justify this. 
> > https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers
> 
> Different how? I'm suggesting to overload `hasName` to work on a 
> `CXXBaseSpecifier` since those have a name.
> 
> > Repurposing hasName would be a pain especially considering 99% of its use 
> > cases wont be for base class matching.
> 
> I'm asking what the right API is for users, though, which is a bit different. 
> Base specifiers have names (there are no unnamed base specifiers), so to me, 
> it makes more sense for `hasName` to work with them directly since that is 
> the thing that does name matching.
> 
> I think you can accomplish this by using a `PolymorphicMatcherWithParam1` 
> like we do for `hasOverloadedOperatorName` which can narrow to either a 
> `CXXOperatorCallExpr` or a `FunctionDecl`.
>> Wouldn't it be strange to treat hasName differently than all the other 
>> narrowing matchers? Honest question - I feel that hasName might be the most 
>> commonly used, just don't know if that's enough to justify this. 
>> https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers

> Different how? I'm suggesting to overload hasName to work on a 
> CXXBaseSpecifier since those have a name.

What I meant is that technically we can overload any `Matcher` 
matcher in the same fashion so having the overloaded version of `hasName` only 
makes it somewhat special (and someone might argue that it'd impact consistency 
of matchers composability). Anyway, I'm fine with your suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81691: [clangd] Set CWD in semaCodeComplete

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

A slightly tangential thing - we recently got an internal bugreport about 
clangd handling combination of working directory and relative paths for 
`-fmodules-cache-path` combination in compile_command.json incorrectly.
I tried some quick hacks but failed - it seems that ultimately the limiting 
factor is  the code in clang dealing with modules not using VFS. It's on our 
list of things to fix although we don't have a timeframe for it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81691



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

aaron.ballman wrote:
> jkorous wrote:
> > Nit: while "[base specifier] `hasType`" sounds natural to me for some 
> > reason `hasClass` doesn't. English is not my first language though.
> I agree that `hasClass` seems unnatural here. Out of curiosity, could we 
> modify the `hasName` matcher to work on base specifiers so you can write: 
> `cxxRecordDecl(hasAnyBase(hasName("Base")))` as shorthand for the more wordy 
> version `cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl(hasName("Base")`?
Wouldn't it be strange to treat `hasName` differently than all the other 
narrowing matchers? Honest question - I feel that `hasName` might be the most 
commonly used, just don't know if that's enough to justify this.
https://clang.llvm.org/docs/LibASTMatchersReference.html#narrowing-matchers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3621
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClassOrClassTemplate,
+  internal::Matcher, InnerMatcher) {

I think we should just use `eachOf` matcher for this kind of composition.

https://clang.llvm.org/docs/LibASTMatchersReference.html#traversal-matchers


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81552: [ASTMatchers] Added hasDirectBase and hasClass Matchers

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@njames93 `hasDirectBase` seems like a useful matcher to me! OTOH I am not 
totally convinced about `hasType` -> `hasClass`. Anyway, don't you want to land 
`hasDirectBase` as a separate patch first and then discuss the rest?

One more thing - I'm just thinking if there isn't some corner case where a base 
class could be interpreted as both direct and indirect. The most ugly case I 
came up with is virtual inheritance. I admit I don't know what the standard 
says about this so it might be a non-issue. Also, it'd still probably make 
sense to match on such base. WDYT?

  struct Base {};
  struct Proxy : virtual Base {};
  struct Derived : Base, Proxy {};




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3537
 AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
-hasType,
-AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl,
-CXXBaseSpecifier),
+hasType, AST_POLYMORPHIC_SUPPORTED_TYPES(Expr, FriendDecl, ValueDecl),
 internal::Matcher, InnerMatcher, 1) {

aaron.ballman wrote:
> njames93 wrote:
> > aaron.ballman wrote:
> > > This is undoing a change that was just added less than two weeks ago, so 
> > > I think the potential for breaking code is small. That said, can you 
> > > explain why you think `hasClass` is a better approach than `hasType`?
> > Yeah, as that change hasn't reached landed onto a release branch breaking 
> > code shouldn't be an issue, If it was I'd leave it in.
> > 
> > - `hasType` is very generic, whereas `hasClass` is specific to what a 
> > `CXXBaseSpecifier` supports.
> > - It makes the matchers marginally simpler.
> >   `hasDirectBase(hasType(cxxRecordDecl(hasName("Base"` vs 
> > `hasDirectBase(hasClass(hasName("Base")))`
> > - In the documentation it also specifies that `hasClass` takes a 
> > `Matcher, making it more user friendly.
> FWIW, I prefer `hasType` to `hasClass`. You can inherit from things which are 
> not a class, such as a struct (so the name is a bit of a misnomer, but not 
> too awful), a class template (which you can't match with this interface), or 
> a template type (which you also can't match with this interface).
I don't feel super strongly about this but I also slightly prefer `hasType`.

To be fair - I didn't really have things like inheritance from template 
parameters on my mind when working on `hasAnyBase` (it's definitely not tested) 
so I'd rather not assume it works.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3553
+/// \endcode
+AST_MATCHER_P(CXXBaseSpecifier, hasClass, internal::Matcher,
+  InnerMatcher) {

Nit: while "[base specifier] `hasType`" sounds natural to me for some reason 
`hasClass` doesn't. English is not my first language though.



Comment at: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:3158
+class Base {};
+class Derived : BAse {};
+  )cc",

Is this (`Base` != `BAse`) a typo or a way how to tease the asserts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81552



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

[PATCH] D81017: [Analyzer][WebKit] Check record definition is available in NoUncountedMembers checker

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd61ad660503d: [Analyzer][WebKit] Check record definition is 
available in NoUncountedMembers… (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81017

Files:
  clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
  clang/test/Analysis/Checkers/WebKit/uncounted-members-regression-46142.cpp


Index: 
clang/test/Analysis/Checkers/WebKit/uncounted-members-regression-46142.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/uncounted-members-regression-46142.cpp
@@ -0,0 +1,9 @@
+// regression test for https://bugs.llvm.org/show_bug.cgi?id=46142
+
+// RUN: %clang_analyze_cc1 
-analyzer-checker=webkit.WebKitNoUncountedMemberChecker -verify %s
+// expected-no-diagnostics
+
+class ClassWithoutADefinition;
+class Foo {
+const ClassWithoutADefinition *foo;
+};
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
@@ -75,7 +75,8 @@
 continue;
 
   if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
-if (isRefCountable(MemberCXXRD))
+// If we don't see the definition we just don't know.
+if (MemberCXXRD->hasDefinition() && isRefCountable(MemberCXXRD))
   reportBug(Member, MemberType, MemberCXXRD, RD);
   }
 }


Index: clang/test/Analysis/Checkers/WebKit/uncounted-members-regression-46142.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/uncounted-members-regression-46142.cpp
@@ -0,0 +1,9 @@
+// regression test for https://bugs.llvm.org/show_bug.cgi?id=46142
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitNoUncountedMemberChecker -verify %s
+// expected-no-diagnostics
+
+class ClassWithoutADefinition;
+class Foo {
+const ClassWithoutADefinition *foo;
+};
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
@@ -75,7 +75,8 @@
 continue;
 
   if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
-if (isRefCountable(MemberCXXRD))
+// If we don't see the definition we just don't know.
+if (MemberCXXRD->hasDefinition() && isRefCountable(MemberCXXRD))
   reportBug(Member, MemberType, MemberCXXRD, RD);
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi @kadircet!
I am investigating a failure of `PatchesAdditionalIncludes` test that we got 
internally. It's a failed assert in `ReplayPreamble::replay`.
Our clangd source code is for all practical purposes identical to upstream 
version but not so with clang source. Specifically what we do is that in 
`CompilerInstance::createPreprocessor` we always add one particular callback.
This means that when in the test we are calling `buildPreamble` for `TestTU TU` 
with empty buffer we never hit the early return in `ReplayPreamble::attach()` 
(ParsedAST.cpp:124) like upstream version does and proceed to create a new 
`ReplayPreamble` object with `PreambleBounds` of `size() == 0` which leads to 
`ReplayPreamble::MainFileTokens` to be empty and later we hit failed assert in 
`ReplayPreamble::replay` about `assert(HashTok != MainFileTokens.end() && ...)`.

Now, while I can just tweak either `ReplayPreamble::attach()` or remove the PP 
callback in the test internally I am wondering whether you consider empty 
preamble & PP with callbacks valid use-case of `ReplayPreamble` and worth a fix.

This is where we are creating the empty `MainFileTokens`:

  * frame #0: 0x000103337649 ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, 
Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, 
SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, 
PB=0x7ffeefbfc658)  + 169 at ParsedAST.cpp:142
frame #1: 0x00010333756d ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::ReplayPreamble(this=0x000114f45da0, 
Includes=0x7ffeefbfc678, Delegate=0x000114f3bd80, 
SM=0x000114f3dd80, PP=0x000115850218, LangOpts=0x000114f38eb0, 
PB=0x7ffeefbfc658)  + 77 at ParsedAST.cpp:139
frame #2: 0x000103334fa7 ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::attach(Includes=0x7ffeefbfc678, 
Clang=0x000114f33ea0, PB=0x7ffeefbfc658)  + 183 at ParsedAST.cpp:126
frame #3: 0x000103334189 ClangdTests` 
clang::clangd::ParsedAST::build(Filename=(Data = "/clangd-test/foo.cpp", Length 
= 20), Inputs=0x7ffeefbfdf98, CI=nullptr, 
CompilerInvocationDiags=ArrayRef @ 0x7ffeefbfd830, 
Preamble=std::__1::shared_ptr::element_type 
@ 0x000114f3e728 strong=2 weak=1)  + 3897 at ParsedAST.cpp:385
frame #4: 0x0001006f1032 ClangdTests` clang::clangd::(anonymous 
namespace)::ParsedASTTest_PatchesAdditionalIncludes_Test::TestBody(this=0x000114f04090)
  + 1778 at ParsedASTTests.cpp:477

This is the failed assert:

  * frame #4: 0x000103337a0c ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::replay(this=0x000114f45da0)  + 556 at 
ParsedAST.cpp:182
frame #5: 0x00010333777c ClangdTests` clang::clangd::(anonymous 
namespace)::ReplayPreamble::FileChanged(this=0x000114f45da0, Loc=(ID = 74), 
Reason=ExitFile, Kind=C_User, PrevFID=(ID = 2))  + 156 at ParsedAST.cpp:166
frame #6: 0x000101b7900a ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204080, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 90 at PPCallbacks.h:390
frame #7: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x0001162040c0, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #8: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204100, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #9: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116204160, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #10: 0x000101b79046 ClangdTests` 
clang::PPChainedCallbacks::FileChanged(this=0x000116205480, Loc=(ID = 74), 
Reason=ExitFile, FileType=C_User, PrevFID=(ID = 2))  + 150 at PPCallbacks.h:391
frame #11: 0x000101b9a590 ClangdTests` 
clang::Preprocessor::HandleEndOfFile(this=0x000115850218, 
Result=0x000116808210, isEndOfMacro=false)  + 3904 at PPLexerChange.cpp:469
frame #12: 0x000101b38713 ClangdTests` 
clang::Lexer::LexEndOfFile(this=0x000116206330, Result=0x000116808210, 
CurPtr="")  + 931 at Lexer.cpp:2833
frame #13: 0x000101b39c44 ClangdTests` 
clang::Lexer::LexTokenInternal(this=0x000116206330, 
Result=0x000116808210, TokAtPhysicalStartOfLine=true)  + 420 at 
Lexer.cpp:3265
frame #14: 0x000101b382f8 ClangdTests` 
clang::Lexer::Lex(this=0x000116206330, Result=0x000116808210)  + 216 at 
Lexer.cpp:3216
frame #15: 0x000101bd7396 ClangdTests` 
clang::Preprocessor::Lex(this=0x000115850218, Result=0x000116808210)  + 
118 at Preprocessor.cpp:900
frame #16: 0x000101b75ef1 ClangdTests` 

[PATCH] D79063: [ASTMatchers] Matchers related to C++ inheritance

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a5c97f3a4b8: [ASTMatchers] Matchers related to C++ 
inheritance (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D79063?vs=265545=267346#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79063

Files:
  clang-tools-extra/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/AST/ASTTypeTraits.h
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/AST/ASTTypeTraits.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -3006,5 +3006,126 @@
   EXPECT_TRUE(matchesWithOpenMP(Source6, Matcher));
 }
 
+TEST(HasAnyBase, DirectBase) {
+  EXPECT_TRUE(matches(
+  "struct Base {};"
+  "struct ExpectedMatch : Base {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+}
+
+TEST(HasAnyBase, IndirectBase) {
+  EXPECT_TRUE(matches(
+  "struct Base {};"
+  "struct Intermediate : Base {};"
+  "struct ExpectedMatch : Intermediate {};",
+  cxxRecordDecl(hasName("ExpectedMatch"),
+hasAnyBase(hasType(cxxRecordDecl(hasName("Base")));
+}
+
+TEST(HasAnyBase, NoBase) {
+  EXPECT_TRUE(notMatches("struct Foo {};"
+ "struct Bar {};",
+ cxxRecordDecl(hasAnyBase(hasType(cxxRecordDecl());
+}
+
+TEST(IsPublicBase, Public) {
+  EXPECT_TRUE(matches("class Base {};"
+  "class Derived : public Base {};",
+  cxxRecordDecl(hasAnyBase(isPublic();
+}
+
+TEST(IsPublicBase, DefaultAccessSpecifierPublic) {
+  EXPECT_TRUE(matches("class Base {};"
+  "struct Derived : Base {};",
+  cxxRecordDecl(hasAnyBase(isPublic();
+}
+
+TEST(IsPublicBase, Private) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : private Base {};",
+ cxxRecordDecl(hasAnyBase(isPublic();
+}
+
+TEST(IsPublicBase, DefaultAccessSpecifierPrivate) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : Base {};",
+ cxxRecordDecl(hasAnyBase(isPublic();
+}
+
+TEST(IsPublicBase, Protected) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : protected Base {};",
+ cxxRecordDecl(hasAnyBase(isPublic();
+}
+
+TEST(IsPrivateBase, Private) {
+  EXPECT_TRUE(matches("class Base {};"
+  "class Derived : private Base {};",
+  cxxRecordDecl(hasAnyBase(isPrivate();
+}
+
+TEST(IsPrivateBase, DefaultAccessSpecifierPrivate) {
+  EXPECT_TRUE(matches("struct Base {};"
+  "class Derived : Base {};",
+  cxxRecordDecl(hasAnyBase(isPrivate();
+}
+
+TEST(IsPrivateBase, Public) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : public Base {};",
+ cxxRecordDecl(hasAnyBase(isPrivate();
+}
+
+TEST(IsPrivateBase, DefaultAccessSpecifierPublic) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "struct Derived : Base {};",
+ cxxRecordDecl(hasAnyBase(isPrivate();
+}
+
+TEST(IsPrivateBase, Protected) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : protected Base {};",
+ cxxRecordDecl(hasAnyBase(isPrivate();
+}
+
+TEST(IsProtectedBase, Protected) {
+  EXPECT_TRUE(matches("class Base {};"
+  "class Derived : protected Base {};",
+  cxxRecordDecl(hasAnyBase(isProtected();
+}
+
+TEST(IsProtectedBase, Public) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : public Base {};",
+ cxxRecordDecl(hasAnyBase(isProtected();
+}
+
+TEST(IsProtectedBase, Private) {
+  EXPECT_TRUE(notMatches("class Base {};"
+ "class Derived : private Base {};",
+ cxxRecordDecl(hasAnyBase(isProtected();
+}
+
+TEST(IsVirtual, Directly) {
+  EXPECT_TRUE(matches("class Base {};"
+  "class Derived : virtual Base {};",
+  cxxRecordDecl(hasAnyBase(isVirtual();
+}
+
+TEST(IsVirtual, 

[PATCH] D77178: [Analyzer][WebKit] NoUncountedMembersChecker

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG660cda572d6e: [Analyzer][WebKit] NoUncountedMembersChecker 
(authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77178

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/WebKit/DiagOutputUtils.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
  clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp

Index: clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/uncounted-members.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitNoUncountedMemberChecker -verify %s
+
+#include "mock-types.h"
+
+namespace members {
+  struct Foo {
+  private:
+RefCountable* a = nullptr;
+// expected-warning@-1{{Member variable 'a' in 'members::Foo' is a raw pointer to ref-countable type 'RefCountable'}}
+
+  protected:
+RefPtr b;
+
+  public:
+RefCountable silenceWarningAboutInit;
+RefCountable& c = silenceWarningAboutInit;
+// expected-warning@-1{{Member variable 'c' in 'members::Foo' is a reference to ref-countable type 'RefCountable'}}
+Ref d;
+  };
+
+  template
+  struct FooTmpl {
+T* a;
+// expected-warning@-1{{Member variable 'a' in 'members::FooTmpl' is a raw pointer to ref-countable type 'RefCountable'}}
+  };
+
+  void forceTmplToInstantiate(FooTmpl) {}
+}
+
+namespace ignore_unions {
+  union Foo {
+RefCountable* a;
+RefPtr b;
+Ref c;
+  };
+
+  template
+  union RefPtr {
+T* a;
+  };
+
+  void forceTmplToInstantiate(RefPtr) {}
+}
Index: clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/WebKit/NoUncountedMembersChecker.cpp
@@ -0,0 +1,150 @@
+//===- NoUncountedMembersChecker.cpp -*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ASTUtils.h"
+#include "DiagOutputUtils.h"
+#include "PtrTypesSemantics.h"
+#include "clang/AST/CXXInheritance.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/Support/Casting.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+
+class NoUncountedMemberChecker
+: public Checker> {
+private:
+  BugType Bug;
+  mutable BugReporter *BR;
+
+public:
+  NoUncountedMemberChecker()
+  : Bug(this,
+"Member variable is a raw-poiner/reference to reference-countable "
+"type",
+"WebKit coding guidelines") {}
+
+  void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager ,
+BugReporter ) const {
+BR = 
+
+// The calls to checkAST* from AnalysisConsumer don't
+// visit template instantiations or lambda classes. We
+// want to visit those, so we make our own RecursiveASTVisitor.
+struct LocalVisitor : public RecursiveASTVisitor {
+  const NoUncountedMemberChecker *Checker;
+  explicit LocalVisitor(const NoUncountedMemberChecker *Checker)
+  : Checker(Checker) {
+assert(Checker);
+  }
+
+  bool shouldVisitTemplateInstantiations() const { return true; }
+  bool shouldVisitImplicitCode() const { return false; }
+
+  bool VisitRecordDecl(const RecordDecl *RD) {
+Checker->visitRecordDecl(RD);
+return true;
+  }
+};
+
+LocalVisitor visitor(this);
+visitor.TraverseDecl(const_cast(TUD));
+  }
+
+  void visitRecordDecl(const RecordDecl *RD) const {
+if (shouldSkipDecl(RD))
+  return;
+
+for (auto Member : RD->fields()) {
+  const Type *MemberType = Member->getType().getTypePtrOrNull();
+  if (!MemberType)
+continue;
+
+  if (auto *MemberCXXRD = MemberType->getPointeeCXXRecordDecl()) {
+if (isRefCountable(MemberCXXRD))
+  reportBug(Member, MemberType, MemberCXXRD, RD);
+  }
+}
+  }
+
+  bool shouldSkipDecl(const RecordDecl *RD) const {
+if (!RD->isThisDeclarationADefinition())
+  return 

[PATCH] D80126: Add documentation URL records to the .dia format and expose them via libclang

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:323
+  // A documentation URL has an ID and path size.
+  if (Record.size() != 2)
+return SDError::MalformedDiagnosticRecord;

owenv wrote:
> owenv wrote:
> > jkorous wrote:
> > > I am just wondering what happens with the ID. Shouldn't we pass it too?
> > I think the ID should be `Record[0]` in the call t 
> > visitDocumentationURLRecord below. I based this on the handling of category 
> > records.
> Sorry, I don't know how to reply without marking this as done
Got it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80126



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

[PATCH] D77177: [Analyzer][WebKit] RefCntblBaseVirtualDtorChecker & shared utils

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D77177#2049805 , @thakis wrote:

> This breaks the build everywhere (e.g. 
> http://45.33.8.238/linux/18283/step_4.txt) and has been in for over an hour. 
> Please watch bots after landing. (I think changes that are created more 
> recently automatically get presubmit compile testing on phab.) Reverted in 
> 1108f5c737dbdab0277874a7e5b237491839c43a 
>  for now.


Sorry about the mess! I shouldn't have landed the patch just before I stepped 
out for 2 hrs...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77177



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

[PATCH] D77177: [Analyzer][WebKit] RefCntblBaseVirtualDtorChecker & shared utils

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf7c7e8a523f5: [Analyzer][WebKit] 
RefCntblBaseVirtualDtorChecker (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D77177?vs=263915=265565#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77177

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/WebKit/ASTUtils.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/DiagOutputUtils.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.cpp
  clang/lib/StaticAnalyzer/Checkers/WebKit/PtrTypesSemantics.h
  clang/lib/StaticAnalyzer/Checkers/WebKit/RefCntblBaseVirtualDtorChecker.cpp
  clang/test/Analysis/Checkers/WebKit/mock-types.h
  clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
  clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor.cpp

Index: clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitRefCntblBaseVirtualDtor -verify %s
+
+struct RefCntblBase {
+  void ref() {}
+  void deref() {}
+};
+
+struct Derived : RefCntblBase { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'Derived' but doesn't have virtual destructor}}
+
+struct DerivedWithVirtualDtor : RefCntblBase {
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedWithVirtualDtor' but doesn't have virtual destructor}}
+  virtual ~DerivedWithVirtualDtor() {}
+};
+
+
+
+template
+struct DerivedClassTmpl : T { };
+typedef DerivedClassTmpl Foo;
+
+
+
+struct RandomBase {};
+struct RandomDerivedClass : RandomBase { };
+
+
+
+struct FakeRefCntblBase1 {
+  private:
+  void ref() {}
+  void deref() {}
+};
+struct Quiet1 : FakeRefCntblBase1 {};
+
+struct FakeRefCntblBase2 {
+  protected:
+  void ref() {}
+  void deref() {}
+};
+struct Quiet2 : FakeRefCntblBase2 {};
+
+class FakeRefCntblBase3 {
+  void ref() {}
+  void deref() {}
+};
+struct Quiet3 : FakeRefCntblBase3 {};
+struct Quiet4 : private RefCntblBase {};
+class Quiet5 : RefCntblBase {};
+
+void foo () {
+  Derived d;
+}
Index: clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/ref-cntbl-base-virtual-dtor-templates.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.WebKitRefCntblBaseVirtualDtor -verify %s
+
+struct RefCntblBase {
+  void ref() {}
+  void deref() {}
+};
+
+template
+struct DerivedClassTmpl1 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl1' but doesn't have virtual destructor}}
+
+DerivedClassTmpl1 a;
+
+
+
+template
+struct DerivedClassTmpl2 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl2' but doesn't have virtual destructor}}
+
+template int foo(T) { DerivedClassTmpl2 f; return 42; }
+int b = foo(RefCntblBase{});
+
+
+
+template
+struct DerivedClassTmpl3 : T { };
+// expected-warning@-1{{Struct 'RefCntblBase' is used as a base of struct 'DerivedClassTmpl3' but doesn't have virtual destructor}}
+
+typedef DerivedClassTmpl3 Foo;
+Foo c;
Index: clang/test/Analysis/Checkers/WebKit/mock-types.h
===
--- /dev/null
+++ clang/test/Analysis/Checkers/WebKit/mock-types.h
@@ -0,0 +1,48 @@
+#ifndef mock_types_1103988513531
+#define mock_types_1103988513531
+
+template  struct Ref {
+  T t;
+
+  Ref() : t{} {};
+  Ref(T *) {}
+  T *get() { return nullptr; }
+  operator const T &() const { return t; }
+  operator T &() { return t; }
+};
+
+template  struct RefPtr {
+  T *t;
+
+  RefPtr() : t(new T) {}
+  RefPtr(T *t) : t(t) {}
+  T *get() { return t; }
+  T *operator->() { return t; }
+  T *() { return *t; }
+  RefPtr =(T *) { return *this; }
+};
+
+template  bool operator==(const RefPtr &, const RefPtr &) {
+  return false;
+}
+
+template  bool operator==(const RefPtr &, T *) { return false; }
+
+template  bool operator==(const RefPtr &, T &) { return false; }
+
+template  bool operator!=(const RefPtr &, const RefPtr &) {
+  return false;
+}
+
+template  bool operator!=(const RefPtr &, T *) { return false; }
+
+template  bool operator!=(const RefPtr &, T &) { return false; }
+
+struct RefCountable {
+  void ref() {}
+  void deref() {}
+};
+
+template  T *downcast(T *t) { return t; }
+
+#endif
Index: 

[PATCH] D80126: Add documentation URL records to the .dia format and expose them via libclang

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Hi Owen,
Do you plan to land the functionality for emitting documentation URLs in clang 
too?




Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:323
+  // A documentation URL has an ID and path size.
+  if (Record.size() != 2)
+return SDError::MalformedDiagnosticRecord;

I am just wondering what happens with the ID. Shouldn't we pass it too?



Comment at: clang/test/Misc/serialized-diags-doumentation.c:4
+// CHECK: hello.swift:1:1: error: non-nominal type '(Int, Int)' cannot be 
extended [] []
+// CHECK: [Documentation URL: 
/Users/owenvoorhees/Documents/Development/swift-source/build/Ninja-ReleaseAssert/swift-macosx-x86_64/share/doc/swift/diagnostics/nominal-types.md]

Tip - you can use regexes in CHECKs.
https://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-regex-matching-syntax

I think we should change this to something like:
```
// CHECK: [Documentation URL: {{.*}}/doc/swift/diagnostics/nominal-types.md]
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80126



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

[PATCH] D80279: [libclang] Extend clang_Cursor_Evaluate().

[Jan Korous via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80279



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

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG05eedf1f5b44: [clang][VerifyDiagnosticConsumer] Support 
filename wildcards (authored by arames, committed by jkorous).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
+
+// expected-error@*:* {{missing error}}
+
+// expected-error@*:123 {{invalid line : "*" required}}
+//
+//  CHECK: error: 'error' diagnostics expected but not seen:
+// CHECK-NEXT:   File * Line * (directive at {{.*}}verify-any-file.c:6): missing error
+// CHECK-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   File {{.*}}verify-any-file.c Line 8: missing or invalid line number following '@' in expected '*'
+// CHECK-NEXT: 2 errors generated.
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max) {}
 
   bool isValid(std::string ) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string ) override {
@@ -294,11 +296,13 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine , const UnattachedDirective ,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc,
+ bool MatchAnyFileAndLine = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
-  std::unique_ptr D =
-  Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+  std::unique_ptr D = Directive::create(
+  UD.RegexKind, UD.DirectivePos, ExpectedLoc, MatchAnyFileAndLine,
+  MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +502,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFileAndLine = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +531,39 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
-
-const FileEntry *FE = >getFileEntry();
-if (SM.translateFile(FE).isInvalid())
-  SM.createFileID(FE, Pos, SrcMgr::C_User);
-
-if (PH.Next(Line) && Line > 0)
-  ExpectedLoc = 

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

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

LGTM! Thanks Alex!


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

https://reviews.llvm.org/D79834



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

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Jan Korous via Phabricator via cfe-commits]

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

LGTM! Thanks for the work!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72100



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

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

Could you please add a test for this method to 
`clang/unittests/Basic/SourceManagerTest.cpp`?




Comment at: clang/include/clang/Basic/SourceManager.h:816
 
+  /// Returns true when the given FileEntry corresponds to the main file.
+  bool isMainFile(FileEntryRef SourceFile);

Please mention the assertion this method makes about the state of the instance 
it's called on.



Comment at: clang/lib/Basic/SourceManager.cpp:397
+return false;
+  return FE->getUID() == SourceFile.getUID();
+}

I don't really understand all the details here but shouldn't we use this 
comparison?
```
bool operator==(const FileEntryRef , const FileEntryRef )
```


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

https://reviews.llvm.org/D79834



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

[PATCH] D79834: Speed up preamble building by replacing the slow translateFile call by a new, faster isMainFile check

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

IIUC the issue is that `SourceManager::translateFile()` basically consists of 
two blocks of code:

  // First, check the main file ID, since it is common to look for a
  // location in the main file.
  if (MainFileID.isValid()) {
bool Invalid = false;
const SLocEntry  = getSLocEntry(MainFileID, );
if (Invalid)
  return FileID();
  
if (MainSLoc.isFile()) {
  const ContentCache *MainContentCache =
  MainSLoc.getFile().getContentCache();
  if (MainContentCache && MainContentCache->OrigEntry == SourceFile)
return MainFileID;
}
  }

and

// The location we're looking for isn't in the main file; look
// through all of the local source locations.
  ...

The comments suggest that the first block is a heuristic related to our case 
and the second block I would assume being the expensive part. 
`SourceManager::getFileEntryRefForID` implementation seems similar to the first 
block.

It makes sense to me to avoid the expensive search. I'm just wondering - how 
much speedup do we get with caching the value?


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

https://reviews.llvm.org/D79834



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

[PATCH] D79451: [libclang] Remove duplicate dependency on LLVMSupport

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG02b303321d3f: [libclang] Remove duplicate dependency on 
LLVMSupport (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79451

Files:
  clang/tools/libclang/CMakeLists.txt


Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -44,7 +44,6 @@
   clangSema
   clangSerialization
   clangTooling
-  LLVMSupport
 )
 
 if (CLANG_ENABLE_ARCMT)


Index: clang/tools/libclang/CMakeLists.txt
===
--- clang/tools/libclang/CMakeLists.txt
+++ clang/tools/libclang/CMakeLists.txt
@@ -44,7 +44,6 @@
   clangSema
   clangSerialization
   clangTooling
-  LLVMSupport
 )
 
 if (CLANG_ENABLE_ARCMT)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78760: Check a class has a definition before iterating over its base classes

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

In D78760#2003118 , @rsmith wrote:

> If so, we'll need to make sure that all code that iterates over base classes 
> checks for this condition (I bet there are more cases than the two that you 
> found).


Just a potentially related thought: I was recently trying to understand under 
what circumstances should I expect `CXXBaseSpecifier::getType()` to return 
`QualType` instance for which `isNull()` could be `true`. I gave up and don't 
make any assumption.
Would the suggested change affect this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78760



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

[PATCH] D77612: [ASTMatchers] Fix isDerivedFrom for recursive templates

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14d89bfbe0b4: [ASTMatchers] Fix isDerivedFrom for recursive 
templates (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D77612?vs=255561=257815#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77612

Files:
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp


Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -453,6 +453,20 @@
   EXPECT_TRUE(notMatches("class X;", IsDerivedFromX));
   EXPECT_TRUE(notMatches("class Y;", IsDerivedFromX));
   EXPECT_TRUE(notMatches("", IsDerivedFromX));
+  EXPECT_TRUE(matches("class X {}; template class Y : Y, X {};",
+IsDerivedFromX));
+  EXPECT_TRUE(matches("class X {}; template class Y : X, Y {};",
+IsDerivedFromX));
+
+  DeclarationMatcher IsZDerivedFromX = cxxRecordDecl(hasName("Z"),
+isDerivedFrom("X"));
+  EXPECT_TRUE(
+matches(
+  "class X {};"
+  "template class Y : Y {};"
+  "template<> class Y<0> : X {};"
+  "class Z : Y<1> {};",
+  IsZDerivedFromX));
 
   DeclarationMatcher IsDirectlyDerivedFromX =
   cxxRecordDecl(isDirectlyDerivedFrom("X"));
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -916,9 +916,8 @@
 if (!ClassDecl)
   continue;
 if (ClassDecl == Declaration) {
-  // This can happen for recursive template definitions; if the
-  // current declaration did not match, we can safely return false.
-  return false;
+  // This can happen for recursive template definitions.
+  continue;
 }
 BoundNodesTreeBuilder Result(*Builder);
 if (Base.matches(*ClassDecl, this, )) {


Index: clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
@@ -453,6 +453,20 @@
   EXPECT_TRUE(notMatches("class X;", IsDerivedFromX));
   EXPECT_TRUE(notMatches("class Y;", IsDerivedFromX));
   EXPECT_TRUE(notMatches("", IsDerivedFromX));
+  EXPECT_TRUE(matches("class X {}; template class Y : Y, X {};",
+IsDerivedFromX));
+  EXPECT_TRUE(matches("class X {}; template class Y : X, Y {};",
+IsDerivedFromX));
+
+  DeclarationMatcher IsZDerivedFromX = cxxRecordDecl(hasName("Z"),
+isDerivedFrom("X"));
+  EXPECT_TRUE(
+matches(
+  "class X {};"
+  "template class Y : Y {};"
+  "template<> class Y<0> : X {};"
+  "class Z : Y<1> {};",
+  IsZDerivedFromX));
 
   DeclarationMatcher IsDirectlyDerivedFromX =
   cxxRecordDecl(isDirectlyDerivedFrom("X"));
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -916,9 +916,8 @@
 if (!ClassDecl)
   continue;
 if (ClassDecl == Declaration) {
-  // This can happen for recursive template definitions; if the
-  // current declaration did not match, we can safely return false.
-  return false;
+  // This can happen for recursive template definitions.
+  continue;
 }
 BoundNodesTreeBuilder Result(*Builder);
 if (Base.matches(*ClassDecl, this, )) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75483: [Sema] Fix a crash when attaching comments to an implicit decl

[Jan Korous via Phabricator via cfe-commits]

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

Thanks a bunch for fixing this!

That also means we can skip all the implicit declarations from the `for (const 
Decl *D : Decls) {` just a couple lines below your fix since implicit 
declaration can hardly have a doc comment attached... I'll send a patch.


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

https://reviews.llvm.org/D75483



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

[PATCH] D74746: [clang][doxygen] Fix -Wdocumentation warning for tag typedefs

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

@vsapsai Sorry! I read your comment only after landing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74746



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

[PATCH] D74746: [clang][doxygen] Fix -Wdocumentation warning for tag typedefs

[Jan Korous via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f56789c8fe8: [clang][doxygen] Fix false -Wdocumentation 
warning for tag typedefs (authored by jkorous).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D74746?vs=245057=245705#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74746

Files:
  clang/include/clang/AST/CommentSema.h
  clang/lib/AST/CommentSema.cpp
  clang/test/Sema/warn-documentation-tag-typedef.cpp

Index: clang/test/Sema/warn-documentation-tag-typedef.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-documentation-tag-typedef.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -Wdocumentation -fsyntax-only %s 2>&1 | FileCheck -allow-empty %s
+
+/*!
+@class Foo
+*/
+typedef class { } Foo;
+// CHECK-NOT: warning:
+
+/*! 
+@struct Bar
+*/
+typedef struct { } Bar;
+// CHECK-NOT: warning:
Index: clang/lib/AST/CommentSema.cpp
===
--- clang/lib/AST/CommentSema.cpp
+++ clang/lib/AST/CommentSema.cpp
@@ -12,6 +12,7 @@
 #include "clang/AST/CommentDiagnostic.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/SmallString.h"
@@ -134,7 +135,9 @@
   unsigned DiagSelect;
   switch (Comment->getCommandID()) {
 case CommandTraits::KCI_class:
-  DiagSelect = (!isClassOrStructDecl() && !isClassTemplateDecl()) ? 1 : 0;
+  DiagSelect =
+  (!isClassOrStructOrTagTypedefDecl() && !isClassTemplateDecl()) ? 1
+ : 0;
   // Allow @class command on @interface declarations.
   // FIXME. Currently, \class and @class are indistinguishable. So,
   // \class is also allowed on an @interface declaration
@@ -148,7 +151,7 @@
   DiagSelect = !isObjCProtocolDecl() ? 3 : 0;
   break;
 case CommandTraits::KCI_struct:
-  DiagSelect = !isClassOrStructDecl() ? 4 : 0;
+  DiagSelect = !isClassOrStructOrTagTypedefDecl() ? 4 : 0;
   break;
 case CommandTraits::KCI_union:
   DiagSelect = !isUnionDecl() ? 5 : 0;
@@ -935,15 +938,50 @@
 return RD->isUnion();
   return false;
 }
+static bool isClassOrStructDeclImpl(const Decl *D) {
+  if (auto *record = dyn_cast_or_null(D))
+return !record->isUnion();
+
+  return false;
+}
 
 bool Sema::isClassOrStructDecl() {
   if (!ThisDeclInfo)
 return false;
   if (!ThisDeclInfo->IsFilled)
 inspectThisDecl();
-  return ThisDeclInfo->CurrentDecl &&
- isa(ThisDeclInfo->CurrentDecl) &&
- !isUnionDecl();
+
+  if (!ThisDeclInfo->CurrentDecl)
+return false;
+
+  return isClassOrStructDeclImpl(ThisDeclInfo->CurrentDecl);
+}
+
+bool Sema::isClassOrStructOrTagTypedefDecl() {
+  if (!ThisDeclInfo)
+return false;
+  if (!ThisDeclInfo->IsFilled)
+inspectThisDecl();
+
+  if (!ThisDeclInfo->CurrentDecl)
+return false;
+
+  if (isClassOrStructDeclImpl(ThisDeclInfo->CurrentDecl))
+return true;
+
+  if (auto *ThisTypedefDecl = dyn_cast(ThisDeclInfo->CurrentDecl)) {
+auto UnderlyingType = ThisTypedefDecl->getUnderlyingType();
+if (auto ThisElaboratedType = dyn_cast(UnderlyingType)) {
+  auto DesugaredType = ThisElaboratedType->desugar();
+  if (auto *DesugaredTypePtr = DesugaredType.getTypePtrOrNull()) {
+if (auto *ThisRecordType = dyn_cast(DesugaredTypePtr)) {
+  return isClassOrStructDeclImpl(ThisRecordType->getAsRecordDecl());
+}
+  }
+}
+  }
+
+  return false;
 }
 
 bool Sema::isClassTemplateDecl() {
Index: clang/include/clang/AST/CommentSema.h
===
--- clang/include/clang/AST/CommentSema.h
+++ clang/include/clang/AST/CommentSema.h
@@ -217,6 +217,9 @@
   bool isTemplateOrSpecialization();
   bool isRecordLikeDecl();
   bool isClassOrStructDecl();
+  /// \return \c true if the declaration that this comment is attached to
+  /// declares either struct, class or tag typedef.
+  bool isClassOrStructOrTagTypedefDecl();
   bool isUnionDecl();
   bool isObjCInterfaceDecl();
   bool isObjCProtocolDecl();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D74371: [DirectoryWatcher] Fix misuse of FSEvents API and data race

[Jan Korous via Phabricator via cfe-commits]

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

LGTM

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74371



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

[PATCH] D74009: [clang] Improve diagnostic note for implicit conversions that are disallowed because they involve more than one user-defined conversion.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a reviewer: jkorous.
jkorous added a comment.
Herald added a subscriber: dexonsmith.

Hi @logan-5!
I suggest you split the patch into two smaller ones so it is easier to review.

1. A NFC patch with refactoring of the interface (`bool` -> 
`UserDefinedConversionsKind`).
2. Patch that introduces and uses 
`diag::note_ovl_candidate_bad_user_defined_conv`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74009



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

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Jan Korous via Phabricator via cfe-commits]

jkorous added a comment.

We should either simplify the implementation to reflect that we don't support 
e. g. `*:42` (seems preferable to me) or have the codepaths that are currently 
not accessible through `-fverify` tested by other means.




Comment at: clang/include/clang/Frontend/VerifyDiagnosticConsumer.h:206
 bool MatchAnyLine;
+bool MatchAnyFile;
 

Should we rename it to `MatchAnyFileAndLine`?



Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:300
+ SourceLocation ExpectedLoc, bool MatchAnyFile = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.

Should we make it clear from the interface that `MatchAnyFile` => 
`MatchAnyLine`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72100



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