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

[Thomas Bluemel]

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,

[Dmitry Chapyshev]

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?

-- 
WBR,
Dmitry Chapyshev

[Thomas Bluemel]

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?
>

[Ged]

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:[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
> <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

[Alex Ionescu]

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

[Thomas Bluemel]

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

[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 <thomas(a)reactsoft.com>wrote;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
>

[ShadowFlare]

Do all use a register for the return value?  If any do anything different in
some way for the return value, it may not work correctly when the function
does not expect anything returned.
On Tue, Jul 21, 2009 at 5:50 PM, Alex Ionescu <ionucu(a)videotron.ca> 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;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
> >  
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[KJK::Hyperion]

ShadowFlare! you're back!

[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

[Timo Kreuzer]

Hi,

In this case it's vice versa. You cast void to DWORD. On x86 this simply
means an undefined value in the return register.

Anyway, the C standard says it results in undefined behaviour if the
types are not "compatible". And the compiler is free to do whatever he
likes in that case.
http://stackoverflow.com/questions/188839/function-pointer-cast-to-differen…

We can now argue about which types are compatible and which are not, but
here I challenge you, Alex, to provide a standard that defines that
these function pointer types are compatible. Not any number of cases
where it works can proof that it's valid, but a single case where it
doesn't work, proves it's invalid.

For example I could create a new architecture which has the nasty nature
of resetting all registers except stack pointer on function return.
So I decide for the ABI that the callee would clean the stack and only
leave the function return value on the stack, if any. The caller would
then pop it from the stack after the call. That would result in stack
corruption is this case.

Regards,
Timo


Jeremy Drake schrieb:
>   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
> >
> >
> >
> >
> >       
>    
>  ------------------------------------------------------------------------
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Thomas Bluemel]

Alex Ionescu wrote:
>   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! 
Like I said I don't know if any "real-world" architecture depends on 
this.  I can however make up my own instruction set where it does matter.

>   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. 
It's not a typecast of a return value, it's a typecast of a pointer to a 
function with an incompatible function signature.  It's like passing a 
callback function with 1 argument to a routine that expects a callback 
with 2 arguments.  Except that in this instance it would most likely 
cause trouble on most architectures immediately.  However, it's no 
different from the problem I described, except that it happens to be not 
much of an issue on x86.

>   I challenge you to provide a test case/example on any
> architecture where 
>  this could possibly happen. It can't. 
If I had the time I would be more than happy to come up with a new 
architecture where it does make a difference.

It doesn't change the fact that it's a non-portable hack.

- Thomas