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

[John McCall]

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

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