Re: [ros-dev] [ros-diffs] [tkreuzer] 64658: [NTDLL] Don't assert that the caller of exported APIs passes correct parameters.

[Alex Ionescu]

Windows does. Why shouldn't we? It's a non-documented API.

Best regards,
Alex Ionescu

On Sat, Oct 11, 2014 at 1:52 AM, [e-mail] wrote:
> Author: tkreuzer
> Date: Sat Oct 11 08:52:33 2014
> New Revision: 64658
>
> URL: http://svn.reactos.org/svn/reactos?rev=64658&view=rev
> Log:
> [NTDLL]
> Don't assert that the caller of exported APIs passes correct parameters.
>
> Modified:
>     trunk/reactos/dll/ntdll/ldr/ldrapi.c
>
> Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/ldr/ldrapi.c?rev=...
>
> ==============================================================================
> --- trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] (original)
> +++ trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] Sat Oct 11
> 08:52:33 2014
> @@ -209,9 +209,6 @@
>          /* A normal failure */
>          return STATUS_INVALID_PARAMETER_3;
>      }
> /* Do or Do Not. There is no Try */
> ASSERT((Disposition != NULL) || !(Flags &
>
> LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY));
>  /* If the flag is set, make sure we have a valid pointer to use */
>  if ((Flags & LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY) && !(Disposition))

[Timo Kreuzer]

Because it causes havoc on our testbot.
Question to you: Why should we? We are replicating "normal" Windows
behavior, not Windows debugging code.

Am 11.10.2014 18:45, schrieb Alex Ionescu:
> Windows does. Why shouldn't we? It's a non-documented API.
>
> Best regards,
> Alex Ionescu
>
> On Sat, Oct 11, 2014 at 1:52 AM, <[e-mail]
> mailto:[e-mail]> wrote:
> Author: tkreuzer
> Date: Sat Oct 11 08:52:33 2014
> New Revision: 64658
>
> URL: http://svn.reactos.org/svn/reactos?rev=64658&view=rev
> Log:
> [NTDLL]
> Don't assert that the caller of exported APIs passes correct
> parameters.
>
> Modified:
>     trunk/reactos/dll/ntdll/ldr/ldrapi.c
>
> Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c
> URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/ldr/ldrapi.c?rev=64658&r1=64657&r2=64658&view=diff
> ==============================================================================
> --- trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1]
> (original)
> +++ trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] Sat
> Oct 11 08:52:33 2014
> @@ -209,9 +209,6 @@
>          /* A normal failure */
>          return STATUS_INVALID_PARAMETER_3;
>      }
> -
> -    /* Do or Do Not. There is no Try */
> -    ASSERT((Disposition != NULL) || !(Flags &
> LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY));
>
>      /* If the flag is set, make sure we have a valid pointer to
> use */
>      if ((Flags & LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY) &&
> !(Disposition))
>
>
>
>
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Alex Ionescu]

So the testbot should stop giving invalid values -- or are you saying that
this would ASSERT on Windows too and also kill Testbot? Then now we are
clearly changing Windows behavior.

Best regards,
Alex Ionescu

On Sat, Oct 11, 2014 at 4:59 PM, Timo Kreuzer [e-mail] wrote:
> Because it causes havoc on our testbot.
> Question to you: Why should we? We are replicating "normal" Windows
> behavior, not Windows debugging code.
>
> Am 11.10.2014 18:45, schrieb Alex Ionescu:
>
> Windows does. Why shouldn't we? It's a non-documented API.
>
> Best regards,
> Alex Ionescu
>
> On Sat, Oct 11, 2014 at 1:52 AM, [e-mail] wrote:
> > Author: tkreuzer
> > Date: Sat Oct 11 08:52:33 2014
> > New Revision: 64658
> >
> > URL: http://svn.reactos.org/svn/reactos?rev=64658&view=rev
> > Log:
> > [NTDLL]
> > Don't assert that the caller of exported APIs passes correct parameters.
> >
> > Modified:
> >     trunk/reactos/dll/ntdll/ldr/ldrapi.c
> >
> > Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/ldr/ldrapi.c?rev=...
> >
> > ==============================================================================
> > --- trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] (original)
> > +++ trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] Sat Oct 11
> > 08:52:33 2014
> > @@ -209,9 +209,6 @@
> >          /* A normal failure */
> >          return STATUS_INVALID_PARAMETER_3;
> >      }
> > /* Do or Do Not. There is no Try */
> > ASSERT((Disposition != NULL) || !(Flags &
> >
> > LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY));
> >  /* If the flag is set, make sure we have a valid pointer to use */
> >  if ((Flags & LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY) && !(Disposition))
>
> Ros-dev mailing [e-mail]://www.reactos.org/mailman/listinfo/ros-dev
>
> Ros-dev mailing list
> [e-mail]
> http://www.reactos.org/mailman/listinfo/ros-dev

[Thomas Faber]

Wine tests are written against free builds of Windows, nobody tests
against the checked version (and I highly doubt they'd accept fixes
with that reasoning).
So we (and Wine) are making a trade-off here, giving up an assertion in
favor of ensuring the correctness of the release build's behavior.
That seems more than reasonable to me.

An alternative would be to introduce a third build type to distinguish
our "checked build for daily use" from a "checked build that behaves
like Windows's." I don't see much benefit in doing that at this point
though.

On 2014-10-12 06:19, Alex Ionescu wrote:
> So the testbot should stop giving invalid values -- or are you saying that
> this would ASSERT on Windows too and also kill Testbot? Then now we are
> clearly changing Windows behavior.
>
> Best regards,
> Alex Ionescu
>
> On Sat, Oct 11, 2014 at 4:59 PM, Timo Kreuzer [e-mail] wrote:
> > Because it causes havoc on our testbot.
> > Question to you: Why should we? We are replicating "normal" Windows
> > behavior, not Windows debugging code.
> >
> > Am 11.10.2014 18:45, schrieb Alex Ionescu:
> >
> > Windows does. Why shouldn't we? It's a non-documented API.
> >
> > Best regards,
> > Alex Ionescu
> >
> > On Sat, Oct 11, 2014 at 1:52 AM, [e-mail] wrote:
> > > Author: tkreuzer
> > > Date: Sat Oct 11 08:52:33 2014
> > > New Revision: 64658
> > >
> > > URL: http://svn.reactos.org/svn/reactos?rev=64658&view=rev
> > > Log:
> > > [NTDLL]
> > > Don't assert that the caller of exported APIs passes correct parameters.
> > >
> > > Modified:
> > >     trunk/reactos/dll/ntdll/ldr/ldrapi.c
> > >
> > > Modified: trunk/reactos/dll/ntdll/ldr/ldrapi.c
> > > URL:
> > > http://svn.reactos.org/svn/reactos/trunk/reactos/dll/ntdll/ldr/ldrapi.c?rev=...
> > >
> > > ==============================================================================
> > > --- trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] (original)
> > > +++ trunk/reactos/dll/ntdll/ldr/ldrapi.c        [iso-8859-1] Sat Oct 11
> > > 08:52:33 2014
> > > @@ -209,9 +209,6 @@
> > >          /* A normal failure */
> > >          return STATUS_INVALID_PARAMETER_3;
> > >      }
> > > /* Do or Do Not. There is no Try */
> > > ASSERT((Disposition != NULL) || !(Flags &
> > >
> > > LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY));
> > >  /* If the flag is set, make sure we have a valid pointer to use */
> > >  if ((Flags & LDR_LOCK_LOADER_LOCK_FLAG_TRY_ONLY) && !(Disposition))