<div dir="ltr">Nico, this is producing tons of warnings on an LLVM build and is actually breaking our internal builds (we build with -Werror).<div><br></div><div>I fixed one file that was producing this, but there are several that have the same problem (e.g., gtest-port.h). Could you fix them or rollback?</div><div><br></div><div><br></div><div>Thanks. Diego.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jan 19, 2016 at 10:15 AM, Nico Weber via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: nico<br>
Date: Tue Jan 19 09:15:31 2016<br>
New Revision: 258128<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=258128&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=258128&view=rev</a><br>
Log:<br>
Add -Wexpansion-to-undefined: warn when using `defined` in a macro definition.<br>
<br>
[cpp.cond]p4:<br>
Prior to evaluation, macro invocations in the list of preprocessing<br>
tokens that will become the controlling constant expression are replaced<br>
(except for those macro names modified by the 'defined' unary operator),<br>
just as in normal text. If the token 'defined' is generated as a result<br>
of this replacement process or use of the 'defined' unary operator does<br>
not match one of the two specified forms prior to macro replacement, the<br>
behavior is undefined.<br>
<br>
This isn't an idle threat, consider this program:<br>
#define FOO<br>
#define BAR defined(FOO)<br>
#if BAR<br>
...<br>
#else<br>
...<br>
#endif<br>
clang and gcc will pick the #if branch while Visual Studio will take the<br>
#else branch. Emit a warning about this undefined behavior.<br>
<br>
One problem is that this also applies to function-like macros. While the<br>
example above can be written like<br>
<br>
#if defined(FOO) && defined(BAR)<br>
#defined HAVE_FOO 1<br>
#else<br>
#define HAVE_FOO 0<br>
#endif<br>
<br>
there is no easy way to rewrite a function-like macro like `#define FOO(x)<br>
(defined __foo_##x && __foo_##x)`. Function-like macros like this are used in<br>
practice, and compilers seem to not have differing behavior in that case. So<br>
this a default-on warning only for object-like macros. For function-like<br>
macros, it is an extension warning that only shows up with `-pedantic`.<br>
(But it's undefined behavior in both cases.)<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td<br>
cfe/trunk/lib/Lex/PPExpressions.cpp<br>
cfe/trunk/test/Lexer/cxx-features.cpp<br>
cfe/trunk/test/Preprocessor/expr_define_expansion.c<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=258128&r1=258127&r2=258128&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=258128&r1=258127&r2=258128&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Tue Jan 19 09:15:31 2016<br>
@@ -204,6 +204,7 @@ def OverloadedShiftOpParentheses: DiagGr<br>
def DanglingElse: DiagGroup<"dangling-else">;<br>
def DanglingField : DiagGroup<"dangling-field">;<br>
def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;<br>
+def ExpansionToUndefined : DiagGroup<"expansion-to-undefined">;<br>
def FlagEnum : DiagGroup<"flag-enum">;<br>
def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>;<br>
def InfiniteRecursion : DiagGroup<"infinite-recursion">;<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=258128&r1=258127&r2=258128&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=258128&r1=258127&r2=258128&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td Tue Jan 19 09:15:31 2016<br>
@@ -658,6 +658,13 @@ def warn_header_guard : Warning<<br>
def note_header_guard : Note<<br>
"%0 is defined here; did you mean %1?">;<br>
<br>
+def warn_defined_in_object_type_macro : Warning<<br>
+ "macro expansion producing 'defined' has undefined behavior">,<br>
+ InGroup<ExpansionToUndefined>;<br>
+def warn_defined_in_function_type_macro : Extension<<br>
+ "macro expansion producing 'defined' has undefined behavior">,<br>
+ InGroup<ExpansionToUndefined>;<br>
+<br>
let CategoryName = "Nullability Issue" in {<br>
<br>
def err_pp_assume_nonnull_syntax : Error<"expected 'begin' or 'end'">;<br>
<br>
Modified: cfe/trunk/lib/Lex/PPExpressions.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPExpressions.cpp?rev=258128&r1=258127&r2=258128&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPExpressions.cpp?rev=258128&r1=258127&r2=258128&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Lex/PPExpressions.cpp (original)<br>
+++ cfe/trunk/lib/Lex/PPExpressions.cpp Tue Jan 19 09:15:31 2016<br>
@@ -140,6 +140,51 @@ static bool EvaluateDefined(PPValue &Res<br>
PP.LexNonComment(PeekTok);<br>
}<br>
<br>
+ // [cpp.cond]p4:<br>
+ // Prior to evaluation, macro invocations in the list of preprocessing<br>
+ // tokens that will become the controlling constant expression are replaced<br>
+ // (except for those macro names modified by the 'defined' unary operator),<br>
+ // just as in normal text. If the token 'defined' is generated as a result<br>
+ // of this replacement process or use of the 'defined' unary operator does<br>
+ // not match one of the two specified forms prior to macro replacement, the<br>
+ // behavior is undefined.<br>
+ // This isn't an idle threat, consider this program:<br>
+ // #define FOO<br>
+ // #define BAR defined(FOO)<br>
+ // #if BAR<br>
+ // ...<br>
+ // #else<br>
+ // ...<br>
+ // #endif<br>
+ // clang and gcc will pick the #if branch while Visual Studio will take the<br>
+ // #else branch. Emit a warning about this undefined behavior.<br>
+ if (beginLoc.isMacroID()) {<br>
+ bool IsFunctionTypeMacro =<br>
+ PP.getSourceManager()<br>
+ .getSLocEntry(PP.getSourceManager().getFileID(beginLoc))<br>
+ .getExpansion()<br>
+ .isFunctionMacroExpansion();<br>
+ // For object-type macros, it's easy to replace<br>
+ // #define FOO defined(BAR)<br>
+ // with<br>
+ // #if defined(BAR)<br>
+ // #define FOO 1<br>
+ // #else<br>
+ // #define FOO 0<br>
+ // #endif<br>
+ // and doing so makes sense since compilers handle this differently in<br>
+ // practice (see example further up). But for function-type macros,<br>
+ // there is no good way to write<br>
+ // # define FOO(x) (defined(M_ ## x) && M_ ## x)<br>
+ // in a different way, and compilers seem to agree on how to behave here.<br>
+ // So warn by default on object-type macros, but only warn in -pedantic<br>
+ // mode on function-type macros.<br>
+ if (IsFunctionTypeMacro)<br>
+ PP.Diag(beginLoc, diag::warn_defined_in_function_type_macro);<br>
+ else<br>
+ PP.Diag(beginLoc, diag::warn_defined_in_object_type_macro);<br>
+ }<br>
+<br>
// Invoke the 'defined' callback.<br>
if (PPCallbacks *Callbacks = PP.getPPCallbacks()) {<br>
Callbacks->Defined(macroToken, Macro,<br>
@@ -177,8 +222,8 @@ static bool EvaluateValue(PPValue &Resul<br>
if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {<br>
// Handle "defined X" and "defined(X)".<br>
if (II->isStr("defined"))<br>
- return(EvaluateDefined(Result, PeekTok, DT, ValueLive, PP));<br>
-<br>
+ return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);<br>
+<br>
// If this identifier isn't 'defined' or one of the special<br>
// preprocessor keywords and it wasn't macro expanded, it turns<br>
// into a simple 0, unless it is the C++ keyword "true", in which case it<br>
<br>
Modified: cfe/trunk/test/Lexer/cxx-features.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Lexer/cxx-features.cpp?rev=258128&r1=258127&r2=258128&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Lexer/cxx-features.cpp?rev=258128&r1=258127&r2=258128&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Lexer/cxx-features.cpp (original)<br>
+++ cfe/trunk/test/Lexer/cxx-features.cpp Tue Jan 19 09:15:31 2016<br>
@@ -6,6 +6,7 @@<br>
<br>
// expected-no-diagnostics<br>
<br>
+// FIXME using `defined` in a macro has undefined behavior.<br>
#if __cplusplus < 201103L<br>
#define check(macro, cxx98, cxx11, cxx1y) cxx98 == 0 ? defined(__cpp_##macro) : __cpp_##macro != cxx98<br>
#elif __cplusplus < 201304L<br>
<br>
Modified: cfe/trunk/test/Preprocessor/expr_define_expansion.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/expr_define_expansion.c?rev=258128&r1=258127&r2=258128&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/expr_define_expansion.c?rev=258128&r1=258127&r2=258128&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Preprocessor/expr_define_expansion.c (original)<br>
+++ cfe/trunk/test/Preprocessor/expr_define_expansion.c Tue Jan 19 09:15:31 2016<br>
@@ -1,6 +1,28 @@<br>
-// RUN: %clang_cc1 %s -E -CC -pedantic -verify<br>
-// expected-no-diagnostics<br>
+// RUN: %clang_cc1 %s -E -CC -verify<br>
+// RUN: %clang_cc1 %s -E -CC -DPEDANTIC -pedantic -verify<br>
<br>
#define FOO && 1<br>
#if defined FOO FOO<br>
#endif<br>
+<br>
+#define A<br>
+#define B defined(A)<br>
+#if B // expected-warning{{macro expansion producing 'defined' has undefined behavior}}<br>
+#endif<br>
+<br>
+#define m_foo<br>
+#define TEST(a) (defined(m_##a) && a)<br>
+<br>
+#if defined(PEDANTIC)<br>
+// expected-warning@+4{{macro expansion producing 'defined' has undefined behavior}}<br>
+#endif<br>
+<br>
+// This shouldn't warn by default, only with pedantic:<br>
+#if TEST(foo)<br>
+#endif<br>
+<br>
+<br>
+// Only one diagnostic for this case:<br>
+#define INVALID defined(<br>
+#if INVALID // expected-error{{macro name missing}}<br>
+#endif<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>