[clang][Interp] Fix discarding void-typed comma expressions

[tbaeder]

Summary

First, we need to handle void types in visitExpr, so we don't run into an assertion there when we try to pop a return value from the stack that isn't there.

Secondly, we need to handle it when visiting comma expressions so we don't do the same thing there.

[erichkeane]

So things cast to 'void' can have side effects, right? Does the call to discard still evaluate them?

https://godbolt.org/z/a3ej9bxKe

constexpr int foo(int start) {
    int i = start;

    (void)i++;
    (void)i++,(void)i++;
    return i;
}
constexpr int Value = foo(5);
static_assert(Value == 8);

[tbaeder]

In D148925#4287218, @erichkeane wrote:

So things cast to 'void' can have side effects, right? Does the call to discard still evaluate them?

https://godbolt.org/z/a3ej9bxKe

Yes. discard(E) is just visit(E), except that it sets DiscardResult to true for that one level of visiting. Following visit() calls will have DiscardResult = false again.

[erichkeane]

In D148925#4287497, @tbaeder wrote:

In D148925#4287218, @erichkeane wrote:

So things cast to 'void' can have side effects, right? Does the call to discard still evaluate them?

https://godbolt.org/z/a3ej9bxKe

Yes. discard(E) is just visit(E), except that it sets DiscardResult to true for that one level of visiting. Following visit() calls will have DiscardResult = false again.

Cool! Mind adding the tests I proposed? it would be nice to ensure/show side-effects are guaranteed.

[tbaeder]

In D148925#4287502, @erichkeane wrote:

Cool! Mind adding the tests I proposed? it would be nice to ensure/show side-effects are guaranteed.

Sure, thanks for the test case!

clang/lib/AST/Interp/ByteCodeExprGen.cpp

bool ByteCodeExprGen<Emitter>::VisitBinaryOperator(const BinaryOperator *BO) {
   auto Discard = [this, T, BO](bool Result) {
     if (!Result)
       return false;
     return DiscardResult ? this->emitPop(*T, BO) : true;
   };
 
   // Deal with operations which have composite or void types.
   if (BO->isCommaOp()) {
-    if (!discard(LHS))
+    if (!this->discard(LHS))
       return false;
+    if (RHS->getType()->isVoidType())
+      return this->discard(RHS);
+
+    // Otherwise, visit RHS and optionally discard its value.
     return Discard(this->visit(RHS));
   }
 
   if (!LT || !RT || !T)
     return this->bail(BO);
 
   // Pointer arithmetic special case.
   if (BO->getOpcode() == BO_Add || BO->getOpcode() == BO_Sub) {

 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitExpr(const Expr *Exp) {
   ExprScope<Emitter> RootScope(this);
   if (!visit(Exp))
     return false;
 
+  if (Exp->getType()->isVoidType())
+    return this->emitRetVoid(Exp);
+
   if (std::optional<PrimType> T = classify(Exp))
     return this->emitRet(*T, Exp);
-  else
-    return this->emitRetValue(Exp);
 }
 
 /// Toplevel visitDecl().
 /// We get here from evaluateAsInitializer().
 /// We need to evaluate the initializer and return its value.
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitDecl(const VarDecl *VD) {
   assert(!VD->isInvalidDecl() && "Trying to constant evaluate an invalid decl");

clang/test/AST/Interp/literals.cpp

constexpr int ignoredExprs() {
   (void)(1 / 2);
   A a;
   a; // expected-warning {{unused}} \
      // ref-warning {{unused}}
   (void)a;
   (a); // expected-warning {{unused}} \
        // ref-warning {{unused}}
 
+  (void)5, (void)6;
+
   return 0;
 }
 
+/// Ignored comma expressions still have their
+/// expressions evaluated.
+constexpr int Comma(int start) {
+    int i = start;
+
+    (void)i++;
+    (void)i++,(void)i++;
+    return i;
+}
+constexpr int Value = Comma(5);
+static_assert(Value == 8, "");
+
 #endif
 
 namespace PredefinedExprs {
 #if __cplusplus >= 201402L
   template<typename CharT>
   constexpr bool strings_match(const CharT *str1, const CharT *str2) {
     while (*str1 && *str2) {
       if (*str1++ != *str2++)