Re: [C/C++ PATCH] Fix promoted switch condition out of range diagnostics (PR c/89888, take 2)
On Fri, Apr 19, 2019 at 1:39 AM Jakub Jelinek wrote: > > On Thu, Apr 18, 2019 at 10:41:55PM -0700, Jason Merrill wrote: > > > + node = splay_tree_predecessor (cases, (splay_tree_key) min_value); > > ... > > > + if (CASE_HIGH ((tree) node->value) > > > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > > > + min_value) >= 0) > > ... > > > + node = splay_tree_predecessor (cases, > > > +(splay_tree_key) min_value); > > > > > + node = splay_tree_lookup (cases, (splay_tree_key) max_value); > > > + if (node == NULL) > > > + node = splay_tree_predecessor (cases, (splay_tree_key) max_value); > > > + /* Handle a single node that might partially overlap the range. */ > > > + if (node > > > + && node->key > > > + && CASE_HIGH ((tree) node->value) > > > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > > > + max_value) > 0) > > ... > > > + while ((node = splay_tree_successor (cases, > > > + (splay_tree_key) max_value)) > > > > Hmm, why are the code sections for dealing with the lower bound and > > upper bound asymmetrical? That is, why not start the upper bound > > consideration with splay_tree_successor? > > Because of the case 1 ... 4: GNU extension. Without that it would be > mostly symmetrical (well, there still would be a difference, because we put > the default: before all other cases, so we still need to test > node->key != 0 for the splay_tree_predecessor case but don't have to test > that for splay_tree_successor. But with the GNU extension, we still use > the low bound of the range as the key, which makes it asymmetrical. > > splay_tree_predecessor (cases, (splay_tree_key) min_value) > if it is not default: can be either the overlapping case (first part of the > range outside of range, then part of the range in range of the corresponding > type) or non-overlapping case. There is at most one overlapping case there > and all further predecessors are necessarily outside of range. > > On the other side, > splay_tree_successor (cases, (splay_tree_key) max_value) > is never default: and is always completely outside of range (and its > successors are as well). If we want to find the (single) overlapping case > that > overlaps the max_value, then it could be either > splay_tree_lookup (cases, (splay_tree_key) max_value) > or > splay_tree_predecessor (cases, (splay_tree_key) max_value) > (the first one if there is e.g. a range case max_value ... max_value + N: > and the second one if there is e.g. a range case max_value - M ... max_value > + N: where both M and N are positive integers). > > Of course, the overlapping case could be also > case min_value - M ... max_value + N: > in which case both the earlier and later code will find the same node and > each will complain about one boundary of that node and adjust that boundary. Aha, thanks. The patch is OK. Jason
Re: [C/C++ PATCH] Fix promoted switch condition out of range diagnostics (PR c/89888, take 2)
On Thu, Apr 18, 2019 at 10:41:55PM -0700, Jason Merrill wrote: > > + node = splay_tree_predecessor (cases, (splay_tree_key) min_value); > ... > > + if (CASE_HIGH ((tree) node->value) > > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > > + min_value) >= 0) > ... > > + node = splay_tree_predecessor (cases, > > +(splay_tree_key) min_value); > > > + node = splay_tree_lookup (cases, (splay_tree_key) max_value); > > + if (node == NULL) > > + node = splay_tree_predecessor (cases, (splay_tree_key) max_value); > > + /* Handle a single node that might partially overlap the range. */ > > + if (node > > + && node->key > > + && CASE_HIGH ((tree) node->value) > > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > > + max_value) > 0) > ... > > + while ((node = splay_tree_successor (cases, > > + (splay_tree_key) max_value)) > > Hmm, why are the code sections for dealing with the lower bound and > upper bound asymmetrical? That is, why not start the upper bound > consideration with splay_tree_successor? Because of the case 1 ... 4: GNU extension. Without that it would be mostly symmetrical (well, there still would be a difference, because we put the default: before all other cases, so we still need to test node->key != 0 for the splay_tree_predecessor case but don't have to test that for splay_tree_successor. But with the GNU extension, we still use the low bound of the range as the key, which makes it asymmetrical. splay_tree_predecessor (cases, (splay_tree_key) min_value) if it is not default: can be either the overlapping case (first part of the range outside of range, then part of the range in range of the corresponding type) or non-overlapping case. There is at most one overlapping case there and all further predecessors are necessarily outside of range. On the other side, splay_tree_successor (cases, (splay_tree_key) max_value) is never default: and is always completely outside of range (and its successors are as well). If we want to find the (single) overlapping case that overlaps the max_value, then it could be either splay_tree_lookup (cases, (splay_tree_key) max_value) or splay_tree_predecessor (cases, (splay_tree_key) max_value) (the first one if there is e.g. a range case max_value ... max_value + N: and the second one if there is e.g. a range case max_value - M ... max_value + N: where both M and N are positive integers). Of course, the overlapping case could be also case min_value - M ... max_value + N: in which case both the earlier and later code will find the same node and each will complain about one boundary of that node and adjust that boundary. Jakub
Re: [C/C++ PATCH] Fix promoted switch condition out of range diagnostics (PR c/89888, take 2)
On Tue, Apr 9, 2019 at 2:39 PM Jakub Jelinek wrote: > On Tue, Apr 09, 2019 at 09:06:33AM +0200, Jakub Jelinek wrote: > > Alternatively, I believe we could remove from the patch the in-place > > replacement of CASE_LABEL_EXPRs with LABEL_EXPRs if we want to remove, > > just splay_tree_remove those, because by my reading of > > preprocess_case_label_vec_for_gimple we also ignore the fully out of range > > CASE_LABEL_EXPRs there, shall I tweak the patch and rebootstrap? > > Here is a version of the patch that doesn't overwrite those CASE_LABEL_EXPRs > as gimplifier handles those, just removes them from the splay tree. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2019-04-09 Jakub Jelinek > > PR c/89888 > * c-common.h (c_add_case_label): Remove orig_type and outside_range_p > arguments. > (c_do_switch_warnings): Remove outside_range_p argument. > * c-common.c (check_case_bounds): Removed. > (c_add_case_label): Remove orig_type and outside_range_p arguments. > Don't call check_case_bounds. Fold low_value as well as high_value. > * c-warn.c (c_do_switch_warnings): Remove outside_range_p argument. > Check for case labels outside of range of original type here and > adjust them. > c/ > * c-typeck.c (struct c_switch): Remove outside_range_p member. > (c_start_case): Don't clear it. > (do_case): Adjust c_add_case_label caller. > (c_finish_case): Adjust c_do_switch_warnings caller. > cp/ > * decl.c (struct cp_switch): Remove outside_range_p member. > (push_switch): Don't clear it. > (pop_switch): Adjust c_do_switch_warnings caller. > (finish_case_label): Adjust c_add_case_label caller. > + node = splay_tree_predecessor (cases, (splay_tree_key) min_value); ... > + if (CASE_HIGH ((tree) node->value) > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > + min_value) >= 0) ... > + node = splay_tree_predecessor (cases, > +(splay_tree_key) min_value); > + node = splay_tree_lookup (cases, (splay_tree_key) max_value); > + if (node == NULL) > + node = splay_tree_predecessor (cases, (splay_tree_key) max_value); > + /* Handle a single node that might partially overlap the range. */ > + if (node > + && node->key > + && CASE_HIGH ((tree) node->value) > + && tree_int_cst_compare (CASE_HIGH ((tree) node->value), > + max_value) > 0) ... > + while ((node = splay_tree_successor (cases, > + (splay_tree_key) max_value)) Hmm, why are the code sections for dealing with the lower bound and upper bound asymmetrical? That is, why not start the upper bound consideration with splay_tree_successor? Jason
[C/C++ PATCH] Fix promoted switch condition out of range diagnostics (PR c/89888, take 2)
On Tue, Apr 09, 2019 at 09:06:33AM +0200, Jakub Jelinek wrote:
> Alternatively, I believe we could remove from the patch the in-place
> replacement of CASE_LABEL_EXPRs with LABEL_EXPRs if we want to remove,
> just splay_tree_remove those, because by my reading of
> preprocess_case_label_vec_for_gimple we also ignore the fully out of range
> CASE_LABEL_EXPRs there, shall I tweak the patch and rebootstrap?
Here is a version of the patch that doesn't overwrite those CASE_LABEL_EXPRs
as gimplifier handles those, just removes them from the splay tree.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2019-04-09 Jakub Jelinek
PR c/89888
* c-common.h (c_add_case_label): Remove orig_type and outside_range_p
arguments.
(c_do_switch_warnings): Remove outside_range_p argument.
* c-common.c (check_case_bounds): Removed.
(c_add_case_label): Remove orig_type and outside_range_p arguments.
Don't call check_case_bounds. Fold low_value as well as high_value.
* c-warn.c (c_do_switch_warnings): Remove outside_range_p argument.
Check for case labels outside of range of original type here and
adjust them.
c/
* c-typeck.c (struct c_switch): Remove outside_range_p member.
(c_start_case): Don't clear it.
(do_case): Adjust c_add_case_label caller.
(c_finish_case): Adjust c_do_switch_warnings caller.
cp/
* decl.c (struct cp_switch): Remove outside_range_p member.
(push_switch): Don't clear it.
(pop_switch): Adjust c_do_switch_warnings caller.
(finish_case_label): Adjust c_add_case_label caller.
testsuite/
* c-c++-common/pr89888.c: New test.
* g++.dg/torture/pr40335.C: Change dg-bogus into dg-warning.
Don't expect -Wswitch-unreachable warning.
--- gcc/c-family/c-common.h.jj 2019-03-20 12:24:57.320308115 +0100
+++ gcc/c-family/c-common.h 2019-04-08 19:02:28.522104090 +0200
@@ -988,8 +988,7 @@ extern tree boolean_increment (enum tree
extern int case_compare (splay_tree_key, splay_tree_key);
-extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree, tree,
- bool *);
+extern tree c_add_case_label (location_t, splay_tree, tree, tree, tree);
extern bool c_switch_covers_all_cases_p (splay_tree, tree);
extern tree build_function_call (location_t, tree, tree);
@@ -1291,8 +1290,7 @@ extern void sizeof_pointer_memaccess_war
bool (*) (tree, tree));
extern void check_main_parameter_types (tree decl);
extern void warnings_for_convert_and_check (location_t, tree, tree, tree);
-extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool,
- bool);
+extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool);
extern void warn_for_omitted_condop (location_t, tree);
extern bool warn_for_restrict (unsigned, tree *, unsigned);
extern void warn_for_address_or_pointer_of_packed_member (tree, tree);
--- gcc/c-family/c-common.c.jj 2019-03-26 08:52:54.938604825 +0100
+++ gcc/c-family/c-common.c 2019-04-09 01:28:01.199754481 +0200
@@ -314,8 +314,6 @@ const struct fname_var_t fname_vars[] =
struct visibility_flags visibility_options;
static tree check_case_value (location_t, tree);
-static bool check_case_bounds (location_t, tree, tree, tree *, tree *,
- bool *);
static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -2103,86 +2101,6 @@ check_case_value (location_t loc, tree v
return value;
}
-/* See if the case values LOW and HIGH are in the range of the original
- type (i.e. before the default conversion to int) of the switch testing
- expression.
- TYPE is the promoted type of the testing expression, and ORIG_TYPE is
- the type before promoting it. CASE_LOW_P is a pointer to the lower
- bound of the case label, and CASE_HIGH_P is the upper bound or NULL
- if the case is not a case range.
- The caller has to make sure that we are not called with NULL for
- CASE_LOW_P (i.e. the default case). OUTSIDE_RANGE_P says whether there
- was a case value that doesn't fit into the range of the ORIG_TYPE.
- Returns true if the case label is in range of ORIG_TYPE (saturated or
- untouched) or false if the label is out of range. */
-
-static bool
-check_case_bounds (location_t loc, tree type, tree orig_type,
- tree *case_low_p, tree *case_high_p,
- bool *outside_range_p)
-{
- tree min_value, max_value;
- tree case_low = *case_low_p;
- tree case_high = case_high_p ? *case_high_p : case_low;
-
- /* If there was a problem with the original type, do nothing. */
- if (orig_type == error_mark_node)
-return true;
-
- min_value = TYPE_MIN_VALUE (orig_type);
- max_value = TYPE_MAX_VALUE (orig_type);
-
- /* We'll really need integer constants here. */
- case_low = fold (case_low);
-
