[CodeGen] Replace '@' characters in block descriptors' symbol names with '\1' on ELF targets.

[ahatanak]

Summary

This fixes a bug introduced in r340041 (see the discussion in https://reviews.llvm.org/D50783).

'@' can't be used in block descriptors' symbol names since it is reserved on ELF platforms as a separator between symbol names and symbol versions.

I don't know of any forbidden symbols on Windows PE/COFF symbols, but if there are, I can make changes similar to the ones I made in this patch.

[rjmccall]

This should not be changing the actual @-encoding value. If we want to use the @-encoding in the symbol name, we can change it when naming the symbol, provided that \1 is not legal in the @encoding.

[ahatanak]

In D54539#1298949, @rjmccall wrote:

This should not be changing the actual @-encoding value. If we want to use the @-encoding in the symbol name, we can change it when naming the symbol, provided that \1 is not legal in the @encoding.

This patch defines a new virtual method getObjCEncodingForBlock, which is called only by getBlockDescriptorName. It doesn't change what @encode returns.

Or perhaps I completely misunderstood your comment?

[rjmccall]

Oh, I see, sorry. Please rename these functions to make their purpose clear.

Also, you could reasonably do this substitution on all targets.

[theraven]

lib/CodeGen/CGObjCRuntime.h

288 ↗

(On Diff #174084)

I'm not sure that this actually belongs in CGObjCRuntime. The runtime doesn't care about the symbol namings, and the need for this mangling applies to any ELF platform (it would also apply to Mach-O if Mach-O adopted GNU-style symbol versioning). It probably isn't relevant, for example, with WebAssembly, irrespective of the runtime, and it would be relevant for the Apple family of runtimes if anyone wanted to use them on ELF.

If it does go in CGObjCRuntime, I think I'd prefer to have the implementation there as well and make it conditional on the object format, not on the runtime. Having a function that maps Objective-C type encodings to valid symbol names in CGObjCRuntime would let us do a little bit of cleanup in CGObjCGNU* and could also be used in the blocks code.

[theraven]

Please can we either merge this or revert the original change that introduced the bug that this is fixing? It's now been 6 months since blocks generated invalid ELF symbols.

[ahatanak]

Sorry for the delay. The updated patch replaces '@' with '\1' on all targets.

[theraven]

Looks like a much cleaner change.

cfe/trunk/lib/CodeGen/CGBlocks.cpp

for (const BlockCaptureManagedEntity &E : ManagedCaptures) {
                                    BlockInfo.BlockAlign, CGM);
       }
     }
     Name += "_";
   }
 
   std::string TypeAtEncoding =
       CGM.getContext().getObjCEncodingForBlock(BlockInfo.getBlockExpr());
+  /// Replace occurrences of '@' with '\1'. '@' is reserved on ELF platforms as
+  /// a separator between symbol name and symbol version.
+  std::replace(TypeAtEncoding.begin(), TypeAtEncoding.end(), '@', '\1');
   Name += "e" + llvm::to_string(TypeAtEncoding.size()) + "_" + TypeAtEncoding;
   Name += "l" + CGM.getObjCRuntime().getRCBlockLayoutStr(CGM, BlockInfo);
   return Name;
 }
 
 /// buildBlockDescriptor - Build the block descriptor meta-data for a block.
 /// buildBlockDescriptor is accessed from 5th field of the Block_literal
 /// meta-data and contains stationary information about the block literal.

cfe/trunk/test/CodeGenObjC/block-desc-str.m

+// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -emit-llvm -fobjc-runtime=gnustep-1.7 -fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -emit-llvm -fobjc-runtime=gcc -fblocks -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple=x86_64-apple-darwin10 -emit-llvm -fblocks -o - %s | FileCheck %s
+
+// Test that descriptor symbol names don't include '@'.
+
+// CHECK: @[[STR:.*]] = private unnamed_addr constant [6 x i8] c"v8@?0\00"
+// CHECK: @"__block_descriptor_40_8_32o_e5_v8\01?0l" = linkonce_odr hidden unnamed_addr constant { i64, i64, i8*, i8*, i8*, {{.*}} } { i64 0, i64 40, i8* bitcast ({{.*}} to i8*), i8* bitcast ({{.*}} to i8*), i8* getelementptr inbounds ([6 x i8], [6 x i8]* @[[STR]], i32 0, i32 0), {{.*}} }, align 8
+
+typedef void (^BlockTy)(void);
+
+void test(id a) {
+  BlockTy b = ^{ (void)a; };
+}

cfe/trunk/test/CodeGenObjCXX/block-nested-in-lambda.mm

 void use(id);
 
 // Block copy/dispose helpers aren't needed because 'a' is captured by
 // reference.
 
 // CHECK-LABEL: define void @_ZN18CaptureByReference5test0Ev(
 // CHECK-LABEL: define internal void @"_ZZN18CaptureByReference5test0EvENK3$_1clEv"(
 // CHECK: %[[BLOCK_DESCRIPTOR:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8** }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8** }>* %{{.*}}, i32 0, i32 4
-// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i64 }* @"__block_descriptor_40_e5_v8@?0ls32l8" to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
+// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i64 }* @"__block_descriptor_40_e5_v8\01?0ls32l8" to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
 
 void test0() {
   id a = getObj();
   [&]{ ^{ a = 0; }(); }();
 }
 
 // Block copy/dispose helpers shouldn't have to retain/release 'a' because it
 // is captured by reference.
 
 // CHECK-LABEL: define void @_ZN18CaptureByReference5test1Ev(
 // CHECK-LABEL: define internal void @"_ZZN18CaptureByReference5test1EvENK3$_2clEv"(
 // CHECK: %[[BLOCK_DESCRIPTOR:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>* %{{.*}}, i32 0, i32 4
-// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i8*, i8*, i64 }* @"__block_descriptor_56_8_32s40s_e5_v8@?0l" to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
+// CHECK: store %[[STRUCT_BLOCK_DESCRIPTOR]]* bitcast ({ i64, i64, i8*, i8*, i8*, i64 }* @"__block_descriptor_56_8_32s40s_e5_v8\01?0l" to %[[STRUCT_BLOCK_DESCRIPTOR]]*), %[[STRUCT_BLOCK_DESCRIPTOR]]** %[[BLOCK_DESCRIPTOR]], align 8
 
 // CHECK-LABEL: define linkonce_odr hidden void @__copy_helper_block_8_32s40s(
 // CHECK-NOT: call void @llvm.objc.storeStrong(
 // CHECK: %[[V4:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>* %{{.*}}, i32 0, i32 5
 // CHECK: %[[V5:.*]] = getelementptr inbounds <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>, <{ i8*, i32, i32, i8*, %[[STRUCT_BLOCK_DESCRIPTOR]]*, i8*, i8*, i8** }>* %{{.*}}, i32 0, i32 5
 // CHECK: %[[BLOCKCOPY_SRC:.*]] = load i8*, i8** %[[V4]], align 8
 // CHECK: store i8* null, i8** %[[V5]], align 8
 // CHECK: call void @llvm.objc.storeStrong(i8** %[[V5]], i8* %[[BLOCKCOPY_SRC]])