Re: [ros-dev] [ros-diffs] [hbelusca] 58110: while (TRUE); (when something is unimplemented) ---> ASSERT(FALSE); // while (TRUE); (unless we deal with a 'noreturn' function), and in some cases, return an adequate value. Part...

[Aleksey Bragin]

With all respect, I don't understand many of these changes. Answering 
between the lines.

On 04.01.2013 15:47, hbelusca(a)svn.reactos.org wrote:
>     NTSTATUS
>  @@ -643,7 +643,8 @@
>        /* FIXME: TODO */
>        DPRINT1("You have implemented the KD routines for searching PCI
> debugger"
>                "devices, but you have forgotten to implement this routine\n");
>  -    while (TRUE);
>  +    UNIMPLEMENTED;
>  +    ASSERT(FALSE); // while (TRUE);
>    } 
It already prints a mandatory log message that this part is 
unimplemented. And execution is supposed to stop after printing this 
message, because it's meaningless to continue (that's why while(TRUE); 
was put there in the first place).

>     
>    static ULONG NTAPI
>  @@ -678,7 +679,7 @@
>        {
>            /* /PCILOCK is not yet supported */
>            UNIMPLEMENTED;
>  -        while (TRUE);
>  +        ASSERT(FALSE); // while (TRUE);
>        }
>    #endif
>        /* Now create the correct resource list based on the supported bus ranges */
It already has UNIMPLEMENTED; and now you added an ASSERT(FALSE); which 
essentially is the same thing. And if continuation is possible, then 
just UNIMPLEMENTED would be enough.

I'd like some general solution to this. Like, UNIMPLEMENTED_FATAL() or 
something like that.

Any thoughts?

Regards,
Aleksey Bragin

[Javier Agustìn Fernàndez Arroyo]

well, IMO, "while(true)" means CPU is permanently busy, while an ASSERT
stops CPU from running ....
just a matter of power saving....

and i repeat, its just an opinion....

On Fri, Jan 4, 2013 at 3:02 PM, Aleksey Bragin <aleksey(a)reactos.org> wrote:

>   With all respect, I don't understand many of these
> changes. Answering
>  between the lines.
>
>  On 04.01.2013 15:47, hbelusca(a)svn.reactos.org wrote:
>
> >     NTSTATUS
> >  @@ -643,7 +643,8 @@
> >        /* FIXME: TODO */
> >        DPRINT1("You have implemented the KD routines for searching PCI
> >  debugger"
> >                "devices, but you have forgotten to implement this
> >  routine\n");
> >  -    while (TRUE);
> >  +    UNIMPLEMENTED;
> >  +    ASSERT(FALSE); // while (TRUE);
> >    }
> >  
>  It already prints a mandatory log message that this part is unimplemented.
>  And execution is supposed to stop after printing this message, because it's
>  meaningless to continue (that's why while(TRUE); was put there in the first
>  place).
>
>       static ULONG NTAPI
> >   @@ -678,7 +679,7 @@
> >        {
> >            /* /PCILOCK is not yet supported */
> >            UNIMPLEMENTED;
> >  -        while (TRUE);
> >  +        ASSERT(FALSE); // while (TRUE);
> >        }
> >    #endif
> >        /* Now create the correct resource list based on the supported bus
> >  ranges */
> >  
>  It already has UNIMPLEMENTED; and now you added an ASSERT(FALSE); which
>  essentially is the same thing. And if continuation is possible, then just
>  UNIMPLEMENTED would be enough.
>
>  I'd like some general solution to this. Like, UNIMPLEMENTED_FATAL() or
>  something like that.
>
>  Any thoughts?
>
>  Regards,
>  Aleksey Bragin
>
>  ______________________________**_________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>
> http://www.reactos.org/**mailman/listinfo/ros-dev<http://www.reactos.org…
>

[Aleksey Bragin]

this while (TRUE); means that "OS can't function anymore if this 
codepath is executed". It's a fatal, terminal way, so there is no power 
to save :)

On 04.01.2013 18:11, Javier Agustìn Fernàndez Arroyo wrote:
>   well, IMO, "while(true)" means CPU is
> permanently busy, while an 
>  ASSERT stops CPU from running ....
>  just a matter of power saving....
>
>  and i repeat, its just an opinion....
>
>  On Fri, Jan 4, 2013 at 3:02 PM, Aleksey Bragin <aleksey(a)reactos.org 
>  <mailto:[e-mail]>> wrote:
>
>      With all respect, I don't understand many of these changes.
>      Answering between the lines.
>
>      On 04.01.2013 15:47, hbelusca(a)svn.reactos.org
>      <mailto:[e-mail]> wrote:
>
>            NTSTATUS
>          @@ -643,7 +643,8 @@
>                /* FIXME: TODO */
>                DPRINT1("You have implemented the KD routines for
>          searching PCI debugger"
>                        "devices, but you have forgotten to implement
>          this routine\n");
>          -    while (TRUE);
>          +    UNIMPLEMENTED;
>          +    ASSERT(FALSE); // while (TRUE);
>            }
>
>      It already prints a mandatory log message that this part is
>      unimplemented. And execution is supposed to stop after printing
>      this message, because it's meaningless to continue (that's why
>      while(TRUE); was put there in the first place).
>
>              static ULONG NTAPI
>          @@ -678,7 +679,7 @@
>                {
>                    /* /PCILOCK is not yet supported */
>                    UNIMPLEMENTED;
>          -        while (TRUE);
>          +        ASSERT(FALSE); // while (TRUE);
>                }
>            #endif
>                /* Now create the correct resource list based on the
>          supported bus ranges */
>
>      It already has UNIMPLEMENTED; and now you added an ASSERT(FALSE);
>      which essentially is the same thing. And if continuation is
>      possible, then just UNIMPLEMENTED would be enough.
>
>      I'd like some general solution to this. Like,
>      UNIMPLEMENTED_FATAL() or something like that.
>
>      Any thoughts?
>
>      Regards,
>      Aleksey Bragin
>

[Ged Murphy]

Well depending on which IRLQ the while loop is in means it could
occasionally loose CPU time only later to be rescheduled.

Also, if you do happen to want examine the running code path, you need to
break in to the running OS, find the runaway thread and attach to it. Having
an ASSERT(FALSE) is surely much simpler?

 

I’m sure Alex had his reasons for using while (TRUE), but I’m not sure what
they are.

 

Ged.

 

 

From: ros-dev-bounces(a)reactos.org [mailto:[e-mail]] On
Behalf Of Aleksey Bragin
Sent: 04 January 2013 14:15
To: ReactOS Development List
Subject: Re: [ros-dev] [ros-diffs] [hbelusca] 58110: while (TRUE); (when
something is unimplemented) ---> ASSERT(FALSE); // while (TRUE); (unless we
deal with a 'noreturn' function), and in some cases, return an adequate
value. Part...

 

this while (TRUE); means that "OS can't function anymore if this codepath is
executed". It's a fatal, terminal way, so there is no power to save :)

On 04.01.2013 18:11, Javier Agustìn Fernàndez Arroyo wrote:

well, IMO, "while(true)" means CPU is permanently busy, while an ASSERT
stops CPU from running ....
just a matter of power saving.... 

and i repeat, its just an opinion....

On Fri, Jan 4, 2013 at 3:02 PM, Aleksey Bragin <aleksey(a)reactos.org
<mailto:[e-mail]> > wrote:

With all respect, I don't understand many of these changes. Answering
between the lines.

On 04.01.2013 15:47, hbelusca(a)svn.reactos.org
<mailto:[e-mail]>  wrote:

  NTSTATUS
@@ -643,7 +643,8 @@
      /* FIXME: TODO */
      DPRINT1("You have implemented the KD routines for searching PCI
debugger"
              "devices, but you have forgotten to implement this
routine\n");
-    while (TRUE);
+    UNIMPLEMENTED;
+    ASSERT(FALSE); // while (TRUE);
  }

It already prints a mandatory log message that this part is unimplemented.
And execution is supposed to stop after printing this message, because it's
meaningless to continue (that's why while(TRUE); was put there in the first
place).

    static ULONG NTAPI
@@ -678,7 +679,7 @@
      {
          /* /PCILOCK is not yet supported */
          UNIMPLEMENTED;
-        while (TRUE);
+        ASSERT(FALSE); // while (TRUE);
      }
  #endif
      /* Now create the correct resource list based on the supported bus
ranges */

It already has UNIMPLEMENTED; and now you added an ASSERT(FALSE); which
essentially is the same thing. And if continuation is possible, then just
UNIMPLEMENTED would be enough.

I'd like some general solution to this. Like, UNIMPLEMENTED_FATAL() or
something like that.

Any thoughts?

Regards,
Aleksey Bragin

[Timo Kreuzer]

Seriously, all these "while (TRUE);"  are retarded. All they do is hide 
bugs.
Especially in usermode and at high irql. (which most of them are from a 
quick glance)
The result is not "The OS cannot continue and halts", but "One thread of 
the OS silently stops doing anything, eating up cpu, while the rest of 
the OS happily continues to mess up the system even more, hiding the 
previous debug print under a shitload of unrelated stuff, so that you 
won't notice anything, except that something doesn't work."
And on high irql it makes the system hang. Which is not much better, 
since usually the reason of a hang is not closely related to the latest 
debug prints, so that's not what you are looking at. "normal users" have 
no idea how to handle that. They won't give you a bt, something people 
often do when experiencing a crash or failed assertion.

If it's fatal, either bugcheck or ASSERT or DbgBreakPoint();

Therefore: good work, killing this crap.
If there is a DPRINT1 + UNIMPLEMENTED or not ... who cares? It's not 
like this is production code ;-)

Timo

[Timo Kreuzer]

Am 04.01.2013 22:39, schrieb Timo Kreuzer:
>   Especially in usermode and at high irql 
Sorry I
meant at low irql!

[Hermès BÉLUSCA - MAÏTO]

Hi !

Today or tomorrow (i.e. this week-end), I will arrange that and put the
Assert(false); // while(true); into a better form (doing a real break) and
introduce, as Aleksey has suggested, a UNIMPLEMENTED_FATAL() macro which
does all that.
As some people "complained" on IRC yesterday about keeping the //
while(true); comment in the code, it makes easiest to find the portions of
the code where the while(true); where placed in order to "stop" the
execution of code because of unimplemented feature, rather than what I call
a "legitimate" while(true); coming from, e.g., code like: do { /* something
with break; */ } while(true); . In other words, doing a search in the code
for the string ' ASSERT(FALSE); // while (TRUE); ' gives you instantly the
places where the original while(true); for unimplemented features, were.

Hermès

-----Message d'origine-----
De : ros-dev-bounces(a)reactos.org [mailto:[e-mail]] De la
part de Timo Kreuzer
Envoyé : vendredi 4 janvier 2013 22:50
À : ReactOS Development List
Objet : Re: [ros-dev] [ros-diffs] [hbelusca] 58110: while (TRUE); (when
something is unimplemented) ---> ASSERT(FALSE); // while (TRUE); (unless we
deal with a 'noreturn' function), and in some cases, return an adequate
value. Part...

Am 04.01.2013 22:39, schrieb Timo Kreuzer:
>   Especially in usermode and at high irql 
Sorry I
meant at low irql!



_______________________________________________
Ros-dev mailing list
Ros-dev(a)reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev