Re: [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(a)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(a)reactos.org [mailto:ros-dev-
> > > bounces(a)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(a)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(a)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(a)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(a)reactos.org
> > >  http://www.reactos.org/mailman/listinfo/ros-dev
> > >
> > >  _______________________________________________
> > >  Ros-dev mailing list
> > >  Ros-dev(a)reactos.org
> > >  http://www.reactos.org/mailman/listinfo/ros-dev 
> >  Best regards,
> >  Alex Ionescu
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
>  
-- 
Unfair animal names:

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