This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen][ObjC] Do not emit objc_storeStrong to initialize a constexpr variable
ClosedPublic

Authored by ahatanak on Oct 12 2016, 10:03 PM.

Details

Summary

When compiling a constexpr NSString initialized with an objective-c string literal, CodeGen currently emits objc_storeStrong on an uninitialized alloca, which causes a crash:

constexpr NSString *S = @"abc";
%S = alloca %0*, align 8
%0 = bitcast %0** %S to i8** ; this points to uninitialized memory
call void @objc_storeStrong(i8** %0, i8* bitcast (%struct.__NSConstantString_tag* @_unnamed_cfstring_ to i8*)) #1

This patch fixes the crash by directly storing the pointer to the __NSConstantString_tag for the string into the allocated alloca. The rationale for this change is that retain or release on an Objective-c string literal (or anything used to initialize a constexpr variable) has no effect and therefore objc_storeStrong isn't needed in this case.

rdar://problem/28562009

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 74473.Oct 12 2016, 10:03 PM
ahatanak retitled this revision from to [CodeGen][ObjC] Do not emit objc_storeStrong to initialize a constexpr variable.
ahatanak updated this object.
ahatanak added a reviewer: rjmccall.
ahatanak added a subscriber: cfe-commits.
rjmccall edited edge metadata.Oct 13 2016, 11:43 AM

The correct fix is to honor isInit by folding the logic for EmitScalarInit into this function. That should allow you to eliminate EmitScalarInit completely, although it would be fine to leave it as just a call to EmitStoreThroughLValue. I did a quick audit of all the calls to EmitStoreThroughLValue that might pass isInit=true, and it does look like none of them are relying on the current behavior for ARC ownership; the most suspicious are the calls from EmitObjCCollectionLiteral, but the l-value there is non lifetime-qualified, so it's fine.

There are two overloaded functions of CodeGenFunction::EmitScalarInit. Are you suggesting we fold both of them into EmitStoreThroughLValue and remove them?

Sorry, no, just the one that takes an llvm::Value* instead of an Expr*.

ahatanak updated this revision to Diff 74760.Oct 14 2016, 5:38 PM
ahatanak edited edge metadata.

Fold EmitScalarInit into EmitStoreThroughLValue and remove EmitScalarInit.

I don't think ElementType in EmitObjCCollectionLiteral has a lifetime qualifier, so it should be safe to call EmitStoreThroughLValue instead of EmitScalarInit in that function.

rjmccall added inline comments.Oct 14 2016, 7:12 PM
lib/CodeGen/CGExpr.cpp
1649

I think you can fold this a bit more. :) You have exactly the same switch statement below, and several of the cases are identical; for the others, you can just sink the isInit check into the case.

Note that calling EmitStoreOfScalar and returning has the same behavior as "falling into the normal path". isObjCWeak() / isObjCStrong() are checking for the GC qualifiers, which are exclusive with ARC lifetime.

ahatanak updated this revision to Diff 74851.Oct 17 2016, 8:16 AM

Simplify the code a bit more.

Thanks! A couple minor tweaks, then LGTM.

lib/CodeGen/CGExpr.cpp
1643

These two cases are actually identical in behavior (because __autoreleased variables don't actually own their current value). I think I prefer the second way of writing it.

lib/CodeGen/CGObjC.cpp
1665 ↗(On Diff #74851)

Please add /*isInit*/ comments here and elsewhere.

ahatanak updated this revision to Diff 74944.Oct 17 2016, 10:36 PM

Address review comments. Simplify and add comments.

rjmccall accepted this revision.Oct 18 2016, 12:19 AM
rjmccall edited edge metadata.

Thanks, that looks great! LGTM.

This revision is now accepted and ready to land.Oct 18 2016, 12:19 AM
This revision was automatically updated to reflect the committed changes.
ahatanak marked 2 inline comments as done.