Ping!
Rgds,
Yvan
________________________________________
From: Torbjorn SVENSSON - foss
Sent: Friday, March 15, 2024 11:32 AM
To: David Malcolm; Alexandre Oliva
Cc: gcc-patches@gcc.gnu.org; Yvan ROUX - foss
Subject: [PING^3] Re: [PATCH] analyzer: deal with -fshort-enums
Ping!
Kind regards,
Torbjörn
On 2024-03-08 10:14, Torbjorn SVENSSON wrote:
> Ping!
>
> Kind regards,
> Torbjörn
>
> On 2024-02-22 09:51, Torbjorn SVENSSON wrote:
>> Ping!
>>
>> Kind regards,
>> Torbjörn
>>
>> On 2024-02-07 17:21, Torbjorn SVENSSON wrote:
>>> Hi,
>>>
>>> Is it okay to backport 3cbab07b08d2f3a3ed34b6ec12e67727c59d285c to
>>> releases/gcc-13?
>>>
>>> Without this backport, I see these failures on arm-none-eabi:
>>>
>>> FAIL: gcc.dg/analyzer/switch-enum-1.c (test for bogus messages, line
>>> 26)
>>> FAIL: gcc.dg/analyzer/switch-enum-1.c (test for bogus messages, line
>>> 44)
>>> FAIL: gcc.dg/analyzer/switch-enum-2.c (test for bogus messages, line
>>> 34)
>>> FAIL: gcc.dg/analyzer/switch-enum-2.c (test for bogus messages, line
>>> 52)
>>> FAIL: gcc.dg/analyzer/torture/switch-enum-pr105273-doom-p_floor.c -O0
>>> (test for bogus messages, line 82)
>>> FAIL: gcc.dg/analyzer/torture/switch-enum-pr105273-doom-p_maputl.c
>>> -O0 (test for bogus messages, line 83)
>>>
>>> Kind regards,
>>> Torbjörn
>>>
>>>
>>> On 2023-12-06 23:22, David Malcolm wrote:
>>>> On Wed, 2023-12-06 at 02:31 -0300, Alexandre Oliva wrote:
>>>>> On Nov 22, 2023, Alexandre Oliva <ol...@adacore.com> wrote:
>>>>>
>>>>>> Ah, nice, that's a great idea, I wish I'd thought of that! Will
>>>>>> do.
>>>>>
>>>>> Sorry it took me so long, here it is. I added two tests, so that,
>>>>> regardless of the defaults, we get both circumstances tested, without
>>>>> repetition.
>>>>>
>>>>> Regstrapped on x86_64-linux-gnu. Also tested on arm-eabi. Ok to
>>>>> install?
>>>>
>>>> Thanks for the updated patch.
>>>>
>>>> Looks good to me.
>>>>
>>>> Dave
>>>>
>>>>>
>>>>>
>>>>> analyzer: deal with -fshort-enums
>>>>>
>>>>> On platforms that enable -fshort-enums by default, various switch-
>>>>> enum
>>>>> analyzer tests fail, because apply_constraints_for_gswitch doesn't
>>>>> expect the integral promotion type cast. I've arranged for the code
>>>>> to cope with those casts.
>>>>>
>>>>>
>>>>> for gcc/analyzer/ChangeLog
>>>>>
>>>>> * region-model.cc (has_nondefault_case_for_value_p): Take
>>>>> enumerate type as a parameter.
>>>>> (region_model::apply_constraints_for_gswitch): Cope with
>>>>> integral promotion type casts.
>>>>>
>>>>> for gcc/testsuite/ChangeLog
>>>>>
>>>>> * gcc.dg/analyzer/switch-short-enum-1.c: New.
>>>>> * gcc.dg/analyzer/switch-no-short-enum-1.c: New.
>>>>> ---
>>>>> gcc/analyzer/region-model.cc | 27 +++-
>>>>> .../gcc.dg/analyzer/switch-no-short-enum-1.c | 141
>>>>> ++++++++++++++++++++
>>>>> .../gcc.dg/analyzer/switch-short-enum-1.c | 140
>>>>> ++++++++++++++++++++
>>>>> 3 files changed, 304 insertions(+), 4 deletions(-)
>>>>> create mode 100644 gcc/testsuite/gcc.dg/analyzer/switch-no-short-
>>>>> enum-1.c
>>>>> create mode 100644 gcc/testsuite/gcc.dg/analyzer/switch-short-enum-
>>>>> 1.c
>>>>>
>>>>> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
>>>>> model.cc
>>>>> index 2157ad2578b85..6a7a8bc9f4884 100644
>>>>> --- a/gcc/analyzer/region-model.cc
>>>>> +++ b/gcc/analyzer/region-model.cc
>>>>> @@ -5387,10 +5387,10 @@ has_nondefault_case_for_value_p (const
>>>>> gswitch *switch_stmt, tree int_cst)
>>>>> has nondefault cases handling all values in the enum. */
>>>>> static bool
>>>>> -has_nondefault_cases_for_all_enum_values_p (const gswitch
>>>>> *switch_stmt)
>>>>> +has_nondefault_cases_for_all_enum_values_p (const gswitch
>>>>> *switch_stmt,
>>>>> + tree type)
>>>>> {
>>>>> gcc_assert (switch_stmt);
>>>>> - tree type = TREE_TYPE (gimple_switch_index (switch_stmt));
>>>>> gcc_assert (TREE_CODE (type) == ENUMERAL_TYPE);
>>>>> for (tree enum_val_iter = TYPE_VALUES (type);
>>>>> @@ -5426,6 +5426,23 @@ apply_constraints_for_gswitch (const
>>>>> switch_cfg_superedge &edge,
>>>>> {
>>>>> tree index = gimple_switch_index (switch_stmt);
>>>>> const svalue *index_sval = get_rvalue (index, ctxt);
>>>>> + bool check_index_type = true;
>>>>> +
>>>>> + /* With -fshort-enum, there may be a type cast. */
>>>>> + if (ctxt && index_sval->get_kind () == SK_UNARYOP
>>>>> + && TREE_CODE (index_sval->get_type ()) == INTEGER_TYPE)
>>>>> + {
>>>>> + const unaryop_svalue *unaryop = as_a <const unaryop_svalue *>
>>>>> (index_sval);
>>>>> + if (unaryop->get_op () == NOP_EXPR
>>>>> + && is_a <const initial_svalue *> (unaryop->get_arg ()))
>>>>> + if (const initial_svalue *initvalop = (as_a <const
>>>>> initial_svalue *>
>>>>> + (unaryop->get_arg
>>>>> ())))
>>>>> + if (TREE_CODE (initvalop->get_type ()) == ENUMERAL_TYPE)
>>>>> + {
>>>>> + index_sval = initvalop;
>>>>> + check_index_type = false;
>>>>> + }
>>>>> + }
>>>>> /* If we're switching based on an enum type, assume that the user
>>>>> is only
>>>>> working with values from the enum. Hence if this is an
>>>>> @@ -5437,12 +5454,14 @@ apply_constraints_for_gswitch (const
>>>>> switch_cfg_superedge &edge,
>>>>> ctxt
>>>>> /* Must be an enum value. */
>>>>> && index_sval->get_type ()
>>>>> - && TREE_CODE (TREE_TYPE (index)) == ENUMERAL_TYPE
>>>>> + && (!check_index_type
>>>>> + || TREE_CODE (TREE_TYPE (index)) == ENUMERAL_TYPE)
>>>>> && TREE_CODE (index_sval->get_type ()) == ENUMERAL_TYPE
>>>>> /* If we have a constant, then we can check it directly. */
>>>>> && index_sval->get_kind () != SK_CONSTANT
>>>>> && edge.implicitly_created_default_p ()
>>>>> - && has_nondefault_cases_for_all_enum_values_p (switch_stmt)
>>>>> + && has_nondefault_cases_for_all_enum_values_p (switch_stmt,
>>>>> + index_sval-
>>>>>> get_type ())
>>>>> /* Don't do this if there's a chance that the index is
>>>>> attacker-controlled. */
>>>>> && !ctxt->possibly_tainted_p (index_sval))
>>>>> diff --git a/gcc/testsuite/gcc.dg/analyzer/switch-no-short-enum-1.c
>>>>> b/gcc/testsuite/gcc.dg/analyzer/switch-no-short-enum-1.c
>>>>> new file mode 100644
>>>>> index 0000000000000..98f6d91f97481
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.dg/analyzer/switch-no-short-enum-1.c
>>>>> @@ -0,0 +1,141 @@
>>>>> +/* { dg-do compile } */
>>>>> +/* { dg-additional-options "-fno-short-enums" } */
>>>>> +/* { dg-skip-if "default" { ! short_enums } } */
>>>>> +
>>>>> +#include "analyzer-decls.h"
>>>>> +
>>>>> +/* Verify the handling of "switch (enum_value)". */
>>>>> +
>>>>> +enum e
>>>>> +{
>>>>> + E_VAL0,
>>>>> + E_VAL1,
>>>>> + E_VAL2
>>>>> +};
>>>>> +
>>>>> +/* Verify that we assume that "switch (enum)" doesn't follow
>>>>> implicit
>>>>> + "default" if all enum values have cases */
>>>>> +
>>>>> +int test_all_values_covered_implicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + case E_VAL2:
>>>>> + return 1945;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> +}
>>>>> +
>>>>> +int test_all_values_covered_implicit_default_2 (enum e x)
>>>>> +{
>>>>> + int result;
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + result = 1066;
>>>>> + break;
>>>>> + case E_VAL1:
>>>>> + result = 1776;
>>>>> + break;
>>>>> + case E_VAL2:
>>>>> + result = 1945;
>>>>> + break;
>>>>> + }
>>>>> + return result; /* { dg-bogus "uninitialized" } */
>>>>> +}
>>>>> +
>>>>> +/* Verify that we consider paths that use the implicit default when
>>>>> not
>>>>> + all enum values are covered by cases. */
>>>>> +
>>>>> +int test_missing_values_implicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x) /* { dg-message "following 'default:' branch" } */
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_implicit_default_2 (enum e x)
>>>>> +{
>>>>> + int result;
>>>>> + switch (x) /* { dg-message "following 'default:' branch" } */
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + result = 1066;
>>>>> + break;
>>>>> + case E_VAL1:
>>>>> + result = 1776;
>>>>> + break;
>>>>> + }
>>>>> + return result; /* { dg-warning "uninitialized" } */
>>>>> +}
>>>>> +
>>>>> +/* Verify that explicit "default" isn't rejected. */
>>>>> +
>>>>> +int test_all_values_covered_explicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + case E_VAL2:
>>>>> + return 1945;
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 0;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_explicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + default:
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_explicit_default_2 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 1945;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_just_default (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 42;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> diff --git a/gcc/testsuite/gcc.dg/analyzer/switch-short-enum-1.c
>>>>> b/gcc/testsuite/gcc.dg/analyzer/switch-short-enum-1.c
>>>>> new file mode 100644
>>>>> index 0000000000000..384113fde5cbf
>>>>> --- /dev/null
>>>>> +++ b/gcc/testsuite/gcc.dg/analyzer/switch-short-enum-1.c
>>>>> @@ -0,0 +1,140 @@
>>>>> +/* { dg-do compile } */
>>>>> +/* { dg-additional-options "-fshort-enums" } */
>>>>> +/* { dg-skip-if "default" { short_enums } } */
>>>>> +
>>>>> +#include "analyzer-decls.h"
>>>>> +
>>>>> +/* Verify the handling of "switch (enum_value)". */
>>>>> +
>>>>> +enum e
>>>>> +{
>>>>> + E_VAL0,
>>>>> + E_VAL1,
>>>>> + E_VAL2
>>>>> +};
>>>>> +
>>>>> +/* Verify that we assume that "switch (enum)" doesn't follow
>>>>> implicit
>>>>> + "default" if all enum values have cases */
>>>>> +
>>>>> +int test_all_values_covered_implicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + case E_VAL2:
>>>>> + return 1945;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> +}
>>>>> +
>>>>> +int test_all_values_covered_implicit_default_2 (enum e x)
>>>>> +{
>>>>> + int result;
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + result = 1066;
>>>>> + break;
>>>>> + case E_VAL1:
>>>>> + result = 1776;
>>>>> + break;
>>>>> + case E_VAL2:
>>>>> + result = 1945;
>>>>> + break;
>>>>> + }
>>>>> + return result; /* { dg-bogus "uninitialized" } */
>>>>> +}
>>>>> +
>>>>> +/* Verify that we consider paths that use the implicit default when
>>>>> not
>>>>> + all enum values are covered by cases. */
>>>>> +
>>>>> +int test_missing_values_implicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x) /* { dg-message "following 'default:' branch" } */
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_implicit_default_2 (enum e x)
>>>>> +{
>>>>> + int result;
>>>>> + switch (x) /* { dg-message "following 'default:' branch" } */
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + result = 1066;
>>>>> + break;
>>>>> + case E_VAL1:
>>>>> + result = 1776;
>>>>> + break;
>>>>> + }
>>>>> + return result; /* { dg-warning "uninitialized" } */
>>>>> +}
>>>>> +
>>>>> +/* Verify that explicit "default" isn't rejected. */
>>>>> +
>>>>> +int test_all_values_covered_explicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + case E_VAL2:
>>>>> + return 1945;
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 0;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_explicit_default_1 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + default:
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_missing_values_explicit_default_2 (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + case E_VAL0:
>>>>> + return 1066;
>>>>> + case E_VAL1:
>>>>> + return 1776;
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 1945;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +int test_just_default (enum e x)
>>>>> +{
>>>>> + switch (x)
>>>>> + {
>>>>> + default:
>>>>> + __analyzer_dump_path (); /* { dg-message "path" } */
>>>>> + return 42;
>>>>> + }
>>>>> + __analyzer_dump_path (); /* { dg-bogus "path" } */
>>>>> + return 0;
>>>>> +}
>>>>>
>>>>>
>>>>
