Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

thakis closed this revision.
thakis added a comment.

r258128


http://reviews.llvm.org/D15866



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

Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

thakis updated this revision to Diff 44958.
thakis added a comment.

For function-type macros, make the warning only show up with -pedantic


http://reviews.llvm.org/D15866

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPExpressions.cpp
  test/Lexer/cxx-features.cpp
  test/Preprocessor/expr_define_expansion.c

Index: test/Lexer/cxx-features.cpp
===
--- test/Lexer/cxx-features.cpp
+++ test/Lexer/cxx-features.cpp
@@ -6,6 +6,7 @@
 
 // expected-no-diagnostics
 
+// FIXME using `defined` in a macro has undefined behavior.
 #if __cplusplus < 201103L
 #define check(macro, cxx98, cxx11, cxx1y) cxx98 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx98
 #elif __cplusplus < 201304L
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -82,6 +82,52 @@
 static bool EvaluateDefined(PPValue , Token , DefinedTracker ,
 bool ValueLive, Preprocessor ) {
   SourceLocation beginLoc(PeekTok.getLocation());
+
+  // [cpp.cond]p4:
+  //   Prior to evaluation, macro invocations in the list of preprocessing
+  //   tokens that will become the controlling constant expression are replaced
+  //   (except for those macro names modified by the 'defined' unary operator),
+  //   just as in normal text. If the token 'defined' is generated as a result
+  //   of this replacement process or use of the 'defined' unary operator does
+  //   not match one of the two specified forms prior to macro replacement, the
+  //   behavior is undefined.
+  // This isn't an idle threat, consider this program:
+  //   #define FOO
+  //   #define BAR defined(FOO)
+  //   #if BAR
+  //   ...
+  //   #else
+  //   ...
+  //   #endif
+  // clang and gcc will pick the #if branch while Visual Studio will take the
+  // #else branch.  Emit a warning about this undefined behavior.
+  if (beginLoc.isMacroID()) {
+bool IsFunctionTypeMacro =
+PP.getSourceManager()
+.getSLocEntry(PP.getSourceManager().getFileID(beginLoc))
+.getExpansion()
+.isFunctionMacroExpansion();
+// For object-type macros, it's easy to replace
+//   #define FOO defined(BAR)
+// with
+//   #if defined(BAR)
+//   #define FOO 1
+//   #else
+//   #define FOO 0
+//   #endif
+// and doing so makes sense since compilers handle this differently in
+// practice (see example further up).  But for function-type macros,
+// there is no good way to write
+//   # define FOO(x) (defined(M_ ## x) && M_ ## x)
+// in a different way, and compilers seem to agree on how to behave here.
+// So warn by default on object-type macros, but only warn in -pedantic
+// mode on function-type macros.
+if (IsFunctionTypeMacro)
+  PP.Diag(beginLoc, diag::warn_defined_in_function_type_macro);
+else
+  PP.Diag(beginLoc, diag::warn_defined_in_object_type_macro);
+  }
+
   Result.setBegin(beginLoc);
 
   // Get the next token, don't expand it.
@@ -177,8 +223,8 @@
   if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
 // Handle "defined X" and "defined(X)".
 if (II->isStr("defined"))
-  return(EvaluateDefined(Result, PeekTok, DT, ValueLive, PP));
-
+  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
+
 // If this identifier isn't 'defined' or one of the special
 // preprocessor keywords and it wasn't macro expanded, it turns
 // into a simple 0, unless it is the C++ keyword "true", in which case it
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -658,6 +658,13 @@
 def note_header_guard : Note<
   "%0 is defined here; did you mean %1?">;
 
+def warn_defined_in_object_type_macro : Warning<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup;
+def warn_defined_in_function_type_macro : Extension<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup;
+
 let CategoryName = "Nullability Issue" in {
 
 def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -204,6 +204,7 @@
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
+def ExpansionToUndefined : DiagGroup<"expansion-to-undefined">;
 def FlagEnum : DiagGroup<"flag-enum">;
 def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>;
 def InfiniteRecursion : DiagGroup<"infinite-recursion">;

Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

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

Also addressed your comment and added a test for that.


http://reviews.llvm.org/D15866



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

Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

thakis updated this revision to Diff 44959.
thakis added a comment.

Address review comment.


http://reviews.llvm.org/D15866

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPExpressions.cpp
  test/Lexer/cxx-features.cpp
  test/Preprocessor/expr_define_expansion.c

Index: test/Lexer/cxx-features.cpp
===
--- test/Lexer/cxx-features.cpp
+++ test/Lexer/cxx-features.cpp
@@ -6,6 +6,7 @@
 
 // expected-no-diagnostics
 
+// FIXME using `defined` in a macro has undefined behavior.
 #if __cplusplus < 201103L
 #define check(macro, cxx98, cxx11, cxx1y) cxx98 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx98
 #elif __cplusplus < 201304L
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -82,6 +82,7 @@
 static bool EvaluateDefined(PPValue , Token , DefinedTracker ,
 bool ValueLive, Preprocessor ) {
   SourceLocation beginLoc(PeekTok.getLocation());
+
   Result.setBegin(beginLoc);
 
   // Get the next token, don't expand it.
@@ -140,6 +141,51 @@
 PP.LexNonComment(PeekTok);
   }
 
+  // [cpp.cond]p4:
+  //   Prior to evaluation, macro invocations in the list of preprocessing
+  //   tokens that will become the controlling constant expression are replaced
+  //   (except for those macro names modified by the 'defined' unary operator),
+  //   just as in normal text. If the token 'defined' is generated as a result
+  //   of this replacement process or use of the 'defined' unary operator does
+  //   not match one of the two specified forms prior to macro replacement, the
+  //   behavior is undefined.
+  // This isn't an idle threat, consider this program:
+  //   #define FOO
+  //   #define BAR defined(FOO)
+  //   #if BAR
+  //   ...
+  //   #else
+  //   ...
+  //   #endif
+  // clang and gcc will pick the #if branch while Visual Studio will take the
+  // #else branch.  Emit a warning about this undefined behavior.
+  if (beginLoc.isMacroID()) {
+bool IsFunctionTypeMacro =
+PP.getSourceManager()
+.getSLocEntry(PP.getSourceManager().getFileID(beginLoc))
+.getExpansion()
+.isFunctionMacroExpansion();
+// For object-type macros, it's easy to replace
+//   #define FOO defined(BAR)
+// with
+//   #if defined(BAR)
+//   #define FOO 1
+//   #else
+//   #define FOO 0
+//   #endif
+// and doing so makes sense since compilers handle this differently in
+// practice (see example further up).  But for function-type macros,
+// there is no good way to write
+//   # define FOO(x) (defined(M_ ## x) && M_ ## x)
+// in a different way, and compilers seem to agree on how to behave here.
+// So warn by default on object-type macros, but only warn in -pedantic
+// mode on function-type macros.
+if (IsFunctionTypeMacro)
+  PP.Diag(beginLoc, diag::warn_defined_in_function_type_macro);
+else
+  PP.Diag(beginLoc, diag::warn_defined_in_object_type_macro);
+  }
+
   // Invoke the 'defined' callback.
   if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {
 Callbacks->Defined(macroToken, Macro,
@@ -177,8 +223,8 @@
   if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
 // Handle "defined X" and "defined(X)".
 if (II->isStr("defined"))
-  return(EvaluateDefined(Result, PeekTok, DT, ValueLive, PP));
-
+  return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
+
 // If this identifier isn't 'defined' or one of the special
 // preprocessor keywords and it wasn't macro expanded, it turns
 // into a simple 0, unless it is the C++ keyword "true", in which case it
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -658,6 +658,13 @@
 def note_header_guard : Note<
   "%0 is defined here; did you mean %1?">;
 
+def warn_defined_in_object_type_macro : Warning<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup;
+def warn_defined_in_function_type_macro : Extension<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup;
+
 let CategoryName = "Nullability Issue" in {
 
 def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -204,6 +204,7 @@
 def DanglingElse: DiagGroup<"dangling-else">;
 def DanglingField : DiagGroup<"dangling-field">;
 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
+def ExpansionToUndefined : DiagGroup<"expansion-to-undefined">;
 def FlagEnum : 

Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

Thanks for the review!

I tweaked it a bit so that this only warns on function-type macros in
-pedantic mode. That way, the warning can even be used in practice :-)

On Thu, Jan 14, 2016 at 10:33 PM, Nico Weber  wrote:

> thakis updated this revision to Diff 44958.
> thakis added a comment.
>
> For function-type macros, make the warning only show up with -pedantic
>
>
> http://reviews.llvm.org/D15866
>
> Files:
>   include/clang/Basic/DiagnosticGroups.td
>   include/clang/Basic/DiagnosticLexKinds.td
>   lib/Lex/PPExpressions.cpp
>   test/Lexer/cxx-features.cpp
>   test/Preprocessor/expr_define_expansion.c
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15866: Warn when using `defined` in a macro definition.

[Richard Smith via cfe-commits]

rsmith accepted this revision.
This revision is now accepted and ready to land.


Comment at: lib/Lex/PPExpressions.cpp:104-105
@@ +103,4 @@
+  // #else branch.  Emit a warning about this undefined behavior.
+  if (beginLoc.isMacroID())
+PP.Diag(beginLoc, diag::warn_defined_in_macro);
+

Move this down to the end of the function, after we've checked that we have a 
syntactically valid `defined` operator, to avoid duplicate diagnostics on a 
case like:

  #define FOO defined(
  #if FOO


http://reviews.llvm.org/D15866



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

[PATCH] D15866: Warn when using `defined` in a macro definition.

[Nico Weber via cfe-commits]

thakis created this revision.
thakis added a reviewer: rsmith.
thakis added a subscriber: cfe-commits.

As far as I can tell, doing

#define HAVE_FOO_BAR defined(FOO) && defined(BAR)
#if HAVE_FOO
...
#endif

has undefined behavior per [cpp.cond]p4. In practice, it can have different 
behavior in gcc and Visual Studio – see the comment in PPExpressions.cpp. So we 
should warn on this.

One problem is that this also applies to function-like macros. While the 
example above can be written like

#if defined(FOO) && defined(BAR)
#defined HAVE_FOO 1
#else
#define HAVE_FOO 0
#endif

there is no easy way to rewrite a function-like macro like `#define FOO(x) 
(defined __foo_##x && __foo_##x)`. Function-like macros like this are used in 
practice, and compilers seem to not have differing behavior in that case. So 
maybe this should be a Warning only for object-like macros and an Extension for 
function-like macros? But it's undefined behavior in both cases as far as I can 
tell.

http://reviews.llvm.org/D15866

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPExpressions.cpp
  test/Lexer/cxx-features.cpp
  test/Preprocessor/expr_define_expansion.c

Index: test/Lexer/cxx-features.cpp
===
--- test/Lexer/cxx-features.cpp
+++ test/Lexer/cxx-features.cpp
@@ -1,11 +1,12 @@
-// RUN: %clang_cc1 -std=c++98 -verify %s
-// RUN: %clang_cc1 -std=c++11 -verify %s
-// RUN: %clang_cc1 -std=c++1y -fsized-deallocation -verify %s
-// RUN: %clang_cc1 -std=c++1y -fsized-deallocation -fconcepts-ts 
-DCONCEPTS_TS=1 -verify %s
-// RUN: %clang_cc1 -fcoroutines -DCOROUTINES -verify %s
+// RUN: %clang_cc1 -Wno-expansion-to-defined -std=c++98 -verify %s
+// RUN: %clang_cc1 -Wno-expansion-to-defined -std=c++11 -verify %s
+// RUN: %clang_cc1 -Wno-expansion-to-defined -std=c++1y -fsized-deallocation 
-verify %s
+// RUN: %clang_cc1 -Wno-expansion-to-defined -std=c++1y -fsized-deallocation 
-fconcepts-ts -DCONCEPTS_TS=1 -verify %s
+// RUN: %clang_cc1 -Wno-expansion-to-defined -fcoroutines -DCOROUTINES -verify 
%s
 
 // expected-no-diagnostics
 
+// FIXME using `defined` in a macro has undefined behavior.
 #if __cplusplus < 201103L
 #define check(macro, cxx98, cxx11, cxx1y) cxx98 == 0 ? defined(__cpp_##macro) 
: __cpp_##macro != cxx98
 #elif __cplusplus < 201304L
Index: lib/Lex/PPExpressions.cpp
===
--- lib/Lex/PPExpressions.cpp
+++ lib/Lex/PPExpressions.cpp
@@ -82,6 +82,28 @@
 static bool EvaluateDefined(PPValue , Token , DefinedTracker 
,
 bool ValueLive, Preprocessor ) {
   SourceLocation beginLoc(PeekTok.getLocation());
+
+  // [cpp.cond]p4:
+  //   Prior to evaluation, macro invocations in the list of preprocessing
+  //   tokens that will become the controlling constant expression are replaced
+  //   (except for those macro names modified by the 'defined' unary operator),
+  //   just as in normal text. If the token 'defined' is generated as a result
+  //   of this replacement process or use of the 'defined' unary operator does
+  //   not match one of the two specified forms prior to macro replacement, the
+  //   behavior is undefined.
+  // This isn't an idle threat, consider this program:
+  //   #define FOO
+  //   #define BAR defined(FOO)
+  //   #if BAR
+  //   ...
+  //   #else
+  //   ...
+  //   #endif
+  // clang and gcc will pick the #if branch while Visual Studio will take the
+  // #else branch.  Emit a warning about this undefined behavior.
+  if (beginLoc.isMacroID())
+PP.Diag(beginLoc, diag::warn_defined_in_macro);
+
   Result.setBegin(beginLoc);
 
   // Get the next token, don't expand it.
Index: include/clang/Basic/DiagnosticLexKinds.td
===
--- include/clang/Basic/DiagnosticLexKinds.td
+++ include/clang/Basic/DiagnosticLexKinds.td
@@ -656,6 +656,10 @@
 def note_header_guard : Note<
   "%0 is defined here; did you mean %1?">;
 
+def warn_defined_in_macro : Warning<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup>;
+
 let CategoryName = "Nullability Issue" in {
 
 def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;
Index: test/Preprocessor/expr_define_expansion.c
===
--- test/Preprocessor/expr_define_expansion.c
+++ test/Preprocessor/expr_define_expansion.c
@@ -1,6 +1,10 @@
 // RUN: %clang_cc1 %s -E -CC -pedantic -verify
-// expected-no-diagnostics
 
 #define FOO && 1
 #if defined FOO FOO
 #endif
+
+#define A
+#define B defined(A)
+#if B // expected-warning{{macro expansion producing 'defined' has undefined 
behavior}}
+#endif


Index: test/Lexer/cxx-features.cpp
===
--- test/Lexer/cxx-features.cpp
+++ test/Lexer/cxx-features.cpp
@@ -1,11 +1,12 @@