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

[Michael Gottesman]

Thanks Bob.

Well what happened with the first approach is that I was stopping by Eli's office to ask him if he could look at the patch. Turns out he was already reviewing it = p. Then he basically brought up some valid criticisms of the first approach which I took at face value so I went to the 2nd. I think he is going to review the 2nd approach and respond on the list to his issues with the first (I will let him speak for himself ; p).

Michael

On Jan 11, 2013, at 3:42 PM, Bob Wilson <bob.wilson at apple.com> wrote:

> I am very uncertain about this approach.  It is significantly expanding the meaning of llvm.used, at least AFAICT from the docs.  Michael and I had a long discussion with John McCall about this, and John pointed out that llvm.used is described as "there is an invisible reference" to the variable -- it does not specify whether the variable can be modified via that reference.  This patch is taking the interpretation that it can be modified.  But, since the magic llvm.used reference is invisible, we would have to assume that it could be modified at any time, which basically means that anything in llvm.used is essentially volatile.  That doesn't make sense to me, and I'm pretty sure we're not consistently checking for llvm.used everywhere that we're checking for volatile accesses.
> 
> What was wrong with the first approach?  (I have some comments on the details of the patch, but let's discuss the overall approach first.)
> 
> On Jan 11, 2013, at 2:36 PM, Michael Gottesman <mgottesman at apple.com> wrote:
> 
>> <0001-Fixed-a-bug-in-GlobalOpt-which-was-causing-evaluatio.patch>
>> 
>> 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
>