Page MenuHomePhabricator

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

Authored by ahatanak on Nov 14 2018, 12:53 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak created this revision.Nov 14 2018, 12:53 PM

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 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?

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

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

theraven added inline comments.Nov 15 2018, 1:05 AM
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.

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 updated this revision to Diff 179676.Sat, Dec 29, 7:13 AM

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

theraven accepted this revision.Sat, Dec 29, 7:58 AM

Looks like a much cleaner change.

This revision is now accepted and ready to land.Sat, Dec 29, 7:58 AM
This revision was automatically updated to reflect the committed changes.