Bugs item #1717943, was opened at 2007-05-13 01:32
Message generated for change (Comment added) made by maartenbrock
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1717943&group_id=599
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: mcs51(8051) target
>Group: fixed
>Status: Closed
>Resolution: Accepted
Priority: 5
Private: No
Submitted By: Wolfgang Heidrich (wolfgang75)
>Assigned to: Maarten Brock (maartenbrock)
Summary: wrong optimized flow
Initial Comment:
/usr/local/bin/sdcc -c bug1.c
SDCC : mcs51/gbz80/z80/avr/ds390/pic16/pic14/TININative/xa51/ds400/hc08 2.5.0 #1020 (May 8 2005) (UNIX)
As far as I understand the compiler performs a wrong optimization. It happens with the compiler running on Windows or GNU/Linux, with both versions 2.5.0 and 2.6.0.
In the example there is the xdata variable "flags", that should be set to zero ("flags=0") inside the loop. This loop contains another loop, which tests this variable "flags" and also calls another function "check()". There is also an volatile "alarm_alarm"-Bit, that may be set by an interrupt service routine. The bug is following: In assembler the compiler puts "flags=0" outside of all loops (before label 00103$ in assembler), see assembler code. Additional it removes the condition test of the inner while loop.
I presume the compiler makes an optimization in the following wrong way: The compiler thinks, that there is no possibility that "flags" might change, but in reality this is wrong, because the function "check()" might change "flags", as "flags" is a global variable.
Here is the wrong assembler code:
;--------------------------------------------------------
; code
;--------------------------------------------------------
.area CSEG (CODE)
;------------------------------------------------------------
;Allocation info for local variables in function 'bug'
;------------------------------------------------------------
;------------------------------------------------------------
;bug1.c:7: void bug( void )
; -----------------------------------------
; function bug
; -----------------------------------------
_bug:
ar2 = 0x02
ar3 = 0x03
ar4 = 0x04
ar5 = 0x05
ar6 = 0x06
ar7 = 0x07
ar0 = 0x00
ar1 = 0x01
;bug1.c:9: do // bughere
; genAssign
mov dptr,#_flags
; Peephole 181 changed mov to clr
clr a
movx @dptr,a
;bug1.c:12: do
00103$:
;bug1.c:14: if( alarm_alarm ) goto fast_exit;
; genIfx
; genIfxJump
; Peephole 112.a removed ljmp by inverse jump logic
jb _alarm_alarm,00110$
00116$:
;bug1.c:15: check();
; genCall
lcall _check
;bug1.c:16: } while( !( flags & 1 ) );
; genAssign
mov dptr,#_flags
movx a,@dptr
; genAnd
; Peephole 105 removed redundant mov
mov r2,a
anl a,#0x01
;bug1.c:18: } while( 1 );
;bug1.c:20: fast_exit:
; Peephole 112.b changed ljmp to sjmp
sjmp 00103$
00110$:
ret
.area CSEG (CODE)
.area XINIT (CODE)
----------------------------------------------------------------------
>Comment By: Maarten Brock (maartenbrock)
Date: 2008-03-30 21:39
Message:
Logged In: YES
user_id=888171
Originator: NO
I should have looked better and seen that Robert also included a fix, not
just regression tests.
Fixed now in SDCC 2.8.1 #5129.
----------------------------------------------------------------------
Comment By: Nobody/Anonymous (nobody)
Date: 2007-07-07 22:57
Message:
Logged In: NO
the test case can be further reduced to
char foo;
bit alarm;
void check(void);
void bug(void)
{
do {
foo = 0;
do {
if(alarm)
return;
check();
} while(alarm);
} while(1);
}
the problem disappears with the option
--noinvariant
stepping with gdb, i think the function
loopInvariants()
erounously classifies the variable foo as a loop invariant,
and moves it out of the loops.
this function allready contains a sequence:
if (fCallsInBlock) {
...
/* if this is an assignment from a global */
if (ic->op=='=' && isOperandGlobal(IC_RIGHT(ic))) {
continue;
}
...
}
which prohibits classification of assignments right side variables
as globals, when there is a function call in the loop body
(fCallsInBlock)
and said variable is global.
so i propose to extend this with another if, which prohibits
variables on the left side of an assignment as well.
here is the suggested patch:
--- sdcc-4870/src/SDCCloop.c 2007-06-28 11:16:10.000000000 +0200
+++ sdcc-4870-gdb/src/SDCCloop.c 2007-07-07 17:01:33.000000000 +0200
@@ -475,32 +475,38 @@ loopInvariants (region * theLoop, ebbInd
/* TODO this is only needed if the call is between
here and the definition, but I am too lazy to do that now */
/* if there are function calls in this block */
if (fCallsInBlock) {
/* if this is a pointer get */
if (POINTER_GET(ic)) {
continue;
}
/* if this is an assignment from a global */
if (ic->op=='=' && isOperandGlobal(IC_RIGHT(ic))) {
continue;
}
+ /* Bug 1717943,
+ * if this is an assignment to a global */
+ if (ic->op=='=' && isOperandGlobal(IC_RESULT(ic))) {
+ continue;
+ }
+
}
if (SKIP_IC (ic) || POINTER_SET (ic) || ic->op == IFX)
continue;
/* iTemp assignment from a literal may be invariant, but it
will needlessly increase register pressure if the
iCode(s) that use this iTemp are not also invariant */
if (ic->op=='=' && IS_ITEMP (IC_RESULT (ic))
&& IS_OP_LITERAL (IC_RIGHT (ic)))
continue;
/* if result is volatile then skip */
if (IC_RESULT (ic) &&
(isOperandVolatile (IC_RESULT (ic), TRUE) ||
IS_OP_PARM (IC_RESULT (ic))))
I've added two smaller test files for the support/regression/tests
directory as well.
Those two reliably trigger without the patch, and don't trigger with the
patch,
I'd propose the second one.
------------------------------------------------------------
bug1717943b.c
/*
bug1717943b.c
an error in the detection of loopinvariants,
will move the foo=0 initialisation out of the loops.
*/
#include <testfwk.h>
char foo;
char alarm_return;
char alarm_while;
void check(void)
{
alarm_while = 0;
alarm_return = 1;
foo = 42;
}
void bug(void)
{
do {
foo = 0;
do {
if(alarm_return)
return;
check();
} while(alarm_while);
} while(1);
}
void
testBug(void)
{
alarm_return = 0;
bug();
ASSERT(foo == 0);
}
------------------------------------------------------------
bug1717943c.c
/*
bug1717943c.c
an error in the detection of loopinvariants,
will move the foo=0 initialisation out of the loops.
*/
#include <testfwk.h>
char foo, firstcall;
char check(void)
{
if(!firstcall)
return 1;
firstcall=0;
foo = 42;
return 0;
}
void bug(void)
{
while(1) {
foo = 0;
while(check())
if(check())
return;
}
}
void
testBug(void)
{
firstcall = 1;
bug();
ASSERT(foo == 0);
}
------------------------------------------------------------
maartenbrock mentioned bug 1618050
might be the same.
unfortunatly the given patch does'nt fix that one.
Robert Larice
larice 0x40 vidisys 0x2e de
----------------------------------------------------------------------
Comment By: Frieder Ferlemann (frief)
Date: 2007-05-20 19:22
Message:
Logged In: YES
user_id=589052
Originator: NO
the attached file bug1717943.c allows to reproduce the bug within
the regression tests.
"make test-host" passes,
but the SDCC compiled tests fail either with
"Assertion failed" on b[1] == 1
(ds390, mcs51, mcs51-large, mcs51-medium, mcs51-stack-auto
mcs51-xstack-auto, pic16)
or (hc08) with:
FATAL Compiler Internal Error in file '../../../sdcc/src/hc08/gen.c' line
number '5039' : AOP_CRY result without ifx
or (ucz80) with:
bug1717943.asm:187: Error: <u> undefined symbol encountered during
assembly
(The label 00115$ is conditionally jumped to but was not defined)
I removed the goto statement within the original bug1.c
and used a return statement there because
"gcc -c -Dbit=char -Dxdata= bug1.c" complained:
"bug1.c:21: error: label at end of compound statement".
gcc (4.1.2 20061115 (prerelease) (SUSE Linux))
seemingly did not like the empty label at the end
of the function bug().
File Added: bug1717943.c
----------------------------------------------------------------------
Comment By: Maarten Brock (maartenbrock)
Date: 2007-05-13 08:52
Message:
Logged In: YES
user_id=888171
Originator: NO
I guess this is related to or the same as bug 1618050.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1717943&group_id=599
|