Constant strings emitted when `-fno-constant-cfstrings` is passed should allow dead stripping

[bendjones]

Summary

Part of rdar://56643852 that makes all possible constant string code paths emit without no_dead_strip so that dead stripping is allowed.

[dexonsmith]

The code looks right, but can you add a test for this in CodeGenObjC? Also, when you re-upload the patch with the tests, please use -U9999999 to provide full context in the diff.

[bendjones]

In D70284#1746853, @dexonsmith wrote:

The code looks right, but can you add a test for this in CodeGenObjC? Also, when you re-upload the patch with the tests, please use -U9999999 to provide full context in the diff.

Will do

[bendjones]

Added "objc_arc_inert" and updated clang/test/CodeGenObjC/ns-constant-strings.m tests to make sure the section is emitted as we want.

[bendjones]

Any additional thoughts @dexonsmith @erik.pilkington @ahatanak?

[dexonsmith]

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

The two comments:

• Please add a period at the end of the sentence in the comment.
• Can you give more context about what objc_arc_inert is doing, and why it's necessary now that no_dead_strip is gone?

[bendjones]

In D70284#1752811, @dexonsmith wrote:

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

Thought I did.

The two comments:

• Please add a period at the end of the sentence in the comment.

Will do.

• Can you give more context about what objc_arc_inert is doing, and why it's necessary now that no_dead_strip is gone?

The objc_arc_inert was added to other similar things so in the spirt of making things match I added it here. I can keep it simple though.

[bendjones]

In D70284#1752806, @bendjones wrote:

Any additional thoughts @dexonsmith @erik.pilkington @ahatanak?

@dexonsmith @erik.pilkington @ahatanak does this look good to go?

[dexonsmith]

In D70284#1752893, @bendjones wrote:

In D70284#1752811, @dexonsmith wrote:

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

Thought I did.

I'm still not able to edit this, so I cannot "accept revision". I'm not sure what's going wrong but this revision object seems corrupted.

The two comments:

• Please add a period at the end of the sentence in the comment.

Will do.

The comment actually looks liable to bitrot, since IIUC it's talking about callers which can change over time.

LGTM if you remove it.

• Can you give more context about what objc_arc_inert is doing, and why it's necessary now that no_dead_strip is gone?

The objc_arc_inert was added to other similar things so in the spirt of making things match I added it here. I can keep it simple though.

Okay, SGTM. There are no ARC operations, and this will allow certain optimizations. I suggest documenting why it's safe in the commit message.

[bendjones]

In D70284#1978948, @dexonsmith wrote:

In D70284#1752893, @bendjones wrote:

In D70284#1752811, @dexonsmith wrote:

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

Thought I did.

I'm still not able to edit this, so I cannot "accept revision". I'm not sure what's going wrong but this revision object seems corrupted.

Hmm ok should I redo this completely then? Not sure what is up but it shows you as a reviewer.... hmm...

The two comments:

• Please add a period at the end of the sentence in the comment.

Will do.

The comment actually looks liable to bitrot, since IIUC it's talking about callers which can change over time.

LGTM if you remove it.

Ok will do...

• Can you give more context about what objc_arc_inert is doing, and why it's necessary now that no_dead_strip is gone?

The objc_arc_inert was added to other similar things so in the spirt of making things match I added it here. I can keep it simple though.

Okay, SGTM. There are no ARC operations, and this will allow certain optimizations. I suggest documenting why it's safe in the commit message.

So just stating that I'm keeping things in sync with other areas?

[dexonsmith]

Hmm ok should I redo this completely then? Not sure what is up but it shows you as a reviewer.... hmm...

This revision is old at this point, and I can't remember if you created a new revision with a separate D-number? I still can't "request changes". If you've already committed you should be able to "abandon" this one.

[dexonsmith]

Ah, I think it's because you filled in the "project" field which I think we usually leave blank. Maybe now I can "request changes".

EDIT: confirmed; this went through after I added myself to the "clang" project.

clang/lib/CodeGen/CGObjCMac.cpp

1981

Please drop this comment.

clang/test/CodeGenObjC/ns-constant-strings.m

47

Please fix this missing newline.

clang/lib/CodeGen/CGObjCMac.cpp

llvm::Constant *CGObjCNonFragileABIMac::getNSConstantStringClassRef() {
 
   // Make sure the result is of the correct type.
   auto V = llvm::ConstantExpr::getBitCast(GV, CGM.IntTy->getPointerTo());
 
   ConstantStringClassRef = V;
   return V;
 }
 
+// This is only used when `-fno-constant-cfstrings` is given
 ConstantAddress
 CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   unsigned StringLength = 0;
   llvm::StringMapEntry<llvm::GlobalVariable *> &Entry =
     GetConstantStringEntry(NSConstantStringMap, Literal, StringLength);
 
   if (auto *C = Entry.second)
     return ConstantAddress(C, CharUnits::fromQuantity(C->getAlignment()));
CGObjCCommonMac::GenerateConstantNSString(const StringLiteral *Literal) {
   // String length.
   Fields.addInt(CGM.IntTy, StringLength);
 
   // The struct.
   CharUnits Alignment = CGM.getPointerAlign();
   GV = Fields.finishAndCreateGlobal("_unnamed_nsstring_", Alignment,
                                     /*constant*/ true,
                                     llvm::GlobalVariable::PrivateLinkage);
-  const char *NSStringSection = "__OBJC,__cstring_object,regular,no_dead_strip";
-  const char *NSStringNonFragileABISection =
-      "__DATA,__objc_stringobj,regular,no_dead_strip";
-  // FIXME. Fix section.
+  const char *NSStringSection = "__OBJC,__cstring_object,regular";
+  const char *NSStringNonFragileABISection = "__DATA,__objc_stringobj,regular";
   GV->setSection(CGM.getLangOpts().ObjCRuntime.isNonFragile()
                      ? NSStringNonFragileABISection
                      : NSStringSection);
+  GV->addAttribute("objc_arc_inert");
   Entry.second = GV;
 
   return ConstantAddress(GV, Alignment);
 }
 
 enum {
   kCFTaggedObjectID_Integer = (1 << 1) + 1
 };

clang/test/CodeGenObjC/ns-constant-strings.m

int main() {
   const NSConstantString *appKey =  @"MyApp";
   const NSConstantString *appKey1 =  @"MyApp1";
 }
 
 // CHECK-FRAGILE: @_NSConstantStringClassReference = external global
 // CHECK-NONFRAGILE: @"OBJC_CLASS_$_NSConstantString" = external global
 
 // CHECK-FRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_ = private constant %struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str, i32 0, i32 0), i32 5 }, section "__OBJC,__cstring_object,regular"
 // CHECK-FRAGILE: @.str.1 = private unnamed_addr constant [7 x i8] c"MyApp1\00"
+// CHECK-FRAGILE: @_unnamed_nsstring_.2 = private constant %struct.__builtin_NSString { i32* getelementptr inbounds ([0 x i32], [0 x i32]* @_NSConstantStringClassReference, i32 0, i32 0), i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str.1, i32 0, i32 0), i32 6 }, section "__OBJC,__cstring_object,regular"
 
 // CHECK-NONFRAGILE: @.str = private unnamed_addr constant [6 x i8] c"MyApp\00"
+// CHECK-NONFRAGILE: @_unnamed_nsstring_ = private constant %struct.__builtin_NSString { i32* bitcast (%struct._class_t* @"OBJC_CLASS_$_NSConstantString" to i32*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @.str, i32 0, i32 0), i32 5 }, section "__DATA,__objc_stringobj,regular"
 // CHECK-NONFRAGILE: @.str.1 = private unnamed_addr constant [7 x i8] c"MyApp1\00"
+// CHECK-NONFRAGILE: @_unnamed_nsstring_.2 = private constant %struct.__builtin_NSString { i32* bitcast (%struct._class_t* @"OBJC_CLASS_$_NSConstantString" to i32*), i8* getelementptr inbounds ([7 x i8], [7 x i8]* @.str.1, i32 0, i32 0), i32 6 }, section "__DATA,__objc_stringobj,regular"
+
+// CHECK-FRAGILE: attributes #0 = { "objc_arc_inert" }
+// CHECK-NONFRAGILE: attributes #0 = { "objc_arc_inert" }
 No newline at end of file