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
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
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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@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
Danny Smith wrote:
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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
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
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; }
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@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
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; } _______________________________________________
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.
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
-
Alex Ionescu -
Danny Smith -
Hartmut Birr -
Joseph Galbraith -
Mark Junker -
Phillip Susi -
Thomas Weidenmueller