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

[Alex Ionescu]

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 [e-mail]:
> 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: [e-mail] [mailto:ros-dev-
> > > [e-mail]] 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 <
>
> [e-mail]
> > > > wrote:
> > > > > This explains why we are using a trampoline function and not just
> > > > > typecast there...  Is there a reason you changed this?
> > > > > Thomas
> > > > >
> > > > > [e-mail] 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
> > > > > [e-mail]
> > > > > 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
> > > [e-mail]
> > > http://www.reactos.org/mailman/listinfo/ros-dev
> > >
> > > Ros-dev mailing list
> > > [e-mail]
> > > http://www.reactos.org/mailman/listinfo/ros-dev
> >
> > Best regards,
> > Alex Ionescu
> >
> > Ros-dev mailing list
> > [e-mail]
> > http://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev