[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
ahatanak added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1125 +llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType()); +return llvm::UndefValue::get(selType); + } mwyman wrote: > nlopes wrote: > > mwyman wrote: > > > nlopes wrote: > > > > Please consider using PoisonValue here instead (if appropriate). We are > > > > trying to get rid of undef. > > > > Thank you! > > > > A poison value is a result of an erroneous operation. > > > > > > I could very well be misunderstanding, but based on this description from > > > LangRef.html `PoisonValue` doesn't seem appropriate here; this is not > > > "erroneous" behavior: it is just continuing the behavior prior to > > > removing the `_cmd` implicit argument from the ABI, which was that the > > > value was `undef` from the generated getter/setter's caller. > > > > > > https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142 > > > > > > Since this is (essentially) replicating that behavior, I'm not sure > > > `PoisonValue` is what we want. > > If the value is not used, then it can be poison. > > If it gets used somehow then it depends on the callers. I don't know > > anything about ObjC to know what the right answer is here. > > As we want to remove undef, the fewer the uses the better. Thank you! > Fair enough. I've changed to pass poison into the helper functions. It looks like `_cmd` is unused in both `objc_getProperty` and `objc_setProperty`, so I think this is fine. https://github.com/opensource-apple/objc4/blob/master/runtime/objc-accessors.mm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman marked 2 inline comments as done. mwyman added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1125 +llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType()); +return llvm::UndefValue::get(selType); + } nlopes wrote: > mwyman wrote: > > nlopes wrote: > > > Please consider using PoisonValue here instead (if appropriate). We are > > > trying to get rid of undef. > > > Thank you! > > > A poison value is a result of an erroneous operation. > > > > I could very well be misunderstanding, but based on this description from > > LangRef.html `PoisonValue` doesn't seem appropriate here; this is not > > "erroneous" behavior: it is just continuing the behavior prior to removing > > the `_cmd` implicit argument from the ABI, which was that the value was > > `undef` from the generated getter/setter's caller. > > > > https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142 > > > > Since this is (essentially) replicating that behavior, I'm not sure > > `PoisonValue` is what we want. > If the value is not used, then it can be poison. > If it gets used somehow then it depends on the callers. I don't know anything > about ObjC to know what the right answer is here. > As we want to remove undef, the fewer the uses the better. Thank you! Fair enough. I've changed to pass poison into the helper functions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman updated this revision to Diff 466212.
mwyman edited the summary of this revision.
mwyman added a comment.
Updated to use `PoisonValue` rather than `UndefValue`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
Files:
clang/lib/CodeGen/CGObjC.cpp
clang/test/CodeGenObjC/direct-method.m
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,15 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method passes undef for the `cmd` argument.
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: [[SELFVAL:%.*]] = load {{.*}} %self.addr,
+// CHECK-NEXT: [[SELF:%.*]] = bitcast {{.*}} [[SELFVAL]] to i8*
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef
poison, i64 noundef [[IVAR]], {{.*}})
+
@interface Foo : Root {
id __strong _cause_cxx_destruct;
}
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -25,6 +25,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Analysis/ObjCARCUtil.h"
#include "llvm/BinaryFormat/MachO.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/InlineAsm.h"
using namespace clang;
@@ -,6 +1112,25 @@
callee, ReturnValueSlot(), args);
}
+// emitCmdValueForGetterSetterBody - Handle emitting the load necessary for
+// the `_cmd` selector argument for getter/setter bodies. For direct methods,
+// this returns an undefined/poison value; this matches behavior prior to
`_cmd`
+// being removed from the direct method ABI as the getter/setter caller would
+// never load one. For non-direct methods, this emits a load of the implicit
+// `_cmd` storage.
+static llvm::Value *emitCmdValueForGetterSetterBody(CodeGenFunction ,
+ ObjCMethodDecl *MD) {
+ if (MD->isDirectMethod()) {
+// Direct methods do not have a `_cmd` argument. Emit an undefined/poison
+// value. This will be passed to objc_getProperty/objc_setProperty, which
+// has not appeared bothered by the `_cmd` argument being undefined before.
+llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+return llvm::PoisonValue::get(selType);
+ }
+
+ return CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(MD->getCmdDecl()),
"cmd");
+}
+
void
CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl
*classImpl,
const ObjCPropertyImplDecl *propImpl,
@@ -1189,8 +1209,7 @@
// Return (ivar-type) objc_getProperty((id) self, _cmd, offset, true).
// FIXME: Can't this be simpler? This might even be worse than the
// corresponding gcc code.
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+llvm::Value *cmd = emitCmdValueForGetterSetterBody(*this, getterMethod);
llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
EmitIvarOffset(classImpl->getClassInterface(), ivar);
@@ -1475,8 +1494,7 @@
// Emit objc_setProperty((id) self, _cmd, offset, arg,
// , ).
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
+llvm::Value *cmd = emitCmdValueForGetterSetterBody(*this, setterMethod);
llvm::Value *self =
Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,15 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method passes undef for the `cmd` argument.
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+//
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
nlopes added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1125 +llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType()); +return llvm::UndefValue::get(selType); + } mwyman wrote: > nlopes wrote: > > Please consider using PoisonValue here instead (if appropriate). We are > > trying to get rid of undef. > > Thank you! > > A poison value is a result of an erroneous operation. > > I could very well be misunderstanding, but based on this description from > LangRef.html `PoisonValue` doesn't seem appropriate here; this is not > "erroneous" behavior: it is just continuing the behavior prior to removing > the `_cmd` implicit argument from the ABI, which was that the value was > `undef` from the generated getter/setter's caller. > > https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142 > > Since this is (essentially) replicating that behavior, I'm not sure > `PoisonValue` is what we want. If the value is not used, then it can be poison. If it gets used somehow then it depends on the callers. I don't know anything about ObjC to know what the right answer is here. As we want to remove undef, the fewer the uses the better. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman added inline comments.
Comment at: clang/lib/CodeGen/CGObjC.cpp:1125
+llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+return llvm::UndefValue::get(selType);
+ }
nlopes wrote:
> Please consider using PoisonValue here instead (if appropriate). We are
> trying to get rid of undef.
> Thank you!
> A poison value is a result of an erroneous operation.
I could very well be misunderstanding, but based on this description from
LangRef.html `PoisonValue` doesn't seem appropriate here; this is not
"erroneous" behavior: it is just continuing the behavior prior to removing the
`_cmd` implicit argument from the ABI, which was that the value was `undef`
from the generated getter/setter's caller.
https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142
Since this is (essentially) replicating that behavior, I'm not sure
`PoisonValue` is what we want.
Comment at: clang/test/CodeGenObjC/direct-method.m:178
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef
undef, i64 noundef [[IVAR]], {{.*}})
+
ahatanak wrote:
> `noundef` means the value isn't undefined, right? Is it safe to use it with
> `undef`? We might want to fix this if it isn't.
It does feel questionable, however the pre-D131424 behavior was similar with
the `undef` value coming from the caller and opaque to these generated
getters/setters.
At least according to the OSS drop of `libobjc`, nothing in
`objc_getProperty`/`objc_setProperty` actually references the `_cmd` argument
so it seems safe-ish for now:
https://github.com/apple-oss-distributions/objc4/blob/8701d5672d3fd3cd817aeb84db1077aafe1a1604/runtime/objc-accessors.mm#L48
FWIW, searching the code, I do see a handful of `noundef undef` in some other
tests.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
nlopes added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1125 +llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType()); +return llvm::UndefValue::get(selType); + } Please consider using PoisonValue here instead (if appropriate). We are trying to get rid of undef. Thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
ahatanak accepted this revision.
ahatanak added a comment.
LGTM
Comment at: clang/test/CodeGenObjC/direct-method.m:178
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef
undef, i64 noundef [[IVAR]], {{.*}})
+
`noundef` means the value isn't undefined, right? Is it safe to use it with
`undef`? We might want to fix this if it isn't.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman marked 2 inline comments as done.
mwyman added inline comments.
Comment at: clang/lib/CodeGen/CGObjC.cpp:1116
+// for the `_cmd` argument that no longer exists for direct methods.
+static llvm::Value *emitCmdLoadForGetterSetterBody(CodeGenFunction ,
+ ObjCMethodDecl *MD) {
ahatanak wrote:
> Since this is loading from an uninitialized alloca, can we just pass an
> `undef` to the call to `objc_get/setProperty`? The optimization passes will
> just do that, but we can still reduce the code size at `-O0`.
Great suggestion! Done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman updated this revision to Diff 465925.
mwyman added a comment.
Herald added a subscriber: nlopes.
Use explicit `undef` for the `cmd` parameter to
`objc_getProperty`/`objc_setProperty` rather declaring and not initializing
storage for the implicit `_cmd`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
Files:
clang/lib/CodeGen/CGObjC.cpp
clang/test/CodeGenObjC/direct-method.m
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,15 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method passes undef for the `cmd` argument.
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: [[SELFVAL:%.*]] = load {{.*}} %self.addr,
+// CHECK-NEXT: [[SELF:%.*]] = bitcast {{.*}} [[SELFVAL]] to i8*
+// CHECK-NEXT: [[IVAR:%.*]] = load {{.*}} @"OBJC_IVAR_$_Root._objectProperty",
+// CHECK-NEXT: call i8* @objc_getProperty(i8* noundef [[SELF]], i8* noundef
undef, i64 noundef [[IVAR]], {{.*}})
+
@interface Foo : Root {
id __strong _cause_cxx_destruct;
}
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -25,6 +25,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Analysis/ObjCARCUtil.h"
#include "llvm/BinaryFormat/MachO.h"
+#include "llvm/IR/Constants.h"
#include "llvm/IR/DataLayout.h"
#include "llvm/IR/InlineAsm.h"
using namespace clang;
@@ -,6 +1112,22 @@
callee, ReturnValueSlot(), args);
}
+// emitCmdLoadForGetterSetterBody - Handle emitting local storage declarations
+// for the `_cmd` argument that no longer exists for direct methods.
+static llvm::Value *emitCmdLoadForGetterSetterBody(CodeGenFunction ,
+ ObjCMethodDecl *MD) {
+ if (MD->isDirectMethod()) {
+// Direct methods no longer have a `_cmd` argument, so storage must be
+// emitted for it to be passed to the property helper. Since the `_cmd`
+// argument was never being initialized by the caller before, still pass
+// an uninitialized/undefined value here.
+llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+return llvm::UndefValue::get(selType);
+ }
+
+ return CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(MD->getCmdDecl()),
"cmd");
+}
+
void
CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl
*classImpl,
const ObjCPropertyImplDecl *propImpl,
@@ -1189,8 +1206,7 @@
// Return (ivar-type) objc_getProperty((id) self, _cmd, offset, true).
// FIXME: Can't this be simpler? This might even be worse than the
// corresponding gcc code.
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+llvm::Value *cmd = emitCmdLoadForGetterSetterBody(*this, getterMethod);
llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
EmitIvarOffset(classImpl->getClassInterface(), ivar);
@@ -1475,8 +1491,7 @@
// Emit objc_setProperty((id) self, _cmd, offset, arg,
// , ).
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
+llvm::Value *cmd = emitCmdLoadForGetterSetterBody(*this, setterMethod);
llvm::Value *self =
Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,15 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method passes undef for the `cmd` argument.
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: [[SELFVAL:%.*]] = load {{.*}} %self.addr,
+// CHECK-NEXT: [[SELF:%.*]] = bitcast {{.*}} [[SELFVAL]] to i8*
+// CHECK-NEXT: [[IVAR:%.*]]
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
ahatanak added inline comments.
Comment at: clang/lib/CodeGen/CGObjC.cpp:1116
+// for the `_cmd` argument that no longer exists for direct methods.
+static llvm::Value *emitCmdLoadForGetterSetterBody(CodeGenFunction ,
+ ObjCMethodDecl *MD) {
Since this is loading from an uninitialized alloca, can we just pass an `undef`
to the call to `objc_get/setProperty`? The optimization passes will just do
that, but we can still reduce the code size at `-O0`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman updated this revision to Diff 465896.
mwyman marked an inline comment as done.
mwyman added a comment.
Extracted the common new code into a helper function.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
Files:
clang/lib/CodeGen/CGObjC.cpp
clang/test/CodeGenObjC/direct-method.m
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,18 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: entry:
+// CHECK-NEXT: [[RETVAL:%.*]] = alloca i8*,
+// CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
+// CHECK-NEXT: [[CMDVAL:%_cmd]] = alloca i8*,
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK: [[CMD:%.*]] = load i8*, i8** [[CMDVAL]],
+// CHECK: call i8* @objc_getProperty({{.*}}, i8*{{.*}} [[CMD]], {{.*}})
+
@interface Foo : Root {
id __strong _cause_cxx_destruct;
}
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -,6 +,21 @@
callee, ReturnValueSlot(), args);
}
+// emitCmdLoadForGetterSetterBody - Handle emitting local storage declarations
+// for the `_cmd` argument that no longer exists for direct methods.
+static llvm::Value *emitCmdLoadForGetterSetterBody(CodeGenFunction ,
+ ObjCMethodDecl *MD) {
+ if (MD->isDirectMethod()) {
+// Direct methods no longer have a `_cmd` argument, so storage must be
+// emitted for it to be passed to the property helper. Since the `_cmd`
+// argument was never being initialized by the caller before, still pass
+// an uninitialized/undefined value here.
+CGF.EmitVarDecl(*MD->getCmdDecl());
+ }
+
+ return CGF.Builder.CreateLoad(CGF.GetAddrOfLocalVar(MD->getCmdDecl()),
"cmd");
+}
+
void
CodeGenFunction::generateObjCGetterBody(const ObjCImplementationDecl
*classImpl,
const ObjCPropertyImplDecl *propImpl,
@@ -1189,8 +1204,7 @@
// Return (ivar-type) objc_getProperty((id) self, _cmd, offset, true).
// FIXME: Can't this be simpler? This might even be worse than the
// corresponding gcc code.
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+llvm::Value *cmd = emitCmdLoadForGetterSetterBody(*this, getterMethod);
llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
EmitIvarOffset(classImpl->getClassInterface(), ivar);
@@ -1475,8 +1489,7 @@
// Emit objc_setProperty((id) self, _cmd, offset, arg,
// , ).
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
+llvm::Value *cmd = emitCmdLoadForGetterSetterBody(*this, setterMethod);
llvm::Value *self =
Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,18 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: entry:
+// CHECK-NEXT: [[RETVAL:%.*]] = alloca i8*,
+// CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
+// CHECK-NEXT: [[CMDVAL:%_cmd]] = alloca i8*,
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK: [[CMD:%.*]] = load i8*, i8** [[CMDVAL]],
+// CHECK: call i8* @objc_getProperty({{.*}}, i8*{{.*}} [[CMD]], {{.*}})
+
@interface Foo : Root {
id __strong _cause_cxx_destruct;
}
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
plotfi added a comment.
LGTM but waiting on Akira would be nice imho.
Comment at: clang/lib/CodeGen/CGObjC.cpp:1192
// corresponding gcc code.
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+if (getterMethod->isDirectMethod()) {
+ // Direct methods no longer have a `_cmd` argument, so storage must be
Could this entire code sequence be moved to a helper or helper method? I think
it could be good if the _cmd argument emission and the associated code comments
were only written once.
Comment at: clang/lib/CodeGen/CGObjC.cpp:1485
// , ).
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
-llvm::Value *self =
- Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
+if (setterMethod->isDirectMethod()) {
+ // Direct methods no longer have a `_cmd` argument, so storage must be
Ditto
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
plotfi added a comment. @ahatanak how does this diff look to you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
stephanemoore accepted this revision. stephanemoore added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1194-1196 + // emitted for it to be passed to the property helper. Since the `_cmd` + // argument was never being initialized by the caller before, still pass + // an uninitialized/undefined value here. Thanks for matching the previous behavior Comment at: clang/lib/CodeGen/CGObjC.cpp:1493 +llvm::Value *cmd = Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()), + "cmd"); +llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy); Previously this was using the default "" for the `Name` instead of "cmd". I can't think of why it would intentionally want to use "". Using "cmd" is consistent with the codegen for the getter (now and before) so I don't disagree with using "cmd" for the name here. Comment at: clang/lib/CodeGen/CGObjC.cpp:1494 + "cmd"); +llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy); llvm::Value *ivarOffset = Strictly speaking, I think we can leave this line untouched now? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135091/new/ https://reviews.llvm.org/D135091 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman marked an inline comment as done.
mwyman added inline comments.
Comment at: clang/test/CodeGenObjC/direct-method.m:171-177
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK-NEXT: load i8*, i8** @OBJC_SELECTOR_REFERENCES_
+// CHECK: call i8* @objc_getProperty({{.*}})
stephanemoore wrote:
> I'm not intricately familiar with the codegen checking mechanism in these
> tests but without fully knowing the details, this seems consistent with the
> checks in `-[Root accessCmd]` above which I would expect to require similar
> checks.
>
> Did you check that these added tests reproduce the previous assertion/failure
> so that we can be confident that the code changes resolve the issue?
Yes, this new test previously triggered the assertion.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.
mwyman updated this revision to Diff 465857.
mwyman retitled this revision from "Load the `_cmd` selector for generated
getters/setters of `direct` Objective-C properties." to "Create storage for the
`_cmd` argument to the helper function for generated getters/setters of
`direct` Objective-C properties.".
mwyman edited the summary of this revision.
mwyman added a comment.
Updated to eliminate loading the selector, as the prior implementation didn't
pass a valid `_cmd` value (it just re-used the `_cmd` argument in the ABI,
which was unset by the caller and so undefined in value).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135091/new/
https://reviews.llvm.org/D135091
Files:
clang/lib/CodeGen/CGObjC.cpp
clang/test/CodeGenObjC/direct-method.m
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,18 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+// CHECK-LABEL: entry:
+// CHECK-NEXT: [[RETVAL:%.*]] = alloca i8*,
+// CHECK-NEXT: [[SELFADDR:%.*]] = alloca %0*,
+// CHECK-NEXT: [[CMDVAL:%_cmd]] = alloca i8*,
+// CHECK-LABEL: objc_direct_method.cont:
+// CHECK: [[CMD:%.*]] = load i8*, i8** [[CMDVAL]],
+// CHECK: call i8* @objc_getProperty({{.*}}, i8*{{.*}} [[CMD]], {{.*}})
+
@interface Foo : Root {
id __strong _cause_cxx_destruct;
}
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -1189,8 +1189,15 @@
// Return (ivar-type) objc_getProperty((id) self, _cmd, offset, true).
// FIXME: Can't this be simpler? This might even be worse than the
// corresponding gcc code.
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()), "cmd");
+if (getterMethod->isDirectMethod()) {
+ // Direct methods no longer have a `_cmd` argument, so storage must be
+ // emitted for it to be passed to the property helper. Since the `_cmd`
+ // argument was never being initialized by the caller before, still pass
+ // an uninitialized/undefined value here.
+ EmitVarDecl(*getterMethod->getCmdDecl());
+}
+llvm::Value *cmd =
Builder.CreateLoad(GetAddrOfLocalVar(getterMethod->getCmdDecl()),
+ "cmd");
llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
EmitIvarOffset(classImpl->getClassInterface(), ivar);
@@ -1475,10 +1482,16 @@
// Emit objc_setProperty((id) self, _cmd, offset, arg,
// , ).
-llvm::Value *cmd =
- Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()));
-llvm::Value *self =
- Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
+if (setterMethod->isDirectMethod()) {
+ // Direct methods no longer have a `_cmd` argument, so storage must be
+ // emitted for it to be passed to the property helper. Since the `_cmd`
+ // argument was never being initialized by the caller before, still pass
+ // an uninitialized/undefined value here.
+ EmitVarDecl(*setterMethod->getCmdDecl());
+}
+llvm::Value *cmd =
Builder.CreateLoad(GetAddrOfLocalVar(setterMethod->getCmdDecl()),
+ "cmd");
+llvm::Value *self = Builder.CreateBitCast(LoadObjCSelf(), VoidPtrTy);
llvm::Value *ivarOffset =
EmitIvarOffset(classImpl->getClassInterface(), ivar);
Address argAddr = GetAddrOfLocalVar(*setterMethod->param_begin());
Index: clang/test/CodeGenObjC/direct-method.m
===
--- clang/test/CodeGenObjC/direct-method.m
+++ clang/test/CodeGenObjC/direct-method.m
@@ -14,6 +14,7 @@
- (int)getInt __attribute__((objc_direct));
@property(direct, readonly) int intProperty;
@property(direct, readonly) int intProperty2;
+@property(direct, readonly) id objectProperty;
@end
@implementation Root
@@ -167,6 +168,18 @@
@end
// CHECK-LABEL: define hidden i32 @"\01-[Root intProperty]"
+// Check the synthesized objectProperty calls objc_getProperty(); this also
+// checks that the synthesized method accesses _cmd (or rather loads the
+// selector, as it is an argument to objc_getProperty).
+// CHECK-LABEL: define hidden i8* @"\01-[Root objectProperty]"(
+//
