Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/21/16 21:03, Jason Merrill wrote:
> On Wed, Sep 21, 2016 at 11:43 AM, Bernd Edlinger
> wrote:
>> On 09/21/16 17:00, Jason Merrill wrote:
>>> On 09/20/2016 02:40 PM, Bernd Edlinger wrote:
On 09/20/16 16:51, Jason Merrill wrote:
> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger
> wrote:
>> I think I will have to suppress the warning if the conditional is in
>> a macro somehow.
>
> from_macro_expansion_at will help with that.
>
> Though it seems to me that the issue here is more that (TARGET_FP16 ?
> 14 : 12) is not in a boolean context, it's in an integer context; only
> the outer ?: is in a boolean context.
>
+ if (warn_int_in_bool_context
+ && !from_macro_expansion_at (EXPR_LOCATION (expr)))
But this seems to suppress all such warnings from an assert
macro too. Like for instance "assert(a?1:2)".
Sure, we would not be interested in a ?: that is part of the assert
macro itself, but the expression that is evaluated by the macro should
be checked, but that is no longer done, because the macro parameter is
now also from the macro expansion. But it is initially from the
macro invocation point. Ideas?
>>>
>>> This should fix that, though I haven't run regression tests yet:
>>>
>>
>> Yes. I think that goes in the right direction,
>> but it does not work yet.
>>
>> #define XXX (a ? 2 : 3)
>>
>> if (XXX) // prints a warning, but it should not.
>
> Indeed, that was too simplistic. This patch adds a
> from_macro_definition_at test that ought to do what you want:
>
Yes, this works for me as well.
Here is an updated warning patch, using the new
from_macro_definition_at.
Bernd.
2016-09-21 Bernd Edlinger
* c-common.c (c_common_truthvalue_conversion): Inhibit
Wint-in-bool-context warning with from_macro_definition_at.
Mention the expression will always evaluate to true.
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c (Revision 240313)
+++ gcc/c-family/c-common.c (Arbeitskopie)
@@ -4652,7 +4652,8 @@ c_common_truthvalue_conversion (location_t locatio
TREE_OPERAND (expr, 0));
case COND_EXPR:
- if (warn_int_in_bool_context)
+ if (warn_int_in_bool_context
+ && !from_macro_definition_at (EXPR_LOCATION (expr)))
{
tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
@@ -4663,7 +4664,8 @@ c_common_truthvalue_conversion (location_t locatio
&& (!integer_onep (val1)
|| !integer_onep (val2)))
warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
- "?: using integer constants in boolean context");
+ "?: using integer constants in boolean context, "
+ "the expression will always evaluate to %");
}
/* Distribute the conversion into the arms of a COND_EXPR. */
if (c_dialect_cxx ())
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On Wed, Sep 21, 2016 at 3:52 PM, Bernd Edlinger wrote: > On 09/21/16 21:03, Jason Merrill wrote: >> On Wed, Sep 21, 2016 at 11:43 AM, Bernd Edlinger >> wrote: >>> On 09/21/16 17:00, Jason Merrill wrote: On 09/20/2016 02:40 PM, Bernd Edlinger wrote: > On 09/20/16 16:51, Jason Merrill wrote: >> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger >> wrote: >>> I think I will have to suppress the warning if the conditional is in >>> a macro somehow. >> >> from_macro_expansion_at will help with that. >> >> Though it seems to me that the issue here is more that (TARGET_FP16 ? >> 14 : 12) is not in a boolean context, it's in an integer context; only >> the outer ?: is in a boolean context. >> > + if (warn_int_in_bool_context > + && !from_macro_expansion_at (EXPR_LOCATION (expr))) > > But this seems to suppress all such warnings from an assert > macro too. Like for instance "assert(a?1:2)". > > Sure, we would not be interested in a ?: that is part of the assert > macro itself, but the expression that is evaluated by the macro should > be checked, but that is no longer done, because the macro parameter is > now also from the macro expansion. But it is initially from the > macro invocation point. Ideas? This should fix that, though I haven't run regression tests yet: >>> >>> Yes. I think that goes in the right direction, >>> but it does not work yet. >>> >>> #define XXX (a ? 2 : 3) >>> >>> if (XXX) // prints a warning, but it should not. >> >> Indeed, that was too simplistic. This patch adds a >> from_macro_definition_at test that ought to do what you want: >> > > Yes, this works for me as well. > > Here is an updated warning patch, using the new > from_macro_definition_at. OK. Jason
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/21/16 21:03, Jason Merrill wrote:
> On Wed, Sep 21, 2016 at 11:43 AM, Bernd Edlinger
> wrote:
>> Yes. I think that goes in the right direction,
>> but it does not work yet.
>>
>> #define XXX (a ? 2 : 3)
>>
>> if (XXX) // prints a warning, but it should not.
>
> Indeed, that was too simplistic. This patch adds a
> from_macro_definition_at test that ought to do what you want:
>
I tried something similar, which seems to work:
--- input.h (Revision 240313)
+++ input.h (Arbeitskopie)
@@ -73,10 +73,19 @@
header, but expanded in a non-system file. */
#define in_system_header_at(LOC) \
(linemap_location_in_system_header_p (line_table, LOC))
-/* Return a positive value if LOCATION is the locus of a token that
- comes from a macro expansion, O otherwise. */
-#define from_macro_expansion_at(LOC) \
- ((linemap_location_from_macro_expansion_p (line_table, LOC)))
+/* Return TRUE if LOCATION is the locus of a token that
+ comes from a macro expansion, FALSE otherwise. */
+static inline bool
+from_macro_expansion_at (location_t loc)
+{
+ /* Resolve to the spelling location so we return false for arguments to a
+ macro. */
+ return linemap_location_from_macro_expansion_p (line_table, loc)
+&& linemap_resolve_location (line_table, loc,
+ LRK_MACRO_DEFINITION_LOCATION, NULL)
+ == linemap_resolve_location (line_table, loc,
+LRK_SPELLING_LOCATION, NULL);
+}
static inline location_t
get_pure_location (location_t loc)
I will try your patch now, and see if it works for me.
Thanks
Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/21/16 17:00, Jason Merrill wrote: > On 09/20/2016 02:40 PM, Bernd Edlinger wrote: >> On 09/20/16 16:51, Jason Merrill wrote: >>> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger >>> wrote: I think I will have to suppress the warning if the conditional is in a macro somehow. >>> >>> from_macro_expansion_at will help with that. >>> >>> Though it seems to me that the issue here is more that (TARGET_FP16 ? >>> 14 : 12) is not in a boolean context, it's in an integer context; only >>> the outer ?: is in a boolean context. >>> >> + if (warn_int_in_bool_context >> + && !from_macro_expansion_at (EXPR_LOCATION (expr))) >> >> But this seems to suppress all such warnings from an assert >> macro too. Like for instance "assert(a?1:2)". >> >> Sure, we would not be interested in a ?: that is part of the assert >> macro itself, but the expression that is evaluated by the macro should >> be checked, but that is no longer done, because the macro parameter is >> now also from the macro expansion. But it is initially from the >> macro invocation point. Ideas? > > This should fix that, though I haven't run regression tests yet: > Yes. I think that goes in the right direction, but it does not work yet. #define XXX (a ? 2 : 3) if (XXX) // prints a warning, but it should not. Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/21/16 16:38, Richard Earnshaw (lists) wrote:
> On 20/09/16 16:30, Bernd Schmidt wrote:
>> On 09/20/2016 05:18 PM, Jeff Law wrote:
I assume HARD_FRAME_POINTER_REGNUM is never zero.
>>> It could be zero. It's just a hard register number. No target has the
>>> property that its hard frame pointer register is 0 though :-)
>>
>> git blame to the rescue. The current state comes from one of tbsaunde's
>> cleanup patches:
>>
>>> diff --git a/gcc/regrename.c b/gcc/regrename.c
>> index 174d3b5..e5248a5 100644
>> --- a/gcc/regrename.c
>> +++ b/gcc/regrename.c
>> @@ -442,12 +442,10 @@ rename_chains (void)
>> continue;
>>
>> if (fixed_regs[reg] || global_regs[reg]
>> -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
>> - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
>> -#else
>> - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
>> -#endif
>> - )
>> + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
>> + && reg == HARD_FRAME_POINTER_REGNUM)
>> + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
>
> Surely, based on the logic of the previous ifdefs, this line should read
> + || (HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
>
I think the block above does not match this logic, it was also modified
recently. Why is it handling FP and HFP, but then only HFP ?
/* Don't clobber traceback for noreturn functions. */
if (frame_pointer_needed)
{
add_to_hard_reg_set (&unavailable, Pmode, FRAME_POINTER_REGNUM);
if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
add_to_hard_reg_set (&unavailable, Pmode,
HARD_FRAME_POINTER_REGNUM);
}
Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 20/09/16 16:30, Bernd Schmidt wrote: > On 09/20/2016 05:18 PM, Jeff Law wrote: >>> I assume HARD_FRAME_POINTER_REGNUM is never zero. >> It could be zero. It's just a hard register number. No target has the >> property that its hard frame pointer register is 0 though :-) > > git blame to the rescue. The current state comes from one of tbsaunde's > cleanup patches: > >> diff --git a/gcc/regrename.c b/gcc/regrename.c > index 174d3b5..e5248a5 100644 > --- a/gcc/regrename.c > +++ b/gcc/regrename.c > @@ -442,12 +442,10 @@ rename_chains (void) > continue; > >if (fixed_regs[reg] || global_regs[reg] > -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER > - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) > -#else > - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) > -#endif > - ) > + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed > + && reg == HARD_FRAME_POINTER_REGNUM) > + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed Surely, based on the logic of the previous ifdefs, this line should read + || (HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed R.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On Tue, Sep 20, 2016 at 11:48:04PM -0400, Eric Gallager wrote: > On 9/20/16, Trevor Saunders wrote: > > On Wed, Sep 21, 2016 at 01:13:41AM +0200, Bernd Schmidt wrote: > >> On 09/21/2016 01:09 AM, Trevor Saunders wrote: > >> > > >> > I thought I remember discussing this macro with you, but see what was > >> > checked in I'll believe I'm thinking of something similar but > >> > different. > >> > >> I think this here was an earlier patch and the one we were discussing > >> recently was the other macro with a similar name. > >> > >> > Any way sorry about the dumb bug > >> > >> Stuff like this happens, no worries. But I've seen it happen a lot over the > >> years, and maybe you can see in this an explanation of why I'm often not > >> the > >> most enthusiastic supporter of pure cleanup patches (those not motivated by > >> more substantial patches depending on them). > > > > yeah, there's always some risk, though I also believe if you define > > something as cleaning up then it has some value compared to pointless > > permutation. Ironically I think one of the big motivating reasons to > > remove ifdefs is to remove a source of bustage. > > > > Trev > > > > > This is kinda changing the topic a bit, but if removing ifdefs is to > remove bustage, maybe GCC should start compiling with -Wundef to > ensure that the ifdef removal doesn't actually introduce any new > bustage? Glibc started using -Wundef for that reason: Well, the goal is to reduce conditional compilation so no #if either, and in other contexts undefined doesn't implicitly convert, so it shouldn't be as important. It might be good to do, but I suspect you'd have to fix things up, and it just doesn't seem as important as other things that can be done. Trev > > https://sourceware.org/ml/libc-alpha/2014-02/msg00828.html > https://www.sourceware.org/ml/libc-alpha/2015-08/msg00751.html
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 9/20/16, Trevor Saunders wrote: > On Wed, Sep 21, 2016 at 01:13:41AM +0200, Bernd Schmidt wrote: >> On 09/21/2016 01:09 AM, Trevor Saunders wrote: >> > >> > I thought I remember discussing this macro with you, but see what was >> > checked in I'll believe I'm thinking of something similar but >> > different. >> >> I think this here was an earlier patch and the one we were discussing >> recently was the other macro with a similar name. >> >> > Any way sorry about the dumb bug >> >> Stuff like this happens, no worries. But I've seen it happen a lot over the >> years, and maybe you can see in this an explanation of why I'm often not the >> most enthusiastic supporter of pure cleanup patches (those not motivated by >> more substantial patches depending on them). > > yeah, there's always some risk, though I also believe if you define > something as cleaning up then it has some value compared to pointless > permutation. Ironically I think one of the big motivating reasons to > remove ifdefs is to remove a source of bustage. > > Trev > This is kinda changing the topic a bit, but if removing ifdefs is to remove bustage, maybe GCC should start compiling with -Wundef to ensure that the ifdef removal doesn't actually introduce any new bustage? Glibc started using -Wundef for that reason: https://sourceware.org/ml/libc-alpha/2014-02/msg00828.html https://www.sourceware.org/ml/libc-alpha/2015-08/msg00751.html
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On Wed, Sep 21, 2016 at 01:13:41AM +0200, Bernd Schmidt wrote: > On 09/21/2016 01:09 AM, Trevor Saunders wrote: > > > > I thought I remember discussing this macro with you, but see what was > > checked in I'll believe I'm thinking of something similar but different. > > I think this here was an earlier patch and the one we were discussing > recently was the other macro with a similar name. > > > Any way sorry about the dumb bug > > Stuff like this happens, no worries. But I've seen it happen a lot over the > years, and maybe you can see in this an explanation of why I'm often not the > most enthusiastic supporter of pure cleanup patches (those not motivated by > more substantial patches depending on them). yeah, there's always some risk, though I also believe if you define something as cleaning up then it has some value compared to pointless permutation. Ironically I think one of the big motivating reasons to remove ifdefs is to remove a source of bustage. Trev > > > Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/21/2016 01:09 AM, Trevor Saunders wrote: I thought I remember discussing this macro with you, but see what was checked in I'll believe I'm thinking of something similar but different. I think this here was an earlier patch and the one we were discussing recently was the other macro with a similar name. Any way sorry about the dumb bug Stuff like this happens, no worries. But I've seen it happen a lot over the years, and maybe you can see in this an explanation of why I'm often not the most enthusiastic supporter of pure cleanup patches (those not motivated by more substantial patches depending on them). Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On Tue, Sep 20, 2016 at 05:30:36PM +0200, Bernd Schmidt wrote: > On 09/20/2016 05:18 PM, Jeff Law wrote: > > > I assume HARD_FRAME_POINTER_REGNUM is never zero. > > It could be zero. It's just a hard register number. No target has the > > property that its hard frame pointer register is 0 though :-) > > git blame to the rescue. The current state comes from one of tbsaunde's > cleanup patches: > > > diff --git a/gcc/regrename.c b/gcc/regrename.c > index 174d3b5..e5248a5 100644 > --- a/gcc/regrename.c > +++ b/gcc/regrename.c > @@ -442,12 +442,10 @@ rename_chains (void) > continue; > >if (fixed_regs[reg] || global_regs[reg] > -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER > - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) > -#else > - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) > -#endif > - ) > + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed > + && reg == HARD_FRAME_POINTER_REGNUM) > + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed > + && reg == FRAME_POINTER_REGNUM)) > continue; > >COPY_HARD_REG_SET (this_unavailable, unavailable); > > Looks like it never got reviewed and was committed as preapproved. I thought I remember discussing this macro with you, but see what was checked in I'll believe I'm thinking of something similar but different. Any way sorry about the dumb bug, though in my defense, or perhaps in further proof I'm not good at detail I missed the HARD_FRAME_POINTER_REGNUM instead of HARD_FRAME_POINTER_IS_FRAME_POINTER the first time I looked at it here too :"( > The #ifdef we had before looks odd too. Maybe this should just read > > if (fixed_regs[reg] || global_regs[reg] > || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)) I'd agree that sounds reasonable. Though I'd want to check for a target where HARD_FRAME_POINTER_IS_FRAME_POINTER is true, but HARD_FRAME_POINTER_REGNUM != FRAME_POINTER_REGNUM, though that could only be arm or mips I believe and it sounds pretty bogus. Thanks for fixing that up for me! Trev > > > Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/16 16:51, Jason Merrill wrote:
> On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger
> wrote:
>> On 09/20/16 13:29, Kyrill Tkachov wrote:
>>>
>>> arm bootstrap is now failing:
>>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in
>>> boolean context [-Werror=int-in-bool-context]
>>> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>> ~^~
>>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro
>>> 'TARGET_ARM_FP'
>>> if (TARGET_ARM_FP)
>>>
>>>
>>> The full definition of TARGET_ARM_FP is:
>>> #define TARGET_ARM_FP\
>>> (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
>>> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
>>> : 0)
>>>
>>> We want it set to 0 when there's no FP but when FP is available we set
>>> it to a bitmask
>>> to suggest the level that is available. That seems like a legitimate use
>>> to me.
>>>
>>
>> Ok, I see, sorry for that.
>>
>> I think I will have to suppress the warning if the conditional is in
>> a macro somehow.
>
> from_macro_expansion_at will help with that.
>
> Though it seems to me that the issue here is more that (TARGET_FP16 ?
> 14 : 12) is not in a boolean context, it's in an integer context; only
> the outer ?: is in a boolean context.
>
> I also still think the warning message should be changed.
>
I try this:
Index: c-common.c
===
--- c-common.c (revision 240268)
+++ c-common.c (working copy)
@@ -4652,7 +4652,8 @@
TREE_OPERAND (expr, 0));
case COND_EXPR:
- if (warn_int_in_bool_context)
+ if (warn_int_in_bool_context
+ && !from_macro_expansion_at (EXPR_LOCATION (expr)))
{
tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
@@ -4663,7 +4664,8 @@
&& (!integer_onep (val1)
|| !integer_onep (val2)))
warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
- "?: using integer constants in boolean context");
+ "?: using integer constants in boolean context, "
+ "the expression will always evaluate to %");
}
/* Distribute the conversion into the arms of a COND_EXPR. */
if (c_dialect_cxx ())
That will fix "if (TARGET_ARM_FP)" which is fine.
But this seems to suppress all such warnings from an assert
macro too. Like for instance "assert(a?1:2)".
Sure, we would not be interested in a ?: that is part of the assert
macro itself, but the expression that is evaluated by the macro should
be checked, but that is no longer done, because the macro parameter is
now also from the macro expansion. But it is initially from the
macro invocation point. Ideas?
Thanks
Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/16 18:29, Kyrill Tkachov wrote: >> >> I'll try bootstrapping and testing this change on >> arm-none-linux-gnueabihf. >> > > ... and I'm hitting the same issue in sel-sched.c: > if (fixed_regs[regno] > || global_regs[regno] > || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed > && regno == HARD_FRAME_POINTER_REGNUM) > || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed > && regno == FRAME_POINTER_REGNUM) > > I think the condition was copied from regrename.c (or the other way > around), > so I'll fix it the same way. > > Kyrill > > Interesting that this warning shakes so many bugs out of the gcc-tree, but hit zero times for the whole linux and even openssl. Even for the more aggressive variant I had before. I would have expected to see at least some false positives there, but there were none. Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 20/09/16 17:12, Kyrill Tkachov wrote: On 20/09/16 16:30, Bernd Schmidt wrote: On 09/20/2016 05:18 PM, Jeff Law wrote: I assume HARD_FRAME_POINTER_REGNUM is never zero. It could be zero. It's just a hard register number. No target has the property that its hard frame pointer register is 0 though :-) git blame to the rescue. The current state comes from one of tbsaunde's cleanup patches: > diff --git a/gcc/regrename.c b/gcc/regrename.c index 174d3b5..e5248a5 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -442,12 +442,10 @@ rename_chains (void) continue; if (fixed_regs[reg] || global_regs[reg] -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) -#else - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) -#endif - ) + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed + && reg == HARD_FRAME_POINTER_REGNUM) + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed + && reg == FRAME_POINTER_REGNUM)) continue; COPY_HARD_REG_SET (this_unavailable, unavailable); Looks like it never got reviewed and was committed as preapproved. The #ifdef we had before looks odd too. Maybe this should just read if (fixed_regs[reg] || global_regs[reg] || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)) I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf. ... and I'm hitting the same issue in sel-sched.c: if (fixed_regs[regno] || global_regs[regno] || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed && regno == HARD_FRAME_POINTER_REGNUM) || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed && regno == FRAME_POINTER_REGNUM) I think the condition was copied from regrename.c (or the other way around), so I'll fix it the same way. Kyrill Thanks, Kyrill Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/16 18:12, Kyrill Tkachov wrote:
>
> On 20/09/16 16:30, Bernd Schmidt wrote:
>> On 09/20/2016 05:18 PM, Jeff Law wrote:
I assume HARD_FRAME_POINTER_REGNUM is never zero.
>>> It could be zero. It's just a hard register number. No target has the
>>> property that its hard frame pointer register is 0 though :-)
>>
>> git blame to the rescue. The current state comes from one of
>> tbsaunde's cleanup patches:
>>
>> > diff --git a/gcc/regrename.c b/gcc/regrename.c
>> index 174d3b5..e5248a5 100644
>> --- a/gcc/regrename.c
>> +++ b/gcc/regrename.c
>> @@ -442,12 +442,10 @@ rename_chains (void)
>> continue;
>>
>>if (fixed_regs[reg] || global_regs[reg]
>> -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER
>> - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)
>> -#else
>> - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM)
>> -#endif
>> - )
>> + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER &&
>> frame_pointer_needed
>> + && reg == HARD_FRAME_POINTER_REGNUM)
>> + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
>> + && reg == FRAME_POINTER_REGNUM))
>> continue;
>>
>>COPY_HARD_REG_SET (this_unavailable, unavailable);
>>
>> Looks like it never got reviewed and was committed as preapproved.
>>
>> The #ifdef we had before looks odd too. Maybe this should just read
>>
>> if (fixed_regs[reg] || global_regs[reg]
>> || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM))
>>
I think this breaks the symmetry with the statement above.
how about this (similar to what Jeff suggested):
Index: regrename.c
===
--- regrename.c (revision 240268)
+++ regrename.c (working copy)
@@ -464,8 +464,7 @@
if (frame_pointer_needed)
{
add_to_hard_reg_set (&unavailable, Pmode, FRAME_POINTER_REGNUM);
- if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
- add_to_hard_reg_set (&unavailable, Pmode,
HARD_FRAME_POINTER_REGNUM);
+ add_to_hard_reg_set (&unavailable, Pmode, HARD_FRAME_POINTER_REGNUM);
}
FOR_EACH_VEC_ELT (id_to_chain, i, this_head)
@@ -479,10 +478,9 @@
continue;
if (fixed_regs[reg] || global_regs[reg]
- || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
- && reg == HARD_FRAME_POINTER_REGNUM)
- || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
- && reg == FRAME_POINTER_REGNUM))
+ || (frame_pointer_needed
+ && (reg == HARD_FRAME_POINTER_REGNUM
+ || reg == FRAME_POINTER_REGNUMi)))
continue;
COPY_HARD_REG_SET (this_unavailable, unavailable);
FRAME_POINTER_REGNUM is always special, and it
may be identical with HFP. But it is not necessary
to use HARD_FRAME_POINTER_IS_FRAME_POINTER at all.
Bernd.
>
> I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf.
>
> Thanks,
> Kyrill
>
>>
>> Bernd
>
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 20/09/16 16:30, Bernd Schmidt wrote: On 09/20/2016 05:18 PM, Jeff Law wrote: I assume HARD_FRAME_POINTER_REGNUM is never zero. It could be zero. It's just a hard register number. No target has the property that its hard frame pointer register is 0 though :-) git blame to the rescue. The current state comes from one of tbsaunde's cleanup patches: > diff --git a/gcc/regrename.c b/gcc/regrename.c index 174d3b5..e5248a5 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -442,12 +442,10 @@ rename_chains (void) continue; if (fixed_regs[reg] || global_regs[reg] -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) -#else - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) -#endif - ) + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed + && reg == HARD_FRAME_POINTER_REGNUM) + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed + && reg == FRAME_POINTER_REGNUM)) continue; COPY_HARD_REG_SET (this_unavailable, unavailable); Looks like it never got reviewed and was committed as preapproved. The #ifdef we had before looks odd too. Maybe this should just read if (fixed_regs[reg] || global_regs[reg] || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)) I'll try bootstrapping and testing this change on arm-none-linux-gnueabihf. Thanks, Kyrill Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/2016 05:18 PM, Jeff Law wrote: I assume HARD_FRAME_POINTER_REGNUM is never zero. It could be zero. It's just a hard register number. No target has the property that its hard frame pointer register is 0 though :-) git blame to the rescue. The current state comes from one of tbsaunde's cleanup patches: > diff --git a/gcc/regrename.c b/gcc/regrename.c index 174d3b5..e5248a5 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -442,12 +442,10 @@ rename_chains (void) continue; if (fixed_regs[reg] || global_regs[reg] -#if !HARD_FRAME_POINTER_IS_FRAME_POINTER - || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) -#else - || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) -#endif - ) + || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed + && reg == HARD_FRAME_POINTER_REGNUM) + || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed + && reg == FRAME_POINTER_REGNUM)) continue; COPY_HARD_REG_SET (this_unavailable, unavailable); Looks like it never got reviewed and was committed as preapproved. The #ifdef we had before looks odd too. Maybe this should just read if (fixed_regs[reg] || global_regs[reg] || (frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM)) Bernd
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/2016 08:38 AM, Bernd Edlinger wrote: On 09/20/16 16:20, Kyrill Tkachov wrote: On 20/09/16 15:11, Bernd Edlinger wrote: On 09/20/16 13:29, Kyrill Tkachov wrote: arm bootstrap is now failing: $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ ~^~ $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro 'TARGET_ARM_FP' if (TARGET_ARM_FP) The full definition of TARGET_ARM_FP is: #define TARGET_ARM_FP\ (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\ : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ : 0) We want it set to 0 when there's no FP but when FP is available we set it to a bitmask to suggest the level that is available. That seems like a legitimate use to me. Ok, I see, sorry for that. I think I will have to suppress the warning if the conditional is in a macro somehow. Can you work around that for a few days? I changed to: if (TARGET_ARM_FP != 0) in my tree to allow the build to proceed but encountered another bootstrap failure: In file included from ./tm.h:38:0, from $SRC/gcc/backend.h:28, from $SRC/gcc/regrename.c:23: $SRC/gcc/regrename.c: In function 'void rename_chains()': $SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] (TARGET_ARM \ ~ ? ARM_HARD_FRAME_POINTER_REGNUM \ ^~ : THUMB_HARD_FRAME_POINTER_REGNUM) ~~ $SRC/gcc/regrename.c:484:8: note: in expansion of macro 'HARD_FRAME_POINTER_REGNUM' || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed ^ That looks like a legitimate bug in regrename.c ? The full condition is: if (fixed_regs[reg] || global_regs[reg] || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed && reg == FRAME_POINTER_REGNUM)) The condition in the second || looks bogus (what use testing if a register is 'non-zero'). So this looks like a useful catch by the warning, though I'm not sure at first glance how to fix it. Kyrill I assume HARD_FRAME_POINTER_REGNUM is never zero. It could be zero. It's just a hard register number. No target has the property that its hard frame pointer register is 0 though :-) That looks like it should be || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) I'm not even entirely sure why this is two conditions. if (fixed_regs[regs] || global_regs[reg] || (frame_pointer_needed && (reg == HARD_FRAME_POINTER_REGNUM || reg == FRAME_POINTER_REGNUM ? Bernd Schmidt ought to be able to guide us. jeff
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On Tue, Sep 20, 2016 at 10:11 AM, Bernd Edlinger wrote: > On 09/20/16 13:29, Kyrill Tkachov wrote: >> >> arm bootstrap is now failing: >> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in >> boolean context [-Werror=int-in-bool-context] >> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ >> ~^~ >> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro >> 'TARGET_ARM_FP' >> if (TARGET_ARM_FP) >> >> >> The full definition of TARGET_ARM_FP is: >> #define TARGET_ARM_FP\ >>(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\ >> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ >>: 0) >> >> We want it set to 0 when there's no FP but when FP is available we set >> it to a bitmask >> to suggest the level that is available. That seems like a legitimate use >> to me. >> > > Ok, I see, sorry for that. > > I think I will have to suppress the warning if the conditional is in > a macro somehow. from_macro_expansion_at will help with that. Though it seems to me that the issue here is more that (TARGET_FP16 ? 14 : 12) is not in a boolean context, it's in an integer context; only the outer ?: is in a boolean context. I also still think the warning message should be changed. Jason
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/16 16:20, Kyrill Tkachov wrote: > > On 20/09/16 15:11, Bernd Edlinger wrote: >> On 09/20/16 13:29, Kyrill Tkachov wrote: >>> arm bootstrap is now failing: >>> $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in >>> boolean context [-Werror=int-in-bool-context] >>> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ >>> ~^~ >>> $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro >>> 'TARGET_ARM_FP' >>> if (TARGET_ARM_FP) >>> >>> >>> The full definition of TARGET_ARM_FP is: >>> #define TARGET_ARM_FP\ >>> (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\ >>> : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ >>> : 0) >>> >>> We want it set to 0 when there's no FP but when FP is available we set >>> it to a bitmask >>> to suggest the level that is available. That seems like a legitimate use >>> to me. >>> >> Ok, I see, sorry for that. >> >> I think I will have to suppress the warning if the conditional is in >> a macro somehow. >> >> Can you work around that for a few days? > > I changed to: > if (TARGET_ARM_FP != 0) > in my tree to allow the build to proceed but encountered another > bootstrap failure: > In file included from ./tm.h:38:0, > from $SRC/gcc/backend.h:28, > from $SRC/gcc/regrename.c:23: > $SRC/gcc/regrename.c: In function 'void rename_chains()': > $SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in > boolean context [-Werror=int-in-bool-context] > (TARGET_ARM \ > ~ > ? ARM_HARD_FRAME_POINTER_REGNUM \ > ^~ > : THUMB_HARD_FRAME_POINTER_REGNUM) > ~~ > $SRC/gcc/regrename.c:484:8: note: in expansion of macro > 'HARD_FRAME_POINTER_REGNUM' > || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed > ^ > That looks like a legitimate bug in regrename.c ? > The full condition is: > if (fixed_regs[reg] || global_regs[reg] > || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && > frame_pointer_needed > && reg == HARD_FRAME_POINTER_REGNUM) > || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed > && reg == FRAME_POINTER_REGNUM)) > > The condition in the second || looks bogus (what use testing if a > register is 'non-zero'). > > So this looks like a useful catch by the warning, though I'm not sure at > first glance how > to fix it. > Kyrill > I assume HARD_FRAME_POINTER_REGNUM is never zero. That looks like it should be || (frame_pointer_needed && reg == FRAME_POINTER_REGNUM) Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 20/09/16 15:11, Bernd Edlinger wrote: On 09/20/16 13:29, Kyrill Tkachov wrote: arm bootstrap is now failing: $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ ~^~ $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro 'TARGET_ARM_FP' if (TARGET_ARM_FP) The full definition of TARGET_ARM_FP is: #define TARGET_ARM_FP\ (!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\ : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ : 0) We want it set to 0 when there's no FP but when FP is available we set it to a bitmask to suggest the level that is available. That seems like a legitimate use to me. Ok, I see, sorry for that. I think I will have to suppress the warning if the conditional is in a macro somehow. Can you work around that for a few days? I changed to: if (TARGET_ARM_FP != 0) in my tree to allow the build to proceed but encountered another bootstrap failure: In file included from ./tm.h:38:0, from $SRC/gcc/backend.h:28, from $SRC/gcc/regrename.c:23: $SRC/gcc/regrename.c: In function 'void rename_chains()': $SRC/gcc/config/arm/arm.h:915:4: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] (TARGET_ARM \ ~ ? ARM_HARD_FRAME_POINTER_REGNUM \ ^~ : THUMB_HARD_FRAME_POINTER_REGNUM) ~~ $SRC/gcc/regrename.c:484:8: note: in expansion of macro 'HARD_FRAME_POINTER_REGNUM' || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed ^ That looks like a legitimate bug in regrename.c ? The full condition is: if (fixed_regs[reg] || global_regs[reg] || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed && reg == HARD_FRAME_POINTER_REGNUM) || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed && reg == FRAME_POINTER_REGNUM)) The condition in the second || looks bogus (what use testing if a register is 'non-zero'). So this looks like a useful catch by the warning, though I'm not sure at first glance how to fix it. Kyrill Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/20/16 13:29, Kyrill Tkachov wrote: > > arm bootstrap is now failing: > $SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in > boolean context [-Werror=int-in-bool-context] > : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ > ~^~ > $SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro > 'TARGET_ARM_FP' > if (TARGET_ARM_FP) > > > The full definition of TARGET_ARM_FP is: > #define TARGET_ARM_FP\ >(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\ > : (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \ >: 0) > > We want it set to 0 when there's no FP but when FP is available we set > it to a bitmask > to suggest the level that is available. That seems like a legitimate use > to me. > Ok, I see, sorry for that. I think I will have to suppress the warning if the conditional is in a macro somehow. Can you work around that for a few days? Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
Hi Bernd,
On 19/09/16 22:32, Bernd Edlinger wrote:
On 09/19/16 21:51, Jeff Law wrote:
On 09/15/2016 03:19 AM, Bernd Edlinger wrote:
On 09/14/16 20:11, Jason Merrill wrote:
Yes. The reasoning I initially had was that it is completely
pointless to have something of the form "if (x ? 1 : 2)" or
"if (x ? 0 : 0)" because the result does not even depend on x
in this case. But something like "if (x ? 4999 : 0)" looks
bogus but does at least not ignore x.
If the false-positives are becoming too much of a problem here,
then I should of course revert to the previous heuristic again.
I think we could have both, where the weaker form is part of -Wall and
people can explicitly select the stronger form.
Yes, agreed. So here is what I would think will be the first version.
It can later be extended to cover the more pedantic cases which
will not be enabled by -Wall.
I would like to send a follow-up patch for the warning on
signed-integer shift left in boolean context, which I think
should also be good for Wall.
(I already had that feature in patch version 2 but that's meanwhile
outdated).
Bootstrap and reg-testing on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
changelog-pr77434v3.txt
gcc:
2016-09-14 Bernd Edlinger
PR c++/77434
* doc/invoke.texi: Document -Wcond-in-bool-context.
PR middle-end/77421
* dwarf2out.c (output_loc_operands): Fix an assertion.
c-family:
2016-09-14 Bernd Edlinger
PR c++/77434
* c.opt (Wcond-in-bool-context): New warning.
* c-common.c (c_common_truthvalue_conversion): Warn on integer
constants in boolean context.
cp:
2016-09-14 Bernd Edlinger
PR c++/77434
* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
testsuite:
2016-09-14 Bernd Edlinger
PR c++/77434
* c-c++-common/Wcond-in-bool-context.c: New test.
patch-pr77434v3.diff
Index: gcc/builtins.c
===
--- gcc/builtins.c(revision 240135)
+++ gcc/builtins.c(working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree
fndecl
tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-signbit_call, integer_zero_node);
+signbit_call, integer_zero_node);
isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
- isinf_call, integer_zero_node);
+ isinf_call, integer_zero_node);
-tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
signbit_call,
- integer_minus_one_node, integer_one_node);
tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
- isinf_call, tmp,
- integer_zero_node);
+ signbit_call, integer_minus_one_node,
+ integer_one_node);
+/* Avoid a possible -Wint-in-bool-context warning in C. */
+tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+ integer_zero_node);
+tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+ isinf_call, tmp, integer_zero_node);
}
return tmp;
This hunk is not mentioned in the ChangeLog and there's a good chance
this has or is going to change significantly. I don't like the tmp+0
workaround either. If there isn't an immediate need, can we let this
hunk slide and come back to it after the other changes from Tamar &
Wilco are wrapped up?
Yes that would be good.
I think this is OK with the builtins.c hunk dropped as long as exclusion
of that hunk doesn't trigger spurious warnings.
It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc. That does only happen with C, not C++.
If I drop that chunk then I must also drop that line
if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */
But I have to come back and silence the warning on that construct
in a follow-up patch.
If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?
arm bootstrap is now failing:
$SRC/gcc/config/arm/arm.h:2229:40: error: ?: using integer constants in boolean
context [-Werror=int-in-bool-context]
: (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
~^~
$SRC/gcc/config/arm/arm-c.c:133:7: note: in expansion of macro 'TARGET_ARM_FP'
if (TARGET_ARM_FP)
The full definition of TARGET_ARM_FP is:
#define TARGET_ARM_FP\
(!TARGET_SOFT_FLOAT ? (TARGET_VFP_SINGLE ? 4\
: (TARGET_VFP_DOUBLE ? (TARGET_FP16 ? 14 : 12) : 0)) \
: 0)
We want it set to 0 when there's no FP but when FP is available we set it to a
bitmask
to suggest the level that i
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
In file included from ./tm.h:25:0, from ../../gcc/target.h:52, from ../../gcc/cp/typeck.c:30: ../../gcc/cp/typeck.c: In function 'tree_node* get_member_function_from_ptrfunc(tree_node**, tree, tsubst_flags_t)': ../../gcc/config/ia64/ia64.h:224:54: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] #define TARGET_VTABLE_USES_DESCRIPTORS (TARGET_ILP32 ? 4 : 2) ~~^~~~ ../../gcc/cp/typeck.c:3441:11: note: in expansion of macro 'TARGET_VTABLE_USES_DESCRIPTORS' if (TARGET_VTABLE_USES_DESCRIPTORS) ^~ Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/19/2016 03:32 PM, Bernd Edlinger wrote:
On 09/19/16 21:51, Jeff Law wrote:
On 09/15/2016 03:19 AM, Bernd Edlinger wrote:
On 09/14/16 20:11, Jason Merrill wrote:
Yes. The reasoning I initially had was that it is completely
pointless to have something of the form "if (x ? 1 : 2)" or
"if (x ? 0 : 0)" because the result does not even depend on x
in this case. But something like "if (x ? 4999 : 0)" looks
bogus but does at least not ignore x.
If the false-positives are becoming too much of a problem here,
then I should of course revert to the previous heuristic again.
I think we could have both, where the weaker form is part of -Wall and
people can explicitly select the stronger form.
Yes, agreed. So here is what I would think will be the first version.
It can later be extended to cover the more pedantic cases which
will not be enabled by -Wall.
I would like to send a follow-up patch for the warning on
signed-integer shift left in boolean context, which I think
should also be good for Wall.
(I already had that feature in patch version 2 but that's meanwhile
outdated).
Bootstrap and reg-testing on x86_64-pc-linux-gnu.
Is it OK for trunk?
Thanks
Bernd.
changelog-pr77434v3.txt
gcc:
2016-09-14 Bernd Edlinger
PR c++/77434
* doc/invoke.texi: Document -Wcond-in-bool-context.
PR middle-end/77421
* dwarf2out.c (output_loc_operands): Fix an assertion.
c-family:
2016-09-14 Bernd Edlinger
PR c++/77434
* c.opt (Wcond-in-bool-context): New warning.
* c-common.c (c_common_truthvalue_conversion): Warn on integer
constants in boolean context.
cp:
2016-09-14 Bernd Edlinger
PR c++/77434
* cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
testsuite:
2016-09-14 Bernd Edlinger
PR c++/77434
* c-c++-common/Wcond-in-bool-context.c: New test.
patch-pr77434v3.diff
Index: gcc/builtins.c
===
--- gcc/builtins.c(revision 240135)
+++ gcc/builtins.c(working copy)
@@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree
fndecl
tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
-signbit_call, integer_zero_node);
+signbit_call, integer_zero_node);
isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
- isinf_call, integer_zero_node);
+ isinf_call, integer_zero_node);
-tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
signbit_call,
- integer_minus_one_node, integer_one_node);
tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
- isinf_call, tmp,
- integer_zero_node);
+ signbit_call, integer_minus_one_node,
+ integer_one_node);
+/* Avoid a possible -Wint-in-bool-context warning in C. */
+tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
+ integer_zero_node);
+tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
+ isinf_call, tmp, integer_zero_node);
}
return tmp;
This hunk is not mentioned in the ChangeLog and there's a good chance
this has or is going to change significantly. I don't like the tmp+0
workaround either. If there isn't an immediate need, can we let this
hunk slide and come back to it after the other changes from Tamar &
Wilco are wrapped up?
Yes that would be good.
I think this is OK with the builtins.c hunk dropped as long as exclusion
of that hunk doesn't trigger spurious warnings.
It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc. That does only happen with C, not C++.
If I drop that chunk then I must also drop that line
if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */
But I have to come back and silence the warning on that construct
in a follow-up patch.
If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?
Yea, that's fine.
I think Tamar & Wilco's work on builtin_classify_fptype is reasonably
close, so it shouldn't be long before you can get back to those bits.
jeff
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/19/16 21:51, Jeff Law wrote:
> On 09/15/2016 03:19 AM, Bernd Edlinger wrote:
>> On 09/14/16 20:11, Jason Merrill wrote:
>>
>> Yes. The reasoning I initially had was that it is completely
>> pointless to have something of the form "if (x ? 1 : 2)" or
>> "if (x ? 0 : 0)" because the result does not even depend on x
>> in this case. But something like "if (x ? 4999 : 0)" looks
>> bogus but does at least not ignore x.
>>
>> If the false-positives are becoming too much of a problem here,
>> then I should of course revert to the previous heuristic again.
>>> >
>>> > I think we could have both, where the weaker form is part of -Wall and
>>> > people can explicitly select the stronger form.
>>> >
>>
>> Yes, agreed. So here is what I would think will be the first version.
>>
>> It can later be extended to cover the more pedantic cases which
>> will not be enabled by -Wall.
>>
>> I would like to send a follow-up patch for the warning on
>> signed-integer shift left in boolean context, which I think
>> should also be good for Wall.
>> (I already had that feature in patch version 2 but that's meanwhile
>> outdated).
>>
>>
>> Bootstrap and reg-testing on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> changelog-pr77434v3.txt
>>
>>
>> gcc:
>> 2016-09-14 Bernd Edlinger
>>
>> PR c++/77434
>> * doc/invoke.texi: Document -Wcond-in-bool-context.
>>
>> PR middle-end/77421
>> * dwarf2out.c (output_loc_operands): Fix an assertion.
>>
>> c-family:
>> 2016-09-14 Bernd Edlinger
>>
>> PR c++/77434
>> * c.opt (Wcond-in-bool-context): New warning.
>> * c-common.c (c_common_truthvalue_conversion): Warn on integer
>> constants in boolean context.
>>
>> cp:
>> 2016-09-14 Bernd Edlinger
>>
>> PR c++/77434
>> * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
>>
>> testsuite:
>> 2016-09-14 Bernd Edlinger
>>
>> PR c++/77434
>> * c-c++-common/Wcond-in-bool-context.c: New test.
>>
>>
>> patch-pr77434v3.diff
>>
>>
>> Index: gcc/builtins.c
>> ===
>> --- gcc/builtins.c(revision 240135)
>> +++ gcc/builtins.c(working copy)
>> @@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree
>> fndecl
>> tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg);
>>
>> signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
>> -signbit_call, integer_zero_node);
>> +signbit_call, integer_zero_node);
>> isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node,
>> - isinf_call, integer_zero_node);
>> + isinf_call, integer_zero_node);
>>
>> -tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
>> signbit_call,
>> - integer_minus_one_node, integer_one_node);
>> tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
>> - isinf_call, tmp,
>> - integer_zero_node);
>> + signbit_call, integer_minus_one_node,
>> + integer_one_node);
>> +/* Avoid a possible -Wint-in-bool-context warning in C. */
>> +tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp,
>> + integer_zero_node);
>> +tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node,
>> + isinf_call, tmp, integer_zero_node);
>>}
>>
>> return tmp;
> This hunk is not mentioned in the ChangeLog and there's a good chance
> this has or is going to change significantly. I don't like the tmp+0
> workaround either. If there isn't an immediate need, can we let this
> hunk slide and come back to it after the other changes from Tamar &
> Wilco are wrapped up?
>
Yes that would be good.
> I think this is OK with the builtins.c hunk dropped as long as exclusion
> of that hunk doesn't trigger spurious warnings.
>
It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc. That does only happen with C, not C++.
If I drop that chunk then I must also drop that line
if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */
But I have to come back and silence the warning on that construct
in a follow-up patch.
If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?
Thanks
Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/19/16 21:17, Jason Merrill wrote:
> On 09/15/2016 05:19 AM, Bernd Edlinger wrote:
>> + if (warn_int_in_bool_context)
>> +{
>> + tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
>> + tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
>> + if (TREE_CODE (val1) == INTEGER_CST
>> + && TREE_CODE (val2) == INTEGER_CST
>> + && !integer_zerop (val1)
>> + && !integer_zerop (val2)
>> + && (!integer_onep (val1)
>> + || !integer_onep (val2)))
>> +warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>> +"?: using integer constants in boolean context");
>
> I think this message could be more helpful; we're warning about code
> that always evaluates to 'true', so we could say something like
> "conditional expression always evaluates to true". In which case maybe
> your original warning name was better.
>
Yes. It is difficult to choose a good name.
I would also like to warn on signed_int shift left in boolean context,
as a follow-up, because it found also a wrong code in
gcc/cp/parser.c (cp_parser_condition) where we have:
bool flags = LOOKUP_ONLYCONVERTING;
but LOOKUP_ONLYCONVERTING is (1 << 2)
This has nothing to do with conditions,
but just with undefined integer operations in boolean context.
So I thought, these warnings would have something in common.
> I'm also not sure we need to check integer_onep at all before giving
> that warning. Were you seeing a lot of false positives without that check?
>
I did not try. This warning is trying to identify code which is
doing funny things because one of the conditional expressions are not
boolean, thus not 0 or 1. That can happen because either brackets
are there, but at the wrong place, or brackets are not there and the
precedence of ?: and && or || result in wrong code. Chances are that
"? 2 : 3" is not right in a boolean context. The fact that the
result used as a boolean is always true, adds just another point to
the score. So we have a two reasons why we would warn here,
that justifies -Wall, otherwise only -Wextra I would say.
My reasoning was that if both sides evaluate to 1 or both evaluate to
0, it may well be possible that we have different defines here, maybe
just configure options that are only both 1 by chance.
However there is PR 64279 which should handle exactly this:
[Bug c/64279] Warning missing for "(cond) ? A : A" / if(cond) expr1;
else expr1; // same expression in if and else branch
I think Marek is already working on it.
Thanks
Bernd.
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/15/2016 03:19 AM, Bernd Edlinger wrote: On 09/14/16 20:11, Jason Merrill wrote: >> >> Yes. The reasoning I initially had was that it is completely >> pointless to have something of the form "if (x ? 1 : 2)" or >> "if (x ? 0 : 0)" because the result does not even depend on x >> in this case. But something like "if (x ? 4999 : 0)" looks >> bogus but does at least not ignore x. >> >> If the false-positives are becoming too much of a problem here, >> then I should of course revert to the previous heuristic again. > > I think we could have both, where the weaker form is part of -Wall and > people can explicitly select the stronger form. > Yes, agreed. So here is what I would think will be the first version. It can later be extended to cover the more pedantic cases which will not be enabled by -Wall. I would like to send a follow-up patch for the warning on signed-integer shift left in boolean context, which I think should also be good for Wall. (I already had that feature in patch version 2 but that's meanwhile outdated). Bootstrap and reg-testing on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. changelog-pr77434v3.txt gcc: 2016-09-14 Bernd Edlinger PR c++/77434 * doc/invoke.texi: Document -Wcond-in-bool-context. PR middle-end/77421 * dwarf2out.c (output_loc_operands): Fix an assertion. c-family: 2016-09-14 Bernd Edlinger PR c++/77434 * c.opt (Wcond-in-bool-context): New warning. * c-common.c (c_common_truthvalue_conversion): Warn on integer constants in boolean context. cp: 2016-09-14 Bernd Edlinger PR c++/77434 * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here. testsuite: 2016-09-14 Bernd Edlinger PR c++/77434 * c-c++-common/Wcond-in-bool-context.c: New test. patch-pr77434v3.diff Index: gcc/builtins.c === --- gcc/builtins.c (revision 240135) +++ gcc/builtins.c (working copy) @@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree fndecl tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg); signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node, - signbit_call, integer_zero_node); + signbit_call, integer_zero_node); isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node, - isinf_call, integer_zero_node); + isinf_call, integer_zero_node); - tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, signbit_call, - integer_minus_one_node, integer_one_node); tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, - isinf_call, tmp, - integer_zero_node); + signbit_call, integer_minus_one_node, + integer_one_node); + /* Avoid a possible -Wint-in-bool-context warning in C. */ + tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp, + integer_zero_node); + tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, + isinf_call, tmp, integer_zero_node); } return tmp; This hunk is not mentioned in the ChangeLog and there's a good chance this has or is going to change significantly. I don't like the tmp+0 workaround either. If there isn't an immediate need, can we let this hunk slide and come back to it after the other changes from Tamar & Wilco are wrapped up? I think this is OK with the builtins.c hunk dropped as long as exclusion of that hunk doesn't trigger spurious warnings. jeff
Re: [PATCHv3, resent] Add a warning for suspicious use of conditional expressions in boolean context
On 09/15/2016 05:19 AM, Bernd Edlinger wrote:
+ if (warn_int_in_bool_context)
+ {
+ tree val1 = fold_for_warn (TREE_OPERAND (expr, 1));
+ tree val2 = fold_for_warn (TREE_OPERAND (expr, 2));
+ if (TREE_CODE (val1) == INTEGER_CST
+ && TREE_CODE (val2) == INTEGER_CST
+ && !integer_zerop (val1)
+ && !integer_zerop (val2)
+ && (!integer_onep (val1)
+ || !integer_onep (val2)))
+ warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
+ "?: using integer constants in boolean context");
I think this message could be more helpful; we're warning about code
that always evaluates to 'true', so we could say something like
"conditional expression always evaluates to true". In which case maybe
your original warning name was better.
I'm also not sure we need to check integer_onep at all before giving
that warning. Were you seeing a lot of false positives without that check?
Jason
