[Clang] Introduce __builtin_is_modifiable_lvalue() #132524

kees · 2025-03-22T05:37:52Z

Provide a way to introspect expressions to see if they are assignable, which becomes very useful in macros that want to perform additional work on arguments that are lvalues. GCC is adding this builtin as well: https://forge.sourceware.org/pinskia/gcc-TEST/commits/branch/is_lvalue

llvmbot · 2025-03-22T05:38:24Z

@llvm/pr-subscribers-clang

Author: Kees Cook (kees)

Changes

Provide a way to introspect expressions to see if they are assignable, which becomes very useful in macros that want to perform additional work on arguments that are lvalues. GCC is adding this builtin as well: https://forge.sourceware.org/pinskia/gcc-TEST/commits/branch/is_lvalue

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

8 Files Affected:

(modified) clang/docs/LanguageExtensions.rst (+41)
(modified) clang/docs/ReleaseNotes.rst (+1)
(modified) clang/include/clang/Basic/Builtins.td (+7)
(modified) clang/lib/AST/Decl.cpp (+4)
(modified) clang/lib/AST/ExprConstant.cpp (+10)
(modified) clang/lib/Sema/SemaChecking.cpp (+1)
(added) clang/test/CodeGen/builtin-is-modifiable-lvalue.c (+50)
(added) clang/test/Sema/builtin-is-modifiable-lvalue.c (+17)

diff --git a/clang/docs/LanguageExtensions.rst b/clang/docs/LanguageExtensions.rst
index d4771775c9739..a45e246a22725 100644
--- a/clang/docs/LanguageExtensions.rst
+++ b/clang/docs/LanguageExtensions.rst
@@ -4067,6 +4067,47 @@ assignment can happen automatically.
 to a variable, have its address taken, or passed into or returned from a
 function, because doing so violates bounds safety conventions.
 
+``__builtin_is_modifiable_lvalue``
+----------------------------------
+
+``__builtin_is_modifiable_lvalue`` returns true when the argument is an assignable lvalue.
+
+The argument does not have any side-effects resolved.
+
+**Syntax**:
+
+.. code-block:: c
+
+  bool __builtin_is_modifiable_lvalue(T expr)
+
+**Examples**:
+
+.. code-block:: c
+
+  #define __force_lvalue_expr(x)  \
+          __builtin_choose_expr(__builtin_is_modifiable_lvalue(x), \
+                                x, (void *){ NULL })
+
+  #define __free_and_null(__do_free, x)   \
+  ({                                      \
+          typeof(x) *__ptr = &(x);        \
+          __do_free(*__ptr);              \
+          *__ptr = NULL;                  \
+  })
+  #define __free_and_maybe_null(__how, x)                          \
+          __builtin_choose_expr(__builtin_is_modifiable_lvalue(x), \
+                  __free_and_null(__how, __force_lvalue_expr(x)),  \
+                  __kfree(x))
+  #define kfree(x)    __free_and_maybe_null(__kfree, x)
+
+**Description**:
+
+When building complex preprocessor macros, it can be helpful to be able
+to determine if a given argument is an lvalue to make choices about
+how to resolve the given arguments. For example, performing assignments
+against an lvalue in a macro becomes possible, but values returned from
+function calls can be ignored.
+
 Multiprecision Arithmetic Builtins
 ----------------------------------
 
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 159991e8db981..d43aa2e481b6a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -158,6 +158,7 @@ Non-comprehensive list of changes in this release
 
 - Support parsing the `cc` operand modifier and alias it to the `c` modifier (#GH127719).
 - Added `__builtin_elementwise_exp10`.
+- Added `__builtin_is_modifiable_lvalue` to identify assignable arguments in macros.
 
 New Compiler Flags
 ------------------
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 72a5e495c4059..b4c979d1267b3 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -964,6 +964,13 @@ def IsConstantEvaluated : LangBuiltin<"CXX_LANG"> {
   let Prototype = "bool()";
 }
 
+def IsLValue : Builtin {
+  let Spellings = ["__builtin_is_modifiable_lvalue"];
+  let Attributes = [NoThrow, CustomTypeChecking, UnevaluatedArguments,
+                    Constexpr];
+  let Prototype = "bool(...)";
+}
+
 def IsWithinLifetime : LangBuiltin<"CXX_LANG"> {
   let Spellings = ["__builtin_is_within_lifetime"];
   let Attributes = [NoThrow, CustomTypeChecking, Consteval];
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index e8aeacf24374f..8ea40aacea3a3 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3676,6 +3676,10 @@ unsigned FunctionDecl::getBuiltinID(bool ConsiderWrapperFunctions) const {
       BuiltinID == Builtin::BI__builtin_counted_by_ref)
     return 0;
 
+  if (getASTContext().getLangOpts().CPlusPlus &&
+      BuiltinID == Builtin::BI__builtin_is_modifiable_lvalue)
+    return 0;
+
   const ASTContext &Context = getASTContext();
   if (!Context.BuiltinInfo.isPredefinedLibFunction(BuiltinID))
     return BuiltinID;
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0165a0a3b0df3..0f48f09a70412 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -12985,6 +12985,16 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
     assert(Src.isInt());
     return Success((Src.getInt() & (Alignment - 1)) == 0 ? 1 : 0, E);
   }
+  case Builtin::BI__builtin_is_modifiable_lvalue: {
+    const Expr *Arg = E->getArg(0);
+    SpeculativeEvaluationRAII SpeculativeEval(Info);
+    IgnoreSideEffectsRAII Fold(Info);
+
+    SourceLocation OrigLoc = Arg->getExprLoc();
+    bool IsLValue = Arg->IgnoreCasts()->isModifiableLvalue(
+                        Info.Ctx, &OrigLoc) == Expr::MLV_Valid;
+    return Success(IsLValue, E);
+  }
   case Builtin::BI__builtin_align_up: {
     APValue Src;
     APSInt Alignment;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 12a8894cc7f47..6551052a8539a 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2325,6 +2325,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
     if (BuiltinComplex(TheCall))
       return ExprError();
     break;
+  case Builtin::BI__builtin_is_modifiable_lvalue:
   case Builtin::BI__builtin_constant_p: {
     if (checkArgCount(TheCall, 1))
       return true;
diff --git a/clang/test/CodeGen/builtin-is-modifiable-lvalue.c b/clang/test/CodeGen/builtin-is-modifiable-lvalue.c
new file mode 100644
index 0000000000000..3e6e0c8106a07
--- /dev/null
+++ b/clang/test/CodeGen/builtin-is-modifiable-lvalue.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm < %s| FileCheck %s
+
+void report(int value);
+
+__attribute__((__noinline__)) int passthru(int a)
+{
+  return a;
+}
+
+int global;
+const int global_ro;
+
+int checkme(int arg, const int arg_ro)
+{
+  int autovar = 7;
+
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(5));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(checkme));
+  // CHECK: call void @report(i32 noundef 1)
+  report(__builtin_is_modifiable_lvalue(arg));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(arg + 5));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(arg_ro));
+  // CHECK: call void @report(i32 noundef 1)
+  report(__builtin_is_modifiable_lvalue(autovar));
+  // CHECK: call void @report(i32 noundef 1)
+  report(__builtin_is_modifiable_lvalue(global));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(global_ro));
+  // CHECK: call void @report(i32 noundef 1)
+  report(__builtin_is_modifiable_lvalue((unsigned char)arg));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(passthru(arg)));
+  // CHECK: call void @report(i32 noundef 0)
+  report(__builtin_is_modifiable_lvalue(""));
+  // CHECK: load
+  arg++;
+  // CHECK: call void @report(i32 noundef 0)
+  // CHECK-NOT: load
+  report(__builtin_is_modifiable_lvalue(arg++));
+  // CHECK: call void @report(i32 noundef 0)
+  // CHECK-NOT: load
+  report(__builtin_is_modifiable_lvalue(++arg));
+
+  // CHECK: load
+  return arg;
+}
diff --git a/clang/test/Sema/builtin-is-modifiable-lvalue.c b/clang/test/Sema/builtin-is-modifiable-lvalue.c
new file mode 100644
index 0000000000000..acf8bb6604413
--- /dev/null
+++ b/clang/test/Sema/builtin-is-modifiable-lvalue.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s
+
+int test0(void *ptr) {
+  return __builtin_is_modifiable_lvalue();	 // expected-error {{too few arguments to function call, expected 1, have 0}}
+}
+
+int test1(void *ptr) {
+  return __builtin_is_modifiable_lvalue(ptr);	 // ok
+}
+
+int test2(void *ptr) {
+  return __builtin_is_modifiable_lvalue(ptr, 5); // expected-error {{too many arguments to function call, expected 1, have 2}}
+}
+
+int test_trash(void *ptr) {
+  return __builtin_is_modifiable_lvalue(trash);	 // expected-error {{use of undeclared identifier}}
+}

tbaederr · 2025-03-22T06:24:30Z

It looks like this is similar to __builtin_constant_p - what is the proposed behavior wrt. side effects in the evaluated expression? gcc and clang disagree about this a lot currently: https://godbolt.org/z/rbneznT9z

kees · 2025-03-22T06:44:30Z

It looks like this is similar to __builtin_constant_p - what is the proposed behavior wrt. side effects in the evaluated expression? gcc and clang disagree about this a lot currently: https://godbolt.org/z/rbneznT9z

It is intended to have no side-effects. I followed the same logic used by __buildin_constant_p, and the CodeGen tests attempt to validate the lack of side-effects.

I didn't see a difference in your godbolt link? Neither incremented a. (Though neither built, so perhaps my tweaking changed the result?)

tbaederr · 2025-03-22T06:52:27Z

It looks like this is similar to __builtin_constant_p - what is the proposed behavior wrt. side effects in the evaluated expression? gcc and clang disagree about this a lot currently: https://godbolt.org/z/rbneznT9z

It is intended to have no side-effects. I followed the same logic used by __buildin_constant_p, and the CodeGen tests attempt to validate the lack of side-effects.

I didn't see a difference in your godbolt link? Neither incremented a. (Though neither built, so perhaps my tweaking changed the result?)

I can see GCC incrementing a and not complaining about the static_assert: https://godbolt.org/z/68r68YMWK

pinskia · 2025-03-22T08:22:51Z

Note the GCC implementtion of __builtin_is_modifiable_lvalue that I have implemented (unlike __builtin_constant_p which in some but not all cases) throws away the full expression that is inside __builtin_is_modifiable_lvalue since it is fully implemented in the parser rather than a mix of the middle-end and parser. In a similar way __builtin_choose_expr is handled for the non chosen side.

frederick-vs-ja · 2025-03-22T08:35:10Z

clang/test/Sema/builtin-is-modifiable-lvalue.c

@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s
+


Would it make more sense to use _Static_assert(__builtin_is_modifiable_lvalue(...), "")?

Also, I think it would be valuable to cover more types.

N1570 6.3.2.1/1, N3467 6.3.3.1/1:

[...] A modifiable lvalue is an lvalue that does not have array type, does not have an incomplete type, does not have a const-qualified type, and if it is a structure or union, does not have any member (including, recursively, any member or element of all contained aggregates or unions) with a const-qualified type.

Edit: Perhaps it's wanted to make this intrinsic accept VLA (and report false) and variably-modified types without evaluating the non-constant array size.

Edit: Perhaps it's wanted to make this intrinsic accept VLA (and report false) and variably-modified types without evaluating the non-constant array size.

That should definitely be possible. I don’t think we really need to evaluate anything here.

pinskia · 2025-03-22T08:39:33Z

clang/lib/AST/ExprConstant.cpp

+    IgnoreSideEffectsRAII Fold(Info);
+
+    SourceLocation OrigLoc = Arg->getExprLoc();
+    bool IsLValue = Arg->IgnoreCasts()->isModifiableLvalue(


I suspect IgnoreCasts() should be removed here. Otherwise (int)var would be allowed. As we talked about on mastodon. The GCC implemention does not ignore casts.

frederick-vs-ja · 2025-03-22T08:50:50Z

It looks like this is similar to __builtin_constant_p - what is the proposed behavior wrt. side effects in the evaluated expression? gcc and clang disagree about this a lot currently: https://godbolt.org/z/rbneznT9z

IIUC this intrinsic shouldn't be similar to __builtin_constant_p. It's just investigating properties of the type and value category of an expression (like std::is_lvalue_reference_v<decltype((expr))> in C++ but with more magics).

tbaederr · 2025-03-22T11:06:39Z

It looks like this is similar to __builtin_constant_p - what is the proposed behavior wrt. side effects in the evaluated expression? gcc and clang disagree about this a lot currently: https://godbolt.org/z/rbneznT9z

IIUC this intrinsic shouldn't be similar to __builtin_constant_p. It's just investigating properties of the type and value category of an expression (like std::is_lvalue_reference_v<decltype((expr))> in C++ but with more magics).

That's fine with me. If the argument isn't evaluated anyway, we don't need the SpeculativeEvaluationRAII and IgnoreSideEffectsRAII either.

Sirraide · 2025-03-22T19:27:14Z

clang/docs/ReleaseNotes.rst


 - Support parsing the `cc` operand modifier and alias it to the `c` modifier (#GH127719).
 - Added `__builtin_elementwise_exp10`.
+- Added `__builtin_is_modifiable_lvalue` to identify assignable arguments in macros.


Suggested change

- Added `__builtin_is_modifiable_lvalue` to identify assignable arguments in macros.

- Added `__builtin_is_modifiable_lvalue`.

I’d honestly just omit this since you could just as well use it outside of macros.

Sirraide · 2025-03-22T19:27:53Z

clang/lib/AST/ExprConstant.cpp

+    SpeculativeEvaluationRAII SpeculativeEval(Info);
+    IgnoreSideEffectsRAII Fold(Info);


Suggested change

SpeculativeEvaluationRAII SpeculativeEval(Info);

IgnoreSideEffectsRAII Fold(Info);

As was already pointed out (I think), we don’t need these here since we’re not doing any evaluating anyway.

Sirraide · 2025-03-22T19:29:47Z

clang/test/CodeGen/builtin-is-modifiable-lvalue.c

@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm < %s| FileCheck %s
+
+void report(int value);


Yeah, all of these should just be sema tests using static_assert; maybe 1 or 2 codegen tests just to make sure we can emit it at all would be nice, but that’s it imo.

Sirraide · 2025-03-22T19:33:29Z

clang/test/Sema/builtin-is-modifiable-lvalue.c

@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s
+


Edit: Perhaps it's wanted to make this intrinsic accept VLA (and report false) and variably-modified types without evaluating the non-constant array size.

That should definitely be possible. I don’t think we really need to evaluate anything here.

shafik · 2025-03-25T00:25:25Z

clang/test/Sema/builtin-is-modifiable-lvalue.c

@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s


This feels like a pretty bare bones set of tests, I think we can do better.

For example, it has been discussed that there should not be side effects, let's verify that. Above in a comment (int)var was mentioned, we should verify that. How about return values from functions which was also mentioned above, we should verify it is usable in constant expressions etc

This feels like a pretty bare bones set of tests, I think we can do better.

For example, it has been discussed that there should not be side effects, let's verify that. Above in a comment (int)var was mentioned, we should verify that. How about return values from functions which was also mentioned above, we should verify it is usable in constant expressions etc

Agreed. I have not written the testcases for GCC patch yet but I will make sure Kees's clang implementation matches up. Since GCC is in stage 4 and will be for one more month I am not going to submit the GCC patch until then so we have plenty of time to get agreement on the semantics of the builtin and such.

kees · 2025-03-25T19:44:04Z

Thanks for all the feedback! I'll continue working on this next week (I'm OoO this week).

kees requested a review from bwendling March 22, 2025 05:37

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 22, 2025

kees requested a review from nickdesaulniers March 22, 2025 05:45

frederick-vs-ja reviewed Mar 22, 2025

View reviewed changes

pinskia reviewed Mar 22, 2025

View reviewed changes

Sirraide reviewed Mar 22, 2025

View reviewed changes

Sirraide requested a review from AaronBallman March 22, 2025 19:33

shafik reviewed Mar 25, 2025

View reviewed changes

		@@ -0,0 +1,17 @@
		// RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s

	- Added `__builtin_is_modifiable_lvalue` to identify assignable arguments in macros.
	- Added `__builtin_is_modifiable_lvalue`.

		SpeculativeEvaluationRAII SpeculativeEval(Info);
		IgnoreSideEffectsRAII Fold(Info);

		@@ -0,0 +1,50 @@
		// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm < %s\| FileCheck %s

		void report(int value);

[Clang] Introduce __builtin_is_modifiable_lvalue() #132524

Are you sure you want to change the base?

[Clang] Introduce __builtin_is_modifiable_lvalue() #132524

Uh oh!

Conversation

kees commented Mar 22, 2025

Uh oh!

llvmbot commented Mar 22, 2025

Uh oh!

tbaederr commented Mar 22, 2025

Uh oh!

kees commented Mar 22, 2025

Uh oh!

tbaederr commented Mar 22, 2025

Uh oh!

pinskia commented Mar 22, 2025

Uh oh!

frederick-vs-ja Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frederick-vs-ja commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tbaederr commented Mar 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kees commented Mar 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

frederick-vs-ja Mar 22, 2025 •

edited

Loading

frederick-vs-ja commented Mar 22, 2025 •

edited

Loading