Bug 89056 - Optimizer generates bad code for non-void function that fails to return a value
Summary: Optimizer generates bad code for non-void function that fails to return a value
Status: RESOLVED DUPLICATE of bug 86761
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 8.2.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-25 00:26 UTC by Darryl Okahata
Modified: 2019-01-26 23:47 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
preprocessed file (37.30 KB, text/plain)
2019-01-25 00:26 UTC, Darryl Okahata
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Darryl Okahata 2019-01-25 00:26:07 UTC
Created attachment 45530 [details]
preprocessed file

System: ancient Red Hat Enterprise Linux Server release 6.10 (Santiago)
Intel x86_64 system.


g++ -v

Using built-in specs.
COLLECT_GCC=/hped/builds/tfstools/gcc540/linux_x86_64//gcc/8.2.0/bin/g++_x86_64
COLLECT_LTO_WRAPPER=/a/new/sr/proton/d11/build/tfstools/gcc472/linux_x86_64/gcc/8.2.0/bin/../libexec/gcc/x86_64-pc-linux-gnu/8.2.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ./configure --enable-checking=release --enable-languages=c,c++ --enable-shared --enable-threads=posix --enable-__cxa_atexit --enable-clocale=gnu --disable-multilib --with-system-zlib --prefix=/hped/builds/tfstools/gcc472/linux_x86_64/gcc/8.2.0_rebld --with-gmp=/gfs/sr/sherry/d1/local/dbjornba/btmp --with-mpfr=/gfs/sr/sherry/d1/local/dbjornba/btmp --with-mpc=/gfs/sr/sherry/d1/local/dbjornba/btmp
Thread model: posix
gcc version 8.2.0 (GCC)


If you have cruddy code with a non-void function that fails to return a value, the gcc optimizer can generate an "infinite" loop for a simple iterative loop (see the end of the *.ii file):

    bool test::bah(void)
    {
        std::deque<struct foo>::iterator iter;

        for (iter = values.begin(); iter != values.end(); iter++)
            iter->myval -= 0.1;
        // returning a value here causes correct code to be generated.
    }

The bug can be easily seen using:

    g++ -S -O badbad.cc
    badbad.cc: In member function 'bool test::bah()':
    badbad.cc:42:1: warning: no return statement in function returning non-void [-Wreturn-type]
     }
     ^

The generated assembly code shows an "infinite" loop for the simple iterative loop (which only "terminates" when a bus error occurs).  The correct code is generated if you add a proper return value.

Yes, this is a poster child for using -Werror=return-type, but gcc should still not generate bad code (the return value will, of course, still be undefined).
Comment 1 Andrew Pinski 2019-01-25 00:47:13 UTC
In C++ it is undefined what happens when a return happens without a value.  This is different from C.  So these returns are marked as calling __builtin_unreachable().

*** This bug has been marked as a duplicate of bug 86761 ***
Comment 2 Andrew Pinski 2019-01-25 00:47:32 UTC
You can use -fsanatizer=undefined to find this behavior at runtime.
Comment 3 Andrew Pinski 2019-01-25 00:48:21 UTC
See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86761#c4 on the differences between C and C++.
Comment 4 Darryl Okahata 2019-01-25 01:15:43 UTC
This seems rather draconian but, if the standard allows for that, so be it.

Thanks.
Comment 5 Jonathan Wakely 2019-01-25 10:23:13 UTC
Yes, it allows it. It's undefined behaviour for your code to reach the end of the function (because there's no return statement) so the compiler assumes that the function will never reach that point. That means the loop must keep going (only exiting if one of the expressions in the loop throws an exception).

If that's what you intended, then you can mark the function with the noreturn attribute to inform the compiler of your intention, or you can explicitly add __builtin_unreachable() at the end of the function. Either of those will suppress the warning.

Of course that's not what you intended here, so you should heed the warning and fix the code.
Comment 6 Darryl Okahata 2019-01-25 22:51:30 UTC
(OK, at this point, I'm just whinging, so please feel free to ignore this.)

I just wish the C++ standard instead just allowed an undefined value to be returned, instead of generating bad optimized code.  With the current state, I either have to add compiler-specific extensions or unreachable return statements to insure that correct code is generated (unexpected and violates POLA).  The issue is that g++ (understandably) can't always detect if there is always a proper return statement (execution can never hit the end of the function).  Grossly-oversimplified example (real code is much more complicated):

    enum E { A, B };

    bool bah(const enum E a)
    {
        if (a == A)
            return false;
        if (a == B)
            return true;
    }

Compiling with (8.2.0):

     g++ -S -O badbad.cc

gives:

    badbad.cc: In function 'bool bah(E)':
    badbad.cc:10:1: warning: control reaches end of non-void function [-Wreturn-type]
     }
     ^

Understandable, as I don't expect g++ to figure out complicated code machinations.  However, I don't know all the circumstances under which this warning means that g++ is generating bad code.  As a result, I have to add unreachable return statements to insure that g++ does not generate bad optimized code.  Our code runs on multiple platforms, and so I'd rather avoid the use of g++ extensions (e.g., __builtin_unreachable() or attributes) and cluttering #ifdefs.  Adding an unreachable return is undesirable but simple and portable:

    enum E { A, B };

    bool bah(const enum E a)
    {
        if (a == A)
            return false;
        if (a == B)
            return true;
        return false;     // UNREACHABLE
    }
Comment 7 Jonathan Wakely 2019-01-26 23:47:41 UTC
(In reply to Darryl Okahata from comment #6)
> I just wish the C++ standard instead just allowed an undefined value to be
> returned, instead of generating bad optimized code.

It does allow it. The behaviour is undefined, so anything is allowed.

Nobody is claiming the standard *requires* GCC to do this. But it allows it.


> With the current state,
> I either have to add compiler-specific extensions or unreachable return
> statements to insure that correct code is generated (unexpected and violates
> POLA).

Or fix the code. Your original example was buggy code. The warning told you about it. So fix the code.

> The issue is that g++ (understandably) can't always detect if there
> is always a proper return statement (execution can never hit the end of the
> function).  Grossly-oversimplified example (real code is much more
> complicated):
> 
>     enum E { A, B };
> 
>     bool bah(const enum E a)
>     {
>         if (a == A)
>             return false;
>         if (a == B)
>             return true;
>     }
> 
> Compiling with (8.2.0):
> 
>      g++ -S -O badbad.cc
> 
> gives:
> 
>     badbad.cc: In function 'bool bah(E)':
>     badbad.cc:10:1: warning: control reaches end of non-void function
> [-Wreturn-type]
>      }
>      ^

If somebody calls bah((E)2) the function has undefined behaviour. 

Consider using the -fstrict-enums option if you want GCC to assume nobody creates invalid enum values.

> Understandable, as I don't expect g++ to figure out complicated code
> machinations.  However, I don't know all the circumstances under which this
> warning means that g++ is generating bad code.

It only generates bad code *if the end of the function can be reached*. If you know it can't be reached, good for you. The warning is bogus in that case. Either suppress the warning for that function, or ignore the warning for that function. If you don't know for sure, and maybe the end of the function can be reached somehow, then the warning is useful and adding the return statement improves the code.

This really does seem like whinging. Some warnings aren't perfect, but in your original report the warning was entirely right, and pointed out a bug that helps fix the code (if you actually heed the warning).