Warn when using `defined` in a macro definition.

[thakis]

Summary

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 make this a default-on warning only for object-like macros and an extension warning that only shows up with pedantic for function-like macros. (But it's undefined behavior in both cases.)

[rsmith]

lib/Lex/PPExpressions.cpp

104–105

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

[thakis]

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

[thakis]

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

[thakis]

Address review comment.

[thakis]

Also addressed your comment and added a test for that.

[thakis]

r258128

include/clang/Basic/DiagnosticGroups.td

 def BitwiseOpParentheses: DiagGroup<"bitwise-op-parentheses">;
 def LogicalOpParentheses: DiagGroup<"logical-op-parentheses">;
 def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
 def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
 def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
 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">;
 def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">;
 def : DiagGroup<"import">;
 def GNUIncludeNext : DiagGroup<"gnu-include-next">;
 def IncompatibleMSStruct : DiagGroup<"incompatible-ms-struct">;

include/clang/Basic/DiagnosticLexKinds.td

def warn_module_conflict : Warning<
   InGroup<ModuleConflict>;
 
 def warn_header_guard : Warning<
   "%0 is used as a header guard here, followed by #define of a different macro">,
   InGroup<DiagGroup<"header-guard">>;
 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<ExpansionToUndefined>;
+def warn_defined_in_function_type_macro : Extension<
+  "macro expansion producing 'defined' has undefined behavior">,
+  InGroup<ExpansionToUndefined>;
+
 let CategoryName = "Nullability Issue" in {
 
 def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;
 def err_pp_double_begin_of_assume_nonnull : Error<
   "already inside '#pragma clang assume_nonnull'">;
 def err_pp_unmatched_end_of_assume_nonnull : Error<
   "not currently inside '#pragma clang assume_nonnull'">;
 def err_pp_include_in_assume_nonnull : Error<
   "cannot #include files inside '#pragma clang assume_nonnull'">;
 def err_pp_eof_in_assume_nonnull : Error<
   "'#pragma clang assume_nonnull' was not ended within this file">;
 
 }
 
 }

lib/Lex/PPExpressions.cpp

struct DefinedTracker {
   /// indicates the macro that was checked.
   IdentifierInfo *TheMacro;
 };
 
 /// EvaluateDefined - Process a 'defined(sym)' expression.
 static bool EvaluateDefined(PPValue &Result, Token &PeekTok, DefinedTracker &DT,
                             bool ValueLive, Preprocessor &PP) {
   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.
   PP.LexUnexpandedNonComment(PeekTok);
 
   // Two options, it can either be a pp-identifier or a (.
   SourceLocation LParenLoc;
   if (PeekTok.is(tok::l_paren)) {
static bool EvaluateValue(PPValue &Result, Token &PeekTok, DefinedTracker &DT,
   }
       
   // If this token's spelling is a pp-identifier, check to see if it is
   // 'defined' or if it is a macro.  Note that we check here because many
   // keywords are pp-identifiers, so we can't check the kind.
   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
     // turns into "1".
     if (ValueLive &&
         II->getTokenID() != tok::kw_true &&
         II->getTokenID() != tok::kw_false)
       PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;

test/Lexer/cxx-features.cpp

 // 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
 
 // 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
 #define check(macro, cxx98, cxx11, cxx1y) cxx11 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx11
 #else
 #define check(macro, cxx98, cxx11, cxx1y) cxx1y == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx1y
 #endif
 

test/Preprocessor/expr_define_expansion.c

-// RUN: %clang_cc1 %s -E -CC -pedantic -verify
-// expected-no-diagnostics
+// RUN: %clang_cc1 %s -E -CC -verify
+// RUN: %clang_cc1 %s -E -CC -DPEDANTIC -pedantic -verify
 
 #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
+
+#define m_foo
+#define TEST(a) (defined(m_##a) && a)
+
+#if defined(PEDANTIC)
+// expected-warning@+4{{macro expansion producing 'defined' has undefined behavior}}
+#endif
+
+// This shouldn't warn by default, only with pedantic:
+#if TEST(foo)
+#endif