#2205 return statement discarded if expected operand missing

closed-fixed
Erik Petrich
None
Front-end
5
2014-06-27
2013-08-18
Oleg N. Cher
No

1: Sample code that reproduces the problem.

static char m;

void *two__init (void)
{
if(m)return;
m=1;
}

2: Exact command used to run SDCC on this sample code

sdcc -mz80 -c two.c

3: SDCC version

SDCC : mcs51/gbz80/z80/z180/r2k/r3ka/ds390/pic16/pic14/TININative/ds400/hc08/s08
/stm8 3.3.1 #8807 (Aug 18 2013) (MINGW32)

4: Copy of the error message or incorrect output, or a clear description of the observed versus expected behavior.

SDCC absoultely does not generate any code for the line "if(m)return;"
two.c:5: warning 110: conditional flow changed by optimizer: so said EVELYN the
modified DOG

;two.c:3: void *twoinit (void)
; ---------------------------------
; Function two
init
; ---------------------------------
_twoinit_start::
_two
init:
;two.c:6: m=1;
ld iy,#_m
ld 0 (iy),#0x01
ret

But if just replace "return" to "return 0" - the problem disappears.

Discussion

  • If the bug is related to the peephole optimizer or peephole rules, it should go away when compiling using --no-peep.

    Philipp

     
    • labels: Z80 peephole optimizer static variable -->
    • Category: Z80 --> other
     
  • I can reproduce this issue in the latest version. It is not related to the peephole optimizer; it also happens for ports other than z80 (tried hc08).
    The bug goes away when the function is changed to return void instead of void*.

    Also, I do not know if this really is a bug: Not returning a value in a function declared tor eturn void * might invoke undefined behaviour, but I want to have a look at the standard before making a judgement.

    Philipp

     
  • Hmm, apparently ISO C99 and ANSI C89 allow this if the return value is not used by the calling function. In ISO C99 and ISO C11 it is a constraint violation.
    So IMO this should be considered a (front-end) bug when using --std-c89. It might make sense for sdcc to produce the same code as in ANSI C89 mode for all modes (unless we lose some optimizations for conforming code that way).

    Philipp

     
    • Category: other --> Front-end
     
    • summary: Probably is it a peephole optimizer's bug? --> if omitted in C89/C90 code that is not valid C99
     
  • Oleg N. Cher
    Oleg N. Cher
    2013-08-18

    Hi Philipp,

    This code with a void* function that must returns a value, but not returns, is generated automatically by Ofront (Oberon-2 to C trasnlator). Please see how we find the problem. That code works good under tcc (gcc), but not works in SDCC:

    http://zx.oberon2.ru/forum/viewtopic.php?f=10&t=135#p613

    Also, I do not know if this really is a bug: Not returning a value in a function declared tor eturn void * might invoke undefined behaviour.

    And the business is not only in having a right result after calling of the function - for this case it doesn't matter. The problem is that needed code with statement, that must return from the function, will be optimized (or was not generated) at all. And independently from result - returns it, or not. In other words - it is not question about is there right result returned from a function, or not right, but must not be breaking the logic of internal returns from the function at all. Or do you think that

    1) Returning wrong result in a function if it is not specified

    and:

    2) Not generating returning code from any place of a function (except end) at all

    is the same?

     
  • Maarten Brock
    Maarten Brock
    2013-08-19

    I think that modifying a variable that should not have been modified is a little far from undefined behavior for a missing return statement. I would have expected the return of corrupt data or even no return at all, but this?

    Further I expect this to be another display of a constant '1' being 'promoted' to _Bool. After that the front-end decides that m is always true after the conditional assignment.

     
  • When using --nolabelopt, the problöem does not appear:

    sdcc -mz80 test.c --fverbose-asm --std-c89 --nolabelopt --i-code-in-asm
    

    gives:

    _two__init:
    ;test.c:5: if(m)return;
    ;ic:2:  if _m [k2 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{char fixed} != 0 goto _iffalse_0($2)
    ;   genIfx
    ;fetchLitPair
    ; peephole 16 loaded _m into a directly instead of going through iy.
        ld  a,(#_m + 0)
        or  a, a
        jr  NZ,00102$
    ; peephole 155 changed absolute to relative conditional jump.
    ;ic:3:   goto _iffalse_0($2)
    ;   genGoto
    ;ic:4:  _iftrue_0($1) :
    ;   genLabel
    ;ic:5:  _iffalse_0($2) :
    ;   genLabel
    ; peephole 65a eliminated jump.
    ; peephole 149 removed unused label 00101$.
    00102$:
    ;test.c:6: m=1;
    ;ic:7:  _m [k2 lr0:0 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{char fixed} := 0x1 {const-char literal}
    ;   genAssign
    ;fetchLitPair
        ld  iy,#_m
        ld  0 (iy),#0x01
    ;ic:8:  _return($3) :
    ;   genLabel
    ; peephole 149 removed unused label 00103$.
    ;ic:9:  eproc _two__init [k1 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{int function ( ) __reentrant fixed}
    ;   genEndFunction
        ret
    
     
  • Maarten Brock
    Maarten Brock
    2013-08-19

    I hate to disappoint you but the problem is still there. The label 00102$ appears too early.

     
  • Yes. Sorry. Even dumpraw0 already looks buggy, so this seems to really be a frontend bug.

    Philipp

     
  • Erik Petrich
    Erik Petrich
    2013-08-19

    • assigned_to: Erik Petrich
     
  • Erik Petrich
    Erik Petrich
    2013-08-19

    The diagnostic message for return missing an expected operand is classified as a warning, but then is being handled as an error (discarding the return operation entirely). I still need to do some testing, but I should have a commit to fix this later today.

     
  • Maarten Brock
    Maarten Brock
    2013-08-19

    Ah, so it was a 'no return at all' for undefined behavior.
    Is there any reason to keep this just a warning and not an error?
    I vote for error.

     
  • Oleg N. Cher
    Oleg N. Cher
    2013-08-19

    I vote for warning.
    Many of C compilers accept such code with just warning.
    Maarten, if you want to improve C language to be more safe and comfortable, you'll need to make a new language better.
    But a C compiler cannot be "improved" by the addition of incompatibility with other C compilers. Example of the compiler support warning in such case? tcc, gcc/mingw/djgpp, ms visual c, borland с builder, turbo c, microsoft quick c, etc, etc.

     
  • Oleg N. Cher
    Oleg N. Cher
    2013-08-19

    As well as C language is not only a programming language. It's a set of traditions, a developers culture and the gigabytes of C sources. We have not right to drop it out IMHO.

    Maarten, please look at this a normal C function written in assembler:

    unsigned char Basic_POINT (short int x, short int y)
    {
    __asm

    ifdef __SDCC

    PUSH IX
    LD IX,#0
    ADD IX,SP

    endif

    LD IY,#0x5C3A
    LD C,4(IX)
    LD B,5(IX)
    CALL #0x22CE
    CALL #0x2DD5
    LD L,A

    ifdef __SDCC

    POP IX

    endif

    __endasm;
    }

    Now it compiles with a warnings:

    two.c:18: warning 59: function 'Basic_POINT' must return value
    two.c:18: warning 85: in function Basic_POINT unreferenced function argument : 'x'
    two.c:18: warning 85: in function Basic_POINT unreferenced function argument : 'y'

    As I understand, you propose to disallow that method of using functions that must return result but not return it, at all. And I have a question for you - how do you propose to rewrite the function "Basic_POINT" with the presence of "return"?

     
    Last edit: Oleg N. Cher 2013-08-19
  • Erik Petrich
    Erik Petrich
    2013-08-19

    It seems to me that the original example is valid under C89/90, so it should not be elevated to an error there. Later C standards only require that we issue a diagnostic message (it's our discretion how we want to classify it). Assuming I find no unexpected result during testing, it would be easiest to leave this as a warning for all standards and just return an undefined value (whatever happens to be in the return register(s), so there's not effect on code efficiency).

     
  • Erik Petrich
    Erik Petrich
    2013-08-20

    Fixed in revision #8809

     
  • Erik Petrich
    Erik Petrich
    2013-08-20

    • summary: if omitted in C89/C90 code that is not valid C99 --> return statement discarded if expected operand missing
    • status: open --> closed-fixed
     
  • Maarten Brock
    Maarten Brock
    2013-08-30

    I've added a regression test for this case in #8824.