[clang] Don't short-circuit constant evaluation for array or record types

[cmagahern]

Summary

FastEvaluateAsRValue returns true without setting a result value for when a
given constant expression is an array or record type.

Clang attributes must be able to support constant expressions that are array or
record types, so proceed with the slower path for evaluation in the case where
FastEvaluateAsRValue does not yield an evaluation result.

[tbaeder]

Is there no way to add a test for this?

[erichkeane]

First, y es, we need some sort of tests for this. Second, I've got no idea what you're trying to accomplish here? If you want the array to be evaluatable as a constant-expression, I'd presume the 'Kind' there would be involved somehow, instead of just whether the evaluation was const.

[cmagahern]

In D141745#4058833, @erichkeane wrote:

First, y es, we need some sort of tests for this.

We have a test for this in Apple's internal branch of llvm. One of the tests verifies the codegen behavior for a clang attribute that expects an inline struct as an argument to the attribute.

I agree that we should have a test for this independent of the clang attribute, which is dependent on yet not entirely related to this part of the codebase.

Second, I've got no idea what you're trying to accomplish here? If you want the array to be evaluatable as a constant-expression, I'd presume the 'Kind' there would be involved somehow, instead of just whether the evaluation was const.

The goal of the attribute is to allow constant expressions to be written as arguments to that attribute, which get parsed and generated as metadata and associated with the attributed decl. The details of this attribute are arguably not important—Expr::EvaluateAsConstantExpr no longer works as promised because it bails out if the constant expression is not representable as an r-value. This has worked in every version of clang until ec782951d7bd3 (https://reviews.llvm.org/D138115) which is the change that introduced this fast path and broke constant evaluation for array and record types.

[cmagahern]

In D141745#4054330, @tbaeder wrote:

Is there no way to add a test for this?

Can you advise on how a test for this can be written? The original change that broke this behavior (https://reviews.llvm.org/D138115) did not add a test either.

[erichkeane]

(I don't have a good idea of how to test this, I'm still not clear on what was 'broken'. We allowed the original patch, since it was an NFC (or at least intended) so not breaking all the existing test suites was sufficient, whereas this has an intended purpose.

[cmagahern]

In D141745#4059893, @erichkeane wrote:

(I don't have a good idea of how to test this, I'm still not clear on what was 'broken'. ...)

EvaluateAsConstantExpr was broken for any expression that is either an array or record type.

For example, the following code that uses the annotate attribute, used by static analyzers, triggers an assertion in clang without this change.

struct my_struct {
	int a;
	const char *b;
};

int x __attribute((annotate(
	"my_annotation", 
	(struct my_struct) { .a = 42, .b = "test" }
)));

The expectation is that this attribute emits constant struct declarations instead:

@x = common global i32 0, align 4
@.str = private unnamed_addr constant [14 x i8] c"my_annotation\00", section "llvm.metadata"
@.str.1 = private unnamed_addr constant [10 x i8] c"./annot.c\00", section "llvm.metadata"
@.str.2 = private unnamed_addr constant [5 x i8] c"test\00", align 1
@.args = private unnamed_addr constant { %struct.my_struct } { %struct.my_struct { i32 42, i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str.2, i32 0, i32 0) } }, section "llvm.metadata"

[erichkeane]

In D141745#4060021, @cmagahern wrote:

In D141745#4059893, @erichkeane wrote:

(I don't have a good idea of how to test this, I'm still not clear on what was 'broken'. ...)

EvaluateAsConstantExpr was broken for any expression that is either an array or record type.

For example, the following code that uses the annotate attribute, used by static analyzers, triggers an assertion in clang without this change.

struct my_struct {
	int a;
	const char *b;
};

int x __attribute((annotate(
	"my_annotation", 
	(struct my_struct) { .a = 42, .b = "test" }
)));

The expectation is that this attribute emits constant struct declarations instead:

@x = common global i32 0, align 4
@.str = private unnamed_addr constant [14 x i8] c"my_annotation\00", section "llvm.metadata"
@.str.1 = private unnamed_addr constant [10 x i8] c"./annot.c\00", section "llvm.metadata"
@.str.2 = private unnamed_addr constant [5 x i8] c"test\00", align 1
@.args = private unnamed_addr constant { %struct.my_struct } { %struct.my_struct { i32 42, i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str.2, i32 0, i32 0) } }, section "llvm.metadata"

That seems like a somewhat reasonable test to me.

[cmagahern]

In D141745#4060024, @erichkeane wrote:

That seems like a somewhat reasonable test to me.

Sounds good—I'll add something like this to the existing attr-annotate.cpp test.

[cmagahern]

Updating D141745: [clang] Don't short-circuit constant evaluation for array or record types

[cmagahern]

Added a test to 2007-06-15-AnnotateAttribute.c since the code has to use a language other than C++11.

[erichkeane]

clang/lib/AST/ExprConstant.cpp

15784

Interestingly, the 'IsConst' is used 1x as the return value from FastEvaluateAsRValue, so I wonder if this originally should have been "return IsConst". @tbaeder : got any comments here?

That said, I'm tempted to approve once others have a look, the 'return value' is true if the function has successfully set IsConst, false otherwise. So it is a touch shocking that we wouldn't need to test it.

[cmagahern]

clang/lib/AST/ExprConstant.cpp

15784

There are definitely some naming improvements to make here. I'm sort of abusing IsConst since it is exposing exactly the right state that I need, but it kind of seems like FastEvaluateAsRValue should return false if it didn't write anything to Result.

[tbaeder]

clang/lib/AST/ExprConstant.cpp

15784

FastEvaluateAsRValue returns "whether it even looked at the given expression" (via the return value) and "whether it checked the expression and decided it's constant" (via IsConst).

Technically speaking, the patch as it is will also try to evaluate expressions with a null type (the third case in FastEvaluateAsRValue), but I don't know if that is relevant at all.

It might make sense to use Result.Val.hasValue() instead of IsConst?

[cmagahern]

clang/lib/AST/ExprConstant.cpp

15784

That sounds better, thanks. I'll make the change.

[cmagahern]

Updating D141745: [clang] Don't short-circuit constant evaluation for array or record types

Use Result.Val.hasValue() instead of IsConst to see if
FastEvaluateAsRValue wrote a value into the result variable.

[tbaeder]

LGTM but I'll wait for one of the others to comment.

clang/lib/AST/ExprConstant.cpp

 //===--- ExprConstant.cpp - Expression Constant Evaluator -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
static bool EvaluateDestruction(const ASTContext &Ctx, APValue::LValueBase Base,
   return true;
 }
 
 bool Expr::EvaluateAsConstantExpr(EvalResult &Result, const ASTContext &Ctx,
                                   ConstantExprKind Kind) const {
   assert(!isValueDependent() &&
          "Expression evaluator can't be called on a dependent expression.");
   bool IsConst;
-  if (FastEvaluateAsRValue(this, Result, Ctx, IsConst))
+  if (FastEvaluateAsRValue(this, Result, Ctx, IsConst) && IsConst)
     return true;
 
   ExprTimeTraceScope TimeScope(this, Ctx, "EvaluateAsConstantExpr");
   EvalInfo::EvaluationMode EM = EvalInfo::EM_ConstantExpression;
   EvalInfo Info(Ctx, Result, EM);
   Info.InConstantContext = true;
 
   // The type of the object we're initializing is 'const T' for a class NTTP.

clang/test/CodeGen/2007-06-15-AnnotateAttribute.c

 // RUN: %clang_cc1 -emit-llvm %s -o - | grep llvm.global.annotations
 // RUN: %clang_cc1 -emit-llvm %s -o - | grep llvm.var.annotation | count 3
 
 /* Global variable with attribute */
 int X __attribute__((annotate("GlobalValAnnotation")));
 
 /* Function with attribute */
 int foo(int y) __attribute__((annotate("GlobalValAnnotation")))
                __attribute__((noinline));
 
 int foo(int y __attribute__((annotate("LocalValAnnotation")))) {
   int x __attribute__((annotate("LocalValAnnotation")));
   x = 34;
   return y + x;
 }
 
+/* Attribute with struct argument. */
+struct TestStruct {
+  int a;
+  int b;
+};
+int Y __attribute__((annotate(
+  "GlobalValAnnotationWithArgs", 
+  42,
+  (struct TestStruct) { .a = 1, .b = 2 }
+)));
+
+// CHECK: @.str.3 = private unnamed_addr constant [28 x i8] c"GlobalValAnnotationWithArgs\00", section "llvm.metadata"
+// CHECK-NEXT: @.args = private unnamed_addr constant { i32, %struct.TestStruct } { i32 42, %struct.TestStruct { i32 1, i32 2 } }, section "llvm.metadata"
+
 int main(void) {
   static int a __attribute__((annotate("GlobalValAnnotation")));
   a = foo(2);
   return 0;
 }