From: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>
To: David Malcolm <dmalcolm@redhat.com>, Alexandre Oliva <oliva@adacore.com>
Cc: <gcc-patches@gcc.gnu.org>, Yvan Roux <yvan.roux@foss.st.com>
Subject: [PING^2] Re: [PATCH] analyzer: deal with -fshort-enums
Date: Fri, 8 Mar 2024 10:14:57 +0100 [thread overview]
Message-ID: <8a06a0b3-5e56-4ff8-9d3c-f5ca5260e6ae@foss.st.com> (raw)
In-Reply-To: <923c3d78-6d0b-482d-9fcf-3a0129ad7062@foss.st.com>
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;
>>>> +}
>>>>
>>>>
>>>
next prev parent reply other threads:[~2024-03-08 9:15 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-19 7:39 Alexandre Oliva
2023-11-20 22:32 ` David Malcolm
2023-11-22 7:58 ` Alexandre Oliva
2023-12-06 5:31 ` Alexandre Oliva
2023-12-06 22:22 ` David Malcolm
2024-02-07 16:21 ` Torbjorn SVENSSON
2024-02-22 8:51 ` [PING] " Torbjorn SVENSSON
2024-03-08 9:14 ` Torbjorn SVENSSON [this message]
2024-03-15 10:32 ` [PING^3] " Torbjorn SVENSSON
2024-03-25 14:59 ` [PING^4] " Yvan ROUX - foss
2024-04-10 13:23 ` [PING^5] " Torbjorn SVENSSON
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8a06a0b3-5e56-4ff8-9d3c-f5ca5260e6ae@foss.st.com \
--to=torbjorn.svensson@foss.st.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=oliva@adacore.com \
--cc=yvan.roux@foss.st.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).