Re: [PING^5] Re: [PATCH] analyzer: deal with -fshort-enums

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

Ping!

Kind regards,
Torbjörn

On 2024-03-25 15:59, Yvan ROUX - foss wrote:
> 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 <oliva@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;
>>>>>> +}
>>>>>>
>>>>>>
>>>>>