[llvm-commits] [PATCH] Miscompile Fix: Ensure that we do not constant fold Globals containing ObjC References in GlobalOpt.

[Nick Lewycky]

John McCall wrote:
> On Jan 13, 2013, at 9:49 AM, Bob Wilson<bob.wilson at apple.com>  wrote:
>> After a bit more thought, I prefer Eli's option (3): add a new kind of linkage.
>
>>
>> On Jan 12, 2013, at 5:39 PM, Bob Wilson<bob.wilson at apple.com>  wrote:
>>
>>> See my comment below.
>>>
>>> On Jan 11, 2013, at 4:04 PM, Eli Friedman<eli.friedman at gmail.com>  wrote:
>>>
>>>> The complete list of alternatives is as follows:
>>>>
>>>> 1. Make llvm.used indicate that the initializer may be modified before
>>>> llvm.global_ctors (this patch)
>
> I agree that this is extending the semantics of llvm.used beyond what we need.
>
> Note that I believe the use of llvm.used here was originally nothing but a
> compiler workaround; without it, global optimization would decide that the
> variable can be marked constant and therefore that *every* load can be
> folded away.
>
>>>> 2. Some llvm.used-like-thing (essentially, this patch, but
>>>> s/llvm.used/llvm.objc_linker_superspecial_stuff/g).
>
> There's a good reason for @llvm.used to be structured how it is:  most
> optimizations that want to transform a global need to check how it's used,
> i.e. walk its use list.  @llvm.used holding the address of the variable
> looks exactly like what it's supposed to model:  the address has escaped,
> and you cannot analyze every use anymore.
>
> That doesn't seem to apply to this use-case.
>
>>>> 3. Add a new kind of linkage, so the global is
>>>> `@"\01L_OBJC_SELECTOR_REFERENCES_" = internal_objc_metadata global i8*
>>>> [...]`.
>
> This badly conflates linkage with the actual properties we're trying to record,
> which are:
>    - the variable is coalesced with semantically similar variables by the
>      static linker, so its address is not meaningful, and

We have an orthogonal property for this, "unnamed_addr".

>    - the variable is subject to a phase of initialization which occurs before
>      this module's static initializers (because we'd like people to be able to
>      use objc_msgSend in static initializers).

@llvm.global_ctors has constructor priorities. We'd like people to be 
able to use std::cout in static initializers too, for instance.

Nick

> The former property is kindof bound up in linkage, but the latter really isn't:
> in principle, this property could hold of variables with any of kind of linkage.
> It's basically down to the language spec, or really, the frontend.
>
>>>> 4. Special-case variables whose names start with "\01L_OBJC" (the
>>>> original patch).
>
> This is totally inappropriate.  It would be almost reasonable if the behavior
> were actually triggered by the linker special-casing the symbol name,
> but it isn't — it's triggered by the section that the symbol appears in.
>
>>>> 5. Always assume globals can be modified before llvm.global_ctors
>>>> runs, and completely suppress the optimization.
>
> This definitely seems unnecessarily pessimistic.
>
> I would add to this:
>
> 6.  Assume globals that can be referenced outside this translation unit
> can be modified before llvm.global_ctors runs.  (So a normal
> internal-linkage global is subject to the global-initialization
> optimization, but a variable with external linkage or whose address
> is derivable (prior to initialization) from a variable with external linkage,
> etc. would not be.
>
> This is the theory under which I suggested keying off of llvm.used, although
> it does still tend to suppress valid optimizations.
>
> 7.  Add a new property to llvm::GlobalVariable that represents this new
> property.
>
> This is essentially just a variant of #2 but not re-using the @llvm.used
> representation.  I favor this.
>
> John.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>