[PATCH] D52344: [Clang][CodeGen][ObjC]: Fix non-bridged CoreFoundation builds on ELF targets that use `-fconstant-cfstrings`.

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Would you be okay with me renaming `cfstring` to `.cfstring` for ELF so it's in line with ELF section naming conventions (I'm not entirely sure if that could cause issues with ObjC stuff)? And yes I think it's best to avoid that code-path altogether if it turns out to be a constant as that's likely from the declaration of the type, I'll write a FileCheck test in a moment and check that I can apply the same logic to ELF aside from the DLL stuff as CoreFoundation needs to export the symbol somehow while preserving the implicit extern attribute for every other TU that comes from the builtin that `CFSTR` expands to. Is what I'm suggesting making sense? If not, let me know, I may be misunderstanding what's happening here.

I'll add the test and another revision in a second, maybe that will clear it up a bit.



================
Comment at: lib/CodeGen/CodeGenModule.cpp:4108
+      // consistent with old behavior for this target as it would fail
+      // on the cast<> instead.
+      assert(GV && "isa<CFConstantStringClassReference> isn't a GlobalValue");
----------------
rjmccall wrote:
> compnerd wrote:
> > I think that the comment isn't particularly helpful - it basically is directing you to look at the history of the file.
> I think we should just skip this step, even on COFF, if it's not a `GlobalValue`.
> 
> Probably the most correct move would be to only apply this logic if the IR declaration was synthesized.  Or maybe even just change this code to look for a global variable called `__CFConstantStringClassReference` in the first place and then emit a reference to it if found, and only otherwise try to build the synthetic variable.  But just bailing out early if something weird is going on is also a legitimate step.
Doesn't this code practically do this since creating a runtime global is a no-op if it already exists within the module, however there is potentially one module which can have it which is CFString.c (assuming no bridging here of any kind) while it makes sense for the rest to refer to it as an implicit extern (in which case I need to add an ELF code path to do exactly the same.


Repository:
  rC Clang

https://reviews.llvm.org/D52344