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; >>>>> +} >>>>> >>>>> >>>>