r160121, thanks!
On Thu, Jul 12, 2012 at 1:04 PM, Timur Iskhodzhanov <[email protected]> wrote: > On Thu, Jul 12, 2012 at 12:33 AM, John McCall <[email protected]> wrote: >> On Jul 11, 2012, at 1:29 PM, Timur Iskhodzhanov wrote: >>> On Wed, Jul 11, 2012 at 10:00 PM, John McCall <[email protected]> wrote: >>>> On Jul 11, 2012, at 4:53 AM, Timur Iskhodzhanov wrote: >>>>> On Mon, Jul 9, 2012 at 9:46 PM, John McCall <[email protected]> wrote: >>>>>> On Jul 9, 2012, at 6:52 AM, Timur Iskhodzhanov wrote: >>>>>>> On Sat, Jul 7, 2012 at 10:44 AM, John McCall <[email protected]> wrote: >>>>>>>> On Jun 28, 2012, at 1:39 PM, Timur Iskhodzhanov wrote: >>>>>>>>> Part one is attached - it propagates the "isInsance" boolean to the >>>>>>>>> two places where it should be read afterwards [in part 2]. >>>>>>>> >>>>>>>> I didn't really like this approach, so I committed a different one as >>>>>>>> r159893. >>>>>>>> Let me know if this helps. >>>>>>> You've meant r159894. >>>>>>> Yeah, I like your approach much more than mine :) >>>>>>> >>>>>>>>> The only logic change is around lib/CodeGen/CGCall.cpp:150. >>>>>>>>> Without this change, the definition of `void __cdecl cdecl_method() >>>>>>>>> {}` >>>>>>>>> ends up using "thiscall" (and the call remains "cdecl" - ouch!). >>>>>>>>> Probably it was a bug in the first place. >>>>>>>> >>>>>>>> Let me know if this is still a problem arising in your second patch. >>>>>>> It still is! >>>>>>> See the FIXME in the attached patch. >>>>>>> Other than that, the tests pass OK and it was easy to achieve this. >>>>>> >>>>>> Aha, that's interesting. Okay, I think the isVariadic bit should >>>>>> probably be >>>>>> passed down to getDefaultMethodCallConv(). While you're at it, could >>>>>> you rename that to getDefaultCXXMethodCallConv()? >>>>> Done. >>>>> Actually, I've found and fixed a mangler bug when adding this argument :) >>>>> See the attached patch >>>> >>>> Looks good, thanks! >>> bug_12785p2_2.patch is OK >> >> Yes. >> >>> On Wed, Jul 11, 2012 at 9:57 PM, John McCall <[email protected]> wrote: >>>> On Jul 11, 2012, at 5:34 AM, Timur Iskhodzhanov wrote: >>>>> With the attached patch the cdecl test passes now. >>>>> >>>>> However, I'm not very sure what I'm doing here and why it doesn't work >>>>> in the first place - can you please take a look? >>>> >>>> Hmm. The problem here is that getCanonicalCallConv is turning CC_C >>>> into CC_Default. That needs to be disabled in Microsoft mode, just like >>>> it apparently is in MRTD mode. >>>> >>>> ...also, the way that MRTD mode is implemented is wrong, and we should >>>> be handling this in IR-generation the same that we're handling the >>>> thiscall rules. >>> a) bug_12785p3.patch is OK as a short-term solution and >>> getCanonicalCallConv can be fixed later >>> OR >>> b) I should just fix getCanonicalCallConv as a part of my patch? >>> >>> If (b) - should I fix it completely (with fixing "is implemented wrong") >>> or just alter one condition in the Microsoft mode? >>> I'm not sure it's clear to me how to fix it "completely" at this point >>> (to be honest, I haven't looked at its code yet - just checking e-mail) >> >> (b). Just alter the one condition in Microsoft mode; this will make it >> so that function types that are explicitly cdecl are canonically different >> from function types that do not have an explicit CC. This should make >> your proposed patch unnecessary. > Got it. > Attached is bug_12785p3_2.patch which does pretty much what you've > asked for instead of bug_12785p3.patch > For some reason, LangOpts.MicrosoftMode is false with "-cxx-abi > microsoft", so I'm just checking for the ABI. > I can change that post-commit if that doesn't sound good for you. > >> If you want to fix the existing MRTD stuff, I'd be happy to provide guidance, >> but that's not really "on you", so to speak. > I can do this if you believe that'd benefit me (like knowing the > codebase better). > Otherwise, I have a lot of "-cxx-abi microsoft" work to do :) > >> John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
