-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Clang] Introduce __builtin_is_modifiable_lvalue() #132524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -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); | ||||||
|
Comment on lines
+12990
to
+12991
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
As was already pointed out (I think), we don’t need these here since we’re not doing any evaluating anyway. |
||||||
|
|
||||||
| SourceLocation OrigLoc = Arg->getExprLoc(); | ||||||
| bool IsLValue = Arg->IgnoreCasts()->isModifiableLvalue( | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect |
||||||
| Info.Ctx, &OrigLoc) == Expr::MLV_Valid; | ||||||
| return Success(IsLValue, E); | ||||||
| } | ||||||
| case Builtin::BI__builtin_align_up: { | ||||||
| APValue Src; | ||||||
| APSInt Alignment; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm < %s| FileCheck %s | ||
|
|
||
| void report(int value); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, all of these should just be sema tests using |
||
|
|
||
| __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; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // RUN: %clang_cc1 -std=c99 -fsyntax-only -verify %s | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense to use Also, I think it would be valuable to cover more types. N1570 6.3.2.1/1, N3467 6.3.3.1/1:
Edit: Perhaps it's wanted to make this intrinsic accept VLA (and report
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That should definitely be possible. I don’t think we really need to evaluate anything here. |
||
| 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}} | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d honestly just omit this since you could just as well use it outside of macros.