| Message ID | 59693cea-164e-bf51-d62a-096edcdb5218@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | v2 warning control by group and location (PR 74765) | expand |
Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571980.html Looking for a review of the LTO changes to switch TREE_NO_WARNING to the suppress_warning() API. On 6/4/21 3:43 PM, Martin Sebor wrote: > The attached patch replaces the uses of TREE_NO_WARNING in the LTO > front end with the new suppress_warning() API. It adds a couple of > FIXMEs that I plan to take care of in a follow up.
On 6/4/2021 3:43 PM, Martin Sebor via Gcc-patches wrote: > The attached patch replaces the uses of TREE_NO_WARNING in the LTO > front end with the new suppress_warning() API. It adds a couple of > FIXMEs that I plan to take care of in a follow up. > > gcc-no-warning-lto.diff > > Add support for per-location warning groups. > > gcc/lto/ChangeLog: > > * gimple-streamer-out.c (output_gimple_stmt): Same. > * lto-common.c (compare_tree_sccs_1): Expand use of TREE_NO_WARNING. > * lto-streamer-out.c (hash_tree): Same. > * tree-streamer-in.c (unpack_ts_base_value_fields): Same. > * tree-streamer-out.c (pack_ts_base_value_fields): Same. OK once prereqs are approved. jeff
On Mon, Jun 21, 2021 at 11:55 PM Martin Sebor via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571980.html > > Looking for a review of the LTO changes to switch TREE_NO_WARNING to > the suppress_warning() API. Hmm, since the warning suppressions are on location ad-hoc data the appropriate thing is to adjust the location streaming and that part seems to be missing? So what you now stream is simply the "everything" fallback, correct? In particular: else - bp_pack_value (bp, TREE_NO_WARNING (expr), 1); + /* FIXME: pack all warning bits. */ + bp_pack_value (bp, warning_suppressed_p (expr), 1); this looks like a wrong comment in that light. - else - compare_values (TREE_NO_WARNING); + else if (t1->base.nowarning_flag != t2->base.nowarning_flag) + return false; uh. Can you keep sth like TREE_NO_WARNING_RAW or so? Thanks, Richard. > On 6/4/21 3:43 PM, Martin Sebor wrote: > > The attached patch replaces the uses of TREE_NO_WARNING in the LTO > > front end with the new suppress_warning() API. It adds a couple of > > FIXMEs that I plan to take care of in a follow up. >
On 6/24/21 3:32 AM, Richard Biener wrote: > On Mon, Jun 21, 2021 at 11:55 PM Martin Sebor via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571980.html >> >> Looking for a review of the LTO changes to switch TREE_NO_WARNING to >> the suppress_warning() API. > > Hmm, since the warning suppressions are on location ad-hoc data the appropriate > thing is to adjust the location streaming and that part seems to be missing? > > So what you now stream is simply the "everything" fallback, correct? > > In particular: > > else > - bp_pack_value (bp, TREE_NO_WARNING (expr), 1); > + /* FIXME: pack all warning bits. */ > + bp_pack_value (bp, warning_suppressed_p (expr), 1); > > this looks like a wrong comment in that light. Yes, this is just a fallback. I haven't thought about how to handle the FIXME yet but from your comment it sounds like this code might stay the same (or maybe even go back to streaming the flag directly) and the nowarn_spec_t bitmap should be streamed elsewhere? > > - else > - compare_values (TREE_NO_WARNING); > + else if (t1->base.nowarning_flag != t2->base.nowarning_flag) > + return false; > > uh. Can you keep sth like TREE_NO_WARNING_RAW or so? The flag is used directly in fold-const.c and cp/module.cc so this would be in keeping with that, but I also don't mind adding a macro for it. My only concern is with macro getting used to inadvertently bypass the API. Martin > > Thanks, > Richard. > >> On 6/4/21 3:43 PM, Martin Sebor wrote: >>> The attached patch replaces the uses of TREE_NO_WARNING in the LTO >>> front end with the new suppress_warning() API. It adds a couple of >>> FIXMEs that I plan to take care of in a follow up. >>
On Thu, Jun 24, 2021 at 5:27 PM Martin Sebor <msebor@gmail.com> wrote: > > On 6/24/21 3:32 AM, Richard Biener wrote: > > On Mon, Jun 21, 2021 at 11:55 PM Martin Sebor via Gcc-patches > > <gcc-patches@gcc.gnu.org> wrote: > >> > >> Ping: https://gcc.gnu.org/pipermail/gcc-patches/2021-June/571980.html > >> > >> Looking for a review of the LTO changes to switch TREE_NO_WARNING to > >> the suppress_warning() API. > > > > Hmm, since the warning suppressions are on location ad-hoc data the appropriate > > thing is to adjust the location streaming and that part seems to be missing? > > > > So what you now stream is simply the "everything" fallback, correct? > > > > In particular: > > > > else > > - bp_pack_value (bp, TREE_NO_WARNING (expr), 1); > > + /* FIXME: pack all warning bits. */ > > + bp_pack_value (bp, warning_suppressed_p (expr), 1); > > > > this looks like a wrong comment in that light. > > Yes, this is just a fallback. I haven't thought about how to handle > the FIXME yet but from your comment it sounds like this code might > stay the same (or maybe even go back to streaming the flag directly) > and the nowarn_spec_t bitmap should be streamed elsewhere? Yes, since the bitmap is per location it should be streamed alongside locations in lto_{input,output}_location. > > > > - else > > - compare_values (TREE_NO_WARNING); > > + else if (t1->base.nowarning_flag != t2->base.nowarning_flag) > > + return false; > > > > uh. Can you keep sth like TREE_NO_WARNING_RAW or so? > > The flag is used directly in fold-const.c and cp/module.cc so > this would be in keeping with that, but I also don't mind adding > a macro for it. My only concern is with macro getting used to > inadvertently bypass the API. There's precedent with other _RAW accessors, it should be clear that it bypasses accessors and thus review should easily spot inadverted uses. Richard. > Martin > > > > > Thanks, > > Richard. > > > >> On 6/4/21 3:43 PM, Martin Sebor wrote: > >>> The attached patch replaces the uses of TREE_NO_WARNING in the LTO > >>> front end with the new suppress_warning() API. It adds a couple of > >>> FIXMEs that I plan to take care of in a follow up. > >> >
Add support for per-location warning groups. gcc/lto/ChangeLog: * gimple-streamer-out.c (output_gimple_stmt): Same. * lto-common.c (compare_tree_sccs_1): Expand use of TREE_NO_WARNING. * lto-streamer-out.c (hash_tree): Same. * tree-streamer-in.c (unpack_ts_base_value_fields): Same. * tree-streamer-out.c (pack_ts_base_value_fields): Same. diff --git a/gcc/gimple-streamer-out.c b/gcc/gimple-streamer-out.c index fcbf92300d4..7f7e06a79b8 100644 --- a/gcc/gimple-streamer-out.c +++ b/gcc/gimple-streamer-out.c @@ -73,7 +73,7 @@ output_gimple_stmt (struct output_block *ob, struct function *fn, gimple *stmt) /* Emit the tuple header. */ bp = bitpack_create (ob->main_stream); bp_pack_var_len_unsigned (&bp, gimple_num_ops (stmt)); - bp_pack_value (&bp, gimple_no_warning_p (stmt), 1); + bp_pack_value (&bp, warning_suppressed_p (stmt), 1); if (is_gimple_assign (stmt)) bp_pack_value (&bp, gimple_assign_nontemporal_move_p ( diff --git a/gcc/lto-streamer-out.c b/gcc/lto-streamer-out.c index a26d4885800..f1809e60c1e 100644 --- a/gcc/lto-streamer-out.c +++ b/gcc/lto-streamer-out.c @@ -1207,7 +1207,7 @@ hash_tree (struct streamer_tree_cache_d *cache, hash_map<tree, hashval_t> *map, if (TYPE_P (t)) hstate.add_flag (TYPE_ARTIFICIAL (t)); else - hstate.add_flag (TREE_NO_WARNING (t)); + hstate.add_flag (warning_suppressed_p (t)); hstate.add_flag (TREE_NOTHROW (t)); hstate.add_flag (TREE_STATIC (t)); hstate.add_flag (TREE_PROTECTED (t)); diff --git a/gcc/lto/lto-common.c b/gcc/lto/lto-common.c index bfe52a2e942..9e7ea877e66 100644 --- a/gcc/lto/lto-common.c +++ b/gcc/lto/lto-common.c @@ -1110,8 +1110,8 @@ compare_tree_sccs_1 (tree t1, tree t2, tree **map) compare_values (TYPE_UNSIGNED); if (TYPE_P (t1)) compare_values (TYPE_ARTIFICIAL); - else - compare_values (TREE_NO_WARNING); + else if (t1->base.nowarning_flag != t2->base.nowarning_flag) + return false; compare_values (TREE_NOTHROW); compare_values (TREE_STATIC); if (code != TREE_BINFO) diff --git a/gcc/tree-streamer-in.c b/gcc/tree-streamer-in.c index e0522bf2ac1..31dbf2fb992 100644 --- a/gcc/tree-streamer-in.c +++ b/gcc/tree-streamer-in.c @@ -131,7 +131,8 @@ unpack_ts_base_value_fields (struct bitpack_d *bp, tree expr) if (TYPE_P (expr)) TYPE_ARTIFICIAL (expr) = (unsigned) bp_unpack_value (bp, 1); else - TREE_NO_WARNING (expr) = (unsigned) bp_unpack_value (bp, 1); + /* FIXME: set all warning bits. */ + suppress_warning (expr, N_OPTS, (unsigned) bp_unpack_value (bp, 1)); TREE_NOTHROW (expr) = (unsigned) bp_unpack_value (bp, 1); TREE_STATIC (expr) = (unsigned) bp_unpack_value (bp, 1); if (TREE_CODE (expr) != TREE_BINFO) diff --git a/gcc/tree-streamer-out.c b/gcc/tree-streamer-out.c index 855d1cd59b9..b76e0c59c6f 100644 --- a/gcc/tree-streamer-out.c +++ b/gcc/tree-streamer-out.c @@ -104,7 +104,8 @@ pack_ts_base_value_fields (struct bitpack_d *bp, tree expr) if (TYPE_P (expr)) bp_pack_value (bp, TYPE_ARTIFICIAL (expr), 1); else - bp_pack_value (bp, TREE_NO_WARNING (expr), 1); + /* FIXME: pack all warning bits. */ + bp_pack_value (bp, warning_suppressed_p (expr), 1); bp_pack_value (bp, TREE_NOTHROW (expr), 1); bp_pack_value (bp, TREE_STATIC (expr), 1); if (TREE_CODE (expr) != TREE_BINFO)