Re: [PATCH][RFC] Instrument function exit with __builtin_unreachable in C++.

[Jason Merrill <jason at redhat dot com>]

On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška <mliska@suse.cz> wrote:
> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says that having a non-return
> function with missing return statement is undefined behavior. We've got -fsanitize=return check for
> that and we can in such case instrument __builtin_unreachable(). This patch does that.

Great.

> And there's still some fallout:
>
> FAIL: g++.dg/cpp0x/constexpr-diag3.C  -std=c++11  (test for errors, line 7)
> FAIL: g++.dg/cpp0x/constexpr-neg3.C  -std=c++11  (test for errors, line 12)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14  (test for errors, line 7)
> FAIL: g++.dg/cpp1y/constexpr-return2.C  -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14  (test for errors, line 9)
> FAIL: g++.dg/cpp1y/pr63996.C  -std=c++14 (test for excess errors)
>
> 1) there are causing:
>
> ./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in constexpr expansion of ‘foo(1)’
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: ‘__builtin_unreachable()’ is not a constant expression
>  foo (int i)
>  ^~~
>
> Where we probably should not emit the built-in call. Am I right?

Or constexpr.c could give a more friendly diagnostic for
__builtin_unreachable.  It's correct to give a diagnostic here for
this testcase.

> FAIL: g++.dg/torture/pr34850.C   -O1   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fno-use-linker-plugin -flto-partition=none   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -O3 -g   (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C   -Os   (test for warnings, line 15)
>
> 2) this test is somehow hard to fix as it requires some unreachability.

Can't we add a return statement to memset?

Also, this testcase seems to indicate a danger from this patch, like
we've seen before with bounds checking: if we have a checking path
that complains about invalid arguments and then has undefined
behavior, we optimize away the diagnostic.  Can we warn in such cases?
 We probably want to provide a way to turn off this behavior.

If we're going to enable this by default, we probably also want
-Wreturn-type on by default.

Jason