On Wed, Feb 3, 2010 at 3:29 PM, David Chisnall <[email protected]> wrote: > Hi Daniel, > > How does this break the NeXT runtime? I ran the test suite and got no > failures, and I tested a few extra @selector() cases with -fnext-runtime and > got code that looked correct. If you can provide me with a test case that > demonstrates how this breaks the NeXT runtime then I'd be happy to fix it.
I'm not sure, but that assert was firing on many projects which currently compile correctly. I don't have a test case yet, but I will get you one. Please ping me if I forget. - Daniel > The code generation code paths for the NeXT runtime should be exactly the > same before and after the patch and the only change in Sema produces the > warning conditionally, suppressing it when the NeXT runtime is used. > > David > > On 3 Feb 2010, at 20:12, Daniel Dunbar wrote: > >> Hi David. >> >> This is *totally* unacceptable. >> >> You cannot just commit a patch which substantially breaks working code >> without notice, without explanation, without coordinating with other >> developers. This code works, it is continuously tested, it is used to >> compile substantial amounts of code, and many people use it to build >> their projects. >> >> Your commit does not even add a test case. >> >> Reverted in r95244, please submit as a patch with explanation if you >> want someone to work with you to bring this code in, in a way that >> doesn't break the NeXT runtime. >> >> - Daniel >> >> On Tue, Feb 2, 2010 at 6:09 PM, David Chisnall <[email protected]> wrote: >>> Author: theraven >>> Date: Tue Feb 2 20:09:30 2010 >>> New Revision: 95189 >>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=95189&view=rev >>> Log: >>> Numerous changes to selector handling: >>> >>> - Don't use GlobalAliases with non-0 GEPs (GNU runtime) - this was >>> unsupported and LLVM will be generating errors if you do it soon. This >>> also simplifies the code generated by the GNU runtime a bit. >>> >>> - Make GetSelector() return a constant (GNU runtime), not a load of a store >>> of a constant. >>> >>> - Recognise @selector() expressions as valid static initialisers (as GCC >>> does). >>> >>> - Add methods to GCObjCRuntime to emit selectors as constants (needed for >>> using @selector() expressions as constants. These need implementing for >>> the Mac runtimes - I couldn't figure out how to do this, they seem to >>> require a load. >>> >>> - Store an ObjCMethodDecl in an ObjCSelectorExpr so that we can get at the >>> type information for the selector. This is needed for generating typed >>> selectors from @selector() expressions (as GCC does). Ideally, this >>> information should be stored in the Selector, but that would be an invasive >>> change. We should eventually add checks for common uses of @selector() >>> expressions. Possibly adding an attribute that can be applied to method >>> args providing the types of a selector so, for example, you'd do something >>> like this: >>> >>> - (id)performSelector: __attribute__((selector_types(id, SEL, id)))(SEL) >>> withObject: (id)object; >>> >>> Then, any @selector() expressions passed to the method will be check to >>> ensure that it conforms to this signature. We do this at run time on the >>> GNU runtime already, but it would be nice to do it at compile time on all >>> runtimes. >>> >>> - Made @selector() expressions emit type info if available and the runtime >>> supports it. >>> >>> Someone more familiar with the Mac runtime needs to implement the >>> GetConstantSelector() function in CGObjCMac. This currently just assert()s. >>> >>> >>> Modified: >>> cfe/trunk/include/clang/AST/ExprObjC.h >>> cfe/trunk/lib/AST/Expr.cpp >>> cfe/trunk/lib/CodeGen/CGExprConstant.cpp >>> cfe/trunk/lib/CodeGen/CGObjC.cpp >>> cfe/trunk/lib/CodeGen/CGObjCGNU.cpp >>> cfe/trunk/lib/CodeGen/CGObjCMac.cpp >>> cfe/trunk/lib/CodeGen/CGObjCRuntime.h >>> cfe/trunk/lib/Sema/SemaExprObjC.cpp >>> >>> Modified: cfe/trunk/include/clang/AST/ExprObjC.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ExprObjC.h?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/include/clang/AST/ExprObjC.h (original) >>> +++ cfe/trunk/include/clang/AST/ExprObjC.h Tue Feb 2 20:09:30 2010 >>> @@ -96,18 +96,22 @@ >>> /// ObjCSelectorExpr used for @selector in Objective-C. >>> class ObjCSelectorExpr : public Expr { >>> Selector SelName; >>> + ObjCMethodDecl *Method; >>> SourceLocation AtLoc, RParenLoc; >>> public: >>> ObjCSelectorExpr(QualType T, Selector selInfo, >>> SourceLocation at, SourceLocation rp) >>> - : Expr(ObjCSelectorExprClass, T, false, false), SelName(selInfo), >>> AtLoc(at), >>> - RParenLoc(rp){} >>> + : Expr(ObjCSelectorExprClass, T, false, false), SelName(selInfo), >>> Method(0), >>> + AtLoc(at), RParenLoc(rp){} >>> explicit ObjCSelectorExpr(EmptyShell Empty) >>> : Expr(ObjCSelectorExprClass, Empty) {} >>> >>> Selector getSelector() const { return SelName; } >>> void setSelector(Selector S) { SelName = S; } >>> >>> + ObjCMethodDecl *getMethodDecl() const { return Method; } >>> + void setMethodDecl(ObjCMethodDecl *M) { Method = M; } >>> + >>> SourceLocation getAtLoc() const { return AtLoc; } >>> SourceLocation getRParenLoc() const { return RParenLoc; } >>> void setAtLoc(SourceLocation L) { AtLoc = L; } >>> >>> Modified: cfe/trunk/lib/AST/Expr.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Expr.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/AST/Expr.cpp (original) >>> +++ cfe/trunk/lib/AST/Expr.cpp Tue Feb 2 20:09:30 2010 >>> @@ -1454,6 +1454,7 @@ >>> case StringLiteralClass: >>> case ObjCStringLiteralClass: >>> case ObjCEncodeExprClass: >>> + case ObjCSelectorExprClass: >>> return true; >>> case CompoundLiteralExprClass: { >>> // This handles gcc's extension that allows global initializers like >>> >>> Modified: cfe/trunk/lib/CodeGen/CGExprConstant.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprConstant.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGExprConstant.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGExprConstant.cpp Tue Feb 2 20:09:30 2010 >>> @@ -701,6 +701,14 @@ >>> CGM.GetStringForStringLiteral(E), >>> false); >>> } >>> >>> + llvm::Constant *VisitObjCSelectorExpr(const ObjCSelectorExpr *E) { >>> + ObjCMethodDecl *OMD = E->getMethodDecl(); >>> + if (OMD) >>> + return CGM.getObjCRuntime().GetConstantTypedSelector(OMD); >>> + else >>> + return CGM.getObjCRuntime().GetConstantSelector(E->getSelector()); >>> + } >>> + >>> llvm::Constant *VisitObjCEncodeExpr(ObjCEncodeExpr *E) { >>> // This must be an @encode initializing an array in a static >>> initializer. >>> // Don't emit it as the address of the string, emit the string data >>> itself >>> >>> Modified: cfe/trunk/lib/CodeGen/CGObjC.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjC.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGObjC.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGObjC.cpp Tue Feb 2 20:09:30 2010 >>> @@ -38,7 +38,11 @@ >>> // Note that this implementation allows for non-constant strings to be >>> passed >>> // as arguments to @selector(). Currently, the only thing preventing this >>> // behaviour is the type checking in the front end. >>> - return CGM.getObjCRuntime().GetSelector(Builder, E->getSelector()); >>> + ObjCMethodDecl *OMD = E->getMethodDecl(); >>> + if (OMD) >>> + return CGM.getObjCRuntime().GetSelector(Builder, OMD); >>> + else >>> + return CGM.getObjCRuntime().GetSelector(Builder, E->getSelector()); >>> } >>> >>> llvm::Value *CodeGenFunction::EmitObjCProtocolExpr(const ObjCProtocolExpr >>> *E) { >>> >>> Modified: cfe/trunk/lib/CodeGen/CGObjCGNU.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCGNU.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGObjCGNU.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGObjCGNU.cpp Tue Feb 2 20:09:30 2010 >>> @@ -146,9 +146,17 @@ >>> const ObjCMethodDecl *Method); >>> virtual llvm::Value *GetClass(CGBuilderTy &Builder, >>> const ObjCInterfaceDecl *OID); >>> - virtual llvm::Value *GetSelector(CGBuilderTy &Builder, Selector Sel); >>> - virtual llvm::Value *GetSelector(CGBuilderTy &Builder, const >>> ObjCMethodDecl >>> - *Method); >>> + virtual llvm::Constant *GetConstantSelector(Selector Sel); >>> + virtual llvm::Constant *GetConstantTypedSelector( >>> + const ObjCMethodDecl *Method); >>> + llvm::Value *GetSelector(CGBuilderTy &Builder, >>> + Selector Sel) { >>> + return cast<llvm::Constant>((GetConstantSelector(Sel))); >>> + } >>> + llvm::Value *GetSelector(CGBuilderTy &Builder, >>> + const ObjCMethodDecl *Method) { >>> + return cast<llvm::Constant>(GetConstantTypedSelector(Method)); >>> + } >>> >>> virtual llvm::Function *GenerateMethod(const ObjCMethodDecl *OMD, >>> const ObjCContainerDecl *CD); >>> @@ -287,18 +295,18 @@ >>> return Builder.CreateCall(ClassLookupFn, ClassName); >>> } >>> >>> -llvm::Value *CGObjCGNU::GetSelector(CGBuilderTy &Builder, Selector Sel) { >>> +llvm::Constant *CGObjCGNU::GetConstantSelector(Selector Sel) { >>> llvm::GlobalAlias *&US = UntypedSelectors[Sel.getAsString()]; >>> if (US == 0) >>> - US = new llvm::GlobalAlias(llvm::PointerType::getUnqual(SelectorTy), >>> + US = new llvm::GlobalAlias(SelectorTy, >>> llvm::GlobalValue::PrivateLinkage, >>> >>> ".objc_untyped_selector_alias"+Sel.getAsString(), >>> NULL, &TheModule); >>> >>> - return Builder.CreateLoad(US); >>> + return US; >>> } >>> >>> -llvm::Value *CGObjCGNU::GetSelector(CGBuilderTy &Builder, const >>> ObjCMethodDecl >>> +llvm::Constant *CGObjCGNU::GetConstantTypedSelector(const ObjCMethodDecl >>> *Method) { >>> >>> std::string SelName = Method->getSelector().getAsString(); >>> @@ -310,17 +318,17 @@ >>> >>> // If it's already cached, return it. >>> if (TypedSelectors[Selector]) { >>> - return Builder.CreateLoad(TypedSelectors[Selector]); >>> + return TypedSelectors[Selector]; >>> } >>> >>> // If it isn't, cache it. >>> llvm::GlobalAlias *Sel = new llvm::GlobalAlias( >>> - llvm::PointerType::getUnqual(SelectorTy), >>> + SelectorTy, >>> llvm::GlobalValue::PrivateLinkage, ".objc_selector_alias" + >>> SelName, >>> NULL, &TheModule); >>> TypedSelectors[Selector] = Sel; >>> >>> - return Builder.CreateLoad(Sel); >>> + return Sel; >>> } >>> >>> llvm::Constant *CGObjCGNU::MakeConstantString(const std::string &Str, >>> @@ -1461,40 +1469,43 @@ >>> >>> // Now that all of the static selectors exist, create pointers to them. >>> int index = 0; >>> + llvm::SmallVector<std::pair<llvm::GlobalAlias*,llvm::Value*>, 16> >>> selectors; >>> for (std::map<TypedSelector, llvm::GlobalAlias*>::iterator >>> iter=TypedSelectors.begin(), iterEnd =TypedSelectors.end(); >>> iter != iterEnd; ++iter) { >>> llvm::Constant *Idxs[] = {Zeros[0], >>> llvm::ConstantInt::get(llvm::Type::getInt32Ty(VMContext), index++), >>> Zeros[0]}; >>> - llvm::Constant *SelPtr = new llvm::GlobalVariable(TheModule, >>> SelStructPtrTy, >>> - true, llvm::GlobalValue::InternalLinkage, >>> - llvm::ConstantExpr::getGetElementPtr(SelectorList, Idxs, 2), >>> - ".objc_sel_ptr"); >>> + llvm::Constant *SelPtr = >>> + llvm::ConstantExpr::getGetElementPtr(SelectorList, Idxs, 2); >>> // If selectors are defined as an opaque type, cast the pointer to this >>> // type. >>> if (isSelOpaque) { >>> - SelPtr = llvm::ConstantExpr::getBitCast(SelPtr, >>> - llvm::PointerType::getUnqual(SelectorTy)); >>> + SelPtr = llvm::ConstantExpr::getBitCast(SelPtr,SelectorTy); >>> } >>> - (*iter).second->setAliasee(SelPtr); >>> + selectors.push_back( >>> + std::pair<llvm::GlobalAlias*,llvm::Value*>((*iter).second, >>> SelPtr)); >>> } >>> for (llvm::StringMap<llvm::GlobalAlias*>::iterator >>> iter=UntypedSelectors.begin(), iterEnd = UntypedSelectors.end(); >>> iter != iterEnd; iter++) { >>> llvm::Constant *Idxs[] = {Zeros[0], >>> llvm::ConstantInt::get(llvm::Type::getInt32Ty(VMContext), index++), >>> Zeros[0]}; >>> - llvm::Constant *SelPtr = new llvm::GlobalVariable >>> - (TheModule, SelStructPtrTy, >>> - true, llvm::GlobalValue::InternalLinkage, >>> - llvm::ConstantExpr::getGetElementPtr(SelectorList, Idxs, 2), >>> - ".objc_sel_ptr"); >>> + llvm::Constant *SelPtr = >>> + llvm::ConstantExpr::getGetElementPtr(SelectorList, Idxs, 2); >>> // If selectors are defined as an opaque type, cast the pointer to this >>> // type. >>> if (isSelOpaque) { >>> - SelPtr = llvm::ConstantExpr::getBitCast(SelPtr, >>> - llvm::PointerType::getUnqual(SelectorTy)); >>> + SelPtr = llvm::ConstantExpr::getBitCast(SelPtr, SelectorTy); >>> } >>> - (*iter).second->setAliasee(SelPtr); >>> + selectors.push_back( >>> + std::pair<llvm::GlobalAlias*,llvm::Value*>((*iter).second, >>> SelPtr)); >>> + } >>> + for (llvm::SmallVectorImpl<std::pair< >>> + llvm::GlobalAlias*,llvm::Value*> >::iterator >>> + iter=selectors.begin(), iterEnd =selectors.end(); >>> + iter != iterEnd; ++iter) { >>> + iter->first->replaceAllUsesWith(iter->second); >>> + iter->first->eraseFromParent(); >>> } >>> // Number of classes defined. >>> >>> Elements.push_back(llvm::ConstantInt::get(llvm::Type::getInt16Ty(VMContext), >>> >>> Modified: cfe/trunk/lib/CodeGen/CGObjCMac.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCMac.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGObjCMac.cpp (original) >>> +++ cfe/trunk/lib/CodeGen/CGObjCMac.cpp Tue Feb 2 20:09:30 2010 >>> @@ -953,6 +953,14 @@ >>> CGObjCCommonMac(CodeGen::CodeGenModule &cgm) : >>> CGM(cgm), VMContext(cgm.getLLVMContext()) { } >>> >>> + virtual llvm::Constant *GetConstantSelector(Selector Sel) { >>> + assert(0 && "Constant Selectors are not yet supported on the Mac >>> runtimes"); >>> + return 0; >>> + } >>> + virtual llvm::Constant *GetConstantTypedSelector( >>> + const ObjCMethodDecl *Method) { >>> + return GetConstantSelector(Method->getSelector()); >>> + } >>> virtual llvm::Constant *GenerateConstantString(const StringLiteral *SL); >>> >>> virtual llvm::Function *GenerateMethod(const ObjCMethodDecl *OMD, >>> >>> Modified: cfe/trunk/lib/CodeGen/CGObjCRuntime.h >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGObjCRuntime.h?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/CodeGen/CGObjCRuntime.h (original) >>> +++ cfe/trunk/lib/CodeGen/CGObjCRuntime.h Tue Feb 2 20:09:30 2010 >>> @@ -95,6 +95,12 @@ >>> /// this compilation unit with the runtime library. >>> virtual llvm::Function *ModuleInitFunction() = 0; >>> >>> + virtual llvm::Constant *GetConstantSelector(Selector Sel) = 0; >>> + >>> + /// Get a typed selector. >>> + virtual llvm::Constant *GetConstantTypedSelector( >>> + const ObjCMethodDecl *Method) = 0; >>> + >>> /// Get a selector for the specified name and type values. The >>> /// return value should have the LLVM type for pointer-to >>> /// ASTContext::getObjCSelType(). >>> >>> Modified: cfe/trunk/lib/Sema/SemaExprObjC.cpp >>> URL: >>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprObjC.cpp?rev=95189&r1=95188&r2=95189&view=diff >>> >>> ============================================================================== >>> --- cfe/trunk/lib/Sema/SemaExprObjC.cpp (original) >>> +++ cfe/trunk/lib/Sema/SemaExprObjC.cpp Tue Feb 2 20:09:30 2010 >>> @@ -140,7 +140,20 @@ >>> Diag(SelLoc, diag::warn_undeclared_selector) << Sel; >>> >>> QualType Ty = Context.getObjCSelType(); >>> - return new (Context) ObjCSelectorExpr(Ty, Sel, AtLoc, RParenLoc); >>> + ObjCSelectorExpr *E = >>> + new (Context) ObjCSelectorExpr(Ty, Sel, AtLoc, RParenLoc); >>> + // Make sure that we have seen this selector. There are lots of checks >>> we >>> + // should be doing on this selector. For example, when this is passed >>> as the >>> + // second argument to objc_msgSend() on the Mac runtime, or as the >>> selector >>> + // argument to the -performSelector:. We can do these checks at run time >>> + // with the GNU runtimes, but the Apple runtimes let you sneak stack >>> + // corruption in easily by passing the wrong selector to these functions >>> if >>> + // there is no static checking. >>> + // >>> + // Only log a warning on the GNU runtime. >>> + E->setMethodDecl(LookupInstanceMethodInGlobalPool(Sel, >>> + SourceRange(LParenLoc, LParenLoc), !LangOpts.NeXTRuntime)); >>> + return E; >>> } >>> >>> Sema::ExprResult Sema::ParseObjCProtocolExpression(IdentifierInfo >>> *ProtocolId, >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > > > -- Sent from my brain > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
