[ros-dev] [ros-diffs] [dchapyshev] 42012: - Samplify SwitchToThread and QueueUserWorkItem - Remove unneeded InternalWorkItemTrampoline function and QUEUE_USER_WORKITEM_CONTEXT structure - Other small changes

[Jeremy Drake]

I don't know if this is the same thing or not, but I just happened to read
this the other day (while looking up QueueUserWorkItem, incidentally) and
this conversation reminded me of it:
http://msdn.microsoft.com/en-us/library/ms686736(VS.85).aspx

Do not declare this callback function with a void return type and cast the
function pointer to LPTHREAD_START_ROUTINE when creating the thread. Code
that does this is common, but it can crash on 64-bit Windows.

It certainly sounds as though somebody at Microsoft actually encountered
such a crash.  I don't think it would be a problem on x64, but I know
next to nothing about Itanium, maybe it's an issue there?


On Tue, 21 Jul 2009, Alex Ionescu wrote:

> That doesn't make any sense.
> All architectures have an ABI that defines which registers are used for
> return values, and so the compiler will NEVER (under ANY circumstances)
> assume that the return register should somehow be "usable", especially for
> an external function pointer!
>
> Furthermore, this is typecasting from a ULONG to a VOID, isn't it? So the
> "real" function will never return anything in the first place, making this a
> non-issue.
>
> I challenge you to provide a test case/example on any architecture where
> this could possibly happen. It can't.
>
> Best regards,
> Alex Ionescu
>
>
> On Tue, Jul 21, 2009 at 1:11 PM, Thomas Bluemel <thomas at reactsoft.com>
> wrote:
>       On an architecture where function return values e.g. modify a
>       register
>       that would otherwise be preserved?  Or that have a different
>       return
>       method depending on whether data needs to be returned?  It's
>       like
>       typecasting a function to one with a different calling method,
>       except
>       that it may work fine for *most* but not all architectures.
>        It's still
>       bad code...
>
>       Thomas
> Alex Ionescu wrote:
> > I have no idea why typecasting would make it "crash" on another
> > architecture...
> >
> > On 21-Jul-09, at 4:02 AM, Ged wrote:
> >
> >> Dmitry do you have a reply for this?
> >> I worry that you're simply copying Windows internals without
> >> thinking about
> >> it.
> >>
> >> I agree with Thomas on this, if we can do something better then we
> >> should.
> >> There's no reason to do everything in exactly the same manner just
> >> to for
> >> the sake of copying. In fact, I would actually advise we do things
> >> slightly
> >> differently whenever possible.
> >>
> >> Ged.
> >>
> >>
> >> -----Original Message-----
> >> From: ros-dev-bounces at reactos.org [mailto:ros-dev-
> >> bounces at reactos.org] On
> >> Behalf Of Thomas Bluemel
> >> Sent: 17 July 2009 22:53
> >> To: ReactOS Development List
> >> Subject: Re: [ros-dev] [ros-diffs] [dchapyshev] 42012: - Samplify
> >> SwitchToThread and QueueUserWorkItem - Remove unneeded
> >> InternalWorkItemTrampoline function and QUEUE_USER_WORKITEM_CONTEXT
> >> structure - Other small changes
> >>
> >> Just because Wine and Windows do it that way doesn't make it any
> more
> >> correct or portable.
> >>
> >> RtlQueueWorkItem expects a function of this type:
> >> typedef VOID (NTAPI *WORKERCALLBACKFUNC)(IN PVOID Context);
> >>
> >> The supplied function is of this type:
> >> typedef ULONG (NTAPI *PTHREAD_START_ROUTINE)(PVOID Parameter);
> >>
> >> This doesn't crash on x86 and "works" because the return value is
> >> simply
> >> ignored, but it's definitely not portable and MAY crash on another
> >> architecture where it does make a difference.  I don't know if it
> >> would
> >> matter on ARM/PPC/... but the old code at least implemented it
> >> correctly/portable.  It's not really an issue but IMO a bad hack.
> >> Typecasting a function pointer to a function with a different
> >> signature
> >> is hardly ever good practice.
> >>
> >> - Thomas
> >>
> >> Dmitry Chapyshev wrote:
> >>> On Fri, 17 Jul 2009 23:03:18 +0400, Thomas Bluemel
> <thomas at reactsoft.com
> >>> wrote:
> >>>
> >>>> This explains why we are using a trampoline function and not just
> >>>> typecast there...  Is there a reason you changed this?
> >>>>
> >>>> - Thomas
> >>>>
> >>>> dchapyshev at svn.reactos.org wrote:
> >>>>> -    /* NOTE: Don't use Function directly since the callback
> >>>>> signature
> >>>>> -             differs. This might cause problems on certain
> >>>>> platforms... */
> >>>>> -    Status = RtlQueueWorkItem(InternalWorkItemTrampoline,
> >>>>> -                              WorkItemContext,
> >>>> _______________________________________________
> >>>> Ros-dev mailing list
> >>>> Ros-dev at reactos.org
> >>>> http://www.reactos.org/mailman/listinfo/ros-dev
> >>>
> >>>
> >>> Wine and Windows do so. Why we should do in another way? Other
> >>> reasons are
> >>> necessary?
> >>>
> >> _______________________________________________
> >> Ros-dev mailing list
> >> Ros-dev at reactos.org
> >> http://www.reactos.org/mailman/listinfo/ros-dev
> >>
> >> _______________________________________________
> >> Ros-dev mailing list
> >> Ros-dev at reactos.org
> >> http://www.reactos.org/mailman/listinfo/ros-dev
> >
> > Best regards,
> > Alex Ionescu
> >
> > _______________________________________________
> > Ros-dev mailing list
> > Ros-dev at reactos.org
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
>

-- 
Unfair animal names:

-- tsetse fly			-- bullhead
-- booby			-- duck-billed platypus
-- sapsucker			-- Clarence
		-- Gary Larson