Re: [ros-dev] [ros-diffs] [jgardou] 64537: [NTOS/MM] - Do not assert in case of stack overflow, just let the page fault handler raise STATUS_STACK_OVERFLOW

[Alex Ionescu]

The ASSERT is there because of the missing functionality. Please see the
comment just above.

Best regards,
Alex Ionescu

On Sun, Oct 5, 2014 at 2:57 AM, <jgardou(a)svn.reactos.org> wrote:

>   Author: jgardou
>  Date: Sun Oct  5 09:57:02 2014
>  New Revision: 64537
>
>  URL: http://svn.reactos.org/svn/reactos?rev=64537&view=rev
>  Log:
>  [NTOS/MM]
>   - Do not assert in case of stack overflow, just let the page fault
>  handler raise STATUS_STACK_OVERFLOW
>
>  Modified:
>      trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
>
>  Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] Sun Oct  5
>  09:57:02 2014
>  @@ -82,7 +82,6 @@
>       {
>           /* We don't -- Windows would try to make this guard page valid
>  now */
>           DPRINT1("Close to our death...\n");
>  -        ASSERT(FALSE);
>           return STATUS_STACK_OVERFLOW;
>       }
>
>
>
>

[Timo Kreuzer]

Well, an ASSERT is supposed to assert something. If it get's triggered,
there's obviously something wrong. Putting an ASSERT that is known to be
(potentially) triggered defies the purpose of an ASSERT. For example it
doesn't make sense to ASSERT that a usermode caller doesn't pass a
certain flag. A BSOD is not the solution to currently mot having
implemented something. A DPRINT1 will do just fine (if we finally
stopped DPRINTing all sorts of crap, creating MBs of debuglogs that
noone can analyze, and instead added DPRINTs in places where it actually
makes sense </rant>)
So the ASSERT should be replaced with a DPRINT1.

Timo

Am 06.10.2014 17:29, schrieb Alex Ionescu:
>   The ASSERT is there because of the missing
> functionality. Please see
>  the comment just above.
>
>  Best regards,
>  Alex Ionescu
>
>  On Sun, Oct 5, 2014 at 2:57 AM, &lt;jgardou(a)svn.reactos.org
>  <mailto:[e-mail]>> wrote:
>
>      Author: jgardou
>      Date: Sun Oct  5 09:57:02 2014
>      New Revision: 64537
>
>      URL: http://svn.reactos.org/svn/reactos?rev=64537&view=rev
>      Log:
>      [NTOS/MM]
>       - Do not assert in case of stack overflow, just let the page
>      fault handler raise STATUS_STACK_OVERFLOW
>
>      Modified:
>          trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
>
>      Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
>      URL:
>     
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
>      ==============================================================================
>      --- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1]
>      (original)
>      +++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] Sun
>      Oct  5 09:57:02 2014
>      @@ -82,7 +82,6 @@
>           {
>               /* We don't -- Windows would try to make this guard page
>      valid now */
>               DPRINT1("Close to our death...\n");
>      -        ASSERT(FALSE);
>               return STATUS_STACK_OVERFLOW;
>           }
>
>
>
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Alex Ionescu]

Timo, if you don't make the guard page valid, you'll just loop forever...
how is that better than an ASSERT? :-)

Best regards,
Alex Ionescu

On Mon, Oct 6, 2014 at 3:21 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:

>  
>  Well, an ASSERT is supposed to assert something. If it get's triggered,
>  there's obviously something wrong. Putting an ASSERT that is known to be
>  (potentially) triggered defies the purpose of an ASSERT. For example it
>  doesn't make sense to ASSERT that a usermode caller doesn't pass a certain
>  flag. A BSOD is not the solution to currently mot having implemented
>  something. A DPRINT1 will do just fine (if we finally stopped DPRINTing all
>  sorts of crap, creating MBs of debuglogs that noone can analyze, and
>  instead added DPRINTs in places where it actually makes sense </rant>)
>  So the ASSERT should be replaced with a DPRINT1.
>
>  Timo
>
>  Am 06.10.2014 17:29, schrieb Alex Ionescu:
>
>  The ASSERT is there because of the missing functionality. Please see the
>  comment just above.
>
>  Best regards,
>  Alex Ionescu
>
>  On Sun, Oct 5, 2014 at 2:57 AM, &lt;jgardou(a)svn.reactos.org&gt; wrote:
>
> >   Author: jgardou
> >  Date: Sun Oct  5 09:57:02 2014
> >  New Revision: 64537
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=64537&view=rev
> >  Log:
> >  [NTOS/MM]
> >   - Do not assert in case of stack overflow, just let the page fault
> >  handler raise STATUS_STACK_OVERFLOW
> >
> >  Modified:
> >      trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> >
> >  Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> >  URL:
> >
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
> >
> >  ==============================================================================
> >  --- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] (original)
> >  +++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] Sun Oct  5
> >  09:57:02 2014
> >  @@ -82,7 +82,6 @@
> >       {
> >           /* We don't -- Windows would try to make this guard page valid
> >  now */
> >           DPRINT1("Close to our death...\n");
> >  -        ASSERT(FALSE);
> >           return STATUS_STACK_OVERFLOW;
> >       }
> >
> >
> >
> >  
>
>  _______________________________________________
>  Ros-dev mailing [e-mail]://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>

[Timo Kreuzer]

>  From what I read the page fault handler will not retry
> in a loop, but 
raise a usermode exception. Or am I missing something here?

Am 11.10.2014 18:34, schrieb Alex Ionescu:
>   Timo, if you don't make the guard page valid,
> you'll just loop
>  forever... how is that better than an ASSERT? :-)
>
>  Best regards,
>  Alex Ionescu
>
>  On Mon, Oct 6, 2014 at 3:21 PM, Timo Kreuzer <timo.kreuzer(a)web.de
>  <mailto:[e-mail]>> wrote:
>
>
>      Well, an ASSERT is supposed to assert something. If it get's
>      triggered, there's obviously something wrong. Putting an ASSERT
>      that is known to be (potentially) triggered defies the purpose of
>      an ASSERT. For example it doesn't make sense to ASSERT that a
>      usermode caller doesn't pass a certain flag. A BSOD is not the
>      solution to currently mot having implemented something. A DPRINT1
>      will do just fine (if we finally stopped DPRINTing all sorts of
>      crap, creating MBs of debuglogs that noone can analyze, and
>      instead added DPRINTs in places where it actually makes sense </rant>)
>      So the ASSERT should be replaced with a DPRINT1.
>
>      Timo
>
>      Am 06.10.2014 17:29, schrieb Alex Ionescu:
> >       The ASSERT is there because of the missing
> > functionality. Please
> >      see the comment just above.
> >
> >      Best regards,
> >      Alex Ionescu
> >
> >      On Sun, Oct 5, 2014 at 2:57 AM, &lt;jgardou(a)svn.reactos.org
> >      <mailto:[e-mail]>> wrote:
> >
> >          Author: jgardou
> >          Date: Sun Oct  5 09:57:02 2014
> >          New Revision: 64537
> >
> >          URL: http://svn.reactos.org/svn/reactos?rev=64537&view=rev
> >          Log:
> >          [NTOS/MM]
> >           - Do not assert in case of stack overflow, just let the page
> >          fault handler raise STATUS_STACK_OVERFLOW
> >
> >          Modified:
> >              trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> >
> >          Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> >          URL:
> >         
> > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
> >          ==============================================================================
> >          --- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1]
> >          (original)
> >          +++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1]
> >          Sun Oct  5 09:57:02 2014
> >          @@ -82,7 +82,6 @@
> >               {
> >                   /* We don't -- Windows would try to make this guard
> >          page valid now */
> >                   DPRINT1("Close to our death...\n");
> >          -        ASSERT(FALSE);
> >                   return STATUS_STACK_OVERFLOW;
> >               }
> >
> >
> >
> >
> >
> >
> >      _______________________________________________
> >      Ros-dev mailing list
> >      Ros-dev(a)reactos.org <mailto:[e-mail]>
> >      http://www.reactos.org/mailman/listinfo/ros-dev 
>
>      _______________________________________________
>      Ros-dev mailing list
>      Ros-dev(a)reactos.org <mailto:[e-mail]>
>      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

[Alex Ionescu]

You're supposed to now make the guard page valid, as per the comment.
Otherwise you'll keep faulting on it.

Best regards,
Alex Ionescu

On Sat, Oct 11, 2014 at 2:32 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:

>  
>  From what I read the page fault handler will not retry in a loop, but
>  raise a usermode exception. Or am I missing something here?
>
>  Am 11.10.2014 18:34, schrieb Alex Ionescu:
>
>  Timo, if you don't make the guard page valid, you'll just loop forever...
>  how is that better than an ASSERT? :-)
>
>  Best regards,
>  Alex Ionescu
>
>  On Mon, Oct 6, 2014 at 3:21 PM, Timo Kreuzer <timo.kreuzer(a)web.de> wrote:
>
> >  
> >  Well, an ASSERT is supposed to assert something. If it get's triggered,
> >  there's obviously something wrong. Putting an ASSERT that is known to be
> >  (potentially) triggered defies the purpose of an ASSERT. For example it
> >  doesn't make sense to ASSERT that a usermode caller doesn't pass a certain
> >  flag. A BSOD is not the solution to currently mot having implemented
> >  something. A DPRINT1 will do just fine (if we finally stopped DPRINTing all
> >  sorts of crap, creating MBs of debuglogs that noone can analyze, and
> >  instead added DPRINTs in places where it actually makes sense </rant>)
> >  So the ASSERT should be replaced with a DPRINT1.
> >
> >  Timo
> >
> >  Am 06.10.2014 17:29, schrieb Alex Ionescu:
> >
> >   The ASSERT is there because of the missing functionality. Please see
> >  the comment just above.
> >
> >  Best regards,
> >  Alex Ionescu
> >
> >  On Sun, Oct 5, 2014 at 2:57 AM, &lt;jgardou(a)svn.reactos.org&gt; wrote:
> >
> > >   Author: jgardou
> > >  Date: Sun Oct  5 09:57:02 2014
> > >  New Revision: 64537
> > >
> > >  URL: http://svn.reactos.org/svn/reactos?rev=64537&view=rev
> > >  Log:
> > >  [NTOS/MM]
> > >   - Do not assert in case of stack overflow, just let the page fault
> > >  handler raise STATUS_STACK_OVERFLOW
> > >
> > >  Modified:
> > >      trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> > >
> > >  Modified: trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c
> > >  URL:
> > >
> > > http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/pagfault.…
> > >
> > >  ==============================================================================
> > >  --- trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] (original)
> > >  +++ trunk/reactos/ntoskrnl/mm/ARM3/pagfault.c   [iso-8859-1] Sun Oct  5
> > >  09:57:02 2014
> > >  @@ -82,7 +82,6 @@
> > >       {
> > >           /* We don't -- Windows would try to make this guard page valid
> > >  now */
> > >           DPRINT1("Close to our death...\n");
> > >  -        ASSERT(FALSE);
> > >           return STATUS_STACK_OVERFLOW;
> > >       }
> > >
> > >
> > >
> > >  
> >
> >   _______________________________________________
> >  Ros-dev mailing [e-mail]://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 [e-mail]://www.reactos.org/mailman/listinfo/ros-dev
>
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>