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

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.

(Copy/pasting the reviewer list from https://reviews.llvm.org/D26856.)

Addresses https://bugs.llvm.org/show_bug.cgi?id=30792 .

In GCC, -mgeneral-regs-only emits errors upon trying to emit floating-point or
vector operations that originate from C/C++ (but not inline assembly).
Currently, our behavior is to accept those, but we proceed to "try to form
some new horribly unsupported soft-float ABI."

Additionally, the way that we disable vector/FP ops causes us to crash when
inline assembly uses any vector/FP operations, which is bad.

This patch attempts to address these by:

- making -mgeneral-regs-only behave exactly like -mno-implicit-float to the 
backend, which lets inline assembly use FP regs/operations as much as it wants, 
and
- emitting errors for any floating-point expressions/operations we encounter in 
the frontend.

The latter is the more interesting bit. We want to allow declarations with
floats/vectors as much as possible, but the moment that we actually
try to use a float/vector, we should diagnose it. In less words:

  float f(float a); // OK
  int j = f(1); // Not OK on two points: returns a float, takes a float
  float f(float a) {  // Not OK: defines a function that takes a float and 
returns
  // a float
return 0; // Not OK: 0 is converted to a float.
  }

A trivial implementation of this leaves us with a terrible diagnostic
experience (e.g.

  int r() {
int i = 0, j = 0;
return 1.0 + i + j;
  }

emits many, many diagnostics about implicit float casts, floating adds, etc.
Other kinds of diagnostics, like diagnosing default args, are also very
low-quality), so the majority of this patch is an attempt to handle common
cases more gracefully.

Since the target audience for this is presumably very small, and the cost of not
emitting a diagnostic when we should is *a lot* of debugging, I erred on the
side of simplicity for a lot of this. I think this patch does a reasonably good
job of offering targeted error messages in the majority of cases.

There are a few cases where we'll allow floating-point/vector values to
conceptually be used:

  int i = 1.0 + 1; // OK: guaranteed to fold to an int
  
  float foo();
  int bar(int i = foo()); // OK: just a decl.
  
  int baz() {
int a = bar(1); // OK: we never actually call foo().
int b = bar(); // Error: calling foo().
return a + b;
  }
  
  struct A { float b; };
  
  void qux(struct A *a, struct A *b) {
// OK: we've been codegening @llvm.memcpys for this seemingly since 2012.
// For the moment, this bit is C-only, and very constrained (e.g. 
assignments
// only, rhs must trivially be an lvalue, ...).
*a = *b;
  }

The vibe I got from the bug is that the soft-float incantations we currently
emit when using -mgeneral-regs-only are basically unused, so I'm unsure
if we want a flag/option that lets users flip back to the current
-mgeneral-regs-only behavior. This patch lacks that feature, but I'm happy
to add it if people believe doing so would be valuable.

One final note: this may seem like a problem better solved in CodeGen.
I avoided doing so because that approach had a few downsides:

- how we codegen any expression that might have FP becomes observable,
- checks for this become spread out across many, many places, making it really 
easy to miss a case/forget to add it in a new place (see the above "few users, 
bugs are expensive" point), and
- it seems really difficult to "look upwards" in CodeGen to pattern match these 
into nicer diagnostics, especially in the case of default arguments, etc.


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 V

[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 like the idea of fixing those things, too; I'll start poking them soon. :)

Even if we do end up fixing all of that, I still think it would be good to try 
to diagnose this in the frontend. So, if anyone has comments on this while I'm 
staring at the aarch64 backend, please let me know.




Comment at: test/CodeGen/aarch64-mgeneral_regs_only.c:2
+// RUN: %clang -target aarch64 -mgeneral-regs-only %s -o - -S -emit-llvm | 
FileCheck %s
+// %clang -target aarch64 -mgeneral-regs-only %s -o - -S | FileCheck %s 
--check-prefix=ASM
+

Oops, forgot a `RUN:` here.


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] D38479: Make -mgeneral-regs-only more like GCC's

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> However, the tests cover floating point, but they don't cover vector calls 
> (arm_neon.h).

`#include ` gives me ~12,000 errors, presumably because there are 
so many functions that take vectors/floats defined in it. The hope was that 
calling `bar` and `foo` in aarch64-mgeneral_regs_only.c would cover similar 
cases when the banned type was a vector. Is there another another case you had 
in mind?




Comment at: lib/Sema/SemaExprCXX.cpp:7477
+static bool typeHasFloatingOrVectorComponent(
+QualType Ty, llvm::SmallDenseMap &TypeCache) {
+  if (Ty.isNull())

rengolin wrote:
> The `TypeCache` object seems local.
> 
> It doesn't look like it needs to survive outside of this function, as per its 
> usage and the comment:
> 
> // We may see recursive types in broken code.
> 
> and it just adds another argument passing.
Good point. I just ended up making this conservative in the face of broken 
code; should work just as well, and we don't have to deal with sketchy corner 
cases. :)


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] 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 119499.
george.burgess.iv marked 2 inline comments as done.
george.burgess.iv added a comment.

Addressed feedback. Thanks!

After looking around and internalizing a little bit of how backends in LLVM 
work, the path forward I have in mind is to basically funnel another bit to the 
backend that indicates whether the user specified `-mgeneral-regs-only`. If so, 
that flag will act like `"-crypto,-fp-armv8,-neon"` when we're setting up the 
AArch64 backend (e.g. floats/vectors will still be illegal, vector regclasses 
won't be added, ...). Importantly, this approach wouldn't actually remove the 
aforementioned feature flags, so I think it'll allow our assembler to handle 
non-general ops without any extra effort. We can also query this bit to see if 
we want to diagnose calls where the ABI mandates that vector regs are used. 
Thoughts welcome :)


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 &f;
+BannedRef(const BannedTy &f): f(f) {}
+  };
+
+  BannedTy &getBanned();
+  BannedTy getBannedVal();
+
+  void foo() {
+BannedTy &a = getBanned();
+BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+const BannedTy &c = getBanned();
+const BannedTy &d = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+const int &e = 1.0;
+const int &f = BannedToInt(getBannedVal()); // expected-error{{use of floating-point or vector values is disabled}}
+
+BannedRef{getBanned()};
+BannedRef{getBannedVa

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv updated this revision to Diff 100201.
george.burgess.iv added a comment.
Herald added a subscriber: jfb.

Fix the aforementioned issue; PTAL.

Note that this fix is slightly backwards incompatible. It disallows code like:

  void foo(int);
  void foo(int) __attribute__((overloadable)); // first decl lacked 
overloadable, so this one must lack it, as well.

I chose to do it this way because r64414 wanted to make us reject code like:

  double sin(double) __attribute__((overloadable));
  #include  // error: sin redecl without overloadable

...but it was trivial to get around that by moving the overloadable `sin` decl 
below the include. So, I feel like us accepting the first code snippet is a 
small bug.

I tested this change on ChromeOS, which uses `overloadable` in some parts of 
its C standard library. This backwards incompatibility caused no build 
breakages on that platform.

If we'd rather stay as backwards-compatible as possible, I have no issues with 
tweaking this to allow the first example.


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = &foo;
   void (*ptr2)(char *) = &foo;
-  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *))&foo; // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)

[PATCH] D33904: Add a __has_attribute_enhancement macro to clang

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv created this revision.
Herald added a subscriber: javed.absar.

Attributes may gain features or added flexibility over time. This patch aims to 
add a simple and uniform way to directly add/query for arbitrary changes in 
attributes, instead of having to rely on other information (e.g. version 
numbers, the existence of other attributes added at around the same time as the 
feature you're interested in, ...).

The only user of this at the moment will be https://reviews.llvm.org/D32332, so 
I won't tag people for review here until that lands.

Better words than "enhancement" are welcome; I tried things like 
`__has_attribute_extension` and `__has_attribute_feature`, but we also have 
both `__has_extension` and `__has_feature`, so... :)


https://reviews.llvm.org/D33904

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Attr.td
  include/clang/Basic/Attributes.h
  include/clang/Lex/Preprocessor.h
  lib/Basic/Attributes.cpp
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/has_attribute.c
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -2664,6 +2664,7 @@
 static void GenerateHasAttrSpellingStringSwitch(
 const std::vector &Attrs, raw_ostream &OS,
 const std::string &Variety = "", const std::string &Scope = "") {
+  const StringRef CaseIndent = "";
   for (const auto *Attr : Attrs) {
 // C++11-style attributes have specific version information associated with
 // them. If the attribute has no scope, the version information must not
@@ -2700,15 +2701,35 @@
   // present in the caller.
   Test = "LangOpts.CPlusPlus11";
 
+// Any enhancements must be checked for, as well.
+if (!Test.empty())
+  Test += " && ";
+
+std::vector Enhancements =
+Attr->getValueAsListOfStrings("Enhancements");
+if (Enhancements.empty()) {
+  Test += "!Enhancement";
+} else {
+  Test += "(!Enhancement || llvm::StringSwitch(EnhancementName)";
+
+  std::string Preamble = ("\n" + CaseIndent + CaseIndent).str();
+  for (const auto &Enhancement : Enhancements) {
+assert(Enhancement.find('"') == std::string::npos &&
+   "Quotes aren't allowed in enhancements!");
+Test += (Preamble + ".Case(\"" + Enhancement + "\", true)").str();
+  }
+  Test += Preamble + ".Default(false))";
+}
+
 std::string TestStr =
 !Test.empty() ? Test + " ? " + llvm::itostr(Version) + " : 0" : "1";
 std::vector Spellings = GetFlattenedSpellings(*Attr);
 for (const auto &S : Spellings)
   if (Variety.empty() || (Variety == S.variety() &&
   (Scope.empty() || Scope == S.nameSpace(
-OS << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
+OS << CaseIndent << ".Case(\"" << S.name() << "\", " << TestStr << ")\n";
   }
-  OS << ".Default(0);\n";
+  OS << CaseIndent << ".Default(0);\n";
 }
 
 // Emits the list of spellings for attributes.
Index: test/Preprocessor/has_attribute.c
===
--- test/Preprocessor/has_attribute.c
+++ test/Preprocessor/has_attribute.c
@@ -56,3 +56,59 @@
 
 #if __has_cpp_attribute(selectany) // expected-error {{function-like macro '__has_cpp_attribute' is not defined}}
 #endif
+
+#if !defined(__has_attribute_enhancement)
+#error "No extended has_attribute support?"
+#endif
+
+#if __has_attribute_enhancement(attr_does_not_exist) // expected-error{{too few arguments}}
+#error "We have an enhancement on an attr that doesn't exist?"
+#endif
+
+#if __has_attribute_enhancement(attr_does_not_exist thing) // expected-error{{missing ',' after 'attr_does_not_exist'}}
+#error "We have an enhancement on an attr that doesn't exist?"
+#endif
+
+#if __has_attribute_enhancement(attr_does_not_exist, )
+#error "We have an enhancement on an attr that doesn't exist?"
+#endif
+
+#if __has_attribute_enhancement(always_inline, )
+#error "Empty enhancement exists on always_inline?"
+#endif
+
+#if __has_attribute_enhancement(always_inline, enhancement_does_not_exist)
+#error "Enhancement checks on enhancementless attributes seem broken"
+#endif
+
+#if !__has_attribute(overloadable)
+#error "The following tests depend on having overloadable"
+#endif
+
+#if __has_attribute_enhancement(overloadable,) // expected-error{{expected identifier}}
+#error "Should assume false"
+#endif
+
+#if __has_attribute_enhancement(overloadable, not a valid enhancement) // expected-error{{missing ')' after 'not'}} expected-note{{to match this}}
+#error "Should assume false"
+#endif
+
+#if __has_attribute_enhancement(overloadable, not_an_existing_enhancement)
+#error "Should assume false"
+#endif
+
+#if __has_attribute_enhancement(overloadable, __not_an_existing_enhancement__)
+#error "Should assume false"
+#endif
+
+#if !__has_

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Ping :)

https://reviews.llvm.org/D33904 is what I imagine would be a good way to detect 
this change.


https://reviews.llvm.org/D32332



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

[PATCH] D33904: Add a __has_attribute_enhancement macro to clang

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> Why not just use __has_feature(overloadable_unmarked) or similar?

My impression was that `__has_feature` was was for larger features than tweaks 
to attributes. If this would be an appropriate use of `__has_feature`, though, 
I'm happy to keep things simple.

I'll update the other review with `__has_feature`. If it goes in with that, 
I'll abandon this.

Thanks!




Comment at: docs/LanguageExtensions.rst:176
+For example, clang's ``overloadable`` attribute has existed since before Clang
+3.5, but in Clang 5.0 it gained was modified to support so-called "unmarked
+overloads".  One can use ``__has_attribute_enhancement`` to query whether clang

ahatanak wrote:
> Do you need "gained" here?
Good catch :)


https://reviews.llvm.org/D33904



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

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

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

Swap to using `__has_feature(overloadable_unmarked)` for detection, as 
recommended by Hal in https://reviews.llvm.org/D33904


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = &foo;
   void (*ptr2)(char *) = &foo;
-  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *))&foo; // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))&foo;
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = &disabled;
-  void (*dptr2)(void *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expressi

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

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

Addressed all feedback


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -106,8 +109,8 @@
   void foo(char *c) __attribute__((overloadable));
   void (*ptr1)(void *) = &foo;
   void (*ptr2)(char *) = &foo;
-  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
-  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void (*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an expression of incompatible type ''}} expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}
 
   void (*specific1)(int *) = (void (*)(void *))&foo; // expected-warning{{incompatible function pointer types initializing 'void (*)(int *)' with an expression of type 'void (*)(void *)'}}
   void *specific2 = (void (*)(void *))&foo;
@@ -117,8 +120,8 @@
   void disabled(char *c) __attribute__((overloadable, enable_if(1, "The function name lies.")));
   // To be clear, these should all point to the last overload of 'disabled'
   void (*dptr1)(char *c) = &disabled;
-  void (*dptr2)(void *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(void *)' with an expression of type ''}} expected-note@115{{candidate function made ineligible by enable_if}} expected-note@116{{candidate function made ineligible by enable_if}} expected-note@117{{candidate function has type mismatch at 1st parameter (expected 'void *' but has 'char *')}}
-  void (*dptr3)(int *c) = &disabled; // expected-warning{{incompatible pointer types initializing 'void (*)(int *)' with an expression of type ''}} expected-note@115{{candidate functi

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

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

Addressed all feedback


https://reviews.llvm.org/D32332

Files:
  include/clang/Basic/AttrDocs.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Lex/PPMacroExpansion.cpp
  lib/Sema/SemaDecl.cpp
  test/CodeGen/mangle-ms.c
  test/CodeGen/mangle.c
  test/CodeGenCXX/mangle-ms.cpp
  test/PCH/attrs.c
  test/Sema/overloadable.c

Index: test/Sema/overloadable.c
===
--- test/Sema/overloadable.c
+++ test/Sema/overloadable.c
@@ -3,12 +3,15 @@
 int var __attribute__((overloadable)); // expected-error{{'overloadable' attribute only applies to functions}}
 void params(void) __attribute__((overloadable(12))); // expected-error {{'overloadable' attribute takes no arguments}}
 
-int *f(int) __attribute__((overloadable)); // expected-note 2{{previous overload of function is here}}
-float *f(float); // expected-error{{overloaded function 'f' must have the 'overloadable' attribute}}
+int *f(int) __attribute__((overloadable)); // expected-note{{previous overload of function is here}}
+float *f(float);
 int *f(int); // expected-error{{redeclaration of 'f' must have the 'overloadable' attribute}} \
  // expected-note{{previous declaration is here}}
 double *f(double) __attribute__((overloadable)); // okay, new
 
+// Ensure we don't complain about overloadable on implicitly declared functions.
+int isdigit(int) __attribute__((overloadable));
+
 void test_f(int iv, float fv, double dv) {
   int *ip = f(iv);
   float *fp = f(fv);
@@ -71,19 +74,19 @@
   f1();
 }
 
-void before_local_1(int) __attribute__((overloadable)); // expected-note {{here}}
+void before_local_1(int) __attribute__((overloadable));
 void before_local_2(int); // expected-note {{here}}
 void before_local_3(int) __attribute__((overloadable));
 void local() {
-  void before_local_1(char); // expected-error {{must have the 'overloadable' attribute}}
-  void before_local_2(char) __attribute__((overloadable)); // expected-error {{conflicting types}}
+  void before_local_1(char);
+  void before_local_2(char); // expected-error {{conflicting types}}
   void before_local_3(char) __attribute__((overloadable));
-  void after_local_1(char); // expected-note {{here}}
-  void after_local_2(char) __attribute__((overloadable)); // expected-note {{here}}
+  void after_local_1(char);
+  void after_local_2(char) __attribute__((overloadable));
   void after_local_3(char) __attribute__((overloadable));
 }
-void after_local_1(int) __attribute__((overloadable)); // expected-error {{conflicting types}}
-void after_local_2(int); // expected-error {{must have the 'overloadable' attribute}}
+void after_local_1(int) __attribute__((overloadable));
+void after_local_2(int);
 void after_local_3(int) __attribute__((overloadable));
 
 // Make sure we allow C-specific conversions in C.
@@ -152,6 +155,85 @@
   foo(vcharbuf); // expected-error{{call to 'foo' is ambiguous}} expected-note@-4{{candidate function}} expected-note@-3{{candidate function}}
 }
 
+void overloadable_with_global() {
+  void wg_foo(void) __attribute__((overloadable)); // expected-note{{previous}}
+  void wg_foo(int) __attribute__((overloadable));
+}
+
+int wg_foo; // expected-error{{redefinition of 'wg_foo' as different kind of symbol}}
+
+#if !__has_extension(overloadable_unmarked)
+#error "We should have unmarked overload support"
+#endif
+
+void to_foo0(int);
+void to_foo0(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo0(int);
+void to_foo0(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo0(int);
+
+void to_foo1(int) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo1(double);
+void to_foo1(int); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo2(int); // expected-note{{previous unmarked overload}}
+void to_foo2(double) __attribute__((overloadable)); // expected-note 2{{previous overload}}
+void to_foo2(int) __attribute__((overloadable)); // expected-error {{must not have the 'overloadable' attribute}}
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+void to_foo2(double); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo2(int);
+
+void to_foo3(int);
+void to_foo3(double) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo3(int);
+void to_foo3(double); // expected-error{{must have the 'overloadable' attribute}}
+
+void to_foo4(int) __attribute__((overloadable)); // expected-note{{previous overload}}
+void to_foo4(int); // expected-error{{must have the 'overloadable' attribute}}
+void to_foo4(double) __attribute__((overloadable));
+
+void to_foo5(int);
+void to_foo5(int

[PATCH] D32332: Add support for transparent overloadable functions in clang

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thank you both for the comments!




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:3294-3295
+def err_attribute_overloadable_multiple_unmarked_overloads : Error<
+  "at most one 'overloadable' function for a given name may lack the "
+  "'overloadable' attribute">;
 def warn_ns_attribute_wrong_return_type : Warning<

rsmith wrote:
> I think calling the functions 'overloadable' in this case is confusing.
Yeah, I couldn't think of a good name, either. Is this better?



Comment at: lib/Lex/PPMacroExpansion.cpp:1136
   .Case("nullability_on_arrays", true)
+  .Case("overloadable_unmarked", true)
   .Case("memory_sanitizer", LangOpts.Sanitize.has(SanitizerKind::Memory))

rsmith wrote:
> Should this be `__has_extension` rather than `__has_feature`, since it's not 
> a standard feature? (Per 
> http://clang.llvm.org/docs/LanguageExtensions.html#has-feature-and-has-extension
>  `__has_feature` is only for detecting standard features, despite us using it 
> for other things.)
Ah, I missed the "For backward compatibility" bit. Good call



Comment at: lib/Sema/SemaDecl.cpp:9225
+/*NewIsUsingDecl=*/false)) {
+  case Sema::Ovl_Match:
+  case Sema::Ovl_NonFunction:

aaron.ballman wrote:
> Why add `Sema::` to these?
Artifact of me moving code around and then putting it back. :)



Comment at: lib/Sema/SemaDecl.cpp:9249-9252
+case Ovl_Match:
+  // We're not guaranteed to be handed the most recent decl, and
+  // __attribute__((overloadable)) depends somewhat on source order.
+  OldDecl = OldDecl->getMostRecentDecl();

rsmith wrote:
> `checkForConflictWithnonVisibleExternC` should probably be responsible for 
> finding the most recent declaration of the entity that is actually visible 
> (and not, say, a block-scope declaration in some unrelated function, or a 
> declaration in an unimported module).
Isn't part of the goal of `checkForConflictWithNonVisibleExternC` to report 
things that (potentially) aren't visible? If so, I don't see how it can also be 
its responsibility to always report things that are visible, unless it's a 
best-effort sort of thing.

In any case, this bit of code boils down to me trying to slip a bugfix in here. 
Since I think fixing this properly will just pollute this diff even more, I'll 
back it out and try again in a separate patch. :)



Comment at: lib/Sema/SemaDecl.cpp:9375-9381
+assert((Previous.empty() ||
+llvm::any_of(
+Previous,
+[](const NamedDecl *ND) {
+  return ND->getMostRecentDecl()->hasAttr();
+})) &&
+   "Non-redecls shouldn't happen without overloadable present");

rsmith wrote:
> Seems worth factoring out this "contains any overloadable functions" check, 
> since it's quite a few lines and you're doing it in two different places.
With the removal of one unnecessary change (see a later response), this is no 
longer repeated. Happy to still pull it out into a function if you think that 
helps readability.



Comment at: test/Sema/overloadable.c:112
   void (*ptr2)(char *) = &foo;
-  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@105{{candidate function}} expected-note@106{{candidate 
function}}
-  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@105{{candidate function}} expected-note@106{{candidate function}}
+  void (*ambiguous)(int *) = &foo; // expected-error{{initializing 'void 
(*)(int *)' with an expression of incompatible type ''}} expected-note@-4{{candidate function}} expected-note@-3{{candidate 
function}}
+  void *vp_ambiguous = &foo; // expected-error{{initializing 'void *' with an 
expression of incompatible type ''}} 
expected-note@-5{{candidate function}} expected-note@-4{{candidate function}}

rsmith wrote:
> It'd be nice to check in these changes from @NNN -> @-M separately.
r305947.


https://reviews.llvm.org/D32332



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

[PATCH] D41261: [libcxxabi][demangler] Special case demangling for pass_object_size attribute

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for working on this!

Unfortunately, I've never touched demanglers before, so I don't think I can 
LGTM this patch.

> This attribute is mangled as if it was meant to match the 
>  production.

Yup, definitely a mistake. Apologies. :)




Comment at: test/test_demangle.pass.cpp:29618
+{"_ZZN7lambdas7LambdasEPcENK3$_0clEPvU17pass_object_size0", 
"lambdas::Lambdas(char*)::$_0::operator()(pass_object_size0 void*) const"},
+{"_ZN8variadic6AsCtorC1EPKcU17pass_object_size0dz", 
"variadic::AsCtor::AsCtor(pass_object_size0 char const*, double, ...)"},
 };

Nit: Can we have a test with pass_object_size(1) (or 2, or 3. Any of the 3 
WFM), as well?


Repository:
  rCXXA libc++abi

https://reviews.llvm.org/D41261



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

[PATCH] D41405: Fix an assertion failure regression in isDesignatorAtObjectEnd for __builtin_object_size with incomplete array type in struct

[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 assuming my nit gets addressed.

Thank you!

> Maybe someone who's more familiar with this builtin could point to the cause 
> of this discrepancy

Yeah, the documentation for this builtin is pretty sparse. My guess: clang 
assumes that the array's size is unknown because it's both an array and at the 
end of a struct. This exists because code will do clever things like

  struct string {
size_t len;
char buf[1]; // actual string is accessed from here; `string` just gets 
overallocated to hold it all.
  };

in FreeBSD-land, they also recommend overallocation with sockaddrs, which have 
a 14-length trailing element: 
https://www.freebsd.org/doc/en/books/developers-handbook/sockets-essential-functions.html

...So, the best compatible heuristic we've been able to settle on here, sadly, 
is "are we touching the final element in a struct, and is that element an 
array?" On the bright side, clang failing just means LLVM gets to try to figure 
it out, so only some hope of getting a useful answer is lost. :)

It's interesting that GCC tries to do better here, since AIUI it has a similar 
heuristic meant to cope with code like the above.




Comment at: test/Sema/builtin-object-size.c:95
+
+typedef struct  {
+  char string[512];

Please clang-format the additions to this file.


Repository:
  rC Clang

https://reviews.llvm.org/D41405



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

[PATCH] D41405: Fix an assertion failure regression in isDesignatorAtObjectEnd for __builtin_object_size with incomplete array type in struct

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added inline comments.



Comment at: test/Sema/builtin-object-size.c:105
+void rd36094951_IAS_builtin_object_size_assertion(IncompleteArrayStruct* p) {
+   __builtin___strlcpy_chk (p->session[0].string, "ab", 2, 
__builtin_object_size(p->session[0].string, 1));
+}

vsapsai wrote:
> Do we execute significantly different code paths when the second 
> `__builtin_object_size` argument is 0, 2, 3? I think it is worth checking 
> locally if these values aren't causing an assertion. Not sure about having 
> such tests permanently, I'll leave it to you as you are more familiar with 
> the code.
In this case, only 1 and 3 should be touching the buggy codepath, and they 
should execute it identically. If 0 and 2 reach there, we have bigger problems, 
since they shouldn't really be poking around in the designator of the given 
LValue.

Since it's presumably only ~10 seconds of copy-pasting, I'd be happy if we 
added sanity checks for other modes, as well. :)


Repository:
  rC Clang

https://reviews.llvm.org/D41405



___
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] 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] 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] 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] 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] 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] 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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[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 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]

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&id=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] 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] 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] 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]

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&id=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] 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 nee

[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] 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] 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] 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] 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&id=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)
-  os.r

[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 &Feature : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto &Feature : 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 &Feature : 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] 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] 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] 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] 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&id=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 &Result) 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 &Result) {
+  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: clang-tools-extra/tru

[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&id=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 &Result) {
+  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] 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] 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&id=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 &Feature : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto &Feature : 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 &Feature : 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 &FeaturesVec) 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 &Features,
 DiagnosticsEngine &Diags) override;
 
___
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] 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 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:
> > 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
cfe-commits@l

[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&id=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:
+Diag(Th

[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 &Result) {

nit: please clang-format



Comment at: clang-tools-extra/clang-tidy/android/PosixReturnCheck.cpp:39
+  const auto &BinOp = *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] 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 &Result) {
+  const std::string &ReplacementText =
+  (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] 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 &Result) {
+  const std::string &ReplacementText =
+  (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] 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] 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 &f;
+  BannedRef(const BannedTy &f) : f(f) {}
+};
+
+BannedTy &getBanned();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy &a = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy &c = getBanned();
+  const BannedTy &d = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+  const int &e = 1.0;
+  const int &f = 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 &f) {
+  Foo a 

[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 &FeaturesVec) 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 &Features,
 DiagnosticsEngine &Diags) 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 &Feature : UpdatedFeaturesVec) {
-if (Feature.compare("+arm") == 0)
-  Feature = "-thumb-mode";
-else if (Feature.compare("+thumb") == 0)
-  Feature = "+thumb-mode";
+  std::vector UpdatedFeaturesVec;
+  for (const auto &Feature : 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 &Feature : 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] 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] 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 &f;
+  BannedRef(const BannedTy &f) : f(f) {}
+};
+
+BannedTy &getBanned();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy &a = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy &c = getBanned();
+  const BannedTy &d = getBannedVal(); // expected-error{{use of floating-point or vector values is disabled}}
+
+  const int &e = 1.0;
+  const int &f = 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 d

[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 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 &f;
+  BannedRef(const BannedTy &f) : f(f) {}
+};
+
+BannedTy &getBanned();
+BannedTy getBannedVal();
+
+void foo() {
+  BannedTy &a = getBanned();
+  BannedTy b = getBanned(); // expected-error{{use of floating-point or vector values is disabled}}
+  const BannedTy &c = getBanned();
+  const Ban

[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 = ConvertT

[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] 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] 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&id=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] 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] 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 &BI;
-FunctionIsDirectlyRecursive(StringRef N, const Builtin::Context &C)
-: Name(N), BI(C) {}
+FunctionIsDirectlyRecursive(const FunctionDecl *OriginalFD, StringRef N,
+const Builtin::Context &C)
+: 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 &BI;
-FunctionIsDirectlyRecursive(StringRef N, const Builtin::Context &C)
-: Name(N), BI(C) {}
+FunctionIsDirectlyRecursive(const FunctionDecl *OriginalFD, StringRef N,
+const Builtin::Context &C)
+: 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-com

[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] 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] 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]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2dd17ff08165: [CodeGen] only add nobuiltin to inline 
builtins if we'll 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] 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] D40940: [ubsan] Use pass_object_size info in bounds checks

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for this!

It's interesting to me that these array-bound checks don't seem to use 
`@llvm.objectsize` in some form already. I can't find any notes about it 
grepping through git/source, so I'm happy with it.




Comment at: lib/CodeGen/CGExpr.cpp:819
+   QualType EltTy) {
+  auto *DRE = dyn_cast(E->IgnoreImpCasts());
+  if (!DRE)

nit: would `IgnoreParenImpCasts` be better?



Comment at: lib/CodeGen/CGExpr.cpp:828
+  // Find the implicit size parameter.
+  auto SizeDeclIt = SizeArguments.find(PVD);
+  if (SizeDeclIt == SizeArguments.end())

We should probably only do this if the argument to `pass_object_size` is `0` or 
`1`. `2` and `3` give lower-bounds on size (and a default value of 0), which 
could result in false-positives.

(And please add a test that we don't do this for 2 or 3.)


https://reviews.llvm.org/D40940



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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

[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!


https://reviews.llvm.org/D40940



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

[PATCH] D40941: [ubsan] Use pass_object_size info in bounds checks (compiler-rt)

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

If the answer to my question is "no, it'll just work," LGTM. Thanks!




Comment at: test/ubsan/TestCases/Misc/bounds.cpp:9
+int get_int(int *const p __attribute__((pass_object_size(0))), int i) {
+  // CHECK-A-2: bounds.cpp:[[@LINE+1]]:10: runtime error: index 2 out of 
bounds for type 'int *'
+  return p[i];

Do we need extra `RUN:` lines/CHECK prefixes for these? I thought ubsan runtime 
errors terminated the program.


https://reviews.llvm.org/D40941



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

[PATCH] D40940: [ubsan] Use pass_object_size info in bounds checks

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

> That said, one of the upsides of the current ubsan is that whether it will 
> produce a diagnostic is predictable (as long as you don't use uninitialized 
> data); you lose that to some extent with llvm.objectsize because it depends 
> on the optimizer.

True. If that's not desirable to have in `array-bounds`, we could potentially 
move these checks under `-fsanitize=object-size` instead. We'd just have to be 
careful about not emitting `object-size` and `array-bounds` checks for the same 
array access.


https://reviews.llvm.org/D40940



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://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 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] 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 &TI = getASTContext().g

[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 &Idents, const Token &ProtoToken) 
{


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 &Idents, const Token &ProtoToken) {
___
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] 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 &Opts) override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  const std::string RawRetryList;
+  SmallVector RetryMacros;
 };
 
 } // namespace android
Index: clang-tools-extra/clang-tidy/android/ComparisonInTempFailureRetryCheck.cpp
=

[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(&GI); // expected-warning {{attempt to call free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+free(&SI); // expected-warning {{attempt to call free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+free(&I); // expected-warning {{attempt to call free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = &I;
+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(&A); // expected-warning {{attempt to call free on non-heap object 'A'}}
+  }
+  {
+S s;
+free(s);
+free(&s); // expected-warning {{attempt to call free on non-heap object 's'}}
+  }
+}
+
+void test2() {
+  {
+std::free(&GI); // expected-warning {{attempt to call std::free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+std::free(&SI); // expected-warning {{attempt to call std::free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+std::free(&I); // expected-warning {{attempt to call std::free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = &I;
+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(&A); // expected-warning {{attempt to call std::free on non-heap object 'A'}}
+  }
+  {
+S s;
+std::free(s);
+std::free(&s); // 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(&GI); // expected-warning {{attempt to call free on non-heap object 'GI'}}
+  }
+  {
+static int SI = 0;
+free(&SI); // expected-warning {{attempt to call free on non-heap object 'SI'}}
+  }
+  {
+int I = 0;
+free(&I); // expected-warning {{attempt to call free on non-heap object 'I'}}
+  }
+  {
+int I = 0;
+int *P = &I;
+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(&A); // 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);
+bre

[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(&ptr); // expected-warning {{attempt to call free on non-heap object 'ptr'}}
+::free(&I);   // expected-warning {{attempt to call free on non-heap object 'I'}}
+::free(ptr);
+  }
+
+  void CXXFree() {
+std::free(&ptr); // expected-warning {{attempt to call std::free on non-heap object 'ptr'}}
+std::free(&I);   // 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(&GI); // expected-warning {{attempt to call free on non-heap object 'GI'}}
@@ -51,6 +66,10 @@
 free(s);
 free(&s); // 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(&s); // 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(&A); // expected-warning {{attempt to call free on non-heap object 'A'}}
   }
+  {
+struct S s;
+free(&s.I); // 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 &S, const std::string &CalleeName,
- 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 &S, const std::string &CalleeName,
+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 &S, const std::string &CalleeName,
+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 &S, const std::string &CalleeName,
+ 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 &S, const std::string &CalleeName,
   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] 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) {
+  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 -std

[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] 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] 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&id=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() {
+  my_hea

[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 &Result, ASTContext &Ctx) 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] 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] 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] 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 &Idents, const Token &ProtoToken) 
{


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 &Idents, const Token &ProtoToken) {
___
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] 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] 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] 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 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.

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 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