| Message ID | CA+yXCZBWAydoV8e-Z0fy79Mw=ZHKVbBoeDQwX4xHjyyGkqyFQg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 08, 2015 at 04:49:19PM +0800, Kito Cheng wrote: > Bootstrapped & regression-tested on x86_64-linux-gnu :) > > 2015-07-08 Kito Cheng <kito.cheng@gmail.com> > > * function.c (stack_protect_epilogue): Use if rather than switch for > check targetm.have_stack_protect_test(). Do you really need this? This should be just non-fatal warning in stage1, and only if you compile with gcc-5, right? You could also just cast the controlling expression of the switch to int to suppress the warning. Marek
Hi Marek: Yes, I know it's non-fatal warning, but I think gcc should build with --enable-werror-always by it's self and it's the *ONLY* warning in trunk now. Of cause, cast to int can suppress the warning, but it's not good solution so gcc complain that switch condition has bool type :) On Wed, Jul 8, 2015 at 9:30 PM, Marek Polacek <polacek@redhat.com> wrote: > On Wed, Jul 08, 2015 at 04:49:19PM +0800, Kito Cheng wrote: >> Bootstrapped & regression-tested on x86_64-linux-gnu :) >> >> 2015-07-08 Kito Cheng <kito.cheng@gmail.com> >> >> * function.c (stack_protect_epilogue): Use if rather than switch for >> check targetm.have_stack_protect_test(). > > Do you really need this? This should be just non-fatal warning in stage1, > and only if you compile with gcc-5, right? > You could also just cast the controlling expression of the switch to int to > suppress the warning. > > Marek
On 07/08/2015 02:49 AM, Kito Cheng wrote: > Bootstrapped & regression-tested on x86_64-linux-gnu :) > > 2015-07-08 Kito Cheng <kito.cheng@gmail.com> > > * function.c (stack_protect_epilogue): Use if rather than switch for > check targetm.have_stack_protect_test(). > OK. Not necessarily because avoid the warning is really all that important, but because the if-else is just as easy to read as the switch and dramatically more compact. Jeff
Hi Jeff: Thanks your review and approve, however I don't have commit right yet, can you help me to commit it :) thanks On Thu, Jul 9, 2015 at 5:00 AM, Jeff Law <law@redhat.com> wrote: > On 07/08/2015 02:49 AM, Kito Cheng wrote: >> >> Bootstrapped & regression-tested on x86_64-linux-gnu :) >> >> 2015-07-08 Kito Cheng <kito.cheng@gmail.com> >> >> * function.c (stack_protect_epilogue): Use if rather than switch >> for >> check targetm.have_stack_protect_test(). >> > OK. Not necessarily because avoid the warning is really all that important, > but because the if-else is just as easy to read as the switch and > dramatically more compact. > > Jeff
On 07/08/2015 09:25 PM, Kito Cheng wrote: > Hi Jeff: > Thanks your review and approve, however I don't have commit right yet, > can you help me to commit it :) Committed to the trunk. jeff
Kito Cheng <kito.cheng@gmail.com> writes: > Yes, I know it's non-fatal warning, but I think gcc should build with > --enable-werror-always by it's self > and it's the *ONLY* warning in trunk now. Yeah, but it should only build with --enable-werror-always if your host compiler is the same version as the one you're building. It would be too much to expect it to compile cleanly with older versions too, since there's no obvious place to draw the line. So Marek's point was that current GCC (deliberately) does not warn about this switch statement. The switch statement used to be: /* Allow the target to compare Y with X without leaking either into a register. */ switch ((int) (HAVE_stack_protect_test != 0)) but the (int) cast was removed at the same time as the warning was adjusted: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00790.html (BTW, although you cc:ed me, it was that patch rather than mine that introduced the warnings when using unpatched host compilers.) Thanks, Richard
From 0306990aac578167872a80ab55085d335e2bea14 Mon Sep 17 00:00:00 2001 From: Kito Cheng <kito@andestech.com> Date: Wed, 8 Jul 2015 15:20:01 +0800 Subject: [PATCH] Drop -Wswitch-bool warning in function.c --- gcc/function.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/gcc/function.c b/gcc/function.c index 972cdc8..b87aef6 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4874,26 +4874,18 @@ stack_protect_epilogue (void) tree guard_decl = targetm.stack_protect_guard (); rtx_code_label *label = gen_label_rtx (); rtx x, y, tmp; + rtx_insn *seq; x = expand_normal (crtl->stack_protect_guard); y = expand_normal (guard_decl); /* Allow the target to compare Y with X without leaking either into a register. */ - switch (targetm.have_stack_protect_test ()) - { - case 1: - if (rtx_insn *seq = targetm.gen_stack_protect_test (x, y, label)) - { - emit_insn (seq); - break; - } - /* FALLTHRU */ - - default: - emit_cmp_and_jump_insns (x, y, EQ, NULL_RTX, ptr_mode, 1, label); - break; - } + if (targetm.have_stack_protect_test () + && ((seq = targetm.gen_stack_protect_test (x, y, label)) != NULL_RTX)) + emit_insn (seq); + else + emit_cmp_and_jump_insns (x, y, EQ, NULL_RTX, ptr_mode, 1, label); /* The noreturn predictor has been moved to the tree level. The rtl-level predictors estimate this branch about 20%, which isn't enough to get -- 2.3.5
Bootstrapped & regression-tested on x86_64-linux-gnu :) 2015-07-08 Kito Cheng <kito.cheng@gmail.com> * function.c (stack_protect_epilogue): Use if rather than switch for check targetm.have_stack_protect_test().