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

[Bob Wilson]

On Jan 14, 2013, at 2:06 AM, Nick Lewycky <nicholas at mxc.ca> wrote:

> Eli Friedman 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)
>> 
>> 2. Some llvm.used-like-thing (essentially, this patch, but
>> s/llvm.used/llvm.objc_linker_superspecial_stuff/g).
>> 
>> 3. Add a new kind of linkage, so the global is
>> `@"\01L_OBJC_SELECTOR_REFERENCES_" = internal_objc_metadata global i8*
>> [...]`.
> 
> I don't get it. Why is a selector marked internal? It's not internal. There are other references to it, outside this Module. What are the semantics you need here? Thus far, it sounds like the problem is that you've been lying to the compiler and now you aren't getting away with it.

Yes, I agree with your assessment about lying to the compiler.  I'm not an expert on ObjectiveC issues, but my understanding is that the dynamic linker and/or ObjectiveC runtime may coalesce selectors or override them in some other way at runtime.

> 
> To take a wild guess, do you require that it not merge with other symbols of the same name, or that the linker gets to hack on it (optimize based on its presence or absence)? If so, I propose 3b) Don't use internal linkage. To avoid the other consequences of losing internal linkage, mark it visibility hidden http://llvm.org/docs/LangRef.html#visibility-styles . The linker can do whatever it likes with it, based on (for instance) variable name, since the hidden visibility guarantees that the symbol can't be seen post-link, or merged with other dylib's even if the symbol has the same name.

We need to be careful about changing the linkage in ways that affect the linker.  The coalescing being done here is at runtime.  If we change the linkage or visibility, we need to make sure it doesn't cause the linker to do the wrong thing.

I don't know about the internal linkage issue.  Eli and John both suggested that these selectors should be internal.

I had also wondered about using linker_private_weak linkage, but that looks like it may cause the linker to handle these selectors incorrectly.

> 
> Nick
> 
>> 4. Special-case variables whose names start with "\01L_OBJC" (the
>> original patch).
>> 
>> 5. Always assume globals can be modified before llvm.global_ctors
>> runs, and completely suppress the optimization.
>> 
>> 
>> 4 is bad because it makes the behavior of IR globals more complicated;
>> we don't want to force people writing IR optimizers to look at LangRef
>> for a special rule for globals which looks normal variables, but
>> actually aren't because the name triggers special behavior.
>> 
>> 5 is bad because it eliminates a valuable optimization.
>> 
>> 
>> The benefit of (2) over (1) is that it doesn't overload "used" quite
>> so much.  The downside of (2) is that it means adding yet another
>> "Intrinsic Global Variable" to LangRef, and the whole concept of an
>> "Intrinsic Global Variable" isn't very clean to work with in the first
>> place.
>> 
>> (3) is in some sense the most pure choice; it doesn't overload "used",
>> and it's difficult to accidentally handle incorrectly. The primary
>> downsides are that it's yet another linkage type, and implementing a
>> new kind of linkage requires a bunch of scattered small changes.
>> 
>> If we go with approach (1), the attached patch looks fine.
>> 
>> -Eli
>> 
>> 2013/1/11 Michael Gottesman<mgottesman at apple.com>:
>>> 
>>> 
>>> Please review this patch = p.
>>> 
>>> Michael
>>> 
>>> On Jan 11, 2013, at 1:37 PM, Michael Gottesman<mgottesman at apple.com>  wrote:
>>> 
>>>> Please ignore this patch, I have a better alternate solution (I will post it in the same thread).
>>>> 
>>>> Michael
>>>> 
>>>> On Jan 11, 2013, at 11:55 AM, Michael Gottesman<mgottesman at apple.com>  wrote:
>>>> 
>>>>> Currently we miscompile Objective-C++ in the case where we have a selector stored in a global constant structure. The miscompile occurs in the IR level pass GlobalOpt.
>>>>> 
>>>>> ----
>>>>> #include<Foundation/Foundation.h>
>>>>> 
>>>>> struct ButtonInitializer {
>>>>> NSString *name;
>>>>> SEL sel;
>>>>> };
>>>>> 
>>>>> const static ButtonInitializer buttonInitData[] = {
>>>>> {
>>>>>   @"Print",
>>>>>   @selector(print:)
>>>>> }
>>>>> };
>>>>> 
>>>>> @interface Test : NSObject {
>>>>> NSMutableDictionary *dict;
>>>>> }
>>>>> - (id)init;
>>>>> @end
>>>>> 
>>>>> @implementation Test
>>>>> - (id)init {
>>>>> self = [super init];
>>>>> if (self) {
>>>>>   dict = [[NSMutableDictionary alloc] init];
>>>>>   for (int i = 0; i<  1; i++) {
>>>>>     [dict setValue:buttonInitData[i].sel forKey:buttonInitData[i].name];
>>>>>   }
>>>>> }
>>>>> return self;
>>>>> }
>>>>> @end
>>>>> ----
>>>>> 
>>>>> The specific error that occurs is that in GlobalOpt we evaluate the Objective C selector reference and put in the local method name string that the selector ref is pointing at in stead of the ref itself. This violates the semantics of objective-c++, which due to dynamic binding, explicitly says that the runtime reserves the right to change what method name string a specific Objective C selector reference is pointing at. If for whatever this occurs, a hard error (does not respond to selector) will occur causing the application to exit abnormally.
>>>>> 
>>>>> The following patch fixes this issue by making the following changes:
>>>>> 
>>>>> 1. A new instance method is added to GlobalValue called mayBeOverriddenAfterStartBeforeMain() that is named as such to suggest that said value may be altered at program start before main is called but after program start.
>>>>> 2. The static method mayBeOverridden is renamed mayBeOverriddenAtLinkTime() to contrast said method with mayBeOverriddenAfterStartBeforeMain() and to make the name of the function fit its function.
>>>>> 3. The instance method mayBeOverridden is changed to return false if and only if both mayBeOverriddenAtLinkTime() and mayBeOverriddenAfterStartBeforeMain() return false.
>>>>> 
>>>>> Additionally a test case is provided.
>>>>> 
>>>>> *NOTE* I am going to fix some comments in the file in a separate patch.
>>>>> 
>>>>> Please Review.
>>>>> 
>>>>> <0001-Fixed-a-bug-in-GlobalOpt-which-was-causing-evaluatio.patch>
>>>>> 
>>>>> Michael
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits