Quuxplusone
commented
9 years ago class PointerExprEvaluator in ExprConstant.cpp says "// FIXME: Missing: @protocol, @selector", that looks related...
Closed Quuxplusone closed 9 years ago
Quuxplusone
commented
9 years ago class PointerExprEvaluator in ExprConstant.cpp says "// FIXME: Missing: @protocol, @selector", that looks related...
Quuxplusone
commented
9 years ago smaller repro:
_Pragma("clang assume_nonnull begin")
void setArgument(void* argumentLocation);
_Pragma("clang assume_nonnull end")
void beginSystemSheet() {
setArgument(&@selector(foo:));
}smaller:
void f(void* _Nonnull argumentLocation);
void g() {
f(&@selector(foo:));
}Making @selectors lvalues (like string literals are) makes this go, but it
doesn't seem like the right direction (also breaks a few tests, but that
generally looks fixable):
Index: include/clang/AST/ExprObjC.h
===================================================================
--- include/clang/AST/ExprObjC.h (revision 247293)
+++ include/clang/AST/ExprObjC.h (working copy)
@@ -405,7 +405,7 @@
public:
ObjCSelectorExpr(QualType T, Selector selInfo,
SourceLocation at, SourceLocation rp)
- : Expr(ObjCSelectorExprClass, T, VK_RValue, OK_Ordinary, false, false,
+ : Expr(ObjCSelectorExprClass, T, VK_LValue, OK_Ordinary, false, false,
false, false),
SelName(selInfo), AtLoc(at), RParenLoc(rp){}
explicit ObjCSelectorExpr(EmptyShell Empty)
Index: lib/AST/ExprClassification.cpp
===================================================================
--- lib/AST/ExprClassification.cpp (revision 247293)
+++ lib/AST/ExprClassification.cpp (working copy)
@@ -137,6 +137,8 @@
case Expr::FunctionParmPackExprClass:
case Expr::MSPropertyRefExprClass:
case Expr::OMPArraySectionExprClass:
+// XXX
+ case Expr::ObjCSelectorExprClass:
return Cl::CL_LValue;
// C99 6.5.2.5p5 says that compound literals are lvalues.
@@ -170,7 +172,6 @@
case Expr::TypeTraitExprClass:
case Expr::ArrayTypeTraitExprClass:
case Expr::ExpressionTraitExprClass:
- case Expr::ObjCSelectorExprClass:
case Expr::ObjCProtocolExprClass:
case Expr::ObjCStringLiteralClass:
case Expr::ObjCBoxedExprClass:Maybe we want something like r180866 for &@selector
Quuxplusone
commented
9 years ago This seems to do the trick:
hummer:clang thakis$ svn diff
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 247293)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -9893,7 +9893,11 @@
OrigOp = op = new (Context)
MaterializeTemporaryExpr(op->getType(), OrigOp.get(), true);
} else if (isa<ObjCSelectorExpr>(op)) {
- return Context.getPointerType(op->getType());
+ // @selector() is an rvalue but can have its address taken. Materialize
+ // it instead of returning a pointer type so that the subexpression of an
+ // UnaryAddrOf has fewer special cases.
+ OrigOp = op = new (Context)
+ MaterializeTemporaryExpr(op->getType(), OrigOp.get(), true);
} else if (lval == Expr::LV_MemberFunction) {
// If it's an instance method, make a member pointer.
// The expression must have exactly the form &A::foo.…but it probably makes us generate the wrong code :-) I'll look into adding some test coverage for that first, I suppose.
Quuxplusone
commented
9 years ago After some further thinking, making @selector expressions lvalues seems like a pretty good idea (it's similar to typeid), so comment 4 looks pretty good.
It has the problem that it makes this compile:
@selector(dealloc) = s;
This could be fixed by giving @selector a const type:
--- lib/Sema/SemaExprObjC.cpp (revision 247709) +++ lib/Sema/SemaExprObjC.cpp (working copy) @@ -1210,7 +1210,7 @@ break; } }
But then SEL* ps2 = &@selector(dealloc); starts warning "cannot initialize a variable of type 'SEL ' with an rvalue of type 'const SEL '".
(SEL can't internally be a const pointer to internal type:
--- lib/AST/ASTContext.cpp (revision 247709) +++ lib/AST/ASTContext.cpp (working copy) @@ -6006,7 +6006,7 @@
TypedefDecl *ASTContext::getObjCSelDecl() const { if (!ObjCSelDecl) {
Then SEL t; t = @selector(foo); doesn't build, of course. And making SEL a pointer to const internal type doesn't help with @selector(foo) = 0; and also has mangling implications.)
Quuxplusone
commented
9 years ago Maybe emitting a diagnostic about "can't convert const SEL* to SEL*" ain't so
bad: Currently, this compiles without warnings and aborts at runtime:
#import <Foundation/Foundation.h>
@interface F : NSObject
@end
@implementation F
- (void)foo {
printf("hi\n");
}
@end
void g(SEL *s) { *s = 0; }
int main() {
g(&@selector(foo));
[[[F alloc] init] foo];
}This is the motivation for this bug, btw:
#import <Foundation/Foundation.h>
@interface Ind : NSObject
@end
@implementation Ind
- (void)delay:(SEL)s {
[self performSelector:s];
}
- (void)foo {
printf("hi\n");
}
@end
int main() {
Ind* ind = [[Ind alloc] init];
SEL theSelector;
NSMethodSignature *aSignature;
NSInvocation *anInvocation;
theSelector = @selector(delay:);
aSignature = [Ind instanceMethodSignatureForSelector:theSelector];
anInvocation = [NSInvocation invocationWithMethodSignature:aSignature];
[anInvocation setSelector:theSelector];
[anInvocation setTarget:ind];
[anInvocation setArgument:&@selector(foo) atIndex:2];
[anInvocation invokeWithTarget:ind];
}
With the lvalue patch, this warns:
testinv.m:26:29: warning: sending 'const SEL *' to parameter of type 'void *'
discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
[anInvocation setArgument:&@selector(foo) atIndex:2];
^~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSInvocation.h:34:29:
note: passing argument to parameter 'argumentLocation'
here
- (void)setArgument:(void *)argumentLocation atIndex:(NSInteger)idx;
^The "real rvalue" approach is fine even with a slightly trickier variant that
looks like so:
#import <Foundation/Foundation.h>
@interface Ind : NSObject
@end
@implementation Ind
- (void)delay:(SEL)s {
[self performSelector:s];
}
- (void)foo {
printf("hi\n");
}
@end
NSInvocation* getinv(Ind* ind) __attribute__((noinline)){
SEL theSelector;
NSMethodSignature *aSignature;
NSInvocation *anInvocation;
theSelector = @selector(delay:);
aSignature = [Ind instanceMethodSignatureForSelector:theSelector];
anInvocation = [NSInvocation invocationWithMethodSignature:aSignature];
[anInvocation setSelector:theSelector];
[anInvocation setTarget:ind];
[anInvocation setArgument:&@selector(foo) atIndex:2];
return anInvocation;
}
char f() __attribute__((noinline)) {
char c[80];
for (int i = 0; i < 80; ++i) c[i] = i;
return c[9];
}
int main() {
Ind* ind = [[Ind alloc] init];
NSInvocation* i = getinv(ind);
int r = f();
[i invokeWithTarget:ind];
return r;
}
That's because setArgument: docs say "This method copies the contents of buffer
as the argument at index. The number of bytes copied is determined by the
argument size."Attached clang-addsel-lval.diff (5483 bytes, text/plain): approach 1: make @selector real lvalues
Quuxplusone
commented
9 years ago Attached clang-addsel-rval.diff (2960 bytes, text/plain): approach 2: make @selector real rvalues
Quuxplusone
commented
9 years ago Attached clang-addsel-bandaid.diff (2156 bytes, text/plain): approach 3: let constexpr evaluator handle current hybrid state
Quuxplusone
commented
9 years ago Currently, ObjCSelectorExpr is an rvalue but it can get its address taken. That's a bit awkward and leads to several problems.
The obvious approach is to make the AST less awkward.
Approach 1:
Make @selector a real lvalue, and make it const so that @selector(foo) = 0 doesn't compile. Fixes 1. Fixes 4. Fixes 3 (still crashes, but warns...also see below). Fixes 2 (returning &@selector no longer warns). Does not change codegen behavior. Does change warnings:
SEL* s = &@selector(foo);
previously would compile without warnings. Now it warns "can't convert const SEL* to SEL". In particular,
[anInvocation setArgument:&@selector(foo) atIndex:2];
now warns, which is probably the reason for &@selector working in the first place.
Approach 2: Make @selector a real rvalue, and materialize it into a temporary when used. Fixes 1. Fixes 3 (doesn't warn, no longer crashes -- only a copy gets overwritten). Changes codegen behavior: Returning &@selector from a function now really returns the address of a local (but we already warn on this). When passing &@selector in a parameter, this is now the address of a temporary too, silently. For -[NSInvocation setArgument:], that's ok, it's guaranteed to make a copy. We could emit a warning in this case too, but if every use of &@selector causes a warning, why support it at all? (This seems like a fairly decent approach, but it either silently changes codegen behavior, or makes us warn on all most uses of &@selector). If this is done, 4 could be fixed in a follow-up patch.
So neither option is super super appealing and "make AST less awkward" is less attractive than it seems at first sight.
Approach 3: Fix the one StmtVisitor that crashes. Does fix 1 but doesn't help with 2-4.
I'm going to check in approach 3 as it fixes this crash and preserves the status quo. If anyone has strong feelings if we should do 1 or 2, please speak up.
Quuxplusone
commented
9 years ago r247740
clang-addsel-lval.diff(5483 bytes, text/plain)clang-addsel-rval.diff(2960 bytes, text/plain)clang-addsel-bandaid.diff(2156 bytes, text/plain)