[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 444397.
george.burgess.iv added a comment.

Rebased on top of 891319f654c102572cf7028ed04bbaf6c59e7bff 
 as 
requested; `ninja check-clang-extra docs-clang-tools-html` passes


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

https://reviews.llvm.org/D126735

Files:
  clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
@@ -592,3 +592,9 @@
   (void)a;
 }
 #undef return_t
+
+// Explicitly specifying `(void)` is a way to sidestep -Wvexing-parse, so we
+// should not warn about it here.
+void most_vexing_parse() {
+  int foo(void);
+}
Index: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -109,9 +109,13 @@
 }
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function definition");
-  } else
+  } else if (!Function->getLexicalDeclContext()->isFunctionOrMethod()) {
+// Only do this if our decl context is outside of a function or method;
+// otherwise, the user may have explicitly written `(void)` to avoid Most
+// Vexing Parse warnings.
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function declaration");
+  }
 }
 
 bool isMacroIdentifier(const IdentifierTable , const Token ) 
{


Index: clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize/redundant-void-arg.cpp
@@ -592,3 +592,9 @@
   (void)a;
 }
 #undef return_t
+
+// Explicitly specifying `(void)` is a way to sidestep -Wvexing-parse, so we
+// should not warn about it here.
+void most_vexing_parse() {
+  int foo(void);
+}
Index: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -109,9 +109,13 @@
 }
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function definition");
-  } else
+  } else if (!Function->getLexicalDeclContext()->isFunctionOrMethod()) {
+// Only do this if our decl context is outside of a function or method;
+// otherwise, the user may have explicitly written `(void)` to avoid Most
+// Vexing Parse warnings.
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function declaration");
+  }
 }
 
 bool isMacroIdentifier(const IdentifierTable , const Token ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126735: [clang-tidy] Silence modernize-redundant-void-arg in the case of vexing parses

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: LegalizeAdulthood, aaron.ballman.
george.burgess.iv added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
george.burgess.iv requested review of this revision.
Herald added a subscriber: cfe-commits.

Clang's `-Wvexing-parse` is enabled by default 
. When the vexing statement was _intended_ to 
be interpreted as a function that takes zero arguments, the function's 
declaration can be rewritten as `T foo(void);` to silence `-Wvexing-parse`. 
This workaround seems to upset `clang-tidy`.

Given my own lack of understanding of the corners of C++ parsing, I'm... not 
entirely confident that this patch catches everything && has no unintended 
side-effects. The bits that `-Wvexing-parse` depends upon are stored within 
`DeclaratorChunk::FunctionTypeInfo`s, which seem to only exist during AST 
formation, so it's not clear to me how to directly get at that information from 
clang-tidy.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126735

Files:
  clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
@@ -592,3 +592,9 @@
   (void)a;
 }
 #undef return_t
+
+// Explicitly specifying `(void)` is a way to sidestep -Wvexing-parse, so we
+// should not warn about it here.
+void most_vexing_parse() {
+  int foo(void);
+}
Index: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -109,9 +109,13 @@
 }
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function definition");
-  } else
+  } else if (!Function->getLexicalDeclContext()->isFunctionOrMethod()) {
+// Only do this if our decl context is outside of a function or method;
+// otherwise, the user may have explicitly written `(void)` to avoid Most
+// Vexing Parse warnings.
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function declaration");
+  }
 }
 
 bool isMacroIdentifier(const IdentifierTable , const Token ) 
{


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-redundant-void-arg.cpp
@@ -592,3 +592,9 @@
   (void)a;
 }
 #undef return_t
+
+// Explicitly specifying `(void)` is a way to sidestep -Wvexing-parse, so we
+// should not warn about it here.
+void most_vexing_parse() {
+  int foo(void);
+}
Index: clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/RedundantVoidArgCheck.cpp
@@ -109,9 +109,13 @@
 }
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function definition");
-  } else
+  } else if (!Function->getLexicalDeclContext()->isFunctionOrMethod()) {
+// Only do this if our decl context is outside of a function or method;
+// otherwise, the user may have explicitly written `(void)` to avoid Most
+// Vexing Parse warnings.
 removeVoidArgumentTokens(Result, SourceRange(Start, End),
  "function declaration");
+  }
 }
 
 bool isMacroIdentifier(const IdentifierTable , const Token ) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang/test/Sema/attr-diagnose-as-builtin.c:62
+
+#ifdef __cplusplus
+template 

mbenfield wrote:
> george.burgess.iv wrote:
> > nit: can we also add a non-templated overload check in here?
> > 
> > if the diag isn't beautiful, that's fine IMO. just having a test-case to 
> > show the expected behavior would be nice
> Sorry, not sure what is being requested. By a "non-templated overload check" 
> do you mean something different from `memcpy2` above?
something like the new edit suggestion below is what i have in mind :)



Comment at: clang/test/Sema/attr-diagnose-as-builtin.c:71
+void failure_template_instantiation(int x) 
__attribute__((diagnose_as_builtin(some_templated_function, 1))) {} // 
expected-error{{'diagnose_as_builtin' attribute requires parameter 1 to be a 
builtin function}}
+#endif




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024

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

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

LGTM % 2 nits. Please feel free to commit after it LGT @aaron.ballman too. :)

Thanks again!




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2961
+  "%0 attribute references function %1, which %plural{0:takes no 
arguments|1:takes one argument|"
+  ":requires exactly %2 arguments}2">;
+def err_attribute_bounds_for_function : Error<

tiny nit: for consistency with the other options here



Comment at: clang/test/Sema/attr-diagnose-as-builtin.c:62
+
+#ifdef __cplusplus
+template 

nit: can we also add a non-templated overload check in here?

if the diag isn't beautiful, that's fine IMO. just having a test-case to show 
the expected behavior would be nice


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.

LGTM. Thanks again!




Comment at: clang/lib/Sema/SemaChecking.cpp:735
+
+auto *FormatExpr = TheCall->getArg(FormatIndex)->IgnoreParenImpCasts();
+

nit: const auto if possible (and below)



Comment at: clang/lib/Sema/SemaChecking.cpp:756
+ScanfDiagnosticFormatHandler H(
+[&](unsigned Index) { return ComputeSizeArgument(Index + DataIndex); },
+Diagnose);

Please put this in a variable and pass that into `H`'s constructor. 
`function_ref` doesn't own the function it points to, so any use of this at 
line >= 758 (e.g., line 768) is a use-after-free.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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

[PATCH] D112024: [clang] diagnose_as attribute for Fortify diagnosing like builtins.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! The idea LGTM, and sounds like a solid way for us to do better 
about diagnosing FORTIFY'ed calls in Clang. I have a handful of mostly 
nits/questions for you :)




Comment at: clang/include/clang/Basic/Attr.td:3822
+def DiagnoseAs : InheritableAttr {
+  let Spellings = [Clang<"diagnose_as">];
+  let Args = [ExprArgument<"Function">,

purely subjective nit: `diagnose_as` feels a bit too generic. if you agree, 
does `diagnose_as_builtin` sound potentially better?



Comment at: clang/lib/Sema/SemaChecking.cpp:598
+  bool UseDAIAttr = false;
+  FunctionDecl *UseDecl = FD;
+

nit: `const`



Comment at: clang/lib/Sema/SemaChecking.cpp:600
+
+  auto DAIAttr = FD->getAttr();
+  if (DAIAttr) {

nit: `const auto *`



Comment at: clang/lib/Sema/SemaChecking.cpp:602-607
+DeclRefExpr *F = dyn_cast_or_null(DAIAttr->getFunction());
+if (!F)
+  return;
+FunctionDecl *AttrDecl = dyn_cast_or_null(F->getFoundDecl());
+if (!AttrDecl)
+  return;

It seems that this is serving as implicit validation of this attribute's 
members (e.g., if we can't figure out the Function that this DAIAttr is 
pointing to, we exit gracefully, etc.)

Would it be better to instead do this validation in `handleDiagnoseAsAttr`, and 
store the results (which we can then presume are valid here) in the 
`DiagnoseAsAttr`?

In that case, it might be easiest to simply store the `FunctionDecl` that's 
being referenced, rather than a DRE to it.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1013
+
+Expr *IndexExpr = AL.getArgAsExpr(I);
+uint32_t Index;

nit: `const`?



Comment at: clang/test/Sema/warn-fortify-source.c:184
 
+void *test_memcpy(const void *src, size_t c, void *dst) 
__attribute__((diagnose_as(__builtin_memcpy, 3, 1, 2))) {
+  return __builtin_memcpy(dst, src, c);

we generally try to extensively test error cases around new attributes. i'd 
recommend the following cases:

- `diagnose_as(__function_that_doesnt_exist)`
- `diagnose_as(function_i_predeclared_but_which_isnt_a_builtin)`
- `diagnose_as(__builtin_memcpy, 1, 2, 3, 4)` (too many args) 
- `diagnose_as(__builtin_memcpy, 1, 2)` (too few args)
- `diagnose_as(__builtin_memcpy, "abc", 2, 3)` 
- `void test_memcpy(double, double, double) 
__attribute__((diagnose_as(__builtin_memcpy, 1, 2, 3));` (mismatched param 
types)
-  `diagnose_as(__some_overloaded_function_name)`

there're probably more that might be nice, but those seem like a good 
foundation :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112024

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

LGTM % nits -- thanks for this! :)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:836
 
+def warn_fortify_scanf_overflow : Warning <
+  "'%0' may overflow; destination buffer in argument %1 has size "

nit: s/Warning ,
+  InGroup;

nit: please s/null/NUL/ for consistency with elsewhere



Comment at: clang/lib/Sema/SemaChecking.cpp:415
+  // argument whose size we want.
+  using ComputeSizeFunction = std::function(unsigned)>;
+

optional: llvm generally prefers `FunctionRef`s for simplicity and speed. if 
it's easy to refactor to use those (seems like it may be), please do. 
otherwise, it's not a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2e75986a21e5: bugprone-argument-comment: ignore mismatches 
from system headers (authored by gbiv).

Changed prior to commit:
  https://reviews.llvm.org/D3?vs=335660=363846#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D3

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-argument-comment %t
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- -- -I 
%S/Inputs/bugprone-argument-comment
 
 // FIXME: clang-tidy should provide a -verify mode to make writing these checks
 // easier and more accurate.
@@ -134,3 +134,20 @@
   std::swap(a, /*num=*/b);
 }
 } // namespace ignore_std_functions
+
+namespace regular_header {
+#include "header-with-decl.h"
+void test() {
+  my_header_function(/*not_arg=*/1);
+// CHECK-NOTES: [[@LINE-1]]:22: warning: argument name 'not_arg' in comment 
does not match parameter name 'arg'
+// CHECK-NOTES: header-with-decl.h:1:29: note: 'arg' declared here
+// CHECK-FIXES: my_header_function(/*not_arg=*/1);
+}
+} // namespace regular_header
+
+namespace system_header {
+#include "system-header-with-decl.h"
+void test() {
+  my_system_header_function(/*not_arg=*/1);
+}
+} // namespace system_header
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
@@ -0,0 +1,3 @@
+#pragma clang system_header
+
+void my_system_header_function(int arg);
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
@@ -0,0 +1 @@
+void my_header_function(int arg);
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -20,10 +20,12 @@
 namespace tidy {
 namespace bugprone {
 namespace {
-AST_MATCHER(Decl, isFromStdNamespace) {
+AST_MATCHER(Decl, isFromStdNamespaceOrSystemHeader) {
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
-return D->isStdNamespace();
-  return false;
+if (D->isStdNamespace())
+  return true;
+  return Node.getASTContext().getSourceManager().isInSystemHeader(
+  Node.getLocation());
 }
 } // namespace
 
@@ -66,13 +68,13 @@
// not specified by the standard, and standard library
// implementations in practice have to use reserved names to
// avoid conflicts with same-named macros.
-   unless(hasDeclaration(isFromStdNamespace(
-  .bind("expr"),
-  this);
-  Finder->addMatcher(
-  cxxConstructExpr(unless(hasDeclaration(isFromStdNamespace(
+   unless(hasDeclaration(isFromStdNamespaceOrSystemHeader(
   .bind("expr"),
   this);
+  Finder->addMatcher(cxxConstructExpr(unless(hasDeclaration(
+  isFromStdNamespaceOrSystemHeader(
+ .bind("expr"),
+ this);
 }
 
 static std::vector>


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-argument-comment %t
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- -- -I %S/Inputs/bugprone-argument-comment
 
 // FIXME: clang-tidy should provide a -verify mode to make writing these checks
 // easier and more accurate.
@@ -134,3 +134,20 @@
   std::swap(a, /*num=*/b);
 }
 } // namespace ignore_std_functions
+
+namespace regular_header {
+#include "header-with-decl.h"
+void test() {
+  

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

...This entirely dropped off my radar. Will try to land it now; thanks, all!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D3

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

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

[George Burgess IV 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 rGe12e02df09a9: [clang] Evaluate strlen of strcpy argument for 
-Wfortify-source. (authored by mbenfield, committed by gbiv).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104887

Files:
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/AST/ExprConstant.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Analysis/security-syntax-checks.m
  clang/test/Sema/warn-fortify-source.c

Index: clang/test/Sema/warn-fortify-source.c
===
--- clang/test/Sema/warn-fortify-source.c
+++ clang/test/Sema/warn-fortify-source.c
@@ -58,6 +58,19 @@
   __builtin_stpncpy(s1, s2, 20); // expected-warning {{'stpncpy' size argument is too large; destination buffer has size 10, but size argument is 20}}
 }
 
+void call_strcpy() {
+  const char *const src = "abcd";
+  char dst[4];
+  __builtin_strcpy(dst, src); // expected-warning {{'strcpy' will always overflow; destination buffer has size 4, but the source string has length 5 (including NUL byte)}}
+}
+
+void call_strcpy_nowarn() {
+  const char *const src = "abcd";
+  char dst[5];
+  // We should not get a warning here.
+  __builtin_strcpy(dst, src);
+}
+
 void call_memmove() {
   char s1[10], s2[20];
   __builtin_memmove(s2, s1, 20);
Index: clang/test/Analysis/security-syntax-checks.m
===
--- clang/test/Analysis/security-syntax-checks.m
+++ clang/test/Analysis/security-syntax-checks.m
@@ -1,37 +1,37 @@
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify -Wno-fortify-source \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify -Wno-fortify-source \
 // RUN:   -DUSE_BUILTINS \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify -Wno-fortify-source \
 // RUN:   -DVARIANT \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify \
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 %s -verify -Wno-fortify-source \
 // RUN:   -DUSE_BUILTINS -DVARIANT \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify -Wno-fortify-source \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify -Wno-fortify-source \
 // RUN:   -DUSE_BUILTINS \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify -Wno-fortify-source \
 // RUN:   -DVARIANT \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
 
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify \
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-cloudabi %s -verify -Wno-fortify-source \
 // RUN:   -DUSE_BUILTINS -DVARIANT \
 // RUN:   -analyzer-checker=security.insecureAPI \
 // RUN:   -analyzer-checker=security.FloatLoopCounter
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -588,14 +588,8 @@
 
 } // namespace
 
-/// Check a call to BuiltinID for buffer overflows. If BuiltinID is a
-/// __builtin_*_chk function, then use the object size argument specified in the
-/// source. Otherwise, infer the object size using __builtin_object_size.
 void Sema::checkFortifiedBuiltinMemoryFunction(FunctionDecl *FD,
CallExpr *TheCall) {
-  // FIXME: There are some more useful checks we could be doing here:
-  //  - Evaluate strlen of strcpy arguments, use as object size.
-
   if (TheCall->isValueDependent() || TheCall->isTypeDependent() ||
   isConstantEvaluated())
 return;
@@ -607,13 +601,66 @@
   const TargetInfo  = 

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

lgtm -- thanks!

please give a day for other reviewers to add any last minute comments, then i 
think we can land this.




Comment at: clang/lib/Sema/SemaChecking.cpp:739
 DiagID = diag::warn_fortify_source_size_mismatch;
-SizeIndex = TheCall->getNumArgs() - 1;
-ObjectIndex = 0;
+SourceSize = ComputeCheckVariantSize(TheCall->getNumArgs() - 1);
+DestinationSize = ComputeSizeArgument(0);

mbenfield wrote:
> george.burgess.iv wrote:
> > i expected `ComputeCheckVariantSize` to imply that the argument was to a 
> > `_chk` function, but these `case`s don't reference `_chk` functions (nor do 
> > we set `IsChkVariant = true;`). should this be calling 
> > `ComputeSizeArgument` instead?
> Maybe the name `ComputeCheckVariantSize` was misleading and it'll be more 
> clear now that I'm changing the name, but these functions like `strncat`, the 
> `memcpy`s below, and `snprintf`, etc, all take an explicit size argument just 
> like the `_chk` functions.
ohh, gotcha. i was misinterpreting the relationship between `IsChkVariant` and 
`SourceSize` then -- thanks for the clarification!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104887

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

[PATCH] D106030: [Clang] add support for error+warning fn attrs

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> Adding something to the IR for the sole purpose of producing a diagnostic 
> feels really weird. I'm not sure I see why the frontend can't see this 
> attribute and directly warn

To add a bit more clarification, the goal of this attribute is specifically to 
emit diagnostics after optimizations such as inlining have taken place. 
`diagnose_if` is clang's hammer if we want frontend diagnostics: 
https://godbolt.org/z/jbzbqEzbG . `diagnose_if` is a bit more flexible than 
"the condition must be an ICE per the standard," but AIUI the kernel has code 
like what Eli mentioned, which clang currently can't work with in the frontend 
(since it requires inlining, etc etc).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106030

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

[PATCH] D104887: [clang] Evaluate strlen of strcpy argument for -Wfortify-source.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

thanks for this! mostly just nits from me




Comment at: clang/lib/AST/ExprConstant.cpp:15755
+
+bool Expr::tryEvaluateStrLen(uint64_t , ASTContext ) const {
+  Expr::EvalStatus Status;

Looks like this is the second "try to evaluate the call to this builtin 
function" API endpoint we have here (the other being for 
`__builtin_object_size`). IMO this isn't an issue, but if we need many more of 
these, it might be worth considering exposing a more general 
`Expr::tryEvaluateBuiltinFunctionCall(APValue &, ASTContext &, BuiltinID, 
ArrayRef)` or similar.



Comment at: clang/lib/Sema/SemaChecking.cpp:604
 
+  auto ComputeCheckVariantSize = [&](unsigned Index) -> Optional 
{
+Expr::EvalResult Result;

nit: i'd rename this `ComputeExplicitObjectSizeArgument`



Comment at: clang/lib/Sema/SemaChecking.cpp:621
+
+Expr *ObjArg = TheCall->getArg(Index);
+uint64_t Result;

nit: would `const Expr *` work here? clang prefers to have `const` where 
possible



Comment at: clang/lib/Sema/SemaChecking.cpp:739
 DiagID = diag::warn_fortify_source_size_mismatch;
-SizeIndex = TheCall->getNumArgs() - 1;
-ObjectIndex = 0;
+SourceSize = ComputeCheckVariantSize(TheCall->getNumArgs() - 1);
+DestinationSize = ComputeSizeArgument(0);

i expected `ComputeCheckVariantSize` to imply that the argument was to a `_chk` 
function, but these `case`s don't reference `_chk` functions (nor do we set 
`IsChkVariant = true;`). should this be calling `ComputeSizeArgument` instead?



Comment at: clang/lib/Sema/SemaChecking.cpp:753
 DiagID = diag::warn_fortify_source_overflow;
-SizeIndex = TheCall->getNumArgs() - 1;
-ObjectIndex = 0;
+SourceSize = ComputeCheckVariantSize(TheCall->getNumArgs() - 1);
+DestinationSize = ComputeSizeArgument(0);

same "shouldn't this be `ComputeSizeArgument`?' question



Comment at: clang/lib/Sema/SemaChecking.cpp:762
 DiagID = diag::warn_fortify_source_size_mismatch;
-SizeIndex = 1;
-ObjectIndex = 0;
+SourceSize = ComputeCheckVariantSize(1);
+DestinationSize = ComputeSizeArgument(0);

same question



Comment at: clang/test/Sema/warn-fortify-source.c:64
+  char dst[4];
+  __builtin_strcpy(dst, src); // expected-warning {{'strcpy' will always 
overflow; destination buffer has size 4, but the source string has length 7 
(including null byte)}}
+}

for completeness and consistency, please include a case where this warning 
doesn't fire.

at the same time, it'd be nice to test for an off-by-one (which i believe is 
handled correctly by this patch already); maybe shorten `src` to `"abcd"` and 
have a test on `char dst2[5];` that doesn't fire?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104887

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

[PATCH] D103623: [Clang] Test case for -Wunused-but-set-variable, warn for volatile.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG20f7b5f3f9c8: [Clang] Test case for 
-Wunused-but-set-variable, warn for volatile. (authored by mbenfield, committed 
by george.burgess.iv).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103623

Files:
  clang/test/Sema/warn-unused-but-set-variables.c


Index: clang/test/Sema/warn-unused-but-set-variables.c
===
--- clang/test/Sema/warn-unused-but-set-variables.c
+++ clang/test/Sema/warn-unused-but-set-variables.c
@@ -23,6 +23,10 @@
   int a;
   w = (a = 0);
 
+  // Following gcc, warn for a volatile variable.
+  volatile int b; // expected-warning{{variable 'b' set but not used}}
+  b = 0;
+
   int x;
   x = 0;
   return x;


Index: clang/test/Sema/warn-unused-but-set-variables.c
===
--- clang/test/Sema/warn-unused-but-set-variables.c
+++ clang/test/Sema/warn-unused-but-set-variables.c
@@ -23,6 +23,10 @@
   int a;
   w = (a = 0);
 
+  // Following gcc, warn for a volatile variable.
+  volatile int b; // expected-warning{{variable 'b' set but not used}}
+  b = 0;
+
   int x;
   x = 0;
   return x;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf49cae278b4: [Clang] -Wunused-but-set-parameter and 
-Wunused-but-set-variable (authored by mbenfield, committed by 
george.burgess.iv).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Also don't warn for a reference.
+void f4(int ) {
+  x = 0;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a reviewer: rtrieu.
george.burgess.iv added a comment.

Just a few more nits and LGTM. We probably want the thoughts of someone with 
ownership in warnings to be sure. +rtrieu might be good?




Comment at: clang/lib/Sema/SemaDecl.cpp:13740
+// other than assigning to it, sets the corresponding value to false.
+static void AreAllUsesSets(Stmt *Body,
+   llvm::SmallDenseMap *Map) {

mbenfield wrote:
> george.burgess.iv wrote:
> > nit: Should this be a `const Stmt*`? I don't think we should be mutating 
> > the `Body`
> Unfortunately the `RecursiveASTVisitor`'s non-overridden member functions 
> don't take `const`.
Yeah, I was thinking of StmtVisitor's interface -- my mistake



Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+ P->getLocation()) ==
+DiagnosticsEngine::Ignored)
+  return false;

mbenfield wrote:
> mbenfield wrote:
> > george.burgess.iv wrote:
> > > If we want to optimize for when this warning is disabled, would it be 
> > > better to hoist this to the start of DiagnoseUnusedButSetParameters?
> > Isn't it essentially at the beginning of the function as-is? If the warning 
> > is disabled for all parameters, it'll return false right away for all of 
> > them. However, I will add an extra check to end as soon as possible once no 
> > candidates are found. Let me know if it isn't ideal.
> I didn't modify this. I'm not sure there would be any advantage to checking 
> if warnings are disabled for every parameter before just doing all the checks 
> for all of them. Please push back if you think otherwise. 
Ah, I was misreading a bit; didn't realize that we were using the `SourceLoc` 
of each individual parameter to feed into `getDiagnosticLevel`. Yeah, this is 
fine as-is.



Comment at: clang/lib/Sema/SemaDecl.cpp:13752
+  // This is not an assignment to one of our NamedDecls.
+  TraverseStmt(LHS);
+}

since we try to exit early, should this be

```
if (!TraverseStmt(LHS))
  return false;
```

(and similar below)?



Comment at: clang/lib/Sema/SemaDecl.cpp:13760
+// If we remove all Decls, no need to keep searching.
+return (!S.erase(DRE->getFoundDecl()) || S.size());
+  }

nit: LLVM doesn't like superfluous parens; please write this as `return 
!S.erase(DRE->getFoundDecl()) || S.size();`



Comment at: clang/lib/Sema/SemaDecl.cpp:13825
+  Decl *D = Decl::castFromDeclContext((*Parameters.begin())->getDeclContext());
+  if (D)
+DiagnoseUnusedButSetDecls(this, D, Candidates,

nit: LLVM generally tries to write

```
auto *Foo = bar();
if (Foo)
  use(Foo);
```

as

```
if (auto *Foo = bar())
  use(Foo)
```

in part because the latter makes a bit better use of scopes



Comment at: clang/test/Sema/vector-gcc-compat.c:1
 // RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple 
x86_64-apple-darwin10
 

nit: is it possible to add -Wno-unused-but-set-parameter & 
-Wno-unused-but-set-variable as a part of the RUN line?

if it becomes too long, you can use \ to wrap it:

```
// RUN: foo bar \
// RUN: baz
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

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

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

friendly ping :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D3

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

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! I think this warning looks valuable.

Most of my comments are various forms of style nits. :)




Comment at: clang/lib/Sema/SemaDecl.cpp:13738
 
+// values in Map should be true. traverses Body; if any key is used in any way
+// other than assigning to it, sets the corresponding value to false.

nit: Please use `///` for documenting functions and methods

secondary nit: it looks like the prevailing comment style here is `// 
Capitalized sentences ending with a period.` Please try to keep with that style 
(here and below)



Comment at: clang/lib/Sema/SemaDecl.cpp:13740
+// other than assigning to it, sets the corresponding value to false.
+static void AreAllUsesSets(Stmt *Body,
+   llvm::SmallDenseMap *Map) {

nit: Should this be a `const Stmt*`? I don't think we should be mutating the 
`Body`



Comment at: clang/lib/Sema/SemaDecl.cpp:13744
+  struct AllUsesAreSetsVisitor : RecursiveASTVisitor {
+llvm::SmallDenseMap *M;
+unsigned FalseCount = 0;

nit: For visitors that're meant primarily to update an external data structure 
and be discarded, I think the preferred style is that they take and hold 
references rather than pointers

Also, now that I'm looking at this again, could `M` be a `SmallPtrSet`? The idea would be "this is the set of things we've not seen outside of an 
assignment so far." 



Comment at: clang/lib/Sema/SemaDecl.cpp:13751
+bool TraverseBinaryOperator(BinaryOperator *BO) {
+  auto *LHS = BO->getLHS();
+  auto *DRE = dyn_cast(LHS);

nit: `const auto *` is preferred when possible



Comment at: clang/lib/Sema/SemaDecl.cpp:13763
+  auto iter = M->find(DRE->getFoundDecl());
+  if (iter != M->end() && iter->second) {
+// we've found a use of this Decl that's not the LHS of an assignment

nit: LLVM prefers early exits over reuniting control flow, so this should 
probably be:

```
if (iter == M->end() || !iter->second) {
  return true;
}

iter->second = false;
++FalseCount;
return FalseCount != M->size();
```



Comment at: clang/lib/Sema/SemaDecl.cpp:13812
+
+  auto IsCandidate = [CPlusPlus,  = Diags](ParmVarDecl *P) {
+// check for Ignored here, because if we have no candidates we can avoid

nit: For lambdas which don't escape, I think the style preference is simply 
`[&]`



Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+ P->getLocation()) ==
+DiagnosticsEngine::Ignored)
+  return false;

If we want to optimize for when this warning is disabled, would it be better to 
hoist this to the start of DiagnoseUnusedButSetParameters?



Comment at: clang/lib/Sema/SemaDecl.cpp:13850-13853
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_variable,
+ VD->getLocation()) ==

Same "why not put this at the beginning of `DiagnoseUnusedButSetVariables`?" 
comment



Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;

It seems like these two warnings are doing analyses with identical requirements 
(e.g., the function's body must be present), but are taking two different sets 
of variables into account while doing so.

Is there a way we can rephrase this so we only need to walk the AST checking 
once for all of this?



Comment at: clang/test/Sema/warn-unused-but-set-variables-cpp.cpp:14
+
+  // no warning for structs in C++
+  struct S s;

Can you please expand on why not?

I can see a case for structs with nontrivial dtors or custom `operator=` 
methods. That said I don't understand why we'd avoid this for trivial structs 
like `S`.

(edit: now that I'm seeing this is for GCC compatibility, please call that out 
as a part of this comment. Maybe we want to do better than GCC? That can easily 
be done in a follow-up patch/commit though, so no strong opinion from me)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100581

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

[PATCH] D99993: [clang-tidy] bugprone-argument-comment: ignore name mismatches for decls from system headers

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for the comment!

> Why `--header-filter` command line option or `HeaderFilterRegex` 
> configuration file option could not solve this problem?

AFAICT, `--header-filter` & `HeaderFilterRegex` exist to filter diagnostics 
that occur in matching headers. These diagnostics are influenced by the 
contents of system headers, but are ultimately emitted inside of source files 
that have interesting lints, e.g.,

  [ub] /l [ 19ms ] ~> cat test.cpp
  #include 
  void foo(void *a, const void *b, size_t n) {
memcpy(/*blah=*/a, b, n);
  }
  [ub] /l [ 2ms ] ~> clang-tidy --checks='bugprone-argument-comment' test.cpp 
--header-filter='dont-match-anything' --
  1 warning generated.
  /l/test.cpp:3:10: warning: argument name 'blah' in comment does not match 
parameter name '__dest' [bugprone-argument-comment]
memcpy(/*blah=*/a, b, n);
   ^
  /usr/include/string.h:43:39: note: '__dest' declared here
  extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
^
  [ub] /l [ 25ms ] ~> clang-tidy --checks='bugprone-argument-comment' test.cpp 
--config 'HeaderFilterRegex: "dont-match-anything"' --
  1 warning generated.
  /l/test.cpp:3:10: warning: argument name 'blah' in comment does not match 
parameter name '__dest' [bugprone-argument-comment]
memcpy(/*blah=*/a, b, n);
   ^
  /usr/include/string.h:43:39: note: '__dest' declared here
  extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
^
  [ub] /l [ 22ms ] ~> 

So I don't think those help here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D3

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

[PATCH] D99993: bugprone-argument-comment: ignore name mismatches for decls from system headers

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: hokein, gribozavr2.
george.burgess.iv added a project: clang.
Herald added a subscriber: jfb.
george.burgess.iv requested review of this revision.
Herald added a project: clang-tools-extra.

As of 2a3498e24f97d 
, we 
ignore parameter name mismatches for functions in the `std::` namespace, since 
those aren't standardized. It seems reasonable to extend this to all functions 
which are declared in system headers, since this lint can be a bit noisy 
otherwise (https://bugs.chromium.org/p/chromium/issues/detail?id=1191507).

I'm on the fence about whether this behavior should be governed by an option. I 
tended toward no in the current patch, since `std::` isn't, but I'm happy to 
add a `CheckSystemHeaderDecls` flag or similar if that seems better.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D3

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
  clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
@@ -1,4 +1,4 @@
-// RUN: %check_clang_tidy %s bugprone-argument-comment %t
+// RUN: %check_clang_tidy %s bugprone-argument-comment %t -- -- -I 
%S/Inputs/bugprone-argument-comment
 
 // FIXME: clang-tidy should provide a -verify mode to make writing these checks
 // easier and more accurate.
@@ -134,3 +134,20 @@
   std::swap(a, /*num=*/b);
 }
 } // namespace ignore_std_functions
+
+namespace regular_header {
+#include "header-with-decl.h"
+void test() {
+  my_header_function(/*not_arg=*/1);
+// CHECK-NOTES: [[@LINE-1]]:22: warning: argument name 'not_arg' in comment 
does not match parameter name 'arg'
+// CHECK-NOTES: header-with-decl.h:1:29: note: 'arg' declared here
+// CHECK-FIXES: my_header_function(/*not_arg=*/1);
+}
+} // namespace regular_header
+
+namespace system_header {
+#include "system-header-with-decl.h"
+void test() {
+  my_system_header_function(/*not_arg=*/1);
+}
+} // namespace system_header
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/system-header-with-decl.h
@@ -0,0 +1,3 @@
+#pragma clang system_header
+
+void my_system_header_function(int arg);
Index: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
===
--- /dev/null
+++ 
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-argument-comment/header-with-decl.h
@@ -0,0 +1 @@
+void my_header_function(int arg);
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -20,10 +20,12 @@
 namespace tidy {
 namespace bugprone {
 namespace {
-AST_MATCHER(Decl, isFromStdNamespace) {
+AST_MATCHER(Decl, isFromStdNamespaceOrSystemHeader) {
   if (const auto *D = Node.getDeclContext()->getEnclosingNamespaceContext())
-return D->isStdNamespace();
-  return false;
+if (D->isStdNamespace())
+  return true;
+  return Node.getASTContext().getSourceManager().isInSystemHeader(
+  Node.getLocation());
 }
 } // namespace
 
@@ -66,13 +68,13 @@
// not specified by the standard, and standard library
// implementations in practice have to use reserved names to
// avoid conflicts with same-named macros.
-   unless(hasDeclaration(isFromStdNamespace(
-  .bind("expr"),
-  this);
-  Finder->addMatcher(
-  cxxConstructExpr(unless(hasDeclaration(isFromStdNamespace(
+   unless(hasDeclaration(isFromStdNamespaceOrSystemHeader(
   .bind("expr"),
   this);
+  Finder->addMatcher(cxxConstructExpr(unless(hasDeclaration(
+  isFromStdNamespaceOrSystemHeader(
+ .bind("expr"),
+ this);
 }
 
 static std::vector>


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-argument-comment.cpp
===
--- 

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

reverted in 
https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea
 until we can figure out how to address the issues outlined above. thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG275f30df8ad6: [clang] Change builtin object size when 
subobject is invalid (authored by jtmott-intel, committed by george.burgess.iv).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/CodeGen/object-size.c


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN([1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN([1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN([9].snd[0], 1);
 
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11408,9 +11408,9 @@
   return false;
   }
 
-  // If we point to before the start of the object, there are no accessible
-  // bytes.
-  if (LVal.getLValueOffset().isNegative()) {
+  // If we point outside of the object, there are no accessible bytes.
+  if (LVal.getLValueOffset().isNegative() ||
+  ((Type & 1) && !LVal.Designator.isValidSubobject())) {
 Size = 0;
 return true;
   }


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN([1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN([1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN([1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

thanks for working on this!

just one tiny nit and lgtm




Comment at: clang/lib/AST/ExprConstant.cpp:11408
 
   // If we point to before the start of the object, there are no accessible
   // bytes.

nit: the new part of this condition makes this comment somewhat outdated. 
should it say something like "outside of" instead of "before"?


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

https://reviews.llvm.org/D92892

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

[PATCH] D91372: Some updates/fixes to the creduce script.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

thanks!




Comment at: clang/utils/creduce-clang-crash.py:156
+  def skip_function(func_name):
+for name in filters:
+  if name in func_name:

nit `return any(name in func_name for name in filters)`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91372

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

[PATCH] D90269: adds -Wfree-nonheap-object member var checks

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGba18bc4925d8: [Sema] adds -Wfree-nonheap-object member var 
checks (authored by cjdb, committed by george.burgess.iv).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90269

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-free-nonheap-object.c
  clang/test/Sema/warn-free-nonheap-object.cpp

Index: clang/test/Sema/warn-free-nonheap-object.cpp
===
--- clang/test/Sema/warn-free-nonheap-object.cpp
+++ clang/test/Sema/warn-free-nonheap-object.cpp
@@ -13,10 +13,25 @@
 struct S {
   operator char *() { return ptr; }
 
+  void CFree() {
+::free(); // expected-warning {{attempt to call free on non-heap object 'ptr'}}
+::free();   // expected-warning {{attempt to call free on non-heap object 'I'}}
+::free(ptr);
+  }
+
+  void CXXFree() {
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 'ptr'}}
+std::free();   // expected-warning {{attempt to call std::free on non-heap object 'I'}}
+std::free(ptr);
+  }
+
 private:
   char *ptr = (char *)std::malloc(10);
+  static int I;
 };
 
+int S::I = 0;
+
 void test1() {
   {
 free(); // expected-warning {{attempt to call free on non-heap object 'GI'}}
@@ -51,6 +66,10 @@
 free(s);
 free(); // expected-warning {{attempt to call free on non-heap object 's'}}
   }
+  {
+S s;
+s.CFree();
+  }
 }
 
 void test2() {
@@ -87,4 +106,8 @@
 std::free(s);
 std::free(); // expected-warning {{attempt to call std::free on non-heap object 's'}}
   }
+  {
+S s;
+s.CXXFree();
+  }
 }
Index: clang/test/Sema/warn-free-nonheap-object.c
===
--- clang/test/Sema/warn-free-nonheap-object.c
+++ clang/test/Sema/warn-free-nonheap-object.c
@@ -4,6 +4,11 @@
 void *malloc(size_t);
 void free(void *);
 
+struct S {
+  int I;
+  char *P;
+};
+
 int GI;
 void test() {
   {
@@ -31,4 +36,9 @@
 free(A);  // expected-warning {{attempt to call free on non-heap object 'A'}}
 free(); // expected-warning {{attempt to call free on non-heap object 'A'}}
   }
+  {
+struct S s;
+free(); // expected-warning {{attempt to call free on non-heap object 'I'}}
+free(s.P);
+  }
 }
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -10107,19 +10107,9 @@
 }
 
 namespace {
-void CheckFreeArgumentsAddressof(Sema , const std::string ,
- const UnaryOperator *UnaryExpr) {
-  if (UnaryExpr->getOpcode() != UnaryOperator::Opcode::UO_AddrOf)
-return;
-
-  const auto *Lvalue = dyn_cast(UnaryExpr->getSubExpr());
-  if (Lvalue == nullptr)
-return;
-
-  const auto *Var = dyn_cast(Lvalue->getDecl());
-  if (Var == nullptr)
-return;
-
+void CheckFreeArgumentsOnLvalue(Sema , const std::string ,
+const UnaryOperator *UnaryExpr,
+const VarDecl *Var) {
   StorageClass Class = Var->getStorageClass();
   if (Class == StorageClass::SC_Extern ||
   Class == StorageClass::SC_PrivateExtern ||
@@ -10130,6 +10120,27 @@
   << CalleeName << Var;
 }
 
+void CheckFreeArgumentsOnLvalue(Sema , const std::string ,
+const UnaryOperator *UnaryExpr, const Decl *D) {
+  if (const auto *Field = dyn_cast(D))
+S.Diag(UnaryExpr->getBeginLoc(), diag::warn_free_nonheap_object)
+<< CalleeName << Field;
+}
+
+void CheckFreeArgumentsAddressof(Sema , const std::string ,
+ const UnaryOperator *UnaryExpr) {
+  if (UnaryExpr->getOpcode() != UnaryOperator::Opcode::UO_AddrOf)
+return;
+
+  if (const auto *Lvalue = dyn_cast(UnaryExpr->getSubExpr()))
+if (const auto *Var = dyn_cast(Lvalue->getDecl()))
+  return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr, Var);
+
+  if (const auto *Lvalue = dyn_cast(UnaryExpr->getSubExpr()))
+return CheckFreeArgumentsOnLvalue(S, CalleeName, UnaryExpr,
+  Lvalue->getMemberDecl());
+}
+
 void CheckFreeArgumentsStackArray(Sema , const std::string ,
   const DeclRefExpr *Lvalue) {
   if (!Lvalue->getType()->isArrayType())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D89988: adds basic -Wfree-nonheap-object functionality

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG425a83a5f069: [Sema] adds basic -Wfree-nonheap-object 
functionality (authored by cjdb, committed by george.burgess.iv).
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89988

Files:
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Decl.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-free-nonheap-object.c
  clang/test/Sema/warn-free-nonheap-object.cpp

Index: clang/test/Sema/warn-free-nonheap-object.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-free-nonheap-object.cpp
@@ -0,0 +1,90 @@
+// RUN: %clang_cc1 -Wfree-nonheap-object -std=c++11 -x c++ -fsyntax-only -verify %s
+
+extern "C" void free(void *) {}
+
+namespace std {
+using size_t = decltype(sizeof(0));
+void *malloc(size_t);
+void free(void *p);
+} // namespace std
+
+int GI;
+
+struct S {
+  operator char *() { return ptr; }
+
+private:
+  char *ptr = (char *)std::malloc(10);
+};
+
+void test1() {
+  {
+free(); // expected-warning {{attempt to call free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+free(); // expected-warning {{attempt to call free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+free(); // expected-warning {{attempt to call free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = 
+free(P);
+  }
+  {
+void *P = std::malloc(8);
+free(P); // FIXME diagnosing this would require control flow analysis.
+  }
+  {
+int A[] = {0, 1, 2, 3};
+free(A); // expected-warning {{attempt to call free on non-heap object 'A'}}
+  }
+  {
+int A[] = {0, 1, 2, 3};
+free(); // expected-warning {{attempt to call free on non-heap object 'A'}}
+  }
+  {
+S s;
+free(s);
+free(); // expected-warning {{attempt to call free on non-heap object 's'}}
+  }
+}
+
+void test2() {
+  {
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = 
+std::free(P); // FIXME diagnosing this would require control flow analysis.
+  }
+  {
+void *P = std::malloc(8);
+std::free(P);
+  }
+  {
+int A[] = {0, 1, 2, 3};
+std::free(A); // expected-warning {{attempt to call std::free on non-heap object 'A'}}
+  }
+  {
+int A[] = {0, 1, 2, 3};
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 'A'}}
+  }
+  {
+S s;
+std::free(s);
+std::free(); // expected-warning {{attempt to call std::free on non-heap object 's'}}
+  }
+}
Index: clang/test/Sema/warn-free-nonheap-object.c
===
--- /dev/null
+++ clang/test/Sema/warn-free-nonheap-object.c
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -Wfree-nonheap-object -fsyntax-only -verify %s
+
+typedef __SIZE_TYPE__ size_t;
+void *malloc(size_t);
+void free(void *);
+
+int GI;
+void test() {
+  {
+free(); // expected-warning {{attempt to call free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+free(); // expected-warning {{attempt to call free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+free(); // expected-warning {{attempt to call free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = 
+free(P); // FIXME diagnosing this would require control flow analysis.
+  }
+  {
+void *P = malloc(8);
+free(P);
+  }
+  {
+int A[] = {0, 1, 2, 3};
+free(A);  // expected-warning {{attempt to call free on non-heap object 'A'}}
+free(); // expected-warning {{attempt to call free on non-heap object 'A'}}
+  }
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -4496,16 +4496,24 @@
 DiagnoseCStringFormatDirectiveInCFAPI(*this, FDecl, Args, NumArgs);
 
   unsigned CMId = FDecl->getMemoryFunctionKind();
-  if (CMId == 0)
-return false;
 
   // Handle memory setting and copying functions.
-  if (CMId == Builtin::BIstrlcpy || CMId == Builtin::BIstrlcat)
+  switch (CMId) {
+  case 0:
+return false;
+  case Builtin::BIstrlcpy: // fallthrough
+  case Builtin::BIstrlcat:
 CheckStrlcpycatArguments(TheCall, FnInfo);
-  else if (CMId == Builtin::BIstrncat)
+break;
+  case Builtin::BIstrncat:
 CheckStrncatArguments(TheCall, FnInfo);
-  else
+break;
+  case Builtin::BIfree:
+CheckFreeArguments(TheCall);
+break;
+  default:
 

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d40fb808fd0: Allow to specify macro names for 
android-comparison-in-temp-failure-retry (authored by fmayer, committed by 
george.burgess.iv).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83144

Files:
  clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
  clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
  
clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
  
clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c

Index: clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry-custom-macro.c
@@ -0,0 +1,46 @@
+// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t -- -config="{CheckOptions: [{key: android-comparison-in-temp-failure-retry.RetryMacros, value: 'MY_TEMP_FAILURE_RETRY,MY_OTHER_TEMP_FAILURE_RETRY'}]}"
+
+#define MY_TEMP_FAILURE_RETRY(x) \
+  ({ \
+typeof(x) __z;   \
+do   \
+  __z = (x); \
+while (__z == -1);   \
+__z; \
+  })
+
+#define MY_OTHER_TEMP_FAILURE_RETRY(x) \
+  ({   \
+typeof(x) __z; \
+do \
+  __z = (x);   \
+while (__z == -1); \
+__z;   \
+  })
+
+int foo();
+int bar(int a);
+
+void with_custom_macro() {
+  MY_TEMP_FAILURE_RETRY(foo());
+  MY_TEMP_FAILURE_RETRY(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+  MY_TEMP_FAILURE_RETRY((foo()));
+  MY_TEMP_FAILURE_RETRY((int)(foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+  MY_TEMP_FAILURE_RETRY((bar(foo() == 1)));
+  MY_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:49: warning: top-level comparison in MY_TEMP_FAILURE_RETRY
+}
+
+void with_other_custom_macro() {
+  MY_OTHER_TEMP_FAILURE_RETRY(foo());
+  MY_OTHER_TEMP_FAILURE_RETRY(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+  MY_OTHER_TEMP_FAILURE_RETRY((foo()));
+  MY_OTHER_TEMP_FAILURE_RETRY((int)(foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+  MY_OTHER_TEMP_FAILURE_RETRY((bar(foo() == 1)));
+  MY_OTHER_TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:55: warning: top-level comparison in MY_OTHER_TEMP_FAILURE_RETRY
+}
Index: clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
+++ clang-tools-extra/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
@@ -34,3 +34,10 @@
   while (TEMP_FAILURE_RETRY(read(STDIN_FILENO, cs, sizeof(cs))) != 0) {
 // Do something with cs.
   }
+
+Options
+---
+
+.. option:: RetryMacros
+
+   A comma-separated list of the names of retry macros to be checked.
Index: clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
===
--- clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
+++ clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
@@ -10,6 +10,9 @@
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_COMPARISONINTEMPFAILURERETRYCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
+#include 
 
 namespace clang {
 namespace tidy {
@@ -22,10 +25,14 @@
 /// TEMP_FAILURE_RETRY is a macro provided by both glibc and Bionic.
 class ComparisonInTempFailureRetryCheck : public ClangTidyCheck {
 public:
-  ComparisonInTempFailureRetryCheck(StringRef Name, ClangTidyContext *Context)
-  : ClangTidyCheck(Name, Context) {}
+  ComparisonInTempFailureRetryCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  const std::string RawRetryList;
+  SmallVector RetryMacros;
 };
 
 } // namespace android
Index: clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a reviewer: alexfh.
george.burgess.iv added a comment.

Concept and implementation LGTM. Please wait for comment from +alexfh before 
landing, since I think they have more ownership over clang-tidy in general than 
I do :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83144



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

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for the report! Looking now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78162



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

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2dd17ff08165: [CodeGen] only add nobuiltin to inline 
builtins if well emit them (authored by george.burgess.iv).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78162

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c


Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | 
FileCheck %s
+//
+// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
+// if the builtin isn't emittable.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit this body. Otherwise, we need >=
+// -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: define void @foo
+void foo(void *a, const void *b, size_t c) {
+  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
+  // later on if optimizations are enabled.
+  // CHECK: call i8* @memcpy
+  memcpy(a, b, c);
+}
+
+// CHECK-NOT: nobuiltin
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1908,7 +1908,8 @@
   else if (const auto *SA = FD->getAttr())
  F->setSection(SA->getName());
 
-  if (FD->isInlineBuiltinDeclaration()) {
+  // If we plan on emitting this inline builtin, we can't treat it as a 
builtin.
+  if (FD->isInlineBuiltinDeclaration() && shouldEmitFunction(FD)) {
 F->addAttribute(llvm::AttributeList::FunctionIndex,
 llvm::Attribute::NoBuiltin);
   }


Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
+// if the builtin isn't emittable.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit this body. Otherwise, we need >=
+// -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: define void @foo
+void foo(void *a, const void *b, size_t c) {
+  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
+  // later on if optimizations are enabled.
+  // CHECK: call i8* @memcpy
+  memcpy(a, b, c);
+}
+
+// CHECK-NOT: nobuiltin
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1908,7 +1908,8 @@
   else if (const auto *SA = FD->getAttr())
  F->setSection(SA->getName());
 
-  if (FD->isInlineBuiltinDeclaration()) {
+  // If we plan on emitting this inline builtin, we can't treat it as a builtin.
+  if (FD->isInlineBuiltinDeclaration() && shouldEmitFunction(FD)) {
 F->addAttribute(llvm::AttributeList::FunctionIndex,
 llvm::Attribute::NoBuiltin);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78162



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

[PATCH] D78162: [CodeGen] Mark inline definitions of builtins as nobuiltin only if we plan to emit them.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: efriedma, serge-sans-paille.
george.burgess.iv added a project: clang.
Herald added a subscriber: cfe-commits.

This suboptimality was encountered in Linux (see some discussion on D71082 
, and 
https://github.com/ClangBuiltLinux/linux/issues/979).

While it'd be great to have the kernel's FORTIFY working well with clang, it 
seems best to preserve old functionality in cases where D71082 
 can't emit a FORTIFY'ed wrapper of a builtin. 
With this patch, the `memcpy` in the test-case here 
https://reviews.llvm.org/D71082#1953975 compiles back to a `mov`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78162

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c


Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | 
FileCheck %s
+//
+// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
+// if the builtin isn't emittable.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit this body. Otherwise, we need >=
+// -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: define void @foo
+void foo(void *a, const void *b, size_t c) {
+  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
+  // later on if optimizations are enabled.
+  // CHECK: call i8* @memcpy
+  memcpy(a, b, c);
+}
+
+// CHECK-NOT: nobuiltin
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1908,7 +1908,8 @@
   else if (const auto *SA = FD->getAttr())
  F->setSection(SA->getName());
 
-  if (FD->isInlineBuiltinDeclaration()) {
+  // If we plan on emitting this inline builtin, we can't treat it as a 
builtin.
+  if (FD->isInlineBuiltinDeclaration() && shouldEmitFunction(FD)) {
 F->addAttribute(llvm::AttributeList::FunctionIndex,
 llvm::Attribute::NoBuiltin);
   }


Index: clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-nobuiltin-if-not-emitted.c
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang doesn't mark an inline builtin definition as `nobuiltin`
+// if the builtin isn't emittable.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit this body. Otherwise, we need >=
+// -O1.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+AVAILABLE_EXTERNALLY void *memcpy(void *a, const void *b, size_t c) {
+  return __builtin_memcpy(a, b, c);
+}
+
+// CHECK-LABEL: define void @foo
+void foo(void *a, const void *b, size_t c) {
+  // Clang will always _emit_ this as memcpy. LLVM turns it into @llvm.memcpy
+  // later on if optimizations are enabled.
+  // CHECK: call i8* @memcpy
+  memcpy(a, b, c);
+}
+
+// CHECK-NOT: nobuiltin
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -1908,7 +1908,8 @@
   else if (const auto *SA = FD->getAttr())
  F->setSection(SA->getName());
 
-  if (FD->isInlineBuiltinDeclaration()) {
+  // If we plan on emitting this inline builtin, we can't treat it as a builtin.
+  if (FD->isInlineBuiltinDeclaration() && shouldEmitFunction(FD)) {
 F->addAttribute(llvm::AttributeList::FunctionIndex,
 llvm::Attribute::NoBuiltin);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D78148: [CodeGen] make isTriviallyRecursive handle more trivial recursion

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv abandoned this revision.
george.burgess.iv added a comment.

Nothing in the real world :)

I was writing test-cases for a soon-to-be-in-your-inbox patch, and initially 
wrote `memcpy` recursion in terms of `memcpy`, rather than `__builtin_memcpy`. 
Wasn't sure if this was an oversight, or intended. The comments around the use 
of this through `isTriviallyRecursive` said "a[n externally-available] function 
that calls itself is clearly not equivalent to the real implementation." Since 
this behavior is intentional, I'll see if I can make that commentary a bit more 
clear instead.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78148



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

[PATCH] D78148: [CodeGen] make isTriviallyRecursive handle more trivial recursion

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: efriedma, serge-sans-paille.
george.burgess.iv added a project: clang.
Herald added a subscriber: cfe-commits.

This function was not catching all forms of trivial recursion, meaning:

  void *memcpy(void *a, const void *b, size_t n) {
return __builtin_memcpy(a, b, n);
  }

would be considered trivially recursive, whereas

  void *memcpy(void *a, const void *b, size_t n) {
return memcpy(a, b, n);
  }

would not.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78148

Files:
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c


Index: clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | 
FileCheck %s
+//
+// Verifies that clang doesn't emit trivially-recursive extern inline 
functions.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit any available_externally function 
in
+// -O0.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+void some_other_fn(void);
+
+AVAILABLE_EXTERNALLY void *some_fn() {
+  some_other_fn();
+  return some_fn();
+}
+
+// CHECK-LABEL: define void @foo
+void foo() {
+  // CHECK-NOT: call void @some_other_fn
+  some_fn();
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2651,15 +2651,19 @@
 namespace {
   struct FunctionIsDirectlyRecursive
   : public ConstStmtVisitor {
+const FunctionDecl *OriginalFD;
 const StringRef Name;
 const Builtin::Context 
-FunctionIsDirectlyRecursive(StringRef N, const Builtin::Context )
-: Name(N), BI(C) {}
+FunctionIsDirectlyRecursive(const FunctionDecl *OriginalFD, StringRef N,
+const Builtin::Context )
+: OriginalFD(OriginalFD), Name(N), BI(C) {}
 
 bool VisitCallExpr(const CallExpr *E) {
   const FunctionDecl *FD = E->getDirectCallee();
   if (!FD)
 return false;
+  if (FD == OriginalFD)
+return true;
   AsmLabelAttr *Attr = FD->getAttr();
   if (Attr && Name == Attr->getLabel())
 return true;
@@ -2762,7 +2766,7 @@
 Name = FD->getName();
   }
 
-  FunctionIsDirectlyRecursive Walker(Name, Context.BuiltinInfo);
+  FunctionIsDirectlyRecursive Walker(FD, Name, Context.BuiltinInfo);
   const Stmt *Body = FD->getBody();
   return Body ? Walker.Visit(Body) : false;
 }


Index: clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c
===
--- /dev/null
+++ clang/test/CodeGen/memcpy-no-emission-trivial-recursion.c
@@ -0,0 +1,23 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
+//
+// Verifies that clang doesn't emit trivially-recursive extern inline functions.
+
+typedef unsigned long size_t;
+
+// always_inline is used so clang will emit any available_externally function in
+// -O0.
+#define AVAILABLE_EXTERNALLY extern inline __attribute__((always_inline)) \
+__attribute__((gnu_inline))
+
+void some_other_fn(void);
+
+AVAILABLE_EXTERNALLY void *some_fn() {
+  some_other_fn();
+  return some_fn();
+}
+
+// CHECK-LABEL: define void @foo
+void foo() {
+  // CHECK-NOT: call void @some_other_fn
+  some_fn();
+}
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -2651,15 +2651,19 @@
 namespace {
   struct FunctionIsDirectlyRecursive
   : public ConstStmtVisitor {
+const FunctionDecl *OriginalFD;
 const StringRef Name;
 const Builtin::Context 
-FunctionIsDirectlyRecursive(StringRef N, const Builtin::Context )
-: Name(N), BI(C) {}
+FunctionIsDirectlyRecursive(const FunctionDecl *OriginalFD, StringRef N,
+const Builtin::Context )
+: OriginalFD(OriginalFD), Name(N), BI(C) {}
 
 bool VisitCallExpr(const CallExpr *E) {
   const FunctionDecl *FD = E->getDirectCallee();
   if (!FD)
 return false;
+  if (FD == OriginalFD)
+return true;
   AsmLabelAttr *Attr = FD->getAttr();
   if (Attr && Name == Attr->getLabel())
 return true;
@@ -2762,7 +2766,7 @@
 Name = FD->getName();
   }
 
-  FunctionIsDirectlyRecursive Walker(Name, Context.BuiltinInfo);
+  FunctionIsDirectlyRecursive Walker(FD, Name, Context.BuiltinInfo);
   const Stmt *Body = FD->getBody();
   return Body ? Walker.Visit(Body) : false;
 }
___
cfe-commits mailing 

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

For a more direct comparison, I offer https://godbolt.org/z/fqAhUC . The lack 
of optimization in the later case is because we're forced to mark the call to 
`__builtin_memcpy` in the inline memcpy as `nobuiltin`. If we instead rename 
things, this issue doesn't happen: https://godbolt.org/z/FKNTWo.

All other FORTIFY impls I know of defer to `_chk` functions for checking that's 
not guarded by `__builtin_constant_p`, rather than having size checks inline 
like the kernel. Both GCC and Clang have special knowledge of these intrinsics, 
so they can optimize them well: https://godbolt.org/z/L7rVHp . It'd be nice if 
the kernel's FORTIFY were more like all of the other existing ones in this way.

Deferring to `_chk` builtins has the side-benefit that the `inline` `memcpy` is 
often smaller, which increases the inlineability of any functions that `memcpy` 
gets inlined into(*). The down-side is that the kernel now needs to carry 
definitions for `__memcpy_chk` et al.

(*) -- not in an underhanded way. It's just that any condition that depends on 
`__builtin_constant_p` or `__builtin_object_size(obj)` is guaranteed to be 
folded away at compile-time; not representing them in IR is more "honest" to 
anything that's trying to determine the inlinability of a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D75492: [clang-tidy]: modernize-use-using: don't diagnose typedefs in `extern "C"` DeclContexts

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv planned changes to this revision.
george.burgess.iv added a comment.

I see -- that seems like a much broader change, but also probably worthwhile. I 
have a few ideas about how to tackle that; let me see what I can come up with 
locally.

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75492



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

[PATCH] D75492: [modernize-use-using] Don't diagnose typedefs in `extern "C"` DeclContexts

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added a reviewer: aaron.ballman.

If code is shared between C and C++, converting a `typedef` to a `using` isn't 
possible. Being more conservative about emitting these lints in `extern "C"` 
blocks seems like a good compromise to me.


https://reviews.llvm.org/D75492

Files:
  clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -278,3 +278,7 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, 
eb2 };
 
+extern "C" {
+typedef int Type;
+// No messages expected, since this code may be shared verbatim with C.
+}
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -48,6 +48,9 @@
   if (StartLoc.isMacroID() && IgnoreMacros)
 return;
 
+  if (MatchedDecl->getDeclContext()->isExternCContext())
+return;
+
   static const char *UseUsingWarning = "use 'using' instead of 'typedef'";
 
   // Warn at StartLoc but do not fix if there is macro or array.


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-use-using.cpp
@@ -278,3 +278,7 @@
 // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: use 'using' instead of 'typedef'
 // CHECK-FIXES: using EnumT2_CheckTypedefImpactFromAnotherFile = enum { ea2, eb2 };
 
+extern "C" {
+typedef int Type;
+// No messages expected, since this code may be shared verbatim with C.
+}
Index: clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
===
--- clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/UseUsingCheck.cpp
@@ -48,6 +48,9 @@
   if (StartLoc.isMacroID() && IgnoreMacros)
 return;
 
+  if (MatchedDecl->getDeclContext()->isExternCContext())
+return;
+
   static const char *UseUsingWarning = "use 'using' instead of 'typedef'";
 
   // Warn at StartLoc but do not fix if there is macro or array.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Until recently, both CrOS and Android disabled FORTIFY wholesale when any of 
asan/msan/tsan were active. Bionic recently got support for FORTIFY playing 
nicely with sanitizers, but that support boils down to "turn off all the 
FORTIFY runtime checks, but leave the ones that generate compiler diags on."

A quick fix here would likely be to disable this builtin interception 
functionality if a sanitizer that isn't ubsan/CFI is enabled. Since all of this 
code + the nop `__warn` builtin we added is specifically targeted at supporting 
glibc's FORTIFY patterns, I don't think adding a `!sanitizers` check would be 
too ugly on its own?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

(It's interesting to me if gcc doesn't warn about that libcxx code, since the 
whole point of the gnuc 5.0 check there was "the compiler should check this for 
us now"...)

If a glibc-side fix for this does materialize, IMO `-fgnuc-version=5.0` isn't a 
good path forward.

It's best if workarounds are limited in scope, lifespan, and user impact:

- Presumably `-fgnuc-version=5.0` implies many more changes that are unrelated 
to this particular bug.
- Glibc upgrades can take years to materialize for users[1].
- A lot of packages build with `-D_FORTIFY_SOURCE=2` by default. Requiring 
extra CPPFLAGS (either `-D_FORTIFY_SOURCE=0` or `-fgnuc-version=5.0`) would 
break `CC=clang make`-style builds that already work today, if any statement in 
them folds to `memset(x, non_zero, 0);` after optimizations.

Looks like this landed on the branch, so unless I'm mistaken, we probably need 
to revert there, as well?

[1] -- Anecdotally, I upgraded a project's host toolchain from using glibc 2.15 
to 2.17 last year. I wanted to do 2.19, but 2.17 usage was still large enough 
that 2.19 wasn't an option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Just a few more nits, and this LGTM. Thanks again!

IDK if Eli wanted to have another look at this; please wait until tomorrow 
before landing to give him time to comment.




Comment at: clang/lib/AST/Decl.cpp:3006
 
+bool FunctionDecl::isInlineBuiltin() const {
+  if (!getBuiltinID())

nit: If we're going with this naming, I think it's important to highlight that 
we're querying if this is a definition. So `isInlineBuiltinDefinition()`?



Comment at: clang/test/CodeGen/memcpy-nobuiltin.c:5
+//
+// CHECK-WITH-DECL: 'memcpy' will always overflow; destination buffer has size 
1, but size argument is 2
+// CHECK-WITH-DECL-NOT: @llvm.memcpy

Generally, I think we try to keep diag checking and codegen checking separate, 
but this is simple enough that I don't think it's a big deal.

Please use clang's `-verify` + `expected-warning` instead of `CHECK`s here.

Looks like, among others, test/CodeGen/const-init.c combines `-verify` with 
FileCheck'ing codegen.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang/lib/AST/Decl.cpp:3006
 
+bool FunctionDecl::isReplaceableSystemFunction() const {
+  FunctionDecl const *Definition;

serge-sans-paille wrote:
> george.burgess.iv wrote:
> > serge-sans-paille wrote:
> > > Note to reviewers: I'm not super happy with that name.
> > Yeah, `isReplaceableBuiltin()` sounds like "can this be replaced at all?" 
> > rather than "is this acting as a replacement for something else?"
> > 
> > `isReplacementForBuiltin()` pops to mind, though in a lot of cases, these 
> > functions may end up calling the "actual" builtin (as you handle later in 
> > the patch), so maybe `isProxyForBuiltin()` is better?
> What about `isBuiltInWrapper()` ?
WFM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Should we also have a quick test-case in Sema/ verifying that we still get the 
warnings that Eli mentioned?




Comment at: clang/lib/AST/Decl.cpp:3006
 
+bool FunctionDecl::isReplaceableSystemFunction() const {
+  FunctionDecl const *Definition;

serge-sans-paille wrote:
> Note to reviewers: I'm not super happy with that name.
Yeah, `isReplaceableBuiltin()` sounds like "can this be replaced at all?" 
rather than "is this acting as a replacement for something else?"

`isReplacementForBuiltin()` pops to mind, though in a lot of cases, these 
functions may end up calling the "actual" builtin (as you handle later in the 
patch), so maybe `isProxyForBuiltin()` is better?



Comment at: clang/lib/AST/Decl.cpp:3010
+
+  FunctionDecl const *Definition;
+  if (hasBody(Definition)) {

`const FunctionDecl *`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for looking into this!

I dunno what implications this has elsewhere, but I'd like to note a few things 
about this general approach:

- AIUI, this'll only work for FORTIFY functions which are literally calling 
`__builtin___foo_chk`; an equivalent implementation without such a builtin, 
like below, should be blocked by other code in clang 
,
 since it results in infinite recursion . glibc 
implements quite a few (most?) FORTIFY'ed functions in this way.



  void *memcpy(void *a, void *b, size_t c) {
if (__builtin_object_size(a, 0) == -1)
  return memcpy(a, b, c);
return __memcpy_chk(a, b, c, __builtin_object_size(a, 0));
  }



- This change only gives us `-D_FORTIFY_SOURCE=1`-level checking here. If we 
want to offer better support for `-D_FORTIFY_SOURCE=2` (including potentially 
FORTIFY's compile-time diagnostics), which is AIUI what most of the world uses, 
we'll need to add clang-specific support into glibc's headers. At that point, 
this patch becomes a nop, since the FORTIFY'ed memcpy becomes an overload for 
the built-in one.

In other words, I think this patch will improve our support for the pieces of 
glibc's FORTIFY that're implemented with `__builtin___*_chk`, but I don't think 
there's much more room to incrementally improve beyond that before we need to 
start hacking on glibc directly. If we get to that point, the changes we need 
to make in glibc will likely obviate the need for this patch.

So if your overarching goal here is to make clang support glibc's FORTIFY as-is 
a bit better, that's great and I'm personally cool with this patch (again, not 
being deeply aware of what interactions this may have in a non-FORTIFY'ed 
context). If the intent is to build off of this to try and make glibc's FORTIFY 
story with clang a great one, I'm unsure how useful this patch will be in the 
long run.




Comment at: clang/lib/AST/Decl.cpp:3007
+bool FunctionDecl::isReplaceableSystemFunction() const {
+  FunctionDecl const *Definition;
+  if (hasBody(Definition)) {

style nit: `const FunctionDecl *` is the preferred ordering in most of LLVM



Comment at: clang/lib/AST/Decl.cpp:3179
+  // C library do that to implement fortified version.
+  if (isReplaceableSystemFunction()) {
+return 0;

style nit: please don't use braces here or below, for consistency with nearby 
code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> Does this have any significant impact on -fsyntax-only performance?

I'm sure there are pathological cases where this hurts perf, but my intuition 
tells me that we won't get bitten badly by any of them in the real world. It 
should be a branch per cast + full expr for people who don't use it. For those 
who do, we're walking 2 types for each cast (plus maybe constexpr evaluations 
for casts from FP/vec to non-FP/vec values), and walking the types for each 
FullExpr.

Again, you can likely craft code that makes this expensive (which we can likely 
fix with strategically-placed caches), but being bitten by that in practice 
seems somewhat unlikely to me. Happy to try and add caching and/or take numbers 
with caches if you'd like.

To evaluate the build time impact of this, I did 20 builds of aarch64 Linux 
with/without this patch. Of the metrics I tracked (memory, system/user/wall 
time), all reported differences were < their stdev except for user time. User 
time regression with this patch was 0.37%, with a stdev of 0.12%. Happy to try 
to gather -fsyntax-only numbers if there's a simple way to do that.




Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:194
 Features.push_back("-crypto");
 Features.push_back("-neon");
+// FIXME: Ideally, we'd also like to pass a `+general-regs-only` feature,

efriedma wrote:
> Do we need to get rid of the "-neon" etc.?
Yeah, good call. Looks like I forgot a "RUN:" in my tests checking for whether 
or not asm works, so the thing that should've caught that wasn't enabled. :)



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7997
+  bool isRValueOfIllegalType(const Expr *E) const {
+return E->getValueKind() != VK_LValue && !isGeneralType(E);
+  }

efriedma wrote:
> Should this be `E->isRValue()`?  Not that the difference really matters for 
> C, but it affects rvalue references in C++.
Yeah, flipped to that and added tests -- thanks


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

https://reviews.llvm.org/D38479



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 222525.
george.burgess.iv marked 6 inline comments as done.
george.burgess.iv added a comment.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

Addressed feedback


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

https://reviews.llvm.org/D38479

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGen/aarch64-mgeneral_regs_only.c
  clang/test/Driver/aarch64-mgeneral_regs_only.c
  clang/test/Sema/aarch64-mgeneral_regs_only.c
  clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
  llvm/lib/Target/AArch64/AArch64.td
  llvm/lib/Target/AArch64/AArch64Subtarget.h

Index: llvm/lib/Target/AArch64/AArch64Subtarget.h
===
--- llvm/lib/Target/AArch64/AArch64Subtarget.h
+++ llvm/lib/Target/AArch64/AArch64Subtarget.h
@@ -86,6 +86,9 @@
   bool HasFP16FML = false;
   bool HasSPE = false;
 
+  // FIXME: Backend support for 'enforcing' this would be nice.
+  bool IsGeneralRegsOnly = false;
+
   // ARMv8.1 extensions
   bool HasVH = false;
   bool HasPAN = false;
Index: llvm/lib/Target/AArch64/AArch64.td
===
--- llvm/lib/Target/AArch64/AArch64.td
+++ llvm/lib/Target/AArch64/AArch64.td
@@ -25,6 +25,10 @@
 def FeatureNEON : SubtargetFeature<"neon", "HasNEON", "true",
   "Enable Advanced SIMD instructions", [FeatureFPARMv8]>;
 
+def FeatureGeneralRegsOnly : SubtargetFeature<"general-regs-only",
+  "IsGeneralRegsOnly", "false",
+  "Restrict IR to only use non-fp/simd registers. Doesn't apply to asm.">;
+
 def FeatureSM4 : SubtargetFeature<
 "sm4", "HasSM4", "true",
 "Enable SM3 and SM4 support", [FeatureNEON]>;
Index: clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
@@ -0,0 +1,222 @@
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=float -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=int '-DVECATTR=__attribute__((ext_vector_type(2)))' -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=FloatTypedef -Wno-unused-value
+
+using FloatTypedef = float;
+
+#ifndef VECATTR
+#define VECATTR
+#define BannedToInt(x) (x)
+#else
+#define BannedToInt(x) ((x)[0])
+#endif
+
+typedef BANNED BannedTy VECATTR;
+
+namespace default_args {
+int foo(BannedTy = 0); // expected-error 2{{use of floating-point or vector values is disabled}}
+int bar(int = 1.0);
+
+void baz(int a = foo()); // expected-note{{from use of default argument here}}
+void bazz(int a = bar());
+
+void test() {
+  foo(); // expected-note{{from use of default argument here}}
+  bar();
+  baz(); // expected-note{{from use of default argument here}}
+  baz(4);
+  bazz();
+}
+} // namespace default_args
+
+namespace conversions {
+struct ConvertToFloat {
+  explicit operator BannedTy();
+};
+struct ConvertToFloatImplicit { /* implicit */
+  operator BannedTy();
+};
+struct ConvertFromFloat {
+  ConvertFromFloat(BannedTy);
+};
+
+void takeFloat(BannedTy);
+void takeConvertFromFloat(ConvertFromFloat);
+void takeConvertFromFloatRef(const ConvertFromFloat &);
+
+void test() {
+  BannedTy(ConvertToFloat());
+
+  takeFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat{0};// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat{ConvertToFloatImplicit{}}; // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(0); // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(ConvertFromFloat(0));// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+}
+
+void takeImplicitFloat(BannedTy = 

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Apologies for the latency of my updates.

> Something I ran into when reviewing https://reviews.llvm.org/D62639 is that 
> on AArch64, for varargs functions, we emit floating-point stores when 
> noimplicitfloat is specified. That seems fine for -mno-implicit-float, but 
> maybe not for -mgeneral-regs-only?

Interesting. Yeah, -mgeneral-regs-only definitely doesn't want to use FP for 
varargs calls. As discussed offline, this doesn't appear to be an issue today, 
and is something we can look into in the future.




Comment at: clang/lib/Sema/SemaExprCXX.cpp:7951
+  // they don't have to write out memcpy() for simple cases. For that reason,
+  // it's very limited in what it will detect.
+  //

george.burgess.iv wrote:
> efriedma wrote:
> > We don't always lower struct copies to memcpy(); I'm not sure this is safe.
> I see; removed. If this check ends up being important (it doesn't seem to be 
> in local builds), we can revisit. :)
(readded as noted)



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8003
+!isRValueOfIllegalType(E) &&
+E->isConstantInitializer(S.getASTContext(), /*ForRef=*/false)) {
+  resetDiagnosticState(InitialState);

efriedma wrote:
> george.burgess.iv wrote:
> > efriedma wrote:
> > > Just because we can constant-fold an expression, doesn't mean we will, 
> > > especially at -O0.
> > Are there any guarantees that we offer along these lines? The code in 
> > particular that this cares about boils down to a bunch of integer literals 
> > doing mixed math with FP literals, all of which gets casted to an `int`. 
> > Conceptually, it seems silly to me to emit an addition for something as 
> > straightforward as `int i = 1 + 2.0;`, even at -O0, though I totally agree 
> > that you're right, and codegen like this is reasonable at -O0: 
> > https://godbolt.org/z/NS0L17
> > 
> > (This also brings up a good point: this visitor probably shouldn't be run 
> > on `IsConstexpr` expressions; fixed that later on)
> On trunk, we now have the notion of a ConstantExpr; this represents an 
> expression which the language guarantees must be constant-evaluated.  For 
> example, initializers for static variables in C are always constant-evaluated.
> 
> (On a related note, now that we have ConstantExpr, the IsConstexpr operand to 
> ActOnFinishFullExpr probably isn't necessary.)
> 
> Beyond that, no, we don't really have any guarantees.  We may or may not try 
> to constant-evaluate an expression, depending on whether we think it'll save 
> compile-time.  For example, we try to fold branch conditions to avoid 
> emitting the guarded block of code, but we don't try to fold the 
> initialization of an arbitrary variable.
> 
> I don't think we want to introduce any additional guarantees here, if we can 
> avoid it.
Resolving per offline discussion; anything wrapped in `ConstantExpr` is no 
longer checked, and we no longer try to constant evaluate anything else.

> On a related note, now that we have ConstantExpr, the IsConstexpr operand to 
> ActOnFinishFullExpr probably isn't necessary

Looks like the lack of `IsConstexpr` fails to save us from one case:

```
constexpr float x = 1.;
```

In this context, all we have to work with from this point is an `IsConstexpr` 
`IntegerLiteral`


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

https://reviews.llvm.org/D38479



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 220416.
george.burgess.iv marked 4 inline comments as done.
george.burgess.iv added a reviewer: efriedma.
george.burgess.iv added a comment.

Chatted with Eli offline; updated here to reflect the conclusions of that.

Importantly, this patch readds some of the peepholes we try to not diagnose, 
since the target users of this quite commonly do things that, after macro 
expansion, fold into e.g., `(int)(3.0 + 1)`. By wrapping these into 
`ConstantExpr`s at the cast point, we get our nice guaranteed lowering to 0 
FP/vector ops in IR.

Similarly for struct assignment, I couldn't find a way to get an assignment of 
a struct of multiple fields to turn into a not-memcpy, so it seems safe to me 
to keep that around. I have tests to this effect, and am happy to add more if 
people can think of cases these tests may not adequately cover.


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

https://reviews.llvm.org/D38479

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/Expr.cpp
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaCast.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGen/aarch64-mgeneral_regs_only.c
  clang/test/Sema/aarch64-mgeneral_regs_only.c
  clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp

Index: clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
@@ -0,0 +1,214 @@
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=float -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=int '-DVECATTR=__attribute__((ext_vector_type(2)))' -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=FloatTypedef -Wno-unused-value
+
+using FloatTypedef = float;
+
+#ifndef VECATTR
+#define VECATTR
+#define BannedToInt(x) (x)
+#else
+#define BannedToInt(x) ((x)[0])
+#endif
+
+typedef BANNED BannedTy VECATTR;
+
+namespace default_args {
+int foo(BannedTy = 0); // expected-error 2{{use of floating-point or vector values is disabled}}
+int bar(int = 1.0);
+
+void baz(int a = foo()); // expected-note{{from use of default argument here}}
+void bazz(int a = bar());
+
+void test() {
+  foo(); // expected-note{{from use of default argument here}}
+  bar();
+  baz(); // expected-note{{from use of default argument here}}
+  baz(4);
+  bazz();
+}
+} // namespace default_args
+
+namespace conversions {
+struct ConvertToFloat {
+  explicit operator BannedTy();
+};
+struct ConvertToFloatImplicit { /* implicit */
+  operator BannedTy();
+};
+struct ConvertFromFloat {
+  ConvertFromFloat(BannedTy);
+};
+
+void takeFloat(BannedTy);
+void takeConvertFromFloat(ConvertFromFloat);
+void takeConvertFromFloatRef(const ConvertFromFloat &);
+
+void test() {
+  BannedTy(ConvertToFloat());
+
+  takeFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat{0};// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat{ConvertToFloatImplicit{}}; // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(0); // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(ConvertFromFloat(0));// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+}
+
+void takeImplicitFloat(BannedTy = ConvertToFloatImplicit()); // expected-error{{use of floating-point or vector values is disabled}}
+void test2() {
+  takeImplicitFloat(); // expected-note{{from use of default argument here}}
+}
+} // namespace conversions
+
+namespace refs {
+struct BannedRef {
+  const BannedTy 
+  BannedRef(const BannedTy ) : f(f) {}
+};
+
+BannedTy ();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy  = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy  = getBanned();
+  const BannedTy  = 

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367067: [Sema] add -Walloca to flag uses of `alloca` 
(authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64883?vs=211816=211837#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64883

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/Sema/warn-alloca.c


Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2779,6 +2779,11 @@
 def err_cannot_find_suitable_accessor : Error<
   "cannot find suitable %select{getter|setter}0 for property %1">;
 
+def warn_alloca : Warning<
+  "use of function %0 is discouraged; there is no way to check for failure but 
"
+  "failure may still occur, resulting in a possibly exploitable security 
vulnerability">,
+  InGroup>, DefaultIgnore;
+
 def warn_alloca_align_alignof : Warning<
   "second argument to __builtin_alloca_with_align is supposed to be in bits">,
   InGroup>;
Index: cfe/trunk/test/Sema/warn-alloca.c
===
--- cfe/trunk/test/Sema/warn-alloca.c
+++ cfe/trunk/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+  // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+  __builtin_alloca(a);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged; 
there is no way to check for failure but failure may still occur, resulting in 
a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+  __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is 
discouraged; there is no way to check for failure but failure may still occur, 
resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -1179,6 +1179,10 @@
   case Builtin::BI__builtin_alloca_with_align:
 if (SemaBuiltinAllocaWithAlign(TheCall))
   return ExprError();
+LLVM_FALLTHROUGH;
+  case Builtin::BI__builtin_alloca:
+Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+<< TheCall->getDirectCallee();
 break;
   case Builtin::BI__assume:
   case Builtin::BI__builtin_assume:


Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2779,6 +2779,11 @@
 def err_cannot_find_suitable_accessor : Error<
   "cannot find suitable %select{getter|setter}0 for property %1">;
 
+def warn_alloca : Warning<
+  "use of function %0 is discouraged; there is no way to check for failure but "
+  "failure may still occur, resulting in a possibly exploitable security vulnerability">,
+  InGroup>, DefaultIgnore;
+
 def warn_alloca_align_alignof : Warning<
   "second argument to __builtin_alloca_with_align is supposed to be in bits">,
   InGroup>;
Index: cfe/trunk/test/Sema/warn-alloca.c
===
--- cfe/trunk/test/Sema/warn-alloca.c
+++ cfe/trunk/test/Sema/warn-alloca.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -DSILENCE -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+#ifdef SILENCE
+  // expected-no-diagnostics
+#endif
+
+void test1(int a) {
+  __builtin_alloca(a);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
+
+void test2(int a) {
+  __builtin_alloca_with_align(a, 32);
+#ifndef SILENCE
+  // expected-warning@-2 {{use of function '__builtin_alloca_with_align' is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability}}
+#endif
+}
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -1179,6 +1179,10 @@
   case Builtin::BI__builtin_alloca_with_align:
 if (SemaBuiltinAllocaWithAlign(TheCall))
   return ExprError();
+LLVM_FALLTHROUGH;
+  case Builtin::BI__builtin_alloca:
+

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup>, DefaultIgnore;

aaron.ballman wrote:
> george.burgess.iv wrote:
> > aaron.ballman wrote:
> > > george.burgess.iv wrote:
> > > > aaron.ballman wrote:
> > > > > george.burgess.iv wrote:
> > > > > > nit: I'd just say "use of function '%0'" here; "builtin" doesn't 
> > > > > > really add much.
> > > > > > 
> > > > > > I also wonder if we should be saying anything more than "we found a 
> > > > > > use of this function." Looks like GCC doesn't 
> > > > > > (https://godbolt.org/z/sYs_8G), but since this warning is sort of 
> > > > > > opinionated in itself, might it be better to expand this to "use of 
> > > > > > '%0' is discouraged"?
> > > > > > 
> > > > > > WDYT, Aaron?
> > > > > What is the purpose to this diagnostic, aside from GCC compatibility? 
> > > > > What does it protect against?
> > > > > 
> > > > > If there's a reason users should not use alloc(), it would be better 
> > > > > for the diagnostic to spell it out.
> > > > > 
> > > > > Btw, I'm okay with this being default-off because the GCC warning is 
> > > > > as well. I'm mostly hoping we can do better with our diagnostic 
> > > > > wording.
> > > > > I'm mostly hoping we can do better with our diagnostic wording
> > > > 
> > > > +1
> > > > 
> > > > > What is the purpose to this diagnostic?
> > > > 
> > > > I think the intent boils down to that `alloca` is easily misused, and 
> > > > leads to e.g., 
> > > > https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . Since 
> > > > its use often boils down to nothing but a tiny micro-optimization, some 
> > > > parties would like to discourage its use.
> > > > 
> > > > Both glibc and bionic recommend against the use of `alloca` in their 
> > > > documentation, though glibc's docs are less assertive than bionic's, 
> > > > and explicitly call out "[alloca's] use can improve efficiency compared 
> > > > to the use of malloc plus free."
> > > > 
> > > > Greping a codebase and investigating the first 15 results:
> > > > - all of them look like microoptimizations; many of them also sit close 
> > > > to other `malloc`/`new` ops, so allocating on these paths presumably 
> > > > isn't prohibitively expensive
> > > > - all but two of the uses of `alloca` have no logic to fall back to the 
> > > > heap `malloc` if the array they want to allocate passes a certain 
> > > > threshold. Some of the uses make it look *really* easy for the array to 
> > > > grow very large.
> > > > - one of the uses compares the result of `alloca` to `NULL`. Since 
> > > > `alloca`'s behavior is undefined if it fails, ...
> > > > 
> > > > I'm having trouble putting this into a concise and actionable 
> > > > diagnostic message, though. The best I can come up with at the moment 
> > > > is something along the lines of "use of function %0 is subtle; consider 
> > > > using heap allocation instead."
> > > Okay, that's along the lines of what I was thinking.
> > > 
> > > Part of me thinks that this should not diagnose calls to `alloca()` that 
> > > are given a constant value that we can test for sanity at compile time. 
> > > e.g., calling `alloca(10)` is highly unlikely to be a problem, but 
> > > calling `alloca(100)` certainly could be, while `alloca(x)` is a 
> > > different class of problem without good static analysis.
> > > 
> > > That said, perhaps we could get away with `use of function %0 is 
> > > discouraged; there is no way to check for failure but failure may still 
> > > occur, resulting in a possibly exploitable security vulnerability` or 
> > > something along those lines?
> > Yeah, GCC has a similar `-Walloca-larger-than=N` that does roughly what you 
> > described. The icky part is exactly what you said. GCC will warn about 
> > unknown values, but considers control flow when doing so: 
> > https://godbolt.org/z/J3pGiT
> > 
> > It looks like it's the same "we apply optimizations and then see what 
> > happens" behavior as similar diagnostics: https://godbolt.org/z/lLyteP
> > 
> > WRT the diag we emit here, maybe we could use notes to break it up and 
> > imply things? e.g.
> > 
> > warning: use of function %0 is discouraged, due to its security implications
> > note: 'malloc' or 'new' are suggested alternatives, since they have 
> > well-defined behavior on failure
> > 
> > ...not sold on the idea, but it's a thought.
> > 
> > If we don't want to break it to pieces, I'm fine with your suggestion
> I'm not certain the note adds value because it will always be printed on the 
> same line as the warning. A note would make sense if we had a secondary 
> location to annotate though.
SGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D64883



___
cfe-commits mailing list

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup>, DefaultIgnore;

aaron.ballman wrote:
> george.burgess.iv wrote:
> > aaron.ballman wrote:
> > > george.burgess.iv wrote:
> > > > nit: I'd just say "use of function '%0'" here; "builtin" doesn't really 
> > > > add much.
> > > > 
> > > > I also wonder if we should be saying anything more than "we found a use 
> > > > of this function." Looks like GCC doesn't 
> > > > (https://godbolt.org/z/sYs_8G), but since this warning is sort of 
> > > > opinionated in itself, might it be better to expand this to "use of 
> > > > '%0' is discouraged"?
> > > > 
> > > > WDYT, Aaron?
> > > What is the purpose to this diagnostic, aside from GCC compatibility? 
> > > What does it protect against?
> > > 
> > > If there's a reason users should not use alloc(), it would be better for 
> > > the diagnostic to spell it out.
> > > 
> > > Btw, I'm okay with this being default-off because the GCC warning is as 
> > > well. I'm mostly hoping we can do better with our diagnostic wording.
> > > I'm mostly hoping we can do better with our diagnostic wording
> > 
> > +1
> > 
> > > What is the purpose to this diagnostic?
> > 
> > I think the intent boils down to that `alloca` is easily misused, and leads 
> > to e.g., https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . 
> > Since its use often boils down to nothing but a tiny micro-optimization, 
> > some parties would like to discourage its use.
> > 
> > Both glibc and bionic recommend against the use of `alloca` in their 
> > documentation, though glibc's docs are less assertive than bionic's, and 
> > explicitly call out "[alloca's] use can improve efficiency compared to the 
> > use of malloc plus free."
> > 
> > Greping a codebase and investigating the first 15 results:
> > - all of them look like microoptimizations; many of them also sit close to 
> > other `malloc`/`new` ops, so allocating on these paths presumably isn't 
> > prohibitively expensive
> > - all but two of the uses of `alloca` have no logic to fall back to the 
> > heap `malloc` if the array they want to allocate passes a certain 
> > threshold. Some of the uses make it look *really* easy for the array to 
> > grow very large.
> > - one of the uses compares the result of `alloca` to `NULL`. Since 
> > `alloca`'s behavior is undefined if it fails, ...
> > 
> > I'm having trouble putting this into a concise and actionable diagnostic 
> > message, though. The best I can come up with at the moment is something 
> > along the lines of "use of function %0 is subtle; consider using heap 
> > allocation instead."
> Okay, that's along the lines of what I was thinking.
> 
> Part of me thinks that this should not diagnose calls to `alloca()` that are 
> given a constant value that we can test for sanity at compile time. e.g., 
> calling `alloca(10)` is highly unlikely to be a problem, but calling 
> `alloca(100)` certainly could be, while `alloca(x)` is a different class 
> of problem without good static analysis.
> 
> That said, perhaps we could get away with `use of function %0 is discouraged; 
> there is no way to check for failure but failure may still occur, resulting 
> in a possibly exploitable security vulnerability` or something along those 
> lines?
Yeah, GCC has a similar `-Walloca-larger-than=N` that does roughly what you 
described. The icky part is exactly what you said. GCC will warn about unknown 
values, but considers control flow when doing so: https://godbolt.org/z/J3pGiT

It looks like it's the same "we apply optimizations and then see what happens" 
behavior as similar diagnostics: https://godbolt.org/z/lLyteP

WRT the diag we emit here, maybe we could use notes to break it up and imply 
things? e.g.

warning: use of function %0 is discouraged, due to its security implications
note: 'malloc' or 'new' are suggested alternatives, since they have 
well-defined behavior on failure

...not sold on the idea, but it's a thought.

If we don't want to break it to pieces, I'm fine with your suggestion


Repository:
  rC Clang

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

https://reviews.llvm.org/D64883



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

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup>, DefaultIgnore;

aaron.ballman wrote:
> george.burgess.iv wrote:
> > nit: I'd just say "use of function '%0'" here; "builtin" doesn't really add 
> > much.
> > 
> > I also wonder if we should be saying anything more than "we found a use of 
> > this function." Looks like GCC doesn't (https://godbolt.org/z/sYs_8G), but 
> > since this warning is sort of opinionated in itself, might it be better to 
> > expand this to "use of '%0' is discouraged"?
> > 
> > WDYT, Aaron?
> What is the purpose to this diagnostic, aside from GCC compatibility? What 
> does it protect against?
> 
> If there's a reason users should not use alloc(), it would be better for the 
> diagnostic to spell it out.
> 
> Btw, I'm okay with this being default-off because the GCC warning is as well. 
> I'm mostly hoping we can do better with our diagnostic wording.
> I'm mostly hoping we can do better with our diagnostic wording

+1

> What is the purpose to this diagnostic?

I think the intent boils down to that `alloca` is easily misused, and leads to 
e.g., https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . Since its 
use often boils down to nothing but a tiny micro-optimization, some parties 
would like to discourage its use.

Both glibc and bionic recommend against the use of `alloca` in their 
documentation, though glibc's docs are less assertive than bionic's, and 
explicitly call out "[alloca's] use can improve efficiency compared to the use 
of malloc plus free."

Greping a codebase and investigating the first 15 results:
- all of them look like microoptimizations; many of them also sit close to 
other `malloc`/`new` ops, so allocating on these paths presumably isn't 
prohibitively expensive
- all but two of the uses of `alloca` have no logic to fall back to the heap 
`malloc` if the array they want to allocate passes a certain threshold. Some of 
the uses make it look *really* easy for the array to grow very large.
- one of the uses compares the result of `alloca` to `NULL`. Since `alloca`'s 
behavior is undefined if it fails, ...

I'm having trouble putting this into a concise and actionable diagnostic 
message, though. The best I can come up with at the moment is something along 
the lines of "use of function %0 is subtle; consider using heap allocation 
instead."


Repository:
  rC Clang

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

https://reviews.llvm.org/D64883



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

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!

Mostly just nitpicking the warning's wording. :)




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2776
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup>, DefaultIgnore;

nit: I'd just say "use of function '%0'" here; "builtin" doesn't really add 
much.

I also wonder if we should be saying anything more than "we found a use of this 
function." Looks like GCC doesn't (https://godbolt.org/z/sYs_8G), but since 
this warning is sort of opinionated in itself, might it be better to expand 
this to "use of '%0' is discouraged"?

WDYT, Aaron?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64883



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

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

just a few drive-by nits/comments from me. as usual, not super familiar with 
clang-tidy, so i won't be able to stamp this.

thanks!




Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:23
+  binaryOperator(
+  hasOperatorName("<"),
+  hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), 
unless(hasName("posix_openpt")),

should we also try to catch `<= ${negative_value}` (or `< ${negative_value}`)?



Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:29
+  binaryOperator(
+  hasOperatorName("=="),
+  hasLHS(callExpr(callee(functionDecl(matchesName("^::posix_"), 
unless(hasName("posix_openpt")),

similarly, should we complain about `!= ${negative_value}`?



Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:36
+
+
+void PosixReturnCheck::check(const MatchFinder::MatchResult ) {

nit: please clang-format



Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:39
+  const auto  = *Result.Nodes.getNodeAs("binop");
+  diag(BinOp.getOperatorLoc(), "posix functions (except posix_openpt) never 
return negative values");
+}

would it be helpful to add fixits for simple cases? e.g. we can probably offer 
to replace `posix_whatever() < 0` with `posix_whatever() > 0` or `!= 0`



Comment at: clang-tools-extra/test/clang-tidy/android-posix-return.cpp:57
+
+void WarningWithMacro () {
+  if (posix_fadvise(0, 0, 0, 0) < ZERO) {}

nit: no space in `Macro ()`, please


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63623



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

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363346: [Targets] Move soft-float-abi filtering to 
`initFeatureMap` (authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61750?vs=200862=204678#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61750

Files:
  cfe/trunk/lib/Basic/Targets/ARM.cpp
  cfe/trunk/lib/Basic/Targets/ARM.h
  cfe/trunk/test/CodeGen/arm-soft-float-abi-filtering.c

Index: cfe/trunk/test/CodeGen/arm-soft-float-abi-filtering.c
===
--- cfe/trunk/test/CodeGen/arm-soft-float-abi-filtering.c
+++ cfe/trunk/test/CodeGen/arm-soft-float-abi-filtering.c
@@ -0,0 +1,9 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s | FileCheck %s
+// RUN: %clang_cc1 -verify -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s
+
+// CHECK-NOT: +soft-float-abi
+
+void foo() {}
+__attribute__((target("crc"))) void bar() {}
+__attribute__((target("soft-float-abi"))) void baz() {} // expected-warning{{unsupported 'soft-float-abi' in the 'target' attribute string}}
Index: cfe/trunk/lib/Basic/Targets/ARM.cpp
===
--- cfe/trunk/lib/Basic/Targets/ARM.cpp
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp
@@ -323,6 +323,8 @@
 this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
? "\01__gnu_mcount_nc"
: "\01mcount";
+
+  SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
@@ -385,12 +387,21 @@
 
   // Convert user-provided arm and thumb GNU target attributes to
   // [-|+]thumb-mode target features respectively.
-  std::vector UpdatedFeaturesVec(FeaturesVec);
-  for (auto  : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto  : FeaturesVec) {
+// Skip soft-float-abi; it's something we only use to initialize a bit of
+// class state, and is otherwise unrecognized.
+if (Feature == "+soft-float-abi")
+  continue;
+
+StringRef FixedFeature;
+if (Feature == "+arm")
+  FixedFeature = "-thumb-mode";
+else if (Feature == "+thumb")
+  FixedFeature = "+thumb-mode";
+else
+  FixedFeature = Feature;
+UpdatedFeaturesVec.push_back(FixedFeature.str());
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
@@ -405,7 +416,8 @@
   Crypto = 0;
   DSP = 0;
   Unaligned = 1;
-  SoftFloat = SoftFloatABI = false;
+  SoftFloat = false;
+  // Note that SoftFloatABI is initialized in our constructor.
   HWDiv = 0;
   DotProd = 0;
   HasFloat16 = true;
@@ -415,8 +427,6 @@
   for (const auto  : Features) {
 if (Feature == "+soft-float") {
   SoftFloat = true;
-} else if (Feature == "+soft-float-abi") {
-  SoftFloatABI = true;
 } else if (Feature == "+vfp2sp" || Feature == "+vfp2d16sp" ||
Feature == "+vfp2" || Feature == "+vfp2d16") {
   FPU |= VFP2FPU;
@@ -510,11 +520,6 @@
   else if (FPMath == FP_VFP)
 Features.push_back("-neonfp");
 
-  // Remove front-end specific options which the backend handles differently.
-  auto Feature = llvm::find(Features, "+soft-float-abi");
-  if (Feature != Features.end())
-Features.erase(Feature);
-
   return true;
 }
 
Index: cfe/trunk/lib/Basic/Targets/ARM.h
===
--- cfe/trunk/lib/Basic/Targets/ARM.h
+++ cfe/trunk/lib/Basic/Targets/ARM.h
@@ -124,6 +124,12 @@
  StringRef CPU,
  const std::vector ) const override;
 
+  bool isValidFeatureName(StringRef Feature) const override {
+// We pass soft-float-abi in as a -target-feature, but the backend figures
+// this out through other means.
+return Feature != "soft-float-abi";
+  }
+
   bool handleTargetFeatures(std::vector ,
 DiagnosticsEngine ) override;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

ping :)


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

https://reviews.llvm.org/D61750



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362672: android: add a close-on-exec check on pipe2() 
(authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62049?vs=203050=203282#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62049

Files:
  clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.cpp
  clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe2.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/android-cloexec-pipe2.cpp

Index: clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.cpp
@@ -0,0 +1,33 @@
+//===--- CloexecPipe2Check.cpp - clang-tidy===//
+//
+// 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 "CloexecPipe2Check.h"
+#include "../utils/ASTUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void CloexecPipe2Check::registerMatchers(MatchFinder *Finder) {
+  registerMatchersImpl(Finder,
+   functionDecl(returns(isInteger()), hasName("pipe2"),
+hasParameter(0, hasType(pointsTo(isInteger(,
+hasParameter(1, hasType(isInteger();
+}
+
+void CloexecPipe2Check::check(const MatchFinder::MatchResult ) {
+  insertMacroFlag(Result, /*MacroFlag=*/"O_CLOEXEC", /*ArgPos=*/1);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "CloexecInotifyInitCheck.h"
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
+#include "CloexecPipe2Check.h"
 #include "CloexecSocketCheck.h"
 #include "ComparisonInTempFailureRetryCheck.h"
 
@@ -49,6 +50,7 @@
 CheckFactories.registerCheck(
 "android-cloexec-memfd-create");
 CheckFactories.registerCheck("android-cloexec-open");
+CheckFactories.registerCheck("android-cloexec-pipe2");
 CheckFactories.registerCheck("android-cloexec-socket");
 CheckFactories.registerCheck(
 "android-comparison-in-temp-failure-retry");
Index: clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
@@ -14,6 +14,7 @@
   CloexecInotifyInitCheck.cpp
   CloexecMemfdCreateCheck.cpp
   CloexecOpenCheck.cpp
+  CloexecPipe2Check.cpp
   CloexecSocketCheck.cpp
   ComparisonInTempFailureRetryCheck.cpp
 
Index: clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.h
===
--- clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.h
+++ clang-tools-extra/trunk/clang-tidy/android/CloexecPipe2Check.h
@@ -0,0 +1,34 @@
+//===--- CloexecPipe2Check.h - clang-tidy*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE2_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE2_H
+
+#include "CloexecCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// Finds code that uses pipe2() without using the O_CLOEXEC flag.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-pipe2.html
+class CloexecPipe2Check : public CloexecCheck {
+public:
+  CloexecPipe2Check(StringRef Name, ClangTidyContext *Context)
+  

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362673: android: add a close-on-exec check on pipe() 
(authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61967?vs=202984=203283#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61967

Files:
  clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.cpp
  clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/android-cloexec-pipe.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/test/clang-tidy/android-cloexec-pipe.cpp

Index: clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.h
+++ clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.h
@@ -0,0 +1,34 @@
+//===--- CloexecPipeCheck.h - clang-tidy-*- 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
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H
+
+#include "CloexecCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+/// Suggests to replace calls to pipe() with calls to pipe2().
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/android-cloexec-pipe.html
+class CloexecPipeCheck : public CloexecCheck {
+public:
+  CloexecPipeCheck(StringRef Name, ClangTidyContext *Context)
+  : CloexecCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+};
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ANDROID_CLOEXEC_PIPE_H
Index: clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
@@ -20,6 +20,7 @@
 #include "CloexecInotifyInitCheck.h"
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
+#include "CloexecPipeCheck.h"
 #include "CloexecPipe2Check.h"
 #include "CloexecSocketCheck.h"
 #include "ComparisonInTempFailureRetryCheck.h"
@@ -50,6 +51,7 @@
 CheckFactories.registerCheck(
 "android-cloexec-memfd-create");
 CheckFactories.registerCheck("android-cloexec-open");
+CheckFactories.registerCheck("android-cloexec-pipe");
 CheckFactories.registerCheck("android-cloexec-pipe2");
 CheckFactories.registerCheck("android-cloexec-socket");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/CloexecPipeCheck.cpp
@@ -0,0 +1,37 @@
+//===--- CloexecPipeCheck.cpp - clang-tidy-===//
+//
+// 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 "CloexecPipeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+void CloexecPipeCheck::registerMatchers(MatchFinder *Finder) {
+  registerMatchersImpl(Finder,
+   functionDecl(returns(isInteger()), hasName("pipe"),
+hasParameter(0, hasType(pointsTo(isInteger());
+}
+
+void CloexecPipeCheck::check(const MatchFinder::MatchResult ) {
+  std::string ReplacementText =
+  (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();
+
+  replaceFunc(
+  Result,
+  "prefer pipe2() with O_CLOEXEC to avoid leaking file descriptors to child processes",
+  ReplacementText);
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
Index: 

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Will submit once gribozavr indicates that they're happy with the new test 
names. Thanks again for working on this!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61967



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for the feedback!

> With this patch, do we pass the general-regs-only attribute to the backend? 
> If so, would that be the attribute we'd want to check to emit errors from the 
> backend from any "accidental" floating-point operations?

Yeah, the current design is for us to pass +general-regs-only as a target 
'feature' per function. Given that there's no code to actually handle that at 
the moment, I've put a FIXME in its place. Please let me know if there's a 
better way to go about this.




Comment at: clang/include/clang/Basic/LangOptions.def:143
 LANGOPT(RelaxedTemplateTemplateArgs, 1, 0, "C++17 relaxed matching of template 
template arguments")
+LANGOPT(GeneralOpsOnly, 1, 0, "Whether to diagnose the use of 
floating-point or vector operations")
 

void wrote:
> Everywhere else you use "general regs only" instead of "ops". Should that be 
> done here?
Yeah, I'm not sure why I named it `Ops`. Fixed



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7840
+
+  const auto *StructTy = Ty.getCanonicalType()->getAsStructureType();
+  if (!StructTy)

efriedma wrote:
> Do you really want to enforce isStruct() here?  That's types declared with 
> the keyword "struct".
Good catch -- generalized this.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7857
+
+  return llvm::any_of(StructTy->getDecl()->fields(), [](const FieldDecl *FD) {
+return typeHasFloatingOrVectorComponent(FD->getType());

efriedma wrote:
> Do we have to be concerned about base classes here?
Yup. Added tests for this, too



Comment at: clang/lib/Sema/SemaExprCXX.cpp:7951
+  // they don't have to write out memcpy() for simple cases. For that reason,
+  // it's very limited in what it will detect.
+  //

efriedma wrote:
> We don't always lower struct copies to memcpy(); I'm not sure this is safe.
I see; removed. If this check ends up being important (it doesn't seem to be in 
local builds), we can revisit. :)



Comment at: clang/lib/Sema/SemaExprCXX.cpp:8003
+!isRValueOfIllegalType(E) &&
+E->isConstantInitializer(S.getASTContext(), /*ForRef=*/false)) {
+  resetDiagnosticState(InitialState);

efriedma wrote:
> Just because we can constant-fold an expression, doesn't mean we will, 
> especially at -O0.
Are there any guarantees that we offer along these lines? The code in 
particular that this cares about boils down to a bunch of integer literals 
doing mixed math with FP literals, all of which gets casted to an `int`. 
Conceptually, it seems silly to me to emit an addition for something as 
straightforward as `int i = 1 + 2.0;`, even at -O0, though I totally agree that 
you're right, and codegen like this is reasonable at -O0: 
https://godbolt.org/z/NS0L17

(This also brings up a good point: this visitor probably shouldn't be run on 
`IsConstexpr` expressions; fixed that later on)


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

https://reviews.llvm.org/D38479



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 201047.
george.burgess.iv marked 10 inline comments as done.
george.burgess.iv added a comment.

Addressed feedback, modulo the constant foldable comment thread.


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

https://reviews.llvm.org/D38479

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGen/aarch64-mgeneral_regs_only.c
  clang/test/Sema/aarch64-mgeneral_regs_only.c
  clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp

Index: clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
@@ -0,0 +1,186 @@
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=float -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=int '-DVECATTR=__attribute__((ext_vector_type(2)))' -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=FloatTypedef -Wno-unused-value
+
+using FloatTypedef = float;
+
+#ifndef VECATTR
+#define VECATTR
+#define BannedToInt(x) (x)
+#else
+#define BannedToInt(x) ((x)[0])
+#endif
+
+typedef BANNED BannedTy VECATTR;
+
+namespace default_args {
+int foo(BannedTy = 0); // expected-error 2{{use of floating-point or vector values is disabled}}
+int bar(int = 1.0);
+
+void baz(int a = foo()); // expected-note{{from use of default argument here}}
+void bazz(int a = bar());
+
+void test() {
+  foo(); // expected-note{{from use of default argument here}}
+  bar();
+  baz(); // expected-note{{from use of default argument here}}
+  baz(4);
+  bazz();
+}
+} // namespace default_args
+
+namespace conversions {
+struct ConvertToFloat {
+  explicit operator BannedTy();
+};
+struct ConvertToFloatImplicit { /* implicit */
+  operator BannedTy();
+};
+struct ConvertFromFloat {
+  ConvertFromFloat(BannedTy);
+};
+
+void takeFloat(BannedTy);
+void takeConvertFromFloat(ConvertFromFloat);
+void takeConvertFromFloatRef(const ConvertFromFloat &);
+
+void test() {
+  BannedTy(ConvertToFloat());
+
+  takeFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat{0};// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat{ConvertToFloatImplicit{}}; // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(0); // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(ConvertFromFloat(0));// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+}
+
+void takeImplicitFloat(BannedTy = ConvertToFloatImplicit()); // expected-error{{use of floating-point or vector values is disabled}}
+void test2() {
+  takeImplicitFloat(); // expected-note{{from use of default argument here}}
+}
+} // namespace conversions
+
+namespace refs {
+struct BannedRef {
+  const BannedTy 
+  BannedRef(const BannedTy ) : f(f) {}
+};
+
+BannedTy ();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy  = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy  = getBanned();
+  const BannedTy  = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+  const int  = 1.0;
+  const int  = BannedToInt(getBannedVal()); // expected-error{{use of floating-point or vector values is disabled}}
+
+  BannedRef{getBanned()};
+  BannedRef{getBannedVal()}; // expected-error{{use of floating-point or vector values is disabled}}
+}
+} // namespace refs
+
+namespace class_init {
+struct Foo {
+  float f = 1.0; // expected-error{{use of floating-point or vector values is disabled}}
+  int i = 1.0;
+  float j;
+
+  Foo() : j(1) // expected-error{{use of floating-point or vector values is disabled}}
+  {}
+};
+} // namespace class_init
+
+namespace copy_move_assign {
+struct Foo { // expected-error 2{{use of floating-point or vector values is disabled}}
+  float f;

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> neonfp isn't passed as a feature in the first place; there's a separate API 
> setFPMath which is used for that. We translate it into a target feature for 
> the sake of the backend. So I'm not sure what you're proposing.

The idea would be for `initFeatureMap` to handle adding `neonfp` instead, so we 
can make the `vector&` we're passing into `handleTargetFeatures` a `const 
vector&`. An old comment claims that this would be desirable from a refactoring 
standpoint

> What happens if someone specifies attribute((target("soft-float-abi"))) or 
> something like that? (There's an API isValidFeatureName to validate feature 
> names, but we currently don't implement it.)

Good catch. It doesn't appear that that works on its own for the `target` 
attribute, since we only filter user attrs, but it's probably something that's 
good to diagnose.


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

https://reviews.llvm.org/D61750



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

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 200862.
george.burgess.iv added a comment.

Address comments -- thanks!


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

https://reviews.llvm.org/D61750

Files:
  clang/lib/Basic/Targets/ARM.cpp
  clang/lib/Basic/Targets/ARM.h
  clang/test/CodeGen/arm-soft-float-abi-filtering.c

Index: clang/test/CodeGen/arm-soft-float-abi-filtering.c
===
--- /dev/null
+++ clang/test/CodeGen/arm-soft-float-abi-filtering.c
@@ -0,0 +1,9 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s | FileCheck %s
+// RUN: %clang_cc1 -verify -triple arm-none-linux-gnueabi -target-feature "+soft-float-abi" %s
+
+// CHECK-NOT: +soft-float-abi
+
+void foo() {}
+__attribute__((target("crc"))) void bar() {}
+__attribute__((target("soft-float-abi"))) void baz() {} // expected-warning{{unsupported 'soft-float-abi' in the 'target' attribute string}}
Index: clang/lib/Basic/Targets/ARM.h
===
--- clang/lib/Basic/Targets/ARM.h
+++ clang/lib/Basic/Targets/ARM.h
@@ -116,6 +116,12 @@
  StringRef CPU,
  const std::vector ) const override;
 
+  bool isValidFeatureName(StringRef Feature) const override {
+// We pass soft-float-abi in as a -target-feature, but the backend figures
+// this out through other means.
+return Feature != "soft-float-abi";
+  }
+
   bool handleTargetFeatures(std::vector ,
 DiagnosticsEngine ) override;
 
Index: clang/lib/Basic/Targets/ARM.cpp
===
--- clang/lib/Basic/Targets/ARM.cpp
+++ clang/lib/Basic/Targets/ARM.cpp
@@ -313,6 +313,8 @@
 this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
? "\01__gnu_mcount_nc"
: "\01mcount";
+
+  SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
@@ -375,12 +377,21 @@
 
   // Convert user-provided arm and thumb GNU target attributes to
   // [-|+]thumb-mode target features respectively.
-  std::vector UpdatedFeaturesVec(FeaturesVec);
-  for (auto  : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto  : FeaturesVec) {
+// Skip soft-float-abi; it's something we only use to initialize a bit of
+// class state, and is otherwise unrecognized.
+if (Feature == "+soft-float-abi")
+  continue;
+
+StringRef FixedFeature;
+if (Feature == "+arm")
+  FixedFeature = "-thumb-mode";
+else if (Feature == "+thumb")
+  FixedFeature = "+thumb-mode";
+else
+  FixedFeature = Feature;
+UpdatedFeaturesVec.push_back(FixedFeature.str());
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
@@ -394,7 +405,8 @@
   Crypto = 0;
   DSP = 0;
   Unaligned = 1;
-  SoftFloat = SoftFloatABI = false;
+  SoftFloat = false;
+  // Note that SoftFloatABI is initialized in our constructor.
   HWDiv = 0;
   DotProd = 0;
   HasFloat16 = true;
@@ -405,8 +417,6 @@
   for (const auto  : Features) {
 if (Feature == "+soft-float") {
   SoftFloat = true;
-} else if (Feature == "+soft-float-abi") {
-  SoftFloatABI = true;
 } else if (Feature == "+vfp2") {
   FPU |= VFP2FPU;
   HW_FP |= HW_FP_SP | HW_FP_DP;
@@ -480,11 +490,6 @@
   else if (FPMath == FP_VFP)
 Features.push_back("-neonfp");
 
-  // Remove front-end specific options which the backend handles differently.
-  auto Feature = llvm::find(Features, "+soft-float-abi");
-  if (Feature != Features.end())
-Features.erase(Feature);
-
   return true;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 200388.
george.burgess.iv added a comment.

Rebased


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

https://reviews.llvm.org/D38479

Files:
  clang/docs/UsersManual.rst
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CodeGen/aarch64-mgeneral_regs_only.c
  clang/test/Driver/aarch64-mgeneral_regs_only.c
  clang/test/Sema/aarch64-mgeneral_regs_only.c
  clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp

Index: clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/aarch64-mgeneral_regs_only.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=float -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=int '-DVECATTR=__attribute__((ext_vector_type(2)))' -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=FloatTypedef -Wno-unused-value
+
+using FloatTypedef = float;
+
+#ifndef VECATTR
+#define VECATTR
+#define BannedToInt(x) (x)
+#else
+#define BannedToInt(x) ((x)[0])
+#endif
+
+typedef BANNED BannedTy VECATTR;
+
+namespace default_args {
+int foo(BannedTy = 0); // expected-error 2{{use of floating-point or vector values is disabled}}
+int bar(int = 1.0);
+
+void baz(int a = foo()); // expected-note{{from use of default argument here}}
+void bazz(int a = bar());
+
+void test() {
+  foo(); // expected-note{{from use of default argument here}}
+  bar();
+  baz(); // expected-note{{from use of default argument here}}
+  baz(4);
+  bazz();
+}
+} // namespace default_args
+
+namespace conversions {
+struct ConvertToFloat {
+  explicit operator BannedTy();
+};
+struct ConvertToFloatImplicit { /* implicit */
+  operator BannedTy();
+};
+struct ConvertFromFloat {
+  ConvertFromFloat(BannedTy);
+};
+
+void takeFloat(BannedTy);
+void takeConvertFromFloat(ConvertFromFloat);
+void takeConvertFromFloatRef(const ConvertFromFloat &);
+
+void test() {
+  BannedTy(ConvertToFloat());
+
+  takeFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat{0};// expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat{ConvertToFloatImplicit{}}; // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(0);// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(0); // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(ConvertFromFloat(0));// expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+}
+
+void takeImplicitFloat(BannedTy = ConvertToFloatImplicit()); // expected-error{{use of floating-point or vector values is disabled}}
+void test2() {
+  takeImplicitFloat(); // expected-note{{from use of default argument here}}
+}
+} // namespace conversions
+
+namespace refs {
+struct BannedRef {
+  const BannedTy 
+  BannedRef(const BannedTy ) : f(f) {}
+};
+
+BannedTy ();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy  = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy  = getBanned();
+  const BannedTy  = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+  const int  = 1.0;
+  const int  = BannedToInt(getBannedVal()); // expected-error{{use of floating-point or vector values is disabled}}
+
+  BannedRef{getBanned()};
+  BannedRef{getBannedVal()}; // expected-error{{use of floating-point or vector values is disabled}}
+}
+} // namespace refs
+
+namespace class_init {
+struct Foo {
+  float f = 1.0; // expected-error{{use of floating-point or vector values is disabled}}
+  int i = 1.0;
+  float j;
+
+  Foo() : j(1) // expected-error{{use of floating-point or vector values is disabled}}
+  {}
+};
+} // namespace class_init
+
+namespace copy_move_assign {
+struct Foo {
+  float f;
+}; // expected-error 2{{use of floating-point or vector values is disabled}}
+
+void copyFoo(Foo ) {
+  Foo a = f; 

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

I'm happy to give rebasing it a shot later this week. My recollection of the 
prior state of this patch was that we wanted some backend work done to 
double-check that no illegal ops get generated by optimizations and such, since 
these checks are purely done in the frontend. I don't foresee myself having 
time in the near future to make that happen, so is that something that we want 
to continue to block this patch on? If so, then someone else is probably going 
to need to do that piece. Otherwise, I think people were happy enough with this 
patch as-is?


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

https://reviews.llvm.org/D38479



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

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:27
+void CloexecPipeCheck::check(const MatchFinder::MatchResult ) {
+  const std::string  =
+  (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();

jcai19 wrote:
> george.burgess.iv wrote:
> > simplicity nit: can this be a `std::string`?
> replaceFunc takes a llvm::StringRef as the third parameter. StringRef is 
> "expected to be used in situations where the character data resides in some 
> other buffer, whose lifetime extends past that of the StringRef", which is 
> true in this case, so I think it should be fine.
Both should be functionally equivalent in this code. My point was that it's not 
common to rely on temporary lifetime extension when writing the non-`const&` 
type would be equivalent (barring maybe cases with `auto`, but that's not 
applicable), and I don't see why we should break with that here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61967



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

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!

I don't have great context on tidy, so I can't stamp this, but I do have a few 
drive-by nits for you.




Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:22
+   functionDecl(returns(isInteger()), hasName("pipe"),
+//hasParameter(0, 
hasType(constantArrayType(hasElementType(isInteger()), hasSize(2)
+hasParameter(0, 
hasType(pointsTo(isInteger());

We probably don't want to commit commented out code



Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.cpp:27
+void CloexecPipeCheck::check(const MatchFinder::MatchResult ) {
+  const std::string  =
+  (Twine("pipe2(") + getSpellingArg(Result, 0) + ", O_CLOEXEC)").str();

simplicity nit: can this be a `std::string`?



Comment at: clang-tools-extra/clang-tidy/android/CloexecPipeCheck.h:18
+
+/// accept() is better to be replaced by accept4().
+///

nit: should probably update this comment



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:9
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer pipe2() to pipe() because 
pipe2() allows O_CLOEXEC [android-cloexec-pipe]
+  // CHECK-FIXES: pipe2(pipefd, O_CLOEXEC);
+}

(Do we have a CHECK-FIXES-NOT or CHECK-MESSAGES-NOT to apply to the things 
below? Or are the CHECKs here meant to be complete like clang's `-verify`?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61967



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

[PATCH] D61750: [Targets] Move soft-float-abi filtering to `initFeatureMap`

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: michaelplatings, efriedma.
Herald added subscribers: kristof.beyls, javed.absar.
Herald added a project: clang.

I'm not convinced this is an excellent approach, in part because I'm unclear on 
where all we expect to funnel the results of `TargetInfo::initFeatureMap`. 
Thought I'd throw it out for review anyway, and see what people with actual 
context think. :)

The underlying problem I'm trying to solve is that, given the following code 
with a suitable ARM target,

  ___attribute__((target("crc"))) void foo() {}

clang ends up codegening a function with the '+soft-float-abi' target feature, 
which we go out of our way to remove in the frontend for the default target 
features, since the backend apparently tries to figure out whether the soft 
float ABI should be used on its own. This is more or less harmless on its own, 
but it causes the backend to emit a diagnostic directly to `errs()`. Rather 
than try to find a way to silence that diag, it seems better to not have to 
emit it in the first place.

An alternate fix would be to somehow try to filter this in cfe/lib/CodeGen, but 
there appear to be many callers of  `initFeatureMap`; I get the vague feeling 
that we shouldn't be letting `+soft-float-abi` slip through any of them. Again, 
could be wrong about that due to my lack of familiarity with `initFeatureMap`'s 
uses.

If we agree that this is a good way forward, there also appears to be 
`+neonfp`/`-neonfp` additions happening in `handleTargetFeatures` that should 
probably be happening in `initFeatureMap` instead? If that's the case, I'm 
happy to do that as a follow-up, and make it so that `handleTargetFeatures` no 
longer mutates its input (which comments indicate would be desirable, along 
with a more general move of all of this initialization stuff to the 
construction of our various `TargetInfo` subclasses).


Repository:
  rC Clang

https://reviews.llvm.org/D61750

Files:
  lib/Basic/Targets/ARM.cpp
  test/CodeGen/arm-soft-float-abi-filtering.c


Index: test/CodeGen/arm-soft-float-abi-filtering.c
===
--- /dev/null
+++ test/CodeGen/arm-soft-float-abi-filtering.c
@@ -0,0 +1,7 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang_cc1 -emit-llvm -o - -triple arm-none-linux-gnueabi 
-target-feature "+soft-float-abi" %s | FileCheck %s
+
+// CHECK-NOT: +soft-float-abi
+
+void foo() {}
+__attribute__((target("crc"))) void bar() {}
Index: lib/Basic/Targets/ARM.cpp
===
--- lib/Basic/Targets/ARM.cpp
+++ lib/Basic/Targets/ARM.cpp
@@ -313,6 +313,8 @@
 this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
? "\01__gnu_mcount_nc"
: "\01mcount";
+
+  SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
@@ -375,12 +377,21 @@
 
   // Convert user-provided arm and thumb GNU target attributes to
   // [-|+]thumb-mode target features respectively.
-  std::vector UpdatedFeaturesVec(FeaturesVec);
-  for (auto  : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto  : FeaturesVec) {
+// Skip soft-float-abi; it's something we only use to initialize a bit of
+// class state, and is otherwise unrecognized.
+if (Feature == "+soft-float-abi")
+  continue;
+
+StringRef FixedFeature;
+if (Feature == "+arm")
+  FixedFeature = "-thumb-mode";
+else if (Feature == "+thumb")
+  FixedFeature = "+thumb-mode";
+else
+  FixedFeature = Feature;
+UpdatedFeaturesVec.push_back(FixedFeature.str());
   }
 
   return TargetInfo::initFeatureMap(Features, Diags, CPU, UpdatedFeaturesVec);
@@ -394,7 +405,8 @@
   Crypto = 0;
   DSP = 0;
   Unaligned = 1;
-  SoftFloat = SoftFloatABI = false;
+  SoftFloat = false;
+  // Note that SoftFloatABI is initialized in our constructor.
   HWDiv = 0;
   DotProd = 0;
   HasFloat16 = true;
@@ -405,8 +417,6 @@
   for (const auto  : Features) {
 if (Feature == "+soft-float") {
   SoftFloat = true;
-} else if (Feature == "+soft-float-abi") {
-  SoftFloatABI = true;
 } else if (Feature == "+vfp2") {
   FPU |= VFP2FPU;
   HW_FP |= HW_FP_SP | HW_FP_DP;
@@ -475,11 +485,6 @@
   else if (FPMath == FP_VFP)
 Features.push_back("-neonfp");
 
-  // Remove front-end specific options which the backend handles differently.
-  auto Feature = llvm::find(Features, "+soft-float-abi");
-  if (Feature != Features.end())
-Features.erase(Feature);
-
   return true;
 }
 


Index: test/CodeGen/arm-soft-float-abi-filtering.c
===
--- /dev/null
+++ 

[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357290: Various fixes and additions to 
creduce-clang-crash.py (authored by gbiv, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D59725?vs=192846=192867#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59725

Files:
  cfe/trunk/utils/creduce-clang-crash.py

Index: cfe/trunk/utils/creduce-clang-crash.py
===
--- cfe/trunk/utils/creduce-clang-crash.py
+++ cfe/trunk/utils/creduce-clang-crash.py
@@ -1,8 +1,14 @@
 #!/usr/bin/env python
 """Calls C-Reduce to create a minimal reproducer for clang crashes.
+
+Output files:
+  *.reduced.sh -- crash reproducer with minimal arguments
+  *.reduced.cpp -- the reduced file
+  *.test.sh -- interestingness test for C-Reduce
 """
 
-from argparse import ArgumentParser
+from __future__ import print_function
+from argparse import ArgumentParser, RawTextHelpFormatter
 import os
 import re
 import stat
@@ -15,10 +21,14 @@
 from distutils.spawn import find_executable
 
 verbose = False
-llvm_bin = None
 creduce_cmd = None
+clang_cmd = None
 not_cmd = None
 
+def verbose_print(*args, **kwargs):
+  if verbose:
+print(*args, **kwargs)
+
 def check_file(fname):
   if not os.path.isfile(fname):
 sys.exit("ERROR: %s does not exist" % (fname))
@@ -33,166 +43,339 @@
 cmd = find_executable(cmd_path)
 if cmd:
   return cmd
-sys.exit("ERROR: executable %s not found" % (cmd_path))
+sys.exit("ERROR: executable `%s` not found" % (cmd_path))
 
   cmd = find_executable(cmd_name, path=cmd_dir)
   if cmd:
 return cmd
-  sys.exit("ERROR: %s not found in %s" % (cmd_name, cmd_dir))
 
-def quote_cmd(cmd):
-  return ' '.join(arg if arg.startswith('$') else pipes.quote(arg)
-  for arg in cmd)
+  if not cmd_dir:
+cmd_dir = "$PATH"
+  sys.exit("ERROR: `%s` not found in %s" % (cmd_name, cmd_dir))
 
-def get_crash_cmd(crash_script):
-  with open(crash_script) as f:
-# Assume clang call is on the last line of the script
-line = f.readlines()[-1]
-cmd = shlex.split(line)
-
-# Overwrite the script's clang with the user's clang path
-new_clang = check_cmd('clang', llvm_bin)
-cmd[0] = pipes.quote(new_clang)
-return cmd
+def quote_cmd(cmd):
+  return ' '.join(pipes.quote(arg) for arg in cmd)
 
-def has_expected_output(crash_cmd, expected_output):
-  p = subprocess.Popen(crash_cmd,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT)
-  crash_output, _ = p.communicate()
-  return all(msg in crash_output for msg in expected_output)
-
-def get_expected_output(crash_cmd):
-  p = subprocess.Popen(crash_cmd,
-   stdout=subprocess.PIPE,
-   stderr=subprocess.STDOUT)
-  crash_output, _ = p.communicate()
-
-  # If there is an assertion failure, use that;
-  # otherwise use the last five stack trace functions
-  assertion_re = r'Assertion `([^\']+)\' failed'
-  assertion_match = re.search(assertion_re, crash_output)
-  if assertion_match:
-return [assertion_match.group(1)]
-  else:
-stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
-matches = re.findall(stacktrace_re, crash_output)
-return matches[-5:]
-
-def write_interestingness_test(testfile, crash_cmd, expected_output,
-   file_to_reduce):
-  filename = os.path.basename(file_to_reduce)
-  if filename not in crash_cmd:
-sys.exit("ERROR: expected %s to be in the crash command" % filename)
-
-  # Replace all instances of file_to_reduce with a command line variable
-  output = ['#!/bin/bash',
-'if [ -z "$1" ] ; then',
-'  f=%s' % (pipes.quote(filename)),
-'else',
-'  f="$1"',
-'fi']
-  cmd = ['$f' if s == filename else s for s in crash_cmd]
-
-  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(not_cmd),
-  quote_cmd(cmd)))
-
-  for msg in expected_output:
-output.append('grep %s t.log || exit 1' % pipes.quote(msg))
-
-  with open(testfile, 'w') as f:
-f.write('\n'.join(output))
-  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
-
-def check_interestingness(testfile, file_to_reduce):
-  testfile = os.path.abspath(testfile)
-
-  # Check that the test considers the original file interesting
-  with open(os.devnull, 'w') as devnull:
-returncode = subprocess.call(testfile, stdout=devnull)
-  if returncode:
-sys.exit("The interestingness test does not pass for the original file.")
-
-  # Check that an empty file is not interesting
-  _, empty_file = tempfile.mkstemp()
-  with open(os.devnull, 'w') as devnull:
-returncode = subprocess.call([testfile, empty_file], stdout=devnull)
-  

[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Only a few more nits on my side, and this LGTM. WDYT, arichardson?




Comment at: clang/utils/creduce-clang-crash.py:137
+
+# If no message was found, use the top five stack trace functions,
+# ignoring some common functions

akhuang wrote:
> george.burgess.iv wrote:
> > Please expand a bit on why 5 was chosen (is there some deep reason behind 
> > it, or does it just seem like a sensible number?)
> There is no deep reason - it was an arbitrary smallish number to hopefully 
> not only pick up common stack trace functions
Sorry -- should've been clearer. I meant "in the comment in the code, please 
expand a bit [...]" :)



Comment at: clang/utils/creduce-clang-crash.py:362
+  r = Reduce(crash_script, file_to_reduce)
+  r.simplify_clang_args()
+  r.write_interestingness_test()

arichardson wrote:
> george.burgess.iv wrote:
> > akhuang wrote:
> > > george.burgess.iv wrote:
> > > > I'm unclear on why we do a partial simplification of clang args here, 
> > > > then a full reduction after creduce. Is there a disadvantage to instead 
> > > > doing:
> > > > 
> > > > r.write_interestingness_test()
> > > > r.clang_preprocess()
> > > > r.reduce_clang_args()
> > > > r.run_creduce()
> > > > r.reduce_clang_args()
> > > > 
> > > > ?
> > > > 
> > > > The final `reduce_clang_args` being there to remove extra 
> > > > `-D`/`-I`/etc. args if preprocessing failed somehow, to remove 
> > > > `-std=foo` args if those aren't relevant after reduction, etc.
> > > > 
> > > > The advantage to this being that we no longer need to maintain a 
> > > > `simplify` function, and creduce runs with a relatively minimal set of 
> > > > args to start with.
> > > > 
> > > > In any case, can we please add comments explaining why we chose 
> > > > whatever order we end up going with, especially where subtleties make 
> > > > it counter to what someone might naively expect?
> > > Basically the disadvantage is that clang takes longer to run before the 
> > > reduction, so it takes unnecessary time to iterate through all the 
> > > arguments beforehand. 
> > > And yeah, the final `reduce_clang_args` is there to minimize the clang 
> > > arguments needed to reproduce the crash on the reduced source file. 
> > > 
> > > If that makes sense, I can add comments for this-
> > Eh, I don't have a strong opinion here. My inclination is to prefer a 
> > simpler reduction script if the difference is len(args) clang invocations, 
> > since I assume creduce is going to invoke clang way more than len(args) 
> > times. I see a docstring detailing that `simplify` is only meant to speed 
> > things up now, so I'm content with the way things are.
> I think it makes sense to remove some clang args before running creduce since 
> removal of some flags can massively speed up reduction later (e.g. not 
> emitting debug info or compiling at -O0, only doing -emit-llvm, etc) if they 
> avoid expensive optimizations that don't cause the crash.
Agreed. My question was more "why do we have special reduction code on both 
sides of this instead of just `reduce_clang_args`'ing on both sides of the 
`run_creduce`." It wasn't clear to me that `simplify_clang_args` was only 
intended to speed things up, but now it is. :)



Comment at: clang/utils/creduce-clang-crash.py:198
+# Instead of modifying the filename in the test file, just run the command
+fd, empty_file = tempfile.mkstemp()
+if self.check_expected_output(filename=empty_file):

Did we want to use `NamedTemporaryFile` here as rnk suggested?

(If not, you can lift the `os.close`s to immediately after this line.)



Comment at: clang/utils/creduce-clang-crash.py:206
+print("\nTrying to preprocess the source file...")
+fd, tmpfile = tempfile.mkstemp()
+

Similar question about `NamedTemporaryFile`.

Please note that you'll probably have to pass `delete=False`, since apparently 
`delete=True` sets O_TEMPORARY on Windows, which... might follow the file 
across renames? I'm unsure.


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

https://reviews.llvm.org/D59725



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

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

(Forgot to refresh before pressing 'Submit', so maybe efriedma's comment 
answers all of the questions in mine ;) )


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



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

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

We have warnings like -Wdivision-by-zero that take reachability into account: 
https://godbolt.org/z/3PD-eF. I don't immediately know how it's all done (e.g. 
in batch because CFG construction is expensive? ...), but looking there for 
inspiration may be useful.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



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

[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv marked an inline comment as done.
george.burgess.iv added inline comments.



Comment at: clang/utils/creduce-clang-crash.py:223
+  if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'):
+x[-1] += ' ' + y
+return x

akhuang wrote:
> george.burgess.iv wrote:
> > Should we be `shlex.quote`'ing y here (and probably in the `return x + [y]` 
> > below)?
> It quotes everything right before writing to file - are there reasons to 
> quote here instead?
We're `shlex.split`ing groups below, and I assume the intent is 
`Reduce.ungroup_args(Reduce.group_args_by_dash(args)) == args`.

If we don't want to quote here, we can also have `ungroup_args` and 
`group_args_by_dash` deal in lists of nonempty lists.



Comment at: clang/utils/creduce-clang-crash.py:279
+ "-debugger-tuning=",
+ "-gdwarf"])
+new_args = self.try_remove_args(new_args,

akhuang wrote:
> george.burgess.iv wrote:
> > If we're replacing other args with their effective negation, does it also 
> > make sense to replace all debug-ish options with `-g0`?
> I guess `-g0` is not a cc1 option, nor is `-gdwarf`? So this is essentially 
> just removing `-gcodeview`. I'm actually not sure what the other flags do. 
Ah, I didn't realize this was dealing with cc1 args. My mistake.

I'm not immediately sure either, but grepping through the code, it looks like 
`-debug-info-kind=` may be the main interesting one to us here.



Comment at: clang/utils/creduce-clang-crash.py:306
+# Remove other cases that aren't covered by the heuristic
+new_args = self.try_remove_args(new_args, msg="Removed -mllvm",
+opts_one_arg_startswith=["-mllvm"])

george.burgess.iv wrote:
> Probably want to do a similar thing for `-Xclang` (which, as far as this code 
> is concerned, acts a lot like `-mllvm`)
(You can ignore this comment if we're dealing in cc1; `-Xclang` is just "pass 
this directly as a cc1 arg")



Comment at: clang/utils/creduce-clang-crash.py:362
+  r = Reduce(crash_script, file_to_reduce)
+  r.simplify_clang_args()
+  r.write_interestingness_test()

akhuang wrote:
> george.burgess.iv wrote:
> > I'm unclear on why we do a partial simplification of clang args here, then 
> > a full reduction after creduce. Is there a disadvantage to instead doing:
> > 
> > r.write_interestingness_test()
> > r.clang_preprocess()
> > r.reduce_clang_args()
> > r.run_creduce()
> > r.reduce_clang_args()
> > 
> > ?
> > 
> > The final `reduce_clang_args` being there to remove extra `-D`/`-I`/etc. 
> > args if preprocessing failed somehow, to remove `-std=foo` args if those 
> > aren't relevant after reduction, etc.
> > 
> > The advantage to this being that we no longer need to maintain a `simplify` 
> > function, and creduce runs with a relatively minimal set of args to start 
> > with.
> > 
> > In any case, can we please add comments explaining why we chose whatever 
> > order we end up going with, especially where subtleties make it counter to 
> > what someone might naively expect?
> Basically the disadvantage is that clang takes longer to run before the 
> reduction, so it takes unnecessary time to iterate through all the arguments 
> beforehand. 
> And yeah, the final `reduce_clang_args` is there to minimize the clang 
> arguments needed to reproduce the crash on the reduced source file. 
> 
> If that makes sense, I can add comments for this-
Eh, I don't have a strong opinion here. My inclination is to prefer a simpler 
reduction script if the difference is len(args) clang invocations, since I 
assume creduce is going to invoke clang way more than len(args) times. I see a 
docstring detailing that `simplify` is only meant to speed things up now, so 
I'm content with the way things are.



Comment at: clang/utils/creduce-clang-crash.py:303
+opts_startswith=["-O"])
+self.clang_args = new_args
+verbose_print("Simplified command:", quote_cmd(self.get_crash_cmd()))

FWIW, opportunistically trying to add `-fsyntax-only` may help here: if the 
crash is in the frontend, it means that non-repros will stop before codegen, 
rather than trying to generate object files, or whatever they were trying to 
generate in the first place.


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

https://reviews.llvm.org/D59725



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

[PATCH] D59725: Additions to creduce script

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!




Comment at: clang/utils/creduce-clang-crash.py:137
+
+# If no message was found, use the top five stack trace functions,
+# ignoring some common functions

Please expand a bit on why 5 was chosen (is there some deep reason behind it, 
or does it just seem like a sensible number?)



Comment at: clang/utils/creduce-clang-crash.py:145
+  matches = re.findall(stacktrace_re, crash_output)
+  result = filter(lambda x: x and x.strip() not in filters, matches)[:5]
+  for msg in result:

nit: please prefer `[x for x in matches if x and x.strip() not in 
filters][:5]`. py3's filter returns a generator, whereas py2's returns a list.



Comment at: clang/utils/creduce-clang-crash.py:193
+# Instead of modifying the filename in the test file, just run the command
+_, empty_file = tempfile.mkstemp()
+if self.check_expected_output(filename=empty_file):

I think `_` is an open file descriptor; please `os.close` it if we don't want 
to use it



Comment at: clang/utils/creduce-clang-crash.py:199
+print("\nTrying to preprocess the source file...")
+_, tmpfile = tempfile.mkstemp()
+

Same nit



Comment at: clang/utils/creduce-clang-crash.py:202
+cmd = self.get_crash_cmd() + ['-E', '-P']
+p = subprocess.Popen(self.get_crash_cmd() + ['-E', '-P'],
+ stdout=subprocess.PIPE,

was this intended to use `cmd`?



Comment at: clang/utils/creduce-clang-crash.py:205
+ stderr=subprocess.STDOUT)
+preprocessed, _ = p.communicate()
+

Do we want to check the exit code of this? Or do we assume that if clang 
crashes during preprocessing, we'll just see a different error during 
`check_expected_output`? (In the latter case, please add a comment)



Comment at: clang/utils/creduce-clang-crash.py:222
+def append_flags(x, y):
+  if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'):
+x[-1] += ' ' + y

Is it intentional to group multiple consecutive non-dashed args? e.g. it seems 
that `clang -ffoo bar baz` will turn into `['clang', '-ffoo bar baz']`





Comment at: clang/utils/creduce-clang-crash.py:223
+  if len(x) > 0 and x[-1].startswith('-') and not y.startswith('-'):
+x[-1] += ' ' + y
+return x

Should we be `shlex.quote`'ing y here (and probably in the `return x + [y]` 
below)?



Comment at: clang/utils/creduce-clang-crash.py:251
+new_args = self.filter_args(args, **kwargs)
+if extra_arg and extra_arg not in new_args:
+  new_args.append(extra_arg)

IMO, if even `extra_arg` is in `new_args`, we should still move it near the 
end. Arg ordering matters in clang, generally with later args taking precedence 
over earlier ones. e.g. the -g$N args in https://godbolt.org/z/Mua8cs



Comment at: clang/utils/creduce-clang-crash.py:279
+ "-debugger-tuning=",
+ "-gdwarf"])
+new_args = self.try_remove_args(new_args,

If we're replacing other args with their effective negation, does it also make 
sense to replace all debug-ish options with `-g0`?



Comment at: clang/utils/creduce-clang-crash.py:296
+new_args = self.try_remove_args(new_args, msg="Removed -D options",
+opts_startswith=["-D "])
+new_args = self.try_remove_args(new_args, msg="Removed -I options",

Might not want to have a space here; `-DFOO=1` is valid (same for `-I` below)



Comment at: clang/utils/creduce-clang-crash.py:306
+# Remove other cases that aren't covered by the heuristic
+new_args = self.try_remove_args(new_args, msg="Removed -mllvm",
+opts_one_arg_startswith=["-mllvm"])

Probably want to do a similar thing for `-Xclang` (which, as far as this code 
is concerned, acts a lot like `-mllvm`)



Comment at: clang/utils/creduce-clang-crash.py:362
+  r = Reduce(crash_script, file_to_reduce)
+  r.simplify_clang_args()
+  r.write_interestingness_test()

I'm unclear on why we do a partial simplification of clang args here, then a 
full reduction after creduce. Is there a disadvantage to instead doing:

r.write_interestingness_test()
r.clang_preprocess()
r.reduce_clang_args()
r.run_creduce()
r.reduce_clang_args()

?

The final `reduce_clang_args` being there to remove extra `-D`/`-I`/etc. args 
if preprocessing failed somehow, to remove `-std=foo` args if those aren't 
relevant after reduction, etc.

The advantage to this being that we no longer 

[PATCH] D59440: add steps to preprocess file and reduce command line args

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356636: creduce-clang-crash.py: preprocess file + reduce 
commandline (authored by gbiv, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59440?vs=191598=191615#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59440

Files:
  utils/creduce-clang-crash.py

Index: utils/creduce-clang-crash.py
===
--- utils/creduce-clang-crash.py
+++ utils/creduce-clang-crash.py
@@ -1,7 +1,5 @@
 #!/usr/bin/env python
 """Calls C-Reduce to create a minimal reproducer for clang crashes.
-
-Requires C-Reduce and not (part of LLVM utils) to be installed.
 """
 
 from argparse import ArgumentParser
@@ -11,108 +9,232 @@
 import sys
 import subprocess
 import pipes
+import shlex
+import tempfile
+import shutil
 from distutils.spawn import find_executable
 
-def create_test(build_script, llvm_not):
+verbose = False
+llvm_bin = None
+creduce_cmd = None
+not_cmd = None
+
+def check_file(fname):
+  if not os.path.isfile(fname):
+sys.exit("ERROR: %s does not exist" % (fname))
+  return fname
+
+def check_cmd(cmd_name, cmd_dir, cmd_path=None):
   """
-  Create an interestingness test from the crash output.
-  Return as a string.
+  Returns absolute path to cmd_path if it is given,
+  or absolute path to cmd_dir/cmd_name.
   """
-  # Get clang call from build script
-  # Assumes the call is the last line of the script
-  with open(build_script) as f:
-cmd = f.readlines()[-1].rstrip('\n\r')
+  if cmd_path:
+cmd = find_executable(cmd_path)
+if cmd:
+  return cmd
+sys.exit("ERROR: executable %s not found" % (cmd_path))
+
+  cmd = find_executable(cmd_name, path=cmd_dir)
+  if cmd:
+return cmd
+  sys.exit("ERROR: %s not found in %s" % (cmd_name, cmd_dir))
+
+def quote_cmd(cmd):
+  return ' '.join(arg if arg.startswith('$') else pipes.quote(arg)
+  for arg in cmd)
+
+def get_crash_cmd(crash_script):
+  with open(crash_script) as f:
+# Assume clang call is on the last line of the script
+line = f.readlines()[-1]
+cmd = shlex.split(line)
+
+# Overwrite the script's clang with the user's clang path
+new_clang = check_cmd('clang', llvm_bin)
+cmd[0] = pipes.quote(new_clang)
+return cmd
 
-  # Get crash output
-  p = subprocess.Popen(build_script,
+def has_expected_output(crash_cmd, expected_output):
+  p = subprocess.Popen(crash_cmd,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT)
   crash_output, _ = p.communicate()
+  return all(msg in crash_output for msg in expected_output)
 
-  output = ['#!/bin/bash']
-  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
-  cmd))
-
-  # Add messages from crash output to the test
-  # If there is an Assertion failure, use that; otherwise use the
-  # last five stack trace functions
+def get_expected_output(crash_cmd):
+  p = subprocess.Popen(crash_cmd,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
+
+  # If there is an assertion failure, use that;
+  # otherwise use the last five stack trace functions
   assertion_re = r'Assertion `([^\']+)\' failed'
   assertion_match = re.search(assertion_re, crash_output)
   if assertion_match:
-msg = assertion_match.group(1)
-output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+return [assertion_match.group(1)]
   else:
 stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
 matches = re.findall(stacktrace_re, crash_output)
-del matches[:-5]
-output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+return matches[-5:]
+
+def write_interestingness_test(testfile, crash_cmd, expected_output,
+   file_to_reduce):
+  filename = os.path.basename(file_to_reduce)
+  if filename not in crash_cmd:
+sys.exit("ERROR: expected %s to be in the crash command" % filename)
+
+  # Replace all instances of file_to_reduce with a command line variable
+  output = ['#!/bin/bash',
+'if [ -z "$1" ] ; then',
+'  f=%s' % (pipes.quote(filename)),
+'else',
+'  f="$1"',
+'fi']
+  cmd = ['$f' if s == filename else s for s in crash_cmd]
+
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(not_cmd),
+  quote_cmd(cmd)))
+
+  for msg in expected_output:
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+
+  with open(testfile, 'w') as f:
+f.write('\n'.join(output))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
 
-  return output
+def check_interestingness(testfile, file_to_reduce):
+  testfile = os.path.abspath(testfile)
+

[PATCH] D59440: add steps to preprocess file and reduce command line args

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

LGTM; thanks again!

Will land shortly


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

https://reviews.llvm.org/D59440



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

[PATCH] D59440: add steps to preprocess file and reduce command line args

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Just a few style nits for you, and this LGTM. I assume rnk and 
serge-sans-paille are content, so I'm happy to check this in for you once these 
are addressed.

Thanks!




Comment at: clang/utils/creduce-clang-crash.py:64
   crash_output, _ = p.communicate()
+  for msg in expected_output:
+if msg not in crash_output:

nit: can be simplified to `return all(msg not in crash_output for msg in 
expected_output)`



Comment at: clang/utils/creduce-clang-crash.py:116
+  with open(os.devnull, 'w') as devnull:
+p = subprocess.Popen(testfile, stdout=devnull)
+p.communicate()

nit: looks like you can use `returncode = subprocess.call(testfile, 
stdout=devnull)` here



Comment at: clang/utils/creduce-clang-crash.py:124
+  with open(os.devnull, 'w') as devnull:
+p = subprocess.Popen([testfile, empty_file], stdout=devnull)
+p.communicate()

same `subprocess.call` nit



Comment at: clang/utils/creduce-clang-crash.py:243
 
+  #FIXME: reduce the clang crash command
+

nit: please add a space: `# FIXME`


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

https://reviews.llvm.org/D59440



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

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

This LGTM; feel free to submit after Aaron stamps this.

Thanks again!




Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+

erik.pilkington wrote:
> george.burgess.iv wrote:
> > Why isn't this in CheckBuiltinFunctionCall?
> For the same reason I added the `bool` parameter to `getBuiltinCallee`, we 
> don't usually consider calls to `__attribute__((overloadable))` be builtins, 
> so we never reach `CheckBuiltinFunctionCall` for them. We're planning on 
> using that attribute though:
> 
> ```
> int sprintf(__attribute__((pass_object_size(_FORTIFY_LEVEL))) char *buffer, 
> const char *format, ...) 
>   __attribute__((overloadable)) 
> __asm__("___sprintf_pass_object_size_chk");
> ```
SGTM


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

https://reviews.llvm.org/D58797



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

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC355944: Add a creduce script for clang crashes (authored by 
gbiv, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D59118?vs=190285=190294#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D59118

Files:
  utils/creduce-clang-crash.py

Index: utils/creduce-clang-crash.py
===
--- utils/creduce-clang-crash.py
+++ utils/creduce-clang-crash.py
@@ -0,0 +1,118 @@
+#!/usr/bin/env python
+"""Calls C-Reduce to create a minimal reproducer for clang crashes.
+
+Requires C-Reduce and not (part of LLVM utils) to be installed.
+"""
+
+from argparse import ArgumentParser
+import os
+import re
+import stat
+import sys
+import subprocess
+import pipes
+from distutils.spawn import find_executable
+
+def create_test(build_script, llvm_not):
+  """
+  Create an interestingness test from the crash output.
+  Return as a string.
+  """
+  # Get clang call from build script
+  # Assumes the call is the last line of the script
+  with open(build_script) as f:
+cmd = f.readlines()[-1].rstrip('\n\r')
+
+  # Get crash output
+  p = subprocess.Popen(build_script,
+   stdout=subprocess.PIPE,
+   stderr=subprocess.STDOUT)
+  crash_output, _ = p.communicate()
+
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (pipes.quote(llvm_not),
+  cmd))
+
+  # Add messages from crash output to the test
+  # If there is an Assertion failure, use that; otherwise use the
+  # last five stack trace functions
+  assertion_re = r'Assertion `([^\']+)\' failed'
+  assertion_match = re.search(assertion_re, crash_output)
+  if assertion_match:
+msg = assertion_match.group(1)
+output.append('grep %s t.log || exit 1' % pipes.quote(msg))
+  else:
+stacktrace_re = r'#[0-9]+\s+0[xX][0-9a-fA-F]+\s*([^(]+)\('
+matches = re.findall(stacktrace_re, crash_output)
+del matches[:-5]
+output += ['grep %s t.log || exit 1' % pipes.quote(msg) for msg in matches]
+
+  return output
+
+def main():
+  parser = ArgumentParser(description=__doc__)
+  parser.add_argument('build_script', type=str, nargs=1,
+  help='Name of the script that generates the crash.')
+  parser.add_argument('file_to_reduce', type=str, nargs=1,
+  help='Name of the file to be reduced.')
+  parser.add_argument('-o', '--output', dest='output', type=str,
+  help='Name of the output file for the reduction. Optional.')
+  parser.add_argument('--llvm-not', dest='llvm_not', type=str,
+  help="The path to the llvm-not executable. "
+  "Required if 'not' is not in PATH environment.");
+  parser.add_argument('--creduce', dest='creduce', type=str,
+  help="The path to the C-Reduce executable. "
+  "Required if 'creduce' is not in PATH environment.");
+  args = parser.parse_args()
+
+  build_script = os.path.abspath(args.build_script[0])
+  file_to_reduce = os.path.abspath(args.file_to_reduce[0])
+  llvm_not = (find_executable(args.llvm_not) if args.llvm_not else
+  find_executable('not'))
+  creduce = (find_executable(args.creduce) if args.creduce else
+ find_executable('creduce'))
+
+  if not os.path.isfile(build_script):
+print(("ERROR: input file '%s' does not exist") % build_script)
+return 1
+
+  if not os.path.isfile(file_to_reduce):
+print(("ERROR: input file '%s' does not exist") % file_to_reduce)
+return 1
+
+  if not llvm_not:
+parser.print_help()
+return 1
+
+  if not creduce:
+parser.print_help()
+return 1
+
+  # Write interestingness test to file
+  test_contents = create_test(build_script, llvm_not)
+  testname, _ = os.path.splitext(file_to_reduce)
+  testfile = testname + '.test.sh'
+  with open(testfile, 'w') as f:
+f.write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)
+
+  # Confirm that the interestingness test passes
+  try:
+with open(os.devnull, 'w') as devnull:
+  subprocess.check_call(testfile, stdout=devnull)
+  except subprocess.CalledProcessError:
+print("For some reason the interestingness test does not return zero")
+return 1
+
+  # FIXME: try running clang preprocessor first
+
+  try:
+p = subprocess.Popen([creduce, testfile, file_to_reduce])
+p.communicate()
+  except KeyboardInterrupt:
+# Hack to kill C-Reduce because it jumps into its own pgid
+print('\n\nctrl-c detected, killed creduce')
+p.kill()
+
+if __name__ == '__main__':
+  sys.exit(main())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

I think that addresses all of the concerns people have put forward; given rnk's 
comment about one more round of fixes, this LGTM. Will check this in for you 
shortly.

Thanks again!




Comment at: clang/utils/creduce-clang-crash.py:1
+#!/usr/bin/env python
+# A tool that calls C-Reduce to create a minimal reproducer for clang crashes

thakis wrote:
> rnk wrote:
> > george.burgess.iv wrote:
> > > nit: please specify a python version here, since the world is steadily 
> > > making `python` == `python3` (if `pipes.quote` is working, I assume 
> > > you'll want `#!/usr/bin/env python2`)
> > Please don't do that, there is no python2.exe or python3.exe on Windows. 
> > `#!/usr/bin/env python` is the simplest thing that could work.
> There's no python2 on mac either. `#!/usr/bin/env python` is the correct 
> first line for executable python scripts.
TIL. Thank you both for the counterexamples :)


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

https://reviews.llvm.org/D59118



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

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> I think we should do one more round of fixes, we can commit that for you, and 
> then move on to the next steps

+1. This looks good to me with outstanding nits fixed

> and filter out the # lines afterwards.

AIUI, the very-recently-merged 
https://github.com/csmith-project/creduce/pull/183 and 
https://github.com/csmith-project/creduce/pull/188 should hopefully handle the 
majority of those?




Comment at: clang/utils/creduce-clang-crash.py:1
+#!/usr/bin/env python
+# A tool that calls C-Reduce to create a minimal reproducer for clang crashes

nit: please specify a python version here, since the world is steadily making 
`python` == `python3` (if `pipes.quote` is working, I assume you'll want 
`#!/usr/bin/env python2`)


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

https://reviews.llvm.org/D59118



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

[PATCH] D59118: creduce script for clang crashes

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! Functionally, this looks good. My comments are mostly just 
simplicity/readability nitpicks.




Comment at: clang/utils/creduce-clang-crash.py:36
+  # Get clang call from build script
+  cmd = None
+  with open(build_script, 'r') as f:

nit: this isn't necessary; `with` doesn't introduce its own scope



Comment at: clang/utils/creduce-clang-crash.py:37
+  cmd = None
+  with open(build_script, 'r') as f:
+cmd = f.read()

nit `, 'r'` is implied; please remove



Comment at: clang/utils/creduce-clang-crash.py:38
+  with open(build_script, 'r') as f:
+cmd = f.read()
+cmd = re.sub("#!.*\n", "", cmd)

Hrm. Looks like there are cases where these crash reproducers are multiple 
lines, though the actually meaningful one (to us) is always the last one? If 
so, it may be cleaner to say `cmd = f.readlines()[-1]` here



Comment at: clang/utils/creduce-clang-crash.py:43
+  # Get crash output
+  p = subprocess.Popen(build_script,
+   stdout=subprocess.PIPE,

nit: can replace with `subprocess.check_output` unless we explicitly want to 
ignore the return value (in which case, we should probably still call `wait()` 
anyway?)



Comment at: clang/utils/creduce-clang-crash.py:49
+  output = ['#!/bin/bash']
+  output.append('%s --crash %s >& t.log || exit 1' % (llvm_not, cmd))
+

please `pipes.quote(llvm_not)` and `pipes.quote(cmd)`



Comment at: clang/utils/creduce-clang-crash.py:54
+  # last five stack trace functions
+  assertion_re = "Assertion \`([^\']+)\' failed"
+  assertion_match = re.search(assertion_re, crash_output)

Why are we escaping the graves and single-quotes?

Also, nit: when possible with regex strings, please use [raw 
strings](https://stackoverflow.com/questions/2081640/what-exactly-do-u-and-r-string-flags-do-and-what-are-raw-string-literals).
 Doing so makes it way easier to reason about what Python's going to transform 
in the string literal before the regex engine gets to see it.



Comment at: clang/utils/creduce-clang-crash.py:58
+msg = assertion_match.group(1).replace('"', '\\"')
+output.append('grep "%s" t.log || exit 1' % msg)
+  else:

nit: please `pipes.quote` instead of adding manual quotes



Comment at: clang/utils/creduce-clang-crash.py:62
+matches = re.findall(stacktrace_re, crash_output)
+del matches[:len(matches)-5]
+output += ['grep "%s" t.log || exit 1' % msg for msg in matches]

nit: please simplify to `del matches[:-5]`



Comment at: clang/utils/creduce-clang-crash.py:63
+del matches[:len(matches)-5]
+output += ['grep "%s" t.log || exit 1' % msg for msg in matches]
+

nit: `pipes.quote` please



Comment at: clang/utils/creduce-clang-crash.py:76
+  parser.add_argument('--llvm-not', dest='llvm_not', type=str,
+  help="""The path to the llvm-not executable.
+  Required if 'not' is not in PATH environment.""");

nit: probably better to concatenate these; otherwise, I think we'll end up with 
a lot of spaces between these sentences? e.g.

```
help="The path to the llvm-not executable. "
 "Required if [...]")
```



Comment at: clang/utils/creduce-clang-crash.py:108
+  testfile = testname + '.test.sh'
+  open(testfile, 'w').write('\n'.join(test_contents))
+  os.chmod(testfile, os.stat(testfile).st_mode | stat.S_IEXEC)

I hate being *that* person, this technically isn't portable Python. I don't 
honestly know if we care about not-CPython being able to run our code, but the 
fix is to simply use `with open(...) as f:` instead of this one-liner.

(Specifically, CPython uses refcounts, so `testfile` will always be closed 
promptly, unless CPython decides someday to make files cyclic with themselves. 
GC'ed python implementations might take a while to flush this file and clean it 
up, which would be bad...)



Comment at: clang/utils/creduce-clang-crash.py:113
+  try:
+p = subprocess.Popen([creduce, testfile, file_to_reduce])
+p.communicate()

Does creduce try and jump out into its own pgid? If not, I think this 
try/except block can be replaced with `sys.exit(subprocess.call([creduce, 
testfile, file_to_reduce]))`



Comment at: clang/utils/creduce-clang-crash.py:120
+
+  sys.exit(0)
+

nit: this is implied; please remove


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59118



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Looks solid to me overall; just a few nits.

I don't think I have actual ownership over any of this code, so I'll defer to 
either Aaron or Richard for the final LGTM

Thanks again!




Comment at: clang/lib/Sema/SemaChecking.cpp:317
+  // buffer size would be.
+  auto computeObjectSize = [&](unsigned ObjIdx) {
+// If the parameter has a pass_object_size attribute, then we should use

nit: I think the prevailing preference is to name lambdas as you'd name 
variables, rather than as you'd name methods/functions



Comment at: clang/lib/Sema/SemaChecking.cpp:321
+// assume type 0.
+int BOSType = 0;
+if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr())

Should this also try to consider `fortify_stdlib`?



Comment at: clang/lib/Sema/SemaChecking.cpp:367
+DiagID = diag::warn_memcpy_chk_overflow;
+if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+!evaluateSizeArg(TheCall->getNumArgs()-2))

nit: All of these cases (and the two lambdas above) look super similar. Might 
it be clearer to just set `SizeIndex` and `ObjectIndex` variables from here, 
and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?

If not, I'm OK with this as-is.



Comment at: clang/lib/Sema/SemaExpr.cpp:5929
 
+checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+

Why isn't this in CheckBuiltinFunctionCall?


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



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

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for working on this!

I hope to take an in-depth look at this patch next week (if someone else 
doesn't beat me to it...), but wanted to note that I think enabling clang to 
emit these warnings on its own is a good thing. `diagnose_if` is great for 
potentially more targeted/implementation-defined things that standard libraries 
want to diagnose, but IMO clang should be able to catch trivially broken code 
like this regardless of the stdlib it's building against.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58797



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

[PATCH] D56760: Add a new builtin: __builtin_dynamic_object_size

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!

> Would it make sense to model this as an (optional) extra flag bit for 
> __builtin_object_size rather than as a new builtin?

+1. That way, we could avoid making a `pass_dynamic_object_size` (assuming we 
wouldn't want to have a different API between that and __builtin_object_size), 
as well. :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D56760



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

[PATCH] D52924: Make __builtin_object_size use the EM_IgnoreSideEffects evaluation mode.

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.

Thanks!


https://reviews.llvm.org/D52924



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

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.

LGTM too.

Thanks again!


Repository:
  rC Clang

https://reviews.llvm.org/D52268



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

[PATCH] D52268: [AST] Squeeze some bits in LinkageComputer

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this! LGTM after erichkeane's comments are resolved.

> I did a little digging on this, and it seems to be to keep track of a 
> declarations linkage for caching sake

Yeah, otherwise, we get exponential behavior on some pathological template-y 
patterns.




Comment at: lib/AST/Linkage.h:93
   static QueryType makeCacheKey(const NamedDecl *ND, LVComputationKind Kind) {
-return std::make_pair(ND, Kind.toBits());
+return QueryType(ND, Kind.toBits());
   }

(FWIW, it looks like `PointerIntPairInfo::UpdateInt` asserts that 
`Kind.toBits()` fits nicely in `NumLVComputationKindBits`. So if anything gets 
added, it'll yell and we can just revert to the current way of doing this :) )


Repository:
  rC Clang

https://reviews.llvm.org/D52268



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

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D47840



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

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL335927: [Parse] Make -Wgcc-compat complain about for loop 
inits in C89 (authored by gbiv, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D47840?vs=150167=153403#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D47840

Files:
  cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
  cfe/trunk/lib/Parse/ParseStmt.cpp
  cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c


Index: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
@@ -173,6 +173,9 @@
 def warn_gcc_attribute_location : Warning<
   "GCC does not allow an attribute in this position on a function 
declaration">, 
   InGroup;
+def warn_gcc_variable_decl_in_for_loop : Warning<
+  "GCC does not allow variable declarations in for loop initializers before "
+  "C99">, InGroup;
 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup;
Index: cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
===
--- cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
+++ cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=gnu89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s -DC99
+
+#ifdef C99
+// expected-no-diagnostics
+#endif
+
+void foo() {
+#ifndef C99
+  // expected-warning@+2{{GCC does not allow variable declarations in for loop 
initializers before C99}}
+#endif
+  for (int i = 0; i < 10; i++)
+;
+}
Index: cfe/trunk/lib/Parse/ParseStmt.cpp
===
--- cfe/trunk/lib/Parse/ParseStmt.cpp
+++ cfe/trunk/lib/Parse/ParseStmt.cpp
@@ -1624,8 +1624,10 @@
 ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
 // Parse declaration, which eats the ';'.
-if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
+if (!C99orCXXorObjC) {   // Use of C99-style for loops in C90 mode?
   Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);
+  Diag(Tok, diag::warn_gcc_variable_decl_in_for_loop);
+}
 
 // In C++0x, "for (T NS:a" might not be a typo for ::
 bool MightBeForRangeStmt = getLangOpts().CPlusPlus;


Index: cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td
@@ -173,6 +173,9 @@
 def warn_gcc_attribute_location : Warning<
   "GCC does not allow an attribute in this position on a function declaration">, 
   InGroup;
+def warn_gcc_variable_decl_in_for_loop : Warning<
+  "GCC does not allow variable declarations in for loop initializers before "
+  "C99">, InGroup;
 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup;
Index: cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
===
--- cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
+++ cfe/trunk/test/Parser/gcc-for-loop-init-compatibility.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=gnu89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s -DC99
+
+#ifdef C99
+// expected-no-diagnostics
+#endif
+
+void foo() {
+#ifndef C99
+  // expected-warning@+2{{GCC does not allow variable declarations in for loop initializers before C99}}
+#endif
+  for (int i = 0; i < 10; i++)
+;
+}
Index: cfe/trunk/lib/Parse/ParseStmt.cpp
===
--- cfe/trunk/lib/Parse/ParseStmt.cpp
+++ cfe/trunk/lib/Parse/ParseStmt.cpp
@@ -1624,8 +1624,10 @@
 ParenBraceBracketBalancer BalancerRAIIObj(*this);
 
 // Parse declaration, which eats the ';'.
-if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
+if (!C99orCXXorObjC) {   // Use of C99-style for loops in C90 mode?
   Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);
+  Diag(Tok, diag::warn_gcc_variable_decl_in_for_loop);
+}
 
 // In C++0x, "for (T NS:a" might not be a typo for ::
 bool MightBeForRangeStmt = getLangOpts().CPlusPlus;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Ping :)


Repository:
  rC Clang

https://reviews.llvm.org/D47840



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

[PATCH] D30760: Record command lines in objects built by clang, Clang part

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv closed this revision.
george.burgess.iv added a comment.
Herald added a subscriber: JDevlieghere.

(Committed as noted by echristo; just trying to clean my queue a bit. :) )


https://reviews.llvm.org/D30760



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

[PATCH] D47840: Make -Wgcc-compat complain about declarations in for loop init statements

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
george.burgess.iv added reviewers: rsmith, aaron.ballman.

The following code is invalid before C99, since we try to declare `i` inside 
the first clause of the for loop:

  void foo() {
for (int i = 0; i < 10; i++);
  }

GCC does not accept this code in c89 or gnu89, but clang does: 
https://godbolt.org/g/ZWr3nA .

If the user cares about GCC compatibility, we should probably warn about this 
if we're not in C99.

I'm not 100% thrilled that we're emitting two warnings about the same thing for 
slightly different reasons; alternatives welcome. :)


Repository:
  rC Clang

https://reviews.llvm.org/D47840

Files:
  include/clang/Basic/DiagnosticParseKinds.td
  lib/Parse/ParseStmt.cpp
  test/Parser/gcc-for-loop-init-compatibility.c


Index: test/Parser/gcc-for-loop-init-compatibility.c
===
--- /dev/null
+++ test/Parser/gcc-for-loop-init-compatibility.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=gnu89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s -DC99
+
+#ifdef C99
+// expected-no-diagnostics
+#endif
+
+void foo() {
+#ifndef C99
+  // expected-warning@+2{{GCC does not allow variable declarations in for loop 
initializers before C99}}
+#endif
+  for (int i = 0; i < 10; i++)
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -1622,8 +1622,10 @@
 ForRange = true;
   } else if (isForInitDeclaration()) {  // for (int X = 4;
 // Parse declaration, which eats the ';'.
-if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
+if (!C99orCXXorObjC) {   // Use of C99-style for loops in C90 mode?
   Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);
+  Diag(Tok, diag::warn_gcc_variable_decl_in_for_loop);
+}
 
 // In C++0x, "for (T NS:a" might not be a typo for ::
 bool MightBeForRangeStmt = getLangOpts().CPlusPlus;
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -173,6 +173,9 @@
 def warn_gcc_attribute_location : Warning<
   "GCC does not allow an attribute in this position on a function 
declaration">, 
   InGroup;
+def warn_gcc_variable_decl_in_for_loop : Warning<
+  "GCC does not allow variable declarations in for loop initializers before "
+  "C99">, InGroup;
 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup;


Index: test/Parser/gcc-for-loop-init-compatibility.c
===
--- /dev/null
+++ test/Parser/gcc-for-loop-init-compatibility.c
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -std=c89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=gnu89 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s -DC99
+
+#ifdef C99
+// expected-no-diagnostics
+#endif
+
+void foo() {
+#ifndef C99
+  // expected-warning@+2{{GCC does not allow variable declarations in for loop initializers before C99}}
+#endif
+  for (int i = 0; i < 10; i++)
+;
+}
Index: lib/Parse/ParseStmt.cpp
===
--- lib/Parse/ParseStmt.cpp
+++ lib/Parse/ParseStmt.cpp
@@ -1622,8 +1622,10 @@
 ForRange = true;
   } else if (isForInitDeclaration()) {  // for (int X = 4;
 // Parse declaration, which eats the ';'.
-if (!C99orCXXorObjC)   // Use of C99-style for loops in C90 mode?
+if (!C99orCXXorObjC) {   // Use of C99-style for loops in C90 mode?
   Diag(Tok, diag::ext_c99_variable_decl_in_for_loop);
+  Diag(Tok, diag::warn_gcc_variable_decl_in_for_loop);
+}
 
 // In C++0x, "for (T NS:a" might not be a typo for ::
 bool MightBeForRangeStmt = getLangOpts().CPlusPlus;
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -173,6 +173,9 @@
 def warn_gcc_attribute_location : Warning<
   "GCC does not allow an attribute in this position on a function declaration">, 
   InGroup;
+def warn_gcc_variable_decl_in_for_loop : Warning<
+  "GCC does not allow variable declarations in for loop initializers before "
+  "C99">, InGroup;
 def warn_attribute_no_decl : Warning<
   "attribute %0 ignored, because it is not attached to a declaration">, 
   InGroup;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL329759: [clang-tidy] Add a 
`android-comparison-in-temp-failure-retry` check (authored by gbiv, committed 
by ).
Herald added subscribers: llvm-commits, klimek.

Changed prior to commit:
  https://reviews.llvm.org/D45059?vs=141768=141914#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D45059

Files:
  clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/android/CMakeLists.txt
  
clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
  clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/test/clang-tidy/android-comparison-in-temp-failure-retry.c

Index: clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
@@ -0,0 +1,84 @@
+//===--- ComparisonInTempFailureRetryCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../utils/Matchers.h"
+#include "ComparisonInTempFailureRetryCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace android {
+
+namespace {
+AST_MATCHER(BinaryOperator, isRHSATempFailureRetryArg) {
+  if (!Node.getLocStart().isMacroID())
+return false;
+
+  const SourceManager  = Finder->getASTContext().getSourceManager();
+  if (!SM.isMacroArgExpansion(Node.getRHS()->IgnoreParenCasts()->getLocStart()))
+return false;
+
+  const LangOptions  = Finder->getASTContext().getLangOpts();
+  SourceLocation LocStart = Node.getLocStart();
+  while (LocStart.isMacroID()) {
+SourceLocation Invocation = SM.getImmediateMacroCallerLoc(LocStart);
+Token Tok;
+if (!Lexer::getRawToken(SM.getSpellingLoc(Invocation), Tok, SM, Opts,
+/*IgnoreWhiteSpace=*/true)) {
+  if (Tok.getKind() == tok::raw_identifier &&
+  Tok.getRawIdentifier() == "TEMP_FAILURE_RETRY")
+return true;
+}
+
+LocStart = Invocation;
+  }
+  return false;
+}
+} // namespace
+
+void ComparisonInTempFailureRetryCheck::registerMatchers(MatchFinder *Finder) {
+  // Both glibc's and Bionic's TEMP_FAILURE_RETRY macros structurally look like:
+  //
+  // #define TEMP_FAILURE_RETRY(x) ({ \
+  //typeof(x) y; \
+  //do y = (x); \
+  //while (y == -1 && errno == EINTR); \
+  //y; \
+  // })
+  //
+  // (glibc uses `long int` instead of `typeof(x)` for the type of y).
+  //
+  // It's unclear how to walk up the AST from inside the expansion of `x`, and
+  // we need to not complain about things like TEMP_FAILURE_RETRY(foo(x == 1)),
+  // so we just match the assignment of `y = (x)` and inspect `x` from there.
+  Finder->addMatcher(
+  binaryOperator(
+  hasOperatorName("="),
+  hasRHS(ignoringParenCasts(
+  binaryOperator(matchers::isComparisonOperator()).bind("binop"))),
+  isRHSATempFailureRetryArg()),
+  this);
+}
+
+void ComparisonInTempFailureRetryCheck::check(
+const MatchFinder::MatchResult ) {
+  const auto  = *Result.Nodes.getNodeAs("binop");
+  diag(BinOp.getOperatorLoc(), "top-level comparison in TEMP_FAILURE_RETRY");
+
+  // FIXME: FixIts would be nice, but potentially nontrivial when nested macros
+  // happen, e.g. `TEMP_FAILURE_RETRY(IS_ZERO(foo()))`
+}
+
+} // namespace android
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/android/AndroidTidyModule.cpp
@@ -22,6 +22,7 @@
 #include "CloexecMemfdCreateCheck.h"
 #include "CloexecOpenCheck.h"
 #include "CloexecSocketCheck.h"
+#include "ComparisonInTempFailureRetryCheck.h"
 
 using namespace clang::ast_matchers;
 
@@ -50,6 +51,8 @@
 "android-cloexec-memfd-create");
 CheckFactories.registerCheck("android-cloexec-open");
 CheckFactories.registerCheck("android-cloexec-socket");
+CheckFactories.registerCheck(
+"android-comparison-in-temp-failure-retry");
   }
 };
 
Index: 

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 141768.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.

Address feedback


https://reviews.llvm.org/D45059

Files:
  clang-tidy/android/AndroidTidyModule.cpp
  clang-tidy/android/CMakeLists.txt
  clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
  clang-tidy/android/ComparisonInTempFailureRetryCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/android-comparison-in-temp-failure-retry.c

Index: test/clang-tidy/android-comparison-in-temp-failure-retry.c
===
--- /dev/null
+++ test/clang-tidy/android-comparison-in-temp-failure-retry.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t
+
+#define TEMP_FAILURE_RETRY(x)  \
+  ({   \
+typeof(x) __z; \
+do \
+  __z = (x);   \
+while (__z == -1); \
+__z;   \
+  })
+
+int foo();
+int bar(int a);
+
+void test() {
+  int i;
+  TEMP_FAILURE_RETRY((i = foo()));
+  TEMP_FAILURE_RETRY(foo());
+  TEMP_FAILURE_RETRY((foo()));
+
+  TEMP_FAILURE_RETRY(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: top-level comparison in TEMP_FAILURE_RETRY [android-comparison-in-temp-failure-retry]
+  TEMP_FAILURE_RETRY((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY((int)(foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  TEMP_FAILURE_RETRY(bar(foo() == 1));
+  TEMP_FAILURE_RETRY((bar(foo() == 1)));
+  TEMP_FAILURE_RETRY((bar(foo() == 1)) == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY(((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define INDIRECT TEMP_FAILURE_RETRY
+  INDIRECT(foo());
+  INDIRECT((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: top-level comparison in TEMP_FAILURE_RETRY
+  INDIRECT(bar(foo() == 1));
+  INDIRECT((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define TFR(x) TEMP_FAILURE_RETRY(x)
+  TFR(foo());
+  TFR((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TFR(bar(foo() == 1));
+  TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define ADD_TFR(x) (1 + TEMP_FAILURE_RETRY(x) + 1)
+  ADD_TFR(foo());
+  ADD_TFR(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  ADD_TFR(bar(foo() == 1));
+  ADD_TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define ADDP_TFR(x) (1 + TEMP_FAILURE_RETRY((x)) + 1)
+  ADDP_TFR(foo());
+  ADDP_TFR((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  ADDP_TFR(bar(foo() == 1));
+  ADDP_TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define MACRO TEMP_FAILURE_RETRY(foo() == 1)
+  MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  // Be sure that being a macro arg doesn't mess with this.
+#define ID(x) (x)
+  ID(ADDP_TFR(bar(foo() == 1)));
+  ID(ADDP_TFR(bar(foo() == 1) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in TEMP_FAILURE_RETRY
+  ID(MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define CMP(x) x == 1
+  TEMP_FAILURE_RETRY(CMP(foo()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: top-level comparison in TEMP_FAILURE_RETRY
+}
+
+// Be sure that it works inside of things like loops, if statements, etc.
+void control_flow() {
+  do {
+if (TEMP_FAILURE_RETRY(foo())) {
+}
+
+if (TEMP_FAILURE_RETRY(foo() == 1)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in TEMP_FAILURE_RETRY
+}
+
+if (TEMP_FAILURE_RETRY(bar(foo() == 1))) {
+}
+
+if (TEMP_FAILURE_RETRY(bar(foo() 

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks!

I plan to commit this tomorrow, to give time for any last-minute comments.

Thanks again to everyone for the review. :)


https://reviews.llvm.org/D45059



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

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 141693.
george.burgess.iv added a comment.

Rebased


https://reviews.llvm.org/D38479

Files:
  docs/UsersManual.rst
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/LangOptions.def
  include/clang/Driver/CC1Options.td
  include/clang/Sema/Sema.h
  lib/Driver/ToolChains/Arch/AArch64.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaExprCXX.cpp
  test/CodeGen/aarch64-mgeneral_regs_only.c
  test/Driver/aarch64-mgeneral_regs_only.c
  test/Sema/aarch64-mgeneral_regs_only.c
  test/SemaCXX/aarch64-mgeneral_regs_only.cpp

Index: test/SemaCXX/aarch64-mgeneral_regs_only.cpp
===
--- /dev/null
+++ test/SemaCXX/aarch64-mgeneral_regs_only.cpp
@@ -0,0 +1,124 @@
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=float -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=int '-DVECATTR=__attribute__((ext_vector_type(2)))' -Wno-unused-value
+// RUN: %clang_cc1 -triple aarch64-linux-eabi -std=c++11 -general-regs-only %s -verify -DBANNED=FloatTypedef -Wno-unused-value
+
+using FloatTypedef = float;
+
+#ifndef VECATTR
+#define VECATTR
+#define BannedToInt(x) (x)
+#else
+#define BannedToInt(x) ((x)[0])
+#endif
+
+typedef BANNED BannedTy VECATTR;
+
+namespace default_args {
+int foo(BannedTy = 0); // expected-error 2{{use of floating-point or vector values is disabled}}
+int bar(int = 1.0);
+
+void baz(int a = foo()); // expected-note{{from use of default argument here}}
+void bazz(int a = bar());
+
+void test() {
+  foo(); // expected-note{{from use of default argument here}}
+  bar();
+  baz(); // expected-note{{from use of default argument here}}
+  baz(4);
+  bazz();
+}
+}
+
+namespace conversions {
+struct ConvertToFloat { explicit operator BannedTy(); };
+struct ConvertToFloatImplicit { /* implicit */ operator BannedTy(); };
+struct ConvertFromFloat { ConvertFromFloat(BannedTy); };
+
+void takeFloat(BannedTy);
+void takeConvertFromFloat(ConvertFromFloat);
+void takeConvertFromFloatRef(const ConvertFromFloat &);
+
+void test() {
+  BannedTy(ConvertToFloat());
+
+  takeFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat(0); // expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat(ConvertToFloatImplicit{}); // expected-error{{use of floating-point or vector values is disabled}}
+
+  ConvertFromFloat{0}; // expected-error{{use of floating-point or vector values is disabled}}
+  ConvertFromFloat{ConvertToFloatImplicit{}}; // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(0); // expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(0); // expected-error{{use of floating-point or vector values is disabled}}
+
+  takeConvertFromFloat(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+  takeConvertFromFloatRef(ConvertFromFloat(0)); // expected-error{{use of floating-point or vector values is disabled}}
+}
+
+
+void takeImplicitFloat(BannedTy = ConvertToFloatImplicit()); // expected-error{{use of floating-point or vector values is disabled}}
+void test2() {
+  takeImplicitFloat(); // expected-note{{from use of default argument here}}
+}
+}
+
+namespace refs {
+  struct BannedRef {
+const BannedTy 
+BannedRef(const BannedTy ): f(f) {}
+  };
+
+  BannedTy ();
+  BannedTy getBannedVal();
+
+  void foo() {
+BannedTy  = getBanned();
+BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+const BannedTy  = getBanned();
+const BannedTy  = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+const int  = 1.0;
+const int  = BannedToInt(getBannedVal()); // expected-error{{use of floating-point or vector values is disabled}}
+
+BannedRef{getBanned()};
+BannedRef{getBannedVal()}; // expected-error{{use of floating-point or vector values is disabled}}
+  }
+}
+
+namespace class_init {
+  struct Foo {
+float f = 1.0; // expected-error{{use of floating-point or vector values is disabled}}
+int i = 1.0;
+float j;
+
+Foo():
+  j(1) // expected-error{{use of floating-point or vector values is disabled}}
+{}
+  };
+}
+
+namespace copy_move_assign {
+  struct Foo { float f; }; // expected-error 2{{use of floating-point or vector values is disabled}}
+
+  void copyFoo(Foo ) {
+Foo a = f; // expected-error{{use of floating-point or vector values is disabled}}
+Foo b(static_cast(f)); // expected-error{{use of floating-point or vector values is disabled}}
+f = a; // expected-note{{in implicit copy assignment operator}}
+f = static_cast(b); // 

[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Hi! It fell off my radar, but I'm happy to put it back on my queue. :)

There's still a few aarch64-specific backend bits I need to fix before this 
patch should go in.


https://reviews.llvm.org/D38479



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

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 141687.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.
Herald added a subscriber: srhines.

Addressed feedback


https://reviews.llvm.org/D45059

Files:
  clang-tidy/android/AndroidTidyModule.cpp
  clang-tidy/android/CMakeLists.txt
  clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
  clang-tidy/android/ComparisonInTempFailureRetryCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/android-comparison-in-temp-failure-retry.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/android-comparison-in-temp-failure-retry.c

Index: test/clang-tidy/android-comparison-in-temp-failure-retry.c
===
--- /dev/null
+++ test/clang-tidy/android-comparison-in-temp-failure-retry.c
@@ -0,0 +1,148 @@
+// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t
+
+#define TEMP_FAILURE_RETRY(x)  \
+  ({   \
+typeof(x) __z; \
+do \
+  __z = (x);   \
+while (__z == -1); \
+__z;   \
+  })
+
+int foo();
+int bar(int a);
+
+void test() {
+  int i;
+  TEMP_FAILURE_RETRY((i = foo()));
+  TEMP_FAILURE_RETRY(foo());
+  TEMP_FAILURE_RETRY((foo()));
+
+  TEMP_FAILURE_RETRY(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: top-level comparison in TEMP_FAILURE_RETRY [android-comparison-in-temp-failure-retry]
+  TEMP_FAILURE_RETRY((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY((int)(foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  TEMP_FAILURE_RETRY(bar(foo() == 1));
+  TEMP_FAILURE_RETRY((bar(foo() == 1)));
+  TEMP_FAILURE_RETRY((bar(foo() == 1)) == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY(((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TEMP_FAILURE_RETRY((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define INDIRECT TEMP_FAILURE_RETRY
+  INDIRECT(foo());
+  INDIRECT((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: top-level comparison in TEMP_FAILURE_RETRY
+  INDIRECT(bar(foo() == 1));
+  INDIRECT((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define TFR(x) TEMP_FAILURE_RETRY(x)
+  TFR(foo());
+  TFR((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: top-level comparison in TEMP_FAILURE_RETRY
+  TFR(bar(foo() == 1));
+  TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define ADD_TFR(x) (1 + TEMP_FAILURE_RETRY(x) + 1)
+  ADD_TFR(foo());
+  ADD_TFR(foo() == 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  ADD_TFR(bar(foo() == 1));
+  ADD_TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define ADDP_TFR(x) (1 + TEMP_FAILURE_RETRY((x)) + 1)
+  ADDP_TFR(foo());
+  ADDP_TFR((foo() == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  ADDP_TFR(bar(foo() == 1));
+  ADDP_TFR((int)((bar(foo() == 1)) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define MACRO TEMP_FAILURE_RETRY(foo() == 1)
+  MACRO;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+  // Be sure that being a macro arg doesn't mess with this.
+#define ID(x) (x)
+  ID(ADDP_TFR(bar(foo() == 1)));
+  ID(ADDP_TFR(bar(foo() == 1) == 1));
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: top-level comparison in TEMP_FAILURE_RETRY
+  ID(MACRO);
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: top-level comparison in TEMP_FAILURE_RETRY
+
+#define CMP(x) x == 1
+  TEMP_FAILURE_RETRY(CMP(foo()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: top-level comparison in TEMP_FAILURE_RETRY
+}
+
+// Be sure that it works inside of things like loops, if statements, etc.
+void control_flow() {
+  do {
+if (TEMP_FAILURE_RETRY(foo())) {
+}
+
+if (TEMP_FAILURE_RETRY(foo() == 1)) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:34: warning: top-level comparison in TEMP_FAILURE_RETRY
+}
+
+if (TEMP_FAILURE_RETRY(bar(foo() == 1))) {
+}
+
+ 

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for the feedback!

> and I suspect that your interest in this check is also related to android?

Yup :)

> Alternatively, if there are other checks specific to the GNU C library 
> planned, we could add a new module for it.

I have nothing planned, so I'm happy with this sitting under `android-`.




Comment at: 
docs/clang-tidy/checks/bugprone-comparison-in-temp-failure-retry.rst:7
+Diagnoses comparisons that appear to be incorrectly placed in the argument to
+the ``TEMP_FAILURE_RETRY`` macro. Having such a use is incorrect in the vast
+majority of cases, and will often silently defeat the purpose of the

alexfh wrote:
> The documentation should provide some context w.r.t what the 
> TEMP_FAILURE_RETRY macro is and where it comes from (maybe also link to its 
> documentation).
Added a paragraph after this one.


https://reviews.llvm.org/D45059



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

[PATCH] D45059: [clang-tidy] Add check to catch comparisons in TEMP_FAILURE_RETRY

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: clang-tidy/bugprone/ComparisonInTempFailureRetryCheck.cpp:78
+
+  diag(RHS.getOperatorLoc(),
+   "Top-level comparisons should be moved out of TEMP_FAILURE_RETRY");

JonasToth wrote:
> You could even provide a fixit to do this. But this can be done in later 
> patches, too.
I initially tried this, but a complete solution appears to be nontrivial when 
nested macros start to happen. Added a FIXME nonetheless. :)



Comment at: test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c:1
+// RUN: %check_clang_tidy %s bugprone-comparison-in-temp-failure-retry %t
+

JonasToth wrote:
> Could you please add a test with control structures, like loops you used in 
> your example?
Added the `control_flow` function



Comment at: test/clang-tidy/bugprone-comparison-in-temp-failure-retry.c:5
+  ({   
\
+typeof(x) __z; 
\
+do 
\

JonasToth wrote:
> I think you could add one test, that shows using `long int` instead of 
> `typeof` is diagnosed, given glibc uses this approach (or even copy there 
> macro)
Added a variant with `long int` near the bottom.


https://reviews.llvm.org/D45059



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