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); -