wrong compiled output code??

[skp888]

hey hey!!

cant seem to get my head around this problem, im sure its something that im missing...

it *seems* that for a particular "for" loop in my C code, the compiler (GNU 4.5.2) has produced an endless loop.
the "for" loop should only loop 200 or so times, but when i step through the code in the debugger while running it on Bochs, the op-code that is being run *correctly* sets ebx to the counting variable's ("i" in the C code) initial value and *correctly* increments ebx on each loop iteration as i step through the running code, *BUT* there is no comparison of ebx to my "upper limit" at which the loop is supposed to cease - in the decompiled assembly there is only an unconditional "jmp .-25" and hence i get an endless loop...

anyway, i need to give some solid information so here it is:
C compiler: GNU 4.5.2
Bochs: my *debugging* version is custom-compiled with just about all features (SMP, APIC, USB etc) (yes i know its slow, but i have a "lean" version for fast non-debugging tests )

here is a snippet of the C-Source code around the offending "for" loop (p.s. IDT_ENTRIES == 256):

Code: Select all

	//...
	idt_SetGate(IRQ_Map[14],	IRQ14, ring0_Code, IDT_FLAGS_RING0);
	idt_SetGate(IRQ_Map[15],	IRQ15, ring0_Code, IDT_FLAGS_RING0); /** eip = 0x0000be47 **/
	// finished remapping PIC IRQs

	// (for now)... just register the rest of the interrupts up to 255 to run ISR048
	for(i = 48; i <= (IDT_ENTRIES - 1); i++) {
		idt_SetGate(i, ISR048, ring0_Code, IDT_FLAGS_RING0);
	}

	// register the kernel data descriptor to use for interrupt routines
	// (write to _ISR_kernel_data_descriptor, located in ISR.asm)
	ISR_kernel_data_descriptor = ring0_Data;
	
	// initialise all external interrupt drivers to NULL
	struct InterruptHandler_s null_Handler;
	null_Handler.handlerFunction = 0;
	for(i = 0; i <= (IDT_ENTRIES - 1); i++) {
		idt_RegisterInterruptHandler(i, null_Handler);
	}
	
	// set the new IDT
	idt_Flush();
}

and the corresponding dissassembled code... (im pretty sure this is it, but maybe ive not followed my debug-stepping closely enough due to lack of sleep... eitherway im going to be rechecking over it so ill update if what i post here turns out to be wrong)

Code: Select all

L.Address		(bytes) Opcode		Mnemonic
---------------------------------------------------------
0x0000be47		(5) e858faffff		call .-1448 (0x0000b8a4) <see corresponding C-code comment>
0x0000be4c		(2) b330			mov bl, 0x30
0x...be4e		(3) 0fb6c3			movzx eax, bl
be51			(7) c704248e000000	mov dword ptr ss:[esp], 0x0000008e
be58			(2) 89f1			mov ecx, esi
be5a			(5) ba7fb40000		mov edx, 0x0000b47f
be5f			(5) e840faffff		call .-1472 (0x0000b8a4)
be64			(1) 43			inc ebx
be65			(2) ebe7			jmp .-25 (0x0000be4e)
be67			(1) 90			nop
be68			(5) 0fb6442404		movzx eax, byte ptr ss:[esp+4]
be6d			(4) 8b542408		mov edx, dword ptr ss:[esp+8]
be71			(7) 89148514f00000	mov dword ptr ds:[eax*4+61460], edx
be78			(1) c3			ret
be79			(1) c3			ret
be7a			(1) 57			push edi
be7b			(1) 56			push esi
be7c			(3) ...			...
--------------------------------------------------------------------------------
at eip = 0x0000be4c (just after the last function call returns, right before entering the offending "for" loop) the registers are:
eax = 0x0000008e
ebx = 0x0000000a
ecx = 0x0000e2b8
edx = 0x00000008
esi = 0x00000008
edi = 0x00007ac0
ebp = 0x00000000
esp = 0x000079a0
(segment registers are all correct GDT entries, etc)

any insight into what im overlooking would be wonderful!! anymore info let me know

thanks!!

[eddyb]

What type is i?
I hope not uint8_t or char (for which it would be an infinite loop and also you'd get a warning (assuming those warnings aren't turned off).

PS: there's a good reason for having your for's like for(int i = 0; i < limit; i++) - especially the int i part. In this case, it'd be really easy to see the type of i. (Been trying to find some online FAQ that approves, but google's throwing forum threads at me. Maybe someone else has better luck).

EDIT: Maybe I wasn't clear enough: an 8 bit unsigned int (uint8_t or unsigned char) will hold values between 0 and 255, including 0 and 255. Your loops are both with the condition i <= 255, which is always true for 8 bit ints. That's the most obvious possibility, because then gcc would output code to increase the counter, but it would overflow and never exit the loop, so why checking for something that won't happen? I hope this helped.

[skp888]

Heh, just an article for you. (there are 3 parts, be sure to check out all of them)

hehehe, the article for me indeed!! very very interesting interesting article, covered the error in my code as well as many other very interesting cases!! thanks!!

(incase someone's interested or finds it useful): just as was very clearly highlighted by eddyb, my sleep deprived muddling with my function lead to my "for" loop's break condition being changed to an impossible to reach case because of overflow of the data type of i (which was indeed of a typedef'ed version of __UINT8_TYPE__), which then the compiler optimized by removing the pointless comparison checks on i and replacing it with an unconditional jmp, which then, when i saw this while stepping through the machine code, just threw me into total confusion

What type is i?
I hope not uint8_t or char (for which it would be an infinite loop and also you'd get a warning (assuming those warnings aren't turned off).

so yea, it was of uint8_t and hence the optimisation leading to the bug, but im compiling with (warning flags anyways) "-Wall -Wno-unused-function", im not sure if that would turn off these so called "infinite loop warnings" or not?? with these flags no "infinite loop warnings" have ever been generated?? (sure would have been useful haha) could the typedef on the __UINT8_TYPE__ have masked them?? [EDIT: or maybe (as i state below) such warnings arent generated under such old standard like c89, which i think gcc defaults to (and i hadnt manually changed)??]

PS: there's a good reason for having your for's like for(int i = 0; i < limit; i++)

haha yea, gcc default "standard" (i think C89??) doesnt allow for the declaration inside the "for" statement and i foolishly hadnt been bothered to add in the flag to change the compiler's standard to something a lil more recent haha thats now promptly been added to my makefile hahah


anyways,
thankyou very much eddyb for spotting & highlighting my crucially added "=" so quickly, even though the data type declaration was not included in the code snippet
& thankyou very much berkus for the perfect article explaining (along with many others) such a case

[Solar]

...but im compiling with (warning flags anyways) "-Wall -Wno-unused-function", im not sure if that would turn off these so called "infinite loop warnings" or not?? with these flags no "infinite loop warnings" have ever been generated?? (sure would have been useful haha)

GCC offers a plethora of warning options, many of which are not included in "-Wall" or even "-Wextra". It's good advice to use as many as humanly possible (which might be less than "everything" due to third-party headers being involved). It's also good advice to repeatedly check the docs for new options, and update your Makefile accordingly (which I haven't done for far too long, so I won't boast with my current list-of-warnings at this point).

Edit: Service of the house. I skimmed the GCC 4.6.0 docs for a good list of warning options for C code, and came up with:

Code: Select all

-std=c99 -pedantic -Wall -Wextra -Wdouble-promotion -Wformat=2 -Winit-self -Wmissing-include-dirs -Wswitch-default -Wswitch-enum -Wsync-nand -Wunused -Wstrict-overflow=5 -Wtrampolines -Wfloat-equal -Wundef -Wno-endif-labels -Wshadow -Wunsafe-loop-optimizations -Wbad-function-cast -Wc++-compat -Wcast-qual -Wcast-align -Wwrite-strings -Wconversion -Wjump-misses-init -Wlogical-op -Waggregate-return -Wstrict-prototypes -Wold-style-definition -Wmissing-prototype -Wmissing-declarations -Wnormalized=nfc -Wpacked -Wpadded -Wredundant-decls -Wnested-externs -Winline -Winvalid-pch -Wdisabled-optimization -Woverlength-strings -Wunsuffixed-float-constants

Tastes differ, of course.

[Love4Boobies]

Heh, just an article for you. (there are 3 parts, be sure to check out all of them)

my sleep deprived muddling with my function lead to my "for" loop's break condition being changed to an impossible to reach case because of overflow of the data type of i (which was indeed of a typedef'ed version of __UINT8_TYPE__)

Unsigned types don't overflow in C/C++; if they did, it could have very well worked. You mean wraparound.

Solar's advice is a worthwhile one, just add -Werror so none of the warnings gets past.

Not sure what to say about -Werror. Sometimes you have a good reason to ignore warnings while still writing standard-compliant code. It obviously can't be used in OS development together with all warnings turned on. As for application development, as Solar put it, tastes differ. I've never used it, at least.

[Combuster]

It obviously can't be used in OS development together with all warnings turned on.

I tried enabling all warnings for my 4.1 crosscompiler, and only found two discutable flags and one problematic one that just won't work. -Wconversion prohibits floats as function arguments, -Wlong-long warns about stuff that don't work in outdated c. Of the three, -Wpadding then warns about structs where padding has been added according to the ABI, and is therefore the only one that produces false positives most of the time rather than always.

And even then, you can program against that slightly unhealthy doctrine. Impossible? no. Proof directly above

The exercise did expose a pair of design errors in pdclib (including something gross), so now I probably have to check for updates.

[Solar]

The exercise did expose a pair of design errors in pdclib (including something gross)...

This weekend my little boy celebrates his 4th birthday, and my brother-in-law his ...th birthday, so I'm kinda busy. But there's some vacation coming up in week 24, and I fully intent to get the number of unfixed bug reports back to zero.

So, if you've got anything beyond bug #46, go ahead and file it. You file it, I fix it.

[Owen]

I commented on an older bug; it seems to have fell between the cracks

--

Regarding warnings: Some are purely down to personal preference. I find that -Wunused-parameters generates far more frustration than actual help