[llvm-commits] [PATCH] Miscompile Fix: Ensure that we do not constant fold Globals containing ObjC References in GlobalOpt.
Eric Christopher
echristo at gmail.com
Sun Jan 13 11:53:56 PST 2013
Yeah. Sad, but seems to correspond more directly to what's going on here.
-eric
On Sun, 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)
> >>
> >> 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*
> >> [...]`.
> >>
> >> 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.
> >
> > I like option (1) for its simplicity, but it still seems to be a
> reinterpretation of what llvm.used means. If everyone agrees with that
> change, and it is clearly documented, then it would be OK.
> >
> > But, I do not agree with the reinterpretation that makes llvm.used
> variables become essentially volatile. (I.E., "there is an invisible
> reference to the variable and the value may be modified or read at any time
> via that reference.") To be consistent with that interpretation, we would
> need to check llvm.used all over the place.
> >
> > Alternatively, what about documenting llvm.used as specifically implying
> that the value may be overridden by the linker or runtime before
> llvm.global_ctors? I would also want to clarify the current doc to state
> that the invisible reference is not used to modify the variable.
> >
> > I have some minor comments about the patch, but let's settle the overall
> approach first. Regardless of which of the above options we use, I'm also
> wondering if perhaps the check should be done in the mayBeOverridden
> function as Michael's first patch did. There are a lot of places checking
> mayBeOverridden and it seems like this may be relevant for at least some of
> them. If we only check this one case in GlobalOpt, we'll miss the others.
> >
> >> (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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130113/a6027ccc/attachment.html>
More information about the llvm-commits
mailing list