gcc problem or not

[Hartmut Birr]

Hi,
I've looked on a map file from an optimized build. I found the following
code:
  /*
   * If the directory containing the file to open doesn't exist then
   * fail immediately
   */
  if (Status == STATUS_OBJECT_PATH_NOT_FOUND ||
   12015:    83 c4 0c                 add    $0xc,%esp
   12018:    89 c3                    mov    %eax,%ebx
   1201a:    3d 3a 00 00 c0           cmp    $0xc000003a,%eax
   1201f:    0f 94 c2                 sete   %dl
   12022:    3d 0d 00 00 c0           cmp    $0xc000000d,%eax
   12027:    0f 94 c0                 sete   %al
   1202a:    09 d0                    or     %edx,%eax
   1202c:    a8 01                    test   $0x1,%al
   1202e:    0f 85 42 01 00 00        jne    12176 <_VfatCreateFile+0x2e9>
   12034:    81 fb 56 00 00 c0        cmp    $0xc0000056,%ebx
   1203a:    0f 84 36 01 00 00        je     12176 <_VfatCreateFile+0x2e9>
      Status == STATUS_INVALID_PARAMETER ||
      Status == STATUS_DELETE_PENDING)
    {
      if (ParentFcb)
      {
         vfatReleaseFCB (DeviceExt, ParentFcb);
      }
      return(Status);
    }
I was always the opinion  that the examination of a test condition stops
if the result can not change again. A test condition like this:
if (pointer == NULL || pointer->member == 0)
should never access pointer->member if pointer is zero. Compared with
the code above, it is possible that gcc build the result from the right
side of the OR statement. This may hit a page fault. Is this a bug in gcc?
- Hartmut

[Thomas Weidenmueller]

Hartmut Birr wrote:
>   Hi,
>  I was always the opinion  that the examination of a test condition stops
>  if the result can not change again. A test condition like this:
>  if (pointer == NULL || pointer->member == 0)
>  should never access pointer->member if pointer is zero. Compared with
>  the code above, it is possible that gcc build the result from the right
>  side of the OR statement. This may hit a page fault. Is this a bug in gcc? 
It should definitely not dereference if it's NULL. The conditions should
be checked from left to right in this case, no matter how much was
optimized.
Thomas

[Danny Smith]

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.com
>  [mailto:[e-mail]] On Behalf Of Thomas Weidenmueller
>  Sent: Wednesday, May 04, 2005 7:16 AM
>  To: ReactOS Development List
>  Subject: Re: [ros-dev] gcc problem or not
>  Hartmut Birr wrote:
> >   Hi,
> >  I was always the opinion  that the examination of a test condition
> >  stops if the result can not change again. A test condition  
>  like this:
> >  if (pointer == NULL || pointer->member == 0)
> >  should never access pointer->member if pointer is zero.  
>  Compared with
> >   the code above, it is possible that gcc build the
> > result from the
> >  right side of the OR statement. This may hit a page fault.  
>  Is this a
> >   bug in gcc? 
>  It should definitely not dereference if it's NULL. The
>  conditions should
>  be checked from left to right in this case, no matter how much was
>  optimized.
>   
To be safe, you might also check
BadReadPtr(pointer,sizeof(struct_pointed_to)).  The pointer may be
undefined, rather than NULL.
Danny
>   Thomas
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.com http://reactos.com:8080/mailman/listinfo/ros-dev
>  --
>  No virus found in this incoming message.
>  Checked by AVG Anti-Virus.
>  Version: 7.0.308 / Virus Database: 266.11.2 - Release Date: 5/2/2005
>   
--
No virus found in this outgoing message.
Checked by AVG Anti-Virus.
Version: 7.0.308 / Virus Database: 266.11.2 - Release Date: 5/2/2005

[Hartmut Birr]

Danny Smith wrote:
> >  -----Original Message-----
> > From: ros-dev-bounces(a)reactos.com
> > [mailto:[e-mail]] On Behalf Of Thomas Weidenmueller
> > Sent: Wednesday, May 04, 2005 7:16 AM
> > To: ReactOS Development List
> > Subject: Re: [ros-dev] gcc problem or not
> > Hartmut Birr wrote:
> > >  Hi,
> > > I was always the opinion  that the examination of a test condition
> > > stops if the result can not change again. A test condition
> > >  
> > like this:
> > >  if (pointer == NULL || pointer->member == 0)
> > > should never access pointer->member if pointer is zero.
> > >  
> > Compared with
> > >  the code above, it is possible that gcc build the
> > > result from the
> > > right side of the OR statement. This may hit a page fault.
> > >  
> > Is this a
> > >  bug in gcc?
> > >  
> > It should definitely not dereference if it's NULL. The
> > conditions should
> > be checked from left to right in this case, no matter how much was
> > optimized.
> >  
> To be safe, you might also check
> BadReadPtr(pointer,sizeof(struct_pointed_to)).  The pointer may be
> undefined, rather than NULL.
> Danny
>  
You are right, but I've still assumed that the pointer is zero or valid.
The real problem is, that gcc doesn't stop to validate an expression if
the result is determined and if the expression consist of simple
expressions which are combined with OR.
- Hartmut

[Mark Junker]

Hartmut Birr schrieb:
>  You are right, but I've still assumed that the
> pointer is zero or valid.
> The real problem is, that gcc doesn't stop to validate an expression if
> the result is determined and if the expression consist of simple
> expressions which are combined with OR.
>  
Does this also happen when using something like the following?
if (p!=NULL && p->val==0) {
}
I would assume that GCC's optimizer realizes that your (not mine above)
sample code is just some kind of switch statement that can be heavily
optimized. This kind of optimization (-> switch) is ok in this situation.
And I have to admit that the optimization is quite good because it
avoids a conditional jump.
Regards,
Mark

[Thomas Weidenmueller]

Hartmut Birr wrote:
>   You are right, but I've still assumed that the
> pointer is zero or valid.
>  The real problem is, that gcc doesn't stop to validate an expression if
>  the result is determined and if the expression consist of simple
>  expressions which are combined with OR. 
i tried the following test application with various configurations of
gcc 4.0 and 3.4.2, but it appears to work correctly.
#include <stdio.h>
int main(int argc, char* argv[])
{
   volatile int *x = NULL;
   if(x == NULL || *x == 0)
   {
      printf("true\n");
   }
   else
   {
      printf("false\n");
   }
   return 0;
}

[Hartmut Birr]

Thomas Weidenmueller wrote:
>   Hartmut Birr wrote:
> >   You are right, but I've still assumed that
> > the pointer is zero or valid.
> >  The real problem is, that gcc doesn't stop to validate an expression if
> >  the result is determined and if the expression consist of simple
> >  expressions which are combined with OR. 
>  i tried the following test application with various configurations of
>  gcc 4.0 and 3.4.2, but it appears to work correctly.
>  #include <stdio.h>
>  int main(int argc, char* argv[])
>  {
>    volatile int *x = NULL;
>    if(x == NULL || *x == 0)
>    {
>       printf("true\n");
>    }
>    else
>    {
>       printf("false\n");
>    }
>    return 0;
>  }
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.com
>  http://reactos.com:8080/mailman/listinfo/ros-dev
>  
This means, gcc is intelligent enough to made a difference between
comparing a variable with different values and between calculating
expressions from pointers.
- Hartmut

[Phillip Susi]

The C and C++ standards REQUIRE the logical operators to short-circuit,
meaning that the left hand side of the || must be evaluated first, and
then the right hand side is evaluated if and only if the left hand side
evaluated as false.
Your test should always print true, and never crash.  If it does, then
the compiler is indeed, very broken.
Thomas Weidenmueller wrote:
>   i tried the following test application with various
> configurations of
>  gcc 4.0 and 3.4.2, but it appears to work correctly.
>  #include <stdio.h>
>  int main(int argc, char* argv[])
>  {
>    volatile int *x = NULL;
>    if(x == NULL || *x == 0)
>    {
>       printf("true\n");
>    }
>    else
>    {
>       printf("false\n");
>    }
>    return 0;
>  }
>  _______________________________________________

[Joseph Galbraith]

Hartmut Birr wrote:
>   Hi,
>  I've looked on a map file from an optimized build. I found the following
>  code:
>    /*
>     * If the directory containing the file to open doesn't exist then
>     * fail immediately
>     */
>    if (Status == STATUS_OBJECT_PATH_NOT_FOUND ||
>     12015:    83 c4 0c                 add    $0xc,%esp
>     12018:    89 c3                    mov    %eax,%ebx
>     1201a:    3d 3a 00 00 c0           cmp    $0xc000003a,%eax
>     1201f:    0f 94 c2                 sete   %dl
>     12022:    3d 0d 00 00 c0           cmp    $0xc000000d,%eax
>     12027:    0f 94 c0                 sete   %al
>     1202a:    09 d0                    or     %edx,%eax
>     1202c:    a8 01                    test   $0x1,%al
>     1202e:    0f 85 42 01 00 00        jne    12176 <_VfatCreateFile+0x2e9>
>     12034:    81 fb 56 00 00 c0        cmp    $0xc0000056,%ebx
>     1203a:    0f 84 36 01 00 00        je     12176 <_VfatCreateFile+0x2e9>
>        Status == STATUS_INVALID_PARAMETER ||
>        Status == STATUS_DELETE_PENDING)
>      {
>        if (ParentFcb)
>        {
>           vfatReleaseFCB (DeviceExt, ParentFcb);
>        }
>        return(Status);
>      }
>  I was always the opinion  that the examination of a test condition stops
>  if the result can not change again. A test condition like this:
>  if (pointer == NULL || pointer->member == 0)
>  should never access pointer->member if pointer is zero. Compared with
>  the code above, it is possible that gcc build the result from the right
>  side of the OR statement. This may hit a page fault. Is this a bug in gcc? 
Hmmm... if gcc were to evaluate pointer->member in the
above example, that would definitely be a horrendous gcc
bug.
However, in the example you gave the dissambly for, I believe
gcc can evaluate eax multiple times before 'shortcutting'
the evaluation, because it knows that evaluating eax multiple
times can't have side effects.
Does the if ( ! p || ! p->x ) case generate similar assembler?
Thanks,
Joseph
PS. I disclaim all responsibility for this answer, I have no
idea what I'm talking about.  Everything here is pure speculation.

[Alex Ionescu]

Hartmut Birr wrote:
>  Hi,
> I've looked on a map file from an optimized build. I found the following
> code:
>   /*
>    * If the directory containing the file to open doesn't exist then
>    * fail immediately
>    */
>   if (Status == STATUS_OBJECT_PATH_NOT_FOUND ||
>    12015:    83 c4 0c                 add    $0xc,%esp
>    12018:    89 c3                    mov    %eax,%ebx
>    1201a:    3d 3a 00 00 c0           cmp    $0xc000003a,%eax
>    1201f:    0f 94 c2                 sete   %dl
>    12022:    3d 0d 00 00 c0           cmp    $0xc000000d,%eax
>    12027:    0f 94 c0                 sete   %al
>    1202a:    09 d0                    or     %edx,%eax
>    1202c:    a8 01                    test   $0x1,%al
>    1202e:    0f 85 42 01 00 00        jne    12176 <_VfatCreateFile+0x2e9>
>    12034:    81 fb 56 00 00 c0        cmp    $0xc0000056,%ebx
>    1203a:    0f 84 36 01 00 00        je     12176 <_VfatCreateFile+0x2e9>
>       Status == STATUS_INVALID_PARAMETER ||
>       Status == STATUS_DELETE_PENDING)
>     {
>       if (ParentFcb)
>       {
>          vfatReleaseFCB (DeviceExt, ParentFcb);
>       }
>       return(Status);
>     }
> I was always the opinion  that the examination of a test condition stops
> if the result can not change again. A test condition like this:
> if (pointer == NULL || pointer->member == 0)
> should never access pointer->member if pointer is zero. Compared with
> the code above, it is possible that gcc build the result from the right
> side of the OR statement. This may hit a page fault. Is this a bug in gcc?
>  
This is not a bug but simply an optimization. GCC correctly realizes
that doing both checks does not result in any possibility of a crash
like in your pointer example, but that instead it allows it to save an
extra jump condition (which saves space and makes the CPU's branch
predictor have one less thing to worry about). Note that the first
checks affects EAX while the second affects EDX. It's quite a cute
optimization actually.
Best regards,
Alex Ionescu