[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak closed this revision.
ahatanak added a comment.

Fixed in d8136f14f125fb27f2326f397df0964bf62078ca 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Yes, that's great, thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1054
+ "pointer to __strong expected");
+  EmitStoreOfScalar(*AI++, LV);
+} else

rjmccall wrote:
> This definitely deserves a comment.
> 
> I don't think the assertion is right; the condition is that the type is legal 
> for a field in a struct that can be passed directly, and while that does 
> exclude `__weak` (because the struct will have to be passed indirectly) and 
> `__autoreleasing` (because that's not legal in a struct), it doesn't exclude 
> `__unsafe_unretained`.
> 
> This function is implementing an operation that's broadly meaningful (it's a 
> store-init of an owned value, in contrast to a store-init with an unowned 
> value which is what `isInit` is implementing) but not extensively used in the 
> C frontend.  On some level, I feel like we should probably teach 
> `EmitStoreThroughLValue` to handle that properly, but that's a more 
> significant refactor.
> 
> It does look like this change isn't enough to handle `__ptrauth`, which will 
> assume that the source value is signed appropriately for the unqualified type 
> when probably it should be signed appropriately for the qualifier (which, 
> like `__weak`, cannot be address-discriminated because it would stop being 
> passed directly).  Probably the default case should be to use 
> `EmitStoreOfScalar`, and `EmitStoreThroughLValue` should only be used if the 
> l-value is a bit-field (the only non-simple case that can actually happen 
> from drilling down to a field).
> 
> The same logic applies on the load side in the abstract, except that it is 
> only causing problems for `__ptrauth` (well, if we change the behavior above) 
> because loads from the ARC qualifiers don't implicitly retain.  Regardless, 
> analogously to this, `EmitRValueForField` should only be calling 
> `EmitLoadOfLValue` for bit-fields and should otherwise call 
> `EmitLoadOfScalar`.  Please add a comment on both sides making it clear that 
> the logic is expected to be parallel.
Ah right, the assertion isn't correct. My intention was to catch `__weak` 
pointers.

Let me know if the comment I added is OK.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak updated this revision to Diff 232027.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.

Call `EmitStoreThroughLValue` and `EmitLoadOfLValue` only when the lvalue is a 
bitfield.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGenObjC/nontrivial-struct-param-init.m


Index: clang/test/CodeGenObjC/nontrivial-struct-param-init.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/nontrivial-struct-param-init.m
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple i386-apple-watchos6.0-simulator -emit-llvm -fblocks 
-fobjc-arc -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+typedef struct {
+  id x;
+} S;
+
+// CHECK: define void @test0(i8* %[[A_0:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_S]], align 4
+// CHECK: %[[X:.*]] = getelementptr inbounds %[[STRUCT_S]], %[[STRUCT_S]]* 
%[[A]], i32 0, i32 0
+// CHECK: store i8* %[[A_0]], i8** %[[X]], align 4
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_S]]* %[[A]] to i8**
+// CHECK: call void @__destructor_4_s0(i8** %[[V0]]) #2
+
+void test0(S a) {
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4544,7 +4544,11 @@
 // don't load reference fields.
 if (FD->getType()->isReferenceType())
   return RValue::get(FieldLV.getPointer(*this));
-return EmitLoadOfLValue(FieldLV, Loc);
+// Call EmitLoadOfScalar except when the lvalue is a bitfield to emit a
+// primitive load.
+if (FieldLV.isBitField())
+  return EmitLoadOfLValue(FieldLV, Loc);
+return RValue::get(EmitLoadOfScalar(FieldLV, Loc));
   }
   llvm_unreachable("bad evaluation kind");
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1047,8 +1047,13 @@
 auto imagValue = *AI++;
 EmitStoreOfComplex(ComplexPairTy(realValue, imagValue), LV, /*init*/ true);
   } else {
+// Call EmitStoreOfScalar except when the lvalue is a bitfield to emit a
+// primitive store.
 assert(isa(Exp.get()));
-EmitStoreThroughLValue(RValue::get(*AI++), LV);
+if (LV.isBitField())
+  EmitStoreThroughLValue(RValue::get(*AI++), LV);
+else
+  EmitStoreOfScalar(*AI++, LV);
   }
 }
 


Index: clang/test/CodeGenObjC/nontrivial-struct-param-init.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/nontrivial-struct-param-init.m
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple i386-apple-watchos6.0-simulator -emit-llvm -fblocks -fobjc-arc -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+typedef struct {
+  id x;
+} S;
+
+// CHECK: define void @test0(i8* %[[A_0:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_S]], align 4
+// CHECK: %[[X:.*]] = getelementptr inbounds %[[STRUCT_S]], %[[STRUCT_S]]* %[[A]], i32 0, i32 0
+// CHECK: store i8* %[[A_0]], i8** %[[X]], align 4
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_S]]* %[[A]] to i8**
+// CHECK: call void @__destructor_4_s0(i8** %[[V0]]) #2
+
+void test0(S a) {
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -4544,7 +4544,11 @@
 // don't load reference fields.
 if (FD->getType()->isReferenceType())
   return RValue::get(FieldLV.getPointer(*this));
-return EmitLoadOfLValue(FieldLV, Loc);
+// Call EmitLoadOfScalar except when the lvalue is a bitfield to emit a
+// primitive load.
+if (FieldLV.isBitField())
+  return EmitLoadOfLValue(FieldLV, Loc);
+return RValue::get(EmitLoadOfScalar(FieldLV, Loc));
   }
   llvm_unreachable("bad evaluation kind");
 }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1047,8 +1047,13 @@
 auto imagValue = *AI++;
 EmitStoreOfComplex(ComplexPairTy(realValue, imagValue), LV, /*init*/ true);
   } else {
+// Call EmitStoreOfScalar except when the lvalue is a bitfield to emit a
+// primitive store.
 assert(isa(Exp.get()));
-EmitStoreThroughLValue(RValue::get(*AI++), LV);
+if (LV.isBitField())
+  EmitStoreThroughLValue(RValue::get(*AI++), LV);
+else
+  EmitStoreOfScalar(*AI++, LV);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:1054
+ "pointer to __strong expected");
+  EmitStoreOfScalar(*AI++, LV);
+} else

This definitely deserves a comment.

I don't think the assertion is right; the condition is that the type is legal 
for a field in a struct that can be passed directly, and while that does 
exclude `__weak` (because the struct will have to be passed indirectly) and 
`__autoreleasing` (because that's not legal in a struct), it doesn't exclude 
`__unsafe_unretained`.

This function is implementing an operation that's broadly meaningful (it's a 
store-init of an owned value, in contrast to a store-init with an unowned value 
which is what `isInit` is implementing) but not extensively used in the C 
frontend.  On some level, I feel like we should probably teach 
`EmitStoreThroughLValue` to handle that properly, but that's a more significant 
refactor.

It does look like this change isn't enough to handle `__ptrauth`, which will 
assume that the source value is signed appropriately for the unqualified type 
when probably it should be signed appropriately for the qualifier (which, like 
`__weak`, cannot be address-discriminated because it would stop being passed 
directly).  Probably the default case should be to use `EmitStoreOfScalar`, and 
`EmitStoreThroughLValue` should only be used if the l-value is a bit-field (the 
only non-simple case that can actually happen from drilling down to a field).

The same logic applies on the load side in the abstract, except that it is only 
causing problems for `__ptrauth` (well, if we change the behavior above) 
because loads from the ARC qualifiers don't implicitly retain.  Regardless, 
analogously to this, `EmitRValueForField` should only be calling 
`EmitLoadOfLValue` for bit-fields and should otherwise call `EmitLoadOfScalar`. 
 Please add a comment on both sides making it clear that the logic is expected 
to be parallel.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

Note that passing `isInit=true` to `EmitStoreThroughLValue` to make it emit 
`llvm.objc.retain` would be incorrect since the callee destructs the struct 
argument.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70935/new/

https://reviews.llvm.org/D70935



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak created this revision.
ahatanak added a reviewer: rjmccall.
ahatanak added a project: clang.
Herald added subscribers: ributzka, dexonsmith, jkorous.

This fixes a bug in IRGen where a call to `llvm.objc.storeStrong` was being 
emitted to initialize a __strong field of an uninitialized temporary struct, 
which caused crashes at runtime.
rdar://problem/51807365


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70935

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGenObjC/nontrivial-struct-param-init.m


Index: clang/test/CodeGenObjC/nontrivial-struct-param-init.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/nontrivial-struct-param-init.m
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple i386-apple-watchos6.0-simulator -emit-llvm -fblocks 
-fobjc-arc -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+typedef struct {
+  id x;
+} S;
+
+// CHECK: define void @test0(i8* %[[A_0:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_S]], align 4
+// CHECK: %[[X:.*]] = getelementptr inbounds %[[STRUCT_S]], %[[STRUCT_S]]* 
%[[A]], i32 0, i32 0
+// CHECK: store i8* %[[A_0]], i8** %[[X]], align 4
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_S]]* %[[A]] to i8**
+// CHECK: call void @__destructor_4_s0(i8** %[[V0]]) #2
+
+void test0(S a) {
+}
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1048,7 +1048,12 @@
 EmitStoreOfComplex(ComplexPairTy(realValue, imagValue), LV, /*init*/ true);
   } else {
 assert(isa(Exp.get()));
-EmitStoreThroughLValue(RValue::get(*AI++), LV);
+if (LV.getQuals().getObjCLifetime()) {
+  assert(LV.getQuals().getObjCLifetime() == Qualifiers::OCL_Strong &&
+ "pointer to __strong expected");
+  EmitStoreOfScalar(*AI++, LV);
+} else
+  EmitStoreThroughLValue(RValue::get(*AI++), LV);
   }
 }
 


Index: clang/test/CodeGenObjC/nontrivial-struct-param-init.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/nontrivial-struct-param-init.m
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple i386-apple-watchos6.0-simulator -emit-llvm -fblocks -fobjc-arc -o - %s | FileCheck %s
+
+// CHECK: %[[STRUCT_S:.*]] = type { i8* }
+
+typedef struct {
+  id x;
+} S;
+
+// CHECK: define void @test0(i8* %[[A_0:.*]])
+// CHECK: %[[A:.*]] = alloca %[[STRUCT_S]], align 4
+// CHECK: %[[X:.*]] = getelementptr inbounds %[[STRUCT_S]], %[[STRUCT_S]]* %[[A]], i32 0, i32 0
+// CHECK: store i8* %[[A_0]], i8** %[[X]], align 4
+// CHECK: %[[V0:.*]] = bitcast %[[STRUCT_S]]* %[[A]] to i8**
+// CHECK: call void @__destructor_4_s0(i8** %[[V0]]) #2
+
+void test0(S a) {
+}
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -1048,7 +1048,12 @@
 EmitStoreOfComplex(ComplexPairTy(realValue, imagValue), LV, /*init*/ true);
   } else {
 assert(isa(Exp.get()));
-EmitStoreThroughLValue(RValue::get(*AI++), LV);
+if (LV.getQuals().getObjCLifetime()) {
+  assert(LV.getQuals().getObjCLifetime() == Qualifiers::OCL_Strong &&
+ "pointer to __strong expected");
+  EmitStoreOfScalar(*AI++, LV);
+} else
+  EmitStoreThroughLValue(RValue::get(*AI++), LV);
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits