Re: [PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Sebastian Lackner]

On 12.10.2016 12:31, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  include/ddk/wdm.h | 2 ++
>  1 file changed, 2 insertions(+)
>
> 0003-include-Define-IoSkipCurrentIrpStackLocation.txt
>
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 5602f7d..5671948 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -1175,9 +1175,11 @@ NTSTATUS WINAPI ObCloseHandle(IN HANDLE handle);
>  # ifdef NONAMELESSSTRUCT
>  #  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation)
>  #  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation - 1)
> +#  define IoSkipCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation++); ((_Irp)->CurrentLocation++)
>  # else
>  #  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation)
>  #  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation - 1)
> +#  define IoSkipCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation++); ((_Irp)->CurrentLocation++)
>  # endif
>  #else
>  # ifdef NONAMELESSSTRUCT

Is this definition based on a Windows header file? I have only found two versions,
either with a "{ ... }" block or with a static inline function. Your definition
would not work correctly in a situation like this:

if (...) IoSkipCurrentIrpStackLocation(irp);

Which would be expanded to:

if (...) irp->Tail.Overlay.s.u2.CurrentStackLocation++;
irp->CurrentLocation++;

Please note that the second statement is no longer conditional. Also, what about
the two other #ifdef branches directly below?

Regards,
Sebastian

[PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Aric Stewart]

On 10/12/16 4:10 PM, Sebastian Lackner wrote:
> On 12.10.2016 12:31, Aric Stewart wrote:
> > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > ---
> >  include/ddk/wdm.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > 0003-include-Define-IoSkipCurrentIrpStackLocation.txt
> >
> > diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> > index 5602f7d..5671948 100644
> > --- a/include/ddk/wdm.h
> > +++ b/include/ddk/wdm.h
> > @@ -1175,9 +1175,11 @@ NTSTATUS WINAPI ObCloseHandle(IN HANDLE handle);
> >  # ifdef NONAMELESSSTRUCT
> >  #  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation)
> >  #  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation - 1)
> > +#  define IoSkipCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation++); ((_Irp)->CurrentLocation++)
> >  # else
> >  #  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation)
> >  #  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation - 1)
> > +#  define IoSkipCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.u2.CurrentStackLocation++); ((_Irp)->CurrentLocation++)
> >  # endif
> >  #else
> >  # ifdef NONAMELESSSTRUCT
>
> Is this definition based on a Windows header file? I have only found two versions,
> either with a "{ ... }" block or with a static inline function. Your definition
> would not work correctly in a situation like this:
>
> if (...) IoSkipCurrentIrpStackLocation(irp);
>
> Which would be expanded to:
>
> if (...) irp->Tail.Overlay.s.u2.CurrentStackLocation++;
> irp->CurrentLocation++;
>
> Please note that the second statement is no longer conditional. Also, what about
> the two other #ifdef branches directly below?

True, the wdm.h I looked at in the ddk did not appear to block the two statements together so your statement would not work on windows either. I was adding them to look like the other statements in our header.


Now I have not opposition to putting them in a block, but just saying that it is not done that way in the windows header.

Yeah, i just missed the other 2 locations. But I should update them with the correct bracketing.

The rest of the code seems to use (...) but do you feel like it should be {..}?

-aric
> Regards,
> Sebastian

[PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Sebastian Lackner]

On 12.10.2016 16:57, Aric Stewart wrote:
> True, the wdm.h I looked at in the ddk did not appear to block the
> two statements together so your statement would not work on windows
> either. I was adding them to look like the other statements in our
> header.
>
> Now I have not opposition to putting them in a block, but just saying
> that it is not done that way in the windows header.

I assume you are looking at an old version then, and Microsoft fixed
this mistake in the meantime. Windows 10 header files contain for example:

--- snip ---
FORCEINLINE
VOID
IoSkipCurrentIrpStackLocation (
    _Inout_ PIRP Irp
)
[...]
{
    NT_ASSERT(Irp->CurrentLocation <= Irp->StackCount);
    Irp->CurrentLocation++;
    Irp->Tail.Overlay.CurrentStackLocation++;
}
--- snip ---

Nevertheless, I think both "{...}" and using an inline function is fine
in this case.
> Yeah, i just missed the other 2 locations. But I should update them
> with the correct bracketing.
>
> The rest of the code seems to use (...) but do you feel like it
> should be {..}?

No, it also wouldn't compile if you replace it with {...} there because
the result is usually assigned to a variable. Those definitions look
fine to me and are not affected by this problem.
> -aric

[PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Alexandre Julliard]

Sebastian Lackner <sebastian(a)fds-team.de> writes:
> I assume you are looking at an old version then, and Microsoft fixed
> this mistake in the meantime. Windows 10 header files contain for example:
>
> --- snip ---
> FORCEINLINE
> VOID
> IoSkipCurrentIrpStackLocation (
>     _Inout_ PIRP Irp
> )
> [...]
> {
>     NT_ASSERT(Irp->CurrentLocation <= Irp->StackCount);
>     Irp->CurrentLocation++;
>     Irp->Tail.Overlay.CurrentStackLocation++;
> }
> --- snip ---
>
> Nevertheless, I think both "{...}" and using an inline function is fine
> in this case.

It should be either an inline function (preferred) or a "do {} while(0)"
block. A "{...}" block will cause syntax errors in some cases.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Aric Stewart]

On 10/13/16 9:09 AM, Alexandre Julliard wrote:
> Sebastian Lackner <sebastian(a)fds-team.de> writes:
> > I assume you are looking at an old version then, and Microsoft fixed
> > this mistake in the meantime. Windows 10 header files contain for example:
> >
> > --- snip ---
> > FORCEINLINE
> > VOID
> > IoSkipCurrentIrpStackLocation (
> >     _Inout_ PIRP Irp
> > )
> > [...]
> > {
> >     NT_ASSERT(Irp->CurrentLocation <= Irp->StackCount);
> >     Irp->CurrentLocation++;
> >     Irp->Tail.Overlay.CurrentStackLocation++;
> > }
> > --- snip ---
> >
> > Nevertheless, I think both "{...}" and using an inline function is fine
> > in this case.
>
> It should be either an inline function (preferred) or a "do {} while(0)"
> block. A "{...}" block will cause syntax errors in some cases.

To do an inline function, which you prefer, would you like it in those  #ifdef blocks?

Like this?

#ifdef NONAMELESSUNION
# ifdef NONAMELESSSTRUCT
#  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation)
#  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation - 1)
   inline void IoSkipCurrentIrpStackLocation(IRP *_Irp) {_Irp->Tail.Overlay.s.u2.CurrentStackLocation++; _Irp->CurrentLocation++;}
# else
...

-aric

[PATCH 3/4] include: Define IoSkipCurrentIrpStackLocation

[Sebastian Lackner]

On 13.10.2016 09:27, Aric Stewart wrote:
> On 10/13/16 9:09 AM, Alexandre Julliard wrote:
> > Sebastian Lackner <sebastian(a)fds-team.de> writes:
> > > I assume you are looking at an old version then, and Microsoft fixed
> > > this mistake in the meantime. Windows 10 header files contain for example:
> > >
> > > --- snip ---
> > > FORCEINLINE
> > > VOID
> > > IoSkipCurrentIrpStackLocation (
> > >     _Inout_ PIRP Irp
> > > )
> > > [...]
> > > {
> > >     NT_ASSERT(Irp->CurrentLocation <= Irp->StackCount);
> > >     Irp->CurrentLocation++;
> > >     Irp->Tail.Overlay.CurrentStackLocation++;
> > > }
> > > --- snip ---
> > >
> > > Nevertheless, I think both "{...}" and using an inline function is fine
> > > in this case.
> >
> > It should be either an inline function (preferred) or a "do {} while(0)"
> > block. A "{...}" block will cause syntax errors in some cases.

@Alexandre:
I know that {...} can cause problems, but didn't object because it is also used in
some Windows header files. Nevertheless, you are right, lets better do it the right
way. ;)
> >
>
> To do an inline function, which you prefer, would you like it in those  #ifdef blocks?
>
> Like this?
>
> #ifdef NONAMELESSUNION
> # ifdef NONAMELESSSTRUCT
> #  define IoGetCurrentIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation)
> #  define IoGetNextIrpStackLocation(_Irp) ((_Irp)->Tail.Overlay.s.u2.CurrentStackLocation - 1)
>    inline void IoSkipCurrentIrpStackLocation(IRP *_Irp) {_Irp->Tail.Overlay.s.u2.CurrentStackLocation++; _Irp->CurrentLocation++;}
> # else

@Aric:
Its up to you if you want to define it in the #ifdef blocks or below, but you
definitely need to handle the different combinations of NONAMELESSUNION/
NONAMELESSSTRUCT to make sure it compiles fine. Please note that the function should
be marked as "static inline", not just "inline". Also, you can rename "_Irp" to "irp"
because there is no longer any risk of name collisions when using an inline function.

Regards,
Sebastian