Re: [PATCH] analyzer: deal with -fshort-enums

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

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