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

[Chris Lattner]

On Jan 14, 2013, at 10:39 AM, John McCall <rjmccall at apple.com> wrote:
>>> 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.

I agree: this is the wrong approach.  llvm.used means something specific, if it isn't enough for this use case, it isn't llvm.used's fault.

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

The property that we're really trying to model here is "the linker is doing special stuff based on the section this global is in".  llvm.used provides part of what we need, but obviously not enough.

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

I agree, this doesn't have anything to do with linkage, it is orthogonal.

> which are:
>  - the variable is coalesced with semantically similar variables by the
>    static linker, so its address is not meaningful, and
>  - 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).
> 
> 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.

Its magic is because it is in a special section, not because of its linkage.

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

Completely agreed.

>>>> 5. Always assume globals can be modified before llvm.global_ctors
>>>> runs, and completely suppress the optimization.
> 
> This definitely seems unnecessarily pessimistic.

Completely agreed.

> 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 will over-pessimize static constructor evaluation optimization in globalopt.

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

+1.

-Chris