Clang/Preprocessor: Support short circuit in directive

[wzssyqa]

Don't Evaluate RHS in if directive when short circuit. Examples include
#if 0 && another_condition
#if 1 || another_condition

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: YunQiang Su (wzssyqa)

Changes

Don't Evaluate RHS in if directive when short circuit. Examples include
#if 0 && another_condition
#if 1 || another_condition

---

Full diff: https://github.com/llvm/llvm-project/pull/123912.diff

2 Files Affected:

• (modified) clang/lib/Lex/PPExpressions.cpp (+27-1)
• (added) clang/test/Preprocessor/directive-short-circuit.c (+74)

diff --git a/clang/lib/Lex/PPExpressions.cpp b/clang/lib/Lex/PPExpressions.cpp
index a3b1384f0fa1d6..7e413ec6076642 100644
--- a/clang/lib/Lex/PPExpressions.cpp
+++ b/clang/lib/Lex/PPExpressions.cpp
@@ -624,7 +624,33 @@ static bool EvaluateDirectiveSubExpr(PPValue &LHS, unsigned MinPrec,
 
     // Consume the operator, remembering the operator's location for reporting.
     SourceLocation OpLoc = PeekTok.getLocation();
-    PP.LexNonComment(PeekTok);
+
+    if (!RHSIsLive && (Operator == tok::ampamp || Operator == tok::pipepipe)) {
+      unsigned ThisPrec = PeekPrec;
+      while (true) {
+        PP.LexUnexpandedNonComment(PeekTok);
+        if (PeekTok.is(tok::l_paren)) {
+          unsigned NestParen = 1;
+          while (true) {
+            PP.LexUnexpandedNonComment(PeekTok);
+            if (PeekTok.is(tok::l_paren))
+              NestParen++;
+            else if (PeekTok.is(tok::r_paren))
+              NestParen--;
+            if (NestParen == 0)
+              break;
+          }
+          PP.LexUnexpandedNonComment(PeekTok);
+        }
+        PeekPrec = getPrecedence(PeekTok.getKind());
+        if (PeekPrec <= ThisPrec) {
+          LHS.setEnd(PeekTok.getEndLoc());
+          break;
+        }
+      }
+      continue;
+    } else
+      PP.LexNonComment(PeekTok);
 
     PPValue RHS(LHS.getBitWidth());
     // Parse the RHS of the operator.
diff --git a/clang/test/Preprocessor/directive-short-circuit.c b/clang/test/Preprocessor/directive-short-circuit.c
new file mode 100644
index 00000000000000..d137f8198cbbcf
--- /dev/null
+++ b/clang/test/Preprocessor/directive-short-circuit.c
@@ -0,0 +1,74 @@
+// RUN: %clang -E -MD -MF - %s | FileCheck -check-prefix ZERO_AND_HAS_INCLUDE %s
+//
+// ZERO_AND_HAS_INCLUDE-NOT: limits.h
+//
+#if 0 && __has_include(<limits.h>)
+#include <limits.h>
+#endif
+
+#if 4==5 && __has_include(<limits.h>)
+#include <limits.h>
+#endif
+
+#if defined(_THIS_IS_NOT_DEFINED) && __has_include(<limits.h>)
+#include <limits.h>
+#endif
+
+#if 0 && (__has_include(<limits.h>))
+#include <limits.h>
+#endif
+
+#if 4==5 && (__has_include(<limits.h>))
+#include <limits.h>
+#endif
+
+#if defined(_THIS_IS_NOT_DEFINED) && (__has_include(<limits.h>))
+#include <limits.h>
+#endif
+
+#if 0 && (5==5 && __has_include(<limits.h>))
+#include <limits.h>
+#endif
+
+#if 1 && (4==5 && __has_include(<limits.h>))
+#include <limits.h>
+#endif
+
+
+
+
+
+
+#if 1 || __has_include(<limits.h>)
+XXXXXXXXXX
+#endif
+#if 5==5 || __has_include(<limits.h>)
+XXXXXXXXXX
+#endif
+
+#if defined(__clang__) || __has_include(<limits.h>)
+XXXXXXXXXX
+#endif
+
+#if 1 || (__has_include(<limits.h>))
+#endif
+
+#if 5==5 || (__has_include(<limits.h>))
+XXXXXXXXXX
+#endif
+
+#if defined(__clang__) || (__has_include(<limits.h>))
+XXXXXXXXXX
+#endif
+
+#if 1 && (5==5 || __has_include(<limits.h>))
+XXXXXXXXXX
+#endif
+
+#if 1 || (5==5 || __has_include(<limits.h>))
+XXXXXXXXXX
+#endif
+
+#if 0 || (5==5 || __has_include(<limits.h>))
+XXXXXXXXXX
+#endif

[cor3ntin]

Can you explain the motivation for this change? Thanks

[Bigcheese]

It's an alternative to #120673

This resolves my concerns with the original patch, and I'm pretty sure it doesn't cause any other issues for just parsing, but I do plan to take a closer look in a bit.

[wzssyqa]

Can you explain the motivation for this change? Thanks

One example is as we talked in #120673

#if 0 && __has_include(<limits.h>)

will introduce <limits.h> as dependency.

[AaronBallman]

Errr, I think this change shows a bug with the RUN line. Should it be passing -fcuda-is-device so that __HIP_DEVICE_COMPILE__ is actually defined? CC @ritter-x2a

[ritter-x2a]

@AaronBallman thanks for notifying me! I think -fcuda-is-device isn't necessary; as far as I'm aware (I just tried) the second RUN line with --offload-device-only leads to a clang run with __HIP_DEVICE_COMPILE__ defined to 1.

I'd prefer if we left this expression in the test as it was and changed the expected-warning comment behind it to an ondevice-warning and used -verify=expected,ondevice in the second RUN line instead, if we change preprocessing such that the deprecation warning isn't triggered due to short-circuit evaluation.

[AaronBallman]

What happens with code like:

#if 0 && absolute garbage goes here
#endif

Do we still get an error for the incorrect code after the &&?

[wzssyqa]

No error message will emit here.

[ritter-x2a]

I think it would be more useful in practice if the deprecation warnings respected lazy evaluation, as the PR suggests, but it's worth noting that this deviates from the C Standard, which specifies that macros are replaced first, before short-circuit evaluation takes place.

[AaronBallman]

I think it would be more useful in practice if the deprecation warnings respected lazy evaluation, as the PR suggests, but it's worth noting that this deviates from the C Standard, which specifies that macros are replaced first, before short-circuit evaluation takes place.

Ooof, that's a really good point.

C23 6.10.2p13:

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

[wzssyqa]

Thanks. So this patch is incorrect. Let's close it.