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

[Martin Liška]

On 10/05/2017 05:07 PM, Jason Merrill wrote:
> 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.

Hi.

That's good idea, any suggestion different from:

./xg++ -B. /home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C
/home/marxin/Programming/gcc2/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23:   in constexpr expansion of ‘foo(1)’
<built-in>: error: constexpr can't contain call of a non-return function ‘__builtin_unreachable’

which is probably misleading as it points to a function call that is actually missing in source code.
Should we distinguish between implicit and explicit __builtin_unreachable?

> 
>> 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?

Does not help :)

> 
> 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?

Good question, can you please provide more info how that happens? Do we have an example for that?

>   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.

Agree.

Thanks,
Martin

> 
> Jason
>