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

[David Malcolm <dmalcolm@redhat.com>]

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