[clang] Remove an incorrect assertion in ConstantFoldAttrs

[tbaederr]

Evaluating the attribute expression can be successful without resulting in a value. Namely, when the expression is of type void.

Fixes #119125

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

Evaluating the attribute expression can be successful without resulting in a value. Namely, when the expression is of type void.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaAttr.cpp (+2-2)
• (modified) clang/test/SemaCXX/attr-annotate.cpp (+4)

diff --git a/clang/lib/Sema/SemaAttr.cpp b/clang/lib/Sema/SemaAttr.cpp
index a1724820472b59..51354963c0831e 100644
--- a/clang/lib/Sema/SemaAttr.cpp
+++ b/clang/lib/Sema/SemaAttr.cpp
@@ -439,8 +439,8 @@ bool Sema::ConstantFoldAttrArgs(const AttributeCommonInfo &CI,
         Diag(Note.first, Note.second);
       return false;
     }
-    assert(Eval.Val.hasValue());
-    E = ConstantExpr::Create(Context, E, Eval.Val);
+    if (Eval.Val.hasValue())
+      E = ConstantExpr::Create(Context, E, Eval.Val);
   }
 
   return true;
diff --git a/clang/test/SemaCXX/attr-annotate.cpp b/clang/test/SemaCXX/attr-annotate.cpp
index 846ef4119f1d7c..ff538fee9cb3c7 100644
--- a/clang/test/SemaCXX/attr-annotate.cpp
+++ b/clang/test/SemaCXX/attr-annotate.cpp
@@ -134,3 +134,7 @@ constexpr int foldable_but_invalid() {
 template <typename T> [[clang::annotate()]] void f2() {}
 // expected-error@-1 {{'annotate' attribute takes at least 1 argument}}
 }
+
+namespace test5 {
+  void bir [[clang::annotate("B", (void)1)]] ();
+}

[tbaederr]

I guess it might make sense to create the ConstantExpr in any case, even for a None APValue? Dumping the ConstantExpr didn't print any APValue though, so I was assuming that it ignores None anyway.

[Sirraide]

I guess it might make sense to create the ConstantExpr in any case, even for a None APValue?

The comment above ConstantFoldAttrArgs seems to suggest that that’s what’s supposed to happen if the expression isn’t dependent—I don’t know if there’s any code that depends on that, though.

[tbaederr]

I guess it might make sense to create the ConstantExpr in any case, even for a None APValue?

The comment above ConstantFoldAttrArgs seems to suggest that that’s what’s supposed to happen if the expression isn’t dependent—I don’t know if there’s any code that depends on that, though.

Hm, what comment are you referring to exactly?

We could of course also just reject void expressions, that's closer to the previous behavior I guess.

[Sirraide]

Hm, what comment are you referring to exactly?

Ah, this one:

llvm-project/clang/include/clang/Sema/Sema.h

Lines 1838 to 1842 in d88876e

	/// ConstantFoldAttrArgs - Folds attribute arguments into ConstantExprs
	/// (unless they are value dependent or type dependent). Returns false
	/// and emits a diagnostic if one or more of the arguments could not be
	/// folded into a constant.
	bool ConstantFoldAttrArgs(const AttributeCommonInfo &CI,

The main thing I’m (if only a bit) worried about is that some code somewhere might be expecting a ConstantExpr to always be there if the thing isn’t dependent, so an ‘empty’ ConstantExpr might be necessary if that’s the case, but then again, if everything works then it’s probably fine?

We could of course also just reject void expressions

I mean, I think accepting them is fine (some attributes might have a use for them maybe?); just wondering how to best represent them in the AST.

[tbaederr]

CC @AaronBallman for an opinion about the AST represenation

[AaronBallman]

CC @AaronBallman for an opinion about the AST represenation

This is a bit awkward in that it's only used for the annotate attributes' argument lists, which should pass all of their arguments down to LLVM IR. However, we don't typically generate LLVM IR for discarded value statements in other circumstances. So I'm not certain what we should pass down to LLVM IR in this case. The arguments could be used positionally, so in that case we'd want to pass something down so the arguments line up to the source. The arguments could be used semantically, so in that case, there's nothing to pass down because there are no semantics.

(CC @erichkeane for more opinions)

[erichkeane]

CC @AaronBallman for an opinion about the AST represenation

This is a bit awkward in that it's only used for the annotate attributes' argument lists, which should pass all of their arguments down to LLVM IR. However, we don't typically generate LLVM IR for discarded value statements in other circumstances. So I'm not certain what we should pass down to LLVM IR in this case. The arguments could be used positionally, so in that case we'd want to pass something down so the arguments line up to the source. The arguments could be used semantically, so in that case, there's nothing to pass down because there are no semantics.

(CC @erichkeane for more opinions)

My immediate read of this before seeing the discussion was that the positional arguments are important, so making one 'disappear' could be problematic. I think a value of 'None', plus a reasonable IR for that would be our best way forward.

As far as what a 'reasonable IR' for a 'None' value would be, I don't know if we have any viable experts here we can discuss this with. The nature of 'annotate' is that we HAVE no expert on the feature (since it is intended to be used by just about anyone in the backend). I think we'd need to see a codegen test for 'none' however.

So in short:

1- Make it a None ConstantExpr

2- Give that a reasonable output for its attribute in IR

3- Write a codegen test that shows it off

4- Add a release note.

[tbaederr]

For

void f() {
  [[clang::annotate("B", (void)1, (void)2, 7)]] int j = 0;
}

The IR is:

@.str = private unnamed_addr constant [2 x i8] c"B\00", section "llvm.metadata"
@.str.1 = private unnamed_addr constant [10 x i8] c"array.cpp\00", section "llvm.metadata"
@.args = private unnamed_addr constant { i8, i8, i32 } { i8 undef, i8 undef, i32 7 }, section "llvm.metadata

; Function Attrs: mustprogress noinline nounwind optnone uwtable
define dso_local void @_Z1fv() #0 {
entry:
  %j = alloca i32, align 4
  call void @llvm.var.annotation.p0.p0(ptr %j, ptr @.str, ptr @.str.1, i32 187, ptr @.args)
  store i32 0, ptr %j, align 4
  ret void
}

So the void arguments get translated to undef. Is that acceptable?

[AaronBallman]

For

void f() {
  [[clang::annotate("B", (void)1, (void)2, 7)]] int j = 0;
}

The IR is:

@.str = private unnamed_addr constant [2 x i8] c"B\00", section "llvm.metadata"
@.str.1 = private unnamed_addr constant [10 x i8] c"array.cpp\00", section "llvm.metadata"
@.args = private unnamed_addr constant { i8, i8, i32 } { i8 undef, i8 undef, i32 7 }, section "llvm.metadata

; Function Attrs: mustprogress noinline nounwind optnone uwtable
define dso_local void @_Z1fv() #0 {
entry:
  %j = alloca i32, align 4
  call void @llvm.var.annotation.p0.p0(ptr %j, ptr @.str, ptr @.str.1, i32 187, ptr @.args)
  store i32 0, ptr %j, align 4
  ret void
}

So the void arguments get translated to undef. Is that acceptable?

That's an interesting situation! I'm not sure what I'd expect the LLVM IR to be there. For Clang AST, we want to keep around the expression AST nodes. But for LLVM IR, perhaps it makes sense to drop anything without a value rather than mark it as undef?

CC @nikic

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 2f2ac3de69dde902c9fe84bdd7faeee320498130 30457d4d267c85f2f81865acff1a02a7fe8ee0c0 clang/lib/Sema/SemaAttr.cpp clang/test/CodeGenCXX/attr-annotate2.cpp clang/test/SemaCXX/attr-annotate.cpp

The following files introduce new uses of undef:

• clang/test/CodeGenCXX/attr-annotate2.cpp

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[nikic]

That's an interesting situation! I'm not sure what I'd expect the LLVM IR to be there. For Clang AST, we want to keep around the expression AST nodes. But for LLVM IR, perhaps it makes sense to drop anything without a value rather than mark it as undef?

No strong opinion, I think either way would be okay. Using undef has the very slight advantage that there's still an indication that there was an argument there, even if it doesn't have a value, so [[clang::annotation("a", (void)1, 2)]] and [[clang::annotation("a", 2)]] can be distinguished.

[tbaederr]

Sounds like an LGTM from Nikita, does anyone else have other reservations? (Other than the missing release note)

[AaronBallman]

LGTM aside from the missing release note.