[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275 Jakub Jelinek changed: What|Removed |Added Target Milestone|5.5 |6.5 --- Comment #6 from Jakub Jelinek --- GCC 5 branch is being closed
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275 Martin Liška changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2017-07-03 Known to work||4.9.0 Summary|[gcc7 gcc8] |[5/6/7/8 Regression] |-fsanitize=thread produce |-fsanitize=thread produce |incorrect -Wreturn-type |incorrect -Wreturn-type |warning |warning Ever confirmed|0 |1 Richard Biener changed: What|Removed |Added Keywords||diagnostic Target Milestone|--- |5.5 --- Comment #1 from Martin Liška --- Confirmed, started with r219202.
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275 Richard Biener changed: What|Removed |Added Priority|P3 |P2
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275
--- Comment #2 from Jakub Jelinek ---
You get the same thing with any other cleanup, say:
struct C { C (); ~C (); };
int
foo (int a, int b)
{
C c;
switch (a)
{
case 0:
switch (b)
{
default:
return 0;
}
break;
default:
return 0;
}
}
with just -Wreturn-type will warn too. And no optimizations, with -O1 and
above the eh pass optimizes it away.
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275 --- Comment #3 from Jakub Jelinek --- To get rid of the warning just with -fsanitize=threads we could do: --- gcc/tree-eh.c.jj2017-07-14 13:04:47.0 +0200 +++ gcc/tree-eh.c 2017-07-25 17:09:58.279461377 +0200 @@ -1598,7 +1598,8 @@ decide_copy_try_finally (int ndests, boo gimple *stmt = gsi_stmt (gsi); if (!is_gimple_debug (stmt) && !gimple_clobber_p (stmt) - && !gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE)) + && !gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE) + && !gimple_call_internal_p (stmt, IFN_TSAN_FUNC_EXIT)) return false; } return true; at the expense of (-O0) having more __tsan_func_exit calls but on the other side perhaps simpler code. But that doesn't solve the #c2 testcase where we'd warn anyway. The problem is that for: switch (a) , case 0: > : switch (b) > : D.2315 = 0; return D.2315; goto ; : D.2315 = 0; return D.2315; : this ends with a label and thus gimple_seq_may_fallthru returns true, even when the only goto to that label is dead. I guess we need to improve switch expansion for these pathological cases.
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275
Jakub Jelinek changed:
What|Removed |Added
CC||mpolacek at gcc dot gnu.org
--- Comment #4 from Jakub Jelinek ---
Marek, could we reuse the fallthrough warning infrastructure for this to
determine whether there is a possible fallthrough or not?
Though, trying:
int
foo (int a, int b, int c)
{
switch (c)
{
case 5:
switch (a)
{
case 0:
switch (b)
{
default:
return 0;
}
break;
#ifdef FT
case 7:
break;
#endif
default:
return 0;
}
case 6:
return 7;
}
return 8;
}
we don't warn about the fallthrough from case 5 to case 6 no matter if FT is
defined (in that case there is obvious fallthrough, say for foo (7, 0, 5), or
when FT is not defined (then there isn't, but gimple_seq_may_fallthru doesn't
know that). Whether a label is emitted after a switch (for the break; label)
is determined already in the FEs based on whether any break; is present, no
matter if reachable or not. So indeed, dead code such as the break; in your
#c0 testcase, or whatever other code followed by a break, is able to confuse
the gimplifier and eh lowering into assuming there might be fallthrough.
While the -Wreturn-type warning is emitted after cfg is built, the problem is
if we decide to emit the try/finally with a finally_tmp variable to avoid
duplicating the cleanup actions, then while after building cfg we optimize away
obviously unreachable code, we have no SSA form yet and thus determining if the
finally_tmp.N variable is always the same in all paths might be expensive.
At -O0 that actually isn't optimized by anything until assembly is emited, so
we still see in the assembly:
testl %eax, %eax
jne .L9
movl$0, %ebx
movl$0, %r12d
jmp .L4
.L9:
movl$0, %ebx
movl$0, %r12d
.L4:
// Here both %ebx and %r12d are 0 (set the same on both paths reaching this
// label.
leaq-17(%rbp), %rax
movq%rax, %rdi
// Here we run ~C()
call_ZN1CD1Ev
// Now we test whether it is 1, which is never, in that case we fall through
// into returning without a value.
cmpl$1, %r12d
jne .L10
nop
jmp .L1
.L10:
movl%ebx, %eax
.L1:
addq$32, %rsp
popq%rbx
popq%r12
popq%rbp
ret
So, if we can't do anyuthing for this at the gimplifier level, we'd need to add
an optimization in the cfg pass that would look for cases like:
;; basic block 4, loop depth 0
;;pred: 3
[0.00%] [count: INV]:
D.2315 = 0;
finally_tmp.0 = 0;
goto ; [INV] [count: INV]
;;succ: 6
;; basic block 5, loop depth 0
;;pred: 3
[0.00%] [count: INV]:
D.2315 = 0;
finally_tmp.0 = 0;
;;succ: 6
;; basic block 6, loop depth 0
;;pred: 4
;;5
C::~C (&c);
switch (finally_tmp.0) [INV] [count: INV], case 1: [INV]
[count: INV]>
and optimize that switch into goto ; after checking that finally_tmp.0 var
is not addressable, not modified in the bb with the switch and set to the same
value at the end of all predecessor bbs.
But that would be an optimization, not sure if we want to do that generally for
-O0, or limit it to somehow specially marked finally_tmp.NN variables.
[Bug sanitizer/81275] [5/6/7/8 Regression] -fsanitize=thread produce incorrect -Wreturn-type warning
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81275
--- Comment #5 from Marek Polacek ---
(In reply to Jakub Jelinek from comment #4)
> Marek, could we reuse the fallthrough warning infrastructure for this to
> determine whether there is a possible fallthrough or not?
> Though, trying:
> int
> foo (int a, int b, int c)
> {
> switch (c)
> {
> case 5:
> switch (a)
> {
> case 0:
> switch (b)
> {
> default:
> return 0;
> }
> break;
> #ifdef FT
> case 7:
> break;
> #endif
> default:
> return 0;
> }
> case 6:
> return 7;
> }
> return 8;
> }
>
> we don't warn about the fallthrough from case 5 to case 6 no matter if FT is
> defined (in that case there is obvious fallthrough, say for foo (7, 0, 5), or
> when FT is not defined (then there isn't, but gimple_seq_may_fallthru
> doesn't know that).
Unfortunately, not yet, because -Wimplicit-fallthrough doesn't scan nested
switches (PR79153).
