rjmccall added a reviewer: theraven. rjmccall added a comment. In https://reviews.llvm.org/D52344#1242451, @kristina wrote:
> 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. Following the ELF conventions seems like the right thing to do, assuming it doesn't cause compatibility problems. David Chisnall might know best here. ================ 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"); ---------------- kristina wrote: > 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. Notice that this logic runs regardless of whether we actually created the global, though; we're potentially overwriting stuff that was decided based on content from the actual declaration. Repository: rC Clang https://reviews.llvm.org/D52344 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
