[PATCH] analyzer: deal with -fshort-enums

[PATCH] analyzer: deal with -fshort-enums

[Alexandre Oliva]

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.

Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
cpu on trunk, and with tms570 on gcc-13.  Ok to install?


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.
---
 gcc/analyzer/region-model.cc |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index dc83440652027..05f716cb7d663 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -5340,10 +5340,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);
@@ -5379,6 +5379,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
@@ -5390,12 +5407,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))

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

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

[David Malcolm]

On Sun, 2023-11-19 at 04:39 -0300, Alexandre Oliva wrote:
> 
> 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.

Thanks for the patch, and sorry for the failing tests.

Which tests failed?  I'm guessing one of the ones from r13-5159-
gccd4df81aa6537.

Can you add a copy of one of those tests that needs the patch, with an
explicit -fshort-enums, to ensure that regression testing is hitting
this case on all targets in the future?

Thanks
Dave

> 
> Regstrapped on x86_64-linux-gnu, also tested on arm-eabi with default
> cpu on trunk, and with tms570 on gcc-13.  Ok to install?
> 
> 
> 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.
> ---
>  gcc/analyzer/region-model.cc |   27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> model.cc
> index dc83440652027..05f716cb7d663 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -5340,10 +5340,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);
> @@ -5379,6 +5379,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
> @@ -5390,12 +5407,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))
> 

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

[Alexandre Oliva]

On Nov 20, 2023, David Malcolm <dmalcolm@redhat.com> wrote:

> On Sun, 2023-11-19 at 04:39 -0300, Alexandre Oliva wrote:
>> 
>> 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.

> Thanks for the patch, and sorry for the failing tests.

No worries.

> Which tests failed?  I'm guessing one of the ones from r13-5159-
> gccd4df81aa6537.

Yeah, I'm afraid I didn't take notes about which tests failed (I'll try
to dig that up), but I'm sure switch-enum-1, switch-enum-2, and
switch-enum-pr105273-doom-p_maputl.c.  I *think* also switch-enum-5 and
switch-enum-pr105273-doom-p_floor.c.

> Can you add a copy of one of those tests that needs the patch, with an
> explicit -fshort-enums, to ensure that regression testing is hitting
> this case on all targets in the future?

Ah, nice, that's a great idea, I wish I'd thought of that!  Will do.

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

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

[Alexandre Oliva]

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?


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


-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

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

[David Malcolm]

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

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

[Torbjorn SVENSSON]

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

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

[Torbjorn SVENSSON]

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

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

[Torbjorn SVENSSON]

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

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

[Torbjorn SVENSSON]

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

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

[Yvan ROUX - foss]

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

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

[Torbjorn SVENSSON]

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