[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

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.

lib/CodeGen/CGBlocks.cpp

for (const BlockCaptureManagedEntity &E : ManagedCaptures) {
         Name += getBlockCaptureStr(E, CaptureStrKind::DisposeHelper,
                                    BlockInfo.BlockAlign, CGM);
       }
     }
     Name += "_";
   }
 
   std::string TypeAtEncoding =
-      CGM.getContext().getObjCEncodingForBlock(BlockInfo.getBlockExpr());
+      CGM.getObjCRuntime().getObjCEncodingForBlock(BlockInfo.getBlockExpr());
   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.

lib/CodeGen/CGObjCGNU.cpp

public:
   llvm::Constant *BuildRCBlockLayout(CodeGenModule &CGM,
                                      const CGBlockInfo &blockInfo) override {
     return NULLPtr;
   }
 
   llvm::Constant *BuildByrefLayout(CodeGenModule &CGM, QualType T) override {
     return NULLPtr;
   }
+
+  /// Replace occurrences of '@' with '\1'. '@' is reserved on ELF platforms as
+  /// a separator between symbol name and symbol version.
+  std::string getObjCEncodingForBlock(const BlockExpr *BE) const override {
+    std::string Str = CGM.getContext().getObjCEncodingForBlock(BE);
+    std::replace(Str.begin(), Str.end(), '@', '\1');
+    return Str;
+  }
 };
 
 /// Class representing the legacy GCC Objective-C ABI.  This is the default when
 /// -fobjc-nonfragile-abi is not specified.
 ///
 /// The GCC ABI target actually generates code that is approximately compatible
 /// with the new GNUstep runtime ABI, but refrains from using any features that
 /// would not work with the GCC runtime.  For example, clang always generates

lib/CodeGen/CGObjCRuntime.h

virtual llvm::Constant *BuildGCBlockLayout(CodeGen::CodeGenModule &CGM,
                                   const CodeGen::CGBlockInfo &blockInfo) = 0;
   virtual llvm::Constant *BuildRCBlockLayout(CodeGen::CodeGenModule &CGM,
                                   const CodeGen::CGBlockInfo &blockInfo) = 0;
   virtual std::string getRCBlockLayoutStr(CodeGen::CodeGenModule &CGM,
                                           const CGBlockInfo &blockInfo) {
     return {};
   }
 
+  /// Return the block's type encoding string that is used in the block
+  /// descriptor's symbol name.
+  virtual std::string getObjCEncodingForBlock(const BlockExpr *BE) const;
+
   /// Returns an i8* which points to the byref layout information.
   virtual llvm::Constant *BuildByrefLayout(CodeGen::CodeGenModule &CGM,
                                            QualType T) = 0;
 
   struct MessageSendInfo {
     const CGFunctionInfo &CallInfo;
     llvm::PointerType *MessengerType;
 

lib/CodeGen/CGObjCRuntime.cpp

void CGObjCRuntime::EmitAtSynchronizedStmt(CodeGenFunction &CGF,
 
   // Register an all-paths cleanup to release the lock.
   CGF.EHStack.pushCleanup<CallSyncExit>(NormalAndEHCleanup, syncExitFn, lock);
 
   // Emit the body of the statement.
   CGF.EmitStmt(S.getSynchBody());
 }
 
+std::string CGObjCRuntime::getObjCEncodingForBlock(const BlockExpr *BE) const {
+  return CGM.getContext().getObjCEncodingForBlock(BE);
+}
+
 /// Compute the pointer-to-function type to which a message send
 /// should be casted in order to correctly call the given method
 /// with the given arguments.
 ///
 /// \param method - may be null
 /// \param resultType - the result type to use if there's no method
 /// \param callArgs - the actual arguments, including implicit ones
 CGObjCRuntime::MessageSendInfo

test/CodeGenObjC/gnu-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
+
+// Test that descriptor symbol names don't include '@', which is reserved on ELF
+// platforms as a separator between symbol name and symbol version.
+
+// 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*, 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), i8* null }, align 8
+
+typedef void (^BlockTy)(void);
+
+void test(id a) {
+  BlockTy b = ^{ (void)a; };
+}