[ros-dev] gcc problem or not

[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