https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104702
--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> (In reply to Jakub Jelinek from comment #2)
> > I think the problem in this case are primarily the warning-control.cc
> > changes.
> > In GCC 11, build_vec_init did at the end:
> > /* Now make the result have the correct type. */
> > if (TREE_CODE (atype) == ARRAY_TYPE)
> > {
> > atype = build_pointer_type (atype);
> > stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
> > stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
> > TREE_NO_WARNING (stmt_expr) = 1;
> > }
> > GCC 12 does instead:
> > if (TREE_CODE (atype) == ARRAY_TYPE)
> > {
> > atype = build_pointer_type (atype);
> > stmt_expr = build1 (NOP_EXPR, atype, stmt_expr);
> > stmt_expr = cp_build_fold_indirect_ref (stmt_expr);
> > suppress_warning (stmt_expr /* What warning? */);
> > }
> > Clearly, at least one of the intended warnings that should be suppressed is
> > OPT_Wunused_value.
> > But as EXPR_LOCATION (stmt_expr) == UNKNOWN_LOCATION here, all
> > suppress_warning
> > does is set TREE_NO_WARNING bit on it (i.e. like GCC 11 used).
> > Later on, add_stmt is called on it and does:
> > if (!EXPR_HAS_LOCATION (t))
> > SET_EXPR_LOCATION (t, input_location);
> > There was also even before build_vec_init a call to
> > suppress_warning with some unrelated expression that had the
> > 284032 {file = 0x3f67310 "pr104702.C", line = 11, column = 31, data = 0x0,
> > sysp = false}
> > location (with OPT_Wparentheses) which is the input_location now newly set
> > by add_stmt on the INDIRECT_REF.
> > Later on, convert_to_void calls
> > && !warning_suppressed_p (expr, OPT_Wunused_value)
> > on the INDIRECT_REF. TREE_NO_WARNING bit is set, but at this point it has a
> > location, so it looks up the location in hash map, finds that
> > OPT_Wparantheses
> > is the warning to be suppressed there and returns that OPT_Wunused_value
> > isn't suppressed there, even when we wanted to suppress all warnings on the
> > tree.
>
> So shouldn't warning_suppressed_p (tree, ...) also honor TREE_NO_WARNING
> even if a location is present? That is,
>
> diff --git a/gcc/warning-control.cc b/gcc/warning-control.cc
> index 0cbb4f079fa..d36f3ff6965 100644
> --- a/gcc/warning-control.cc
> +++ b/gcc/warning-control.cc
> @@ -120,10 +120,12 @@ get_nowarn_spec (const gimple *stmt)
> bool
> warning_suppressed_p (const_tree expr, opt_code opt /* = all_warnings */)
> {
> - const nowarn_spec_t *spec = get_nowarn_spec (expr);
> + if (get_no_warning_bit (expr))
> + return true;
>
> + const nowarn_spec_t *spec = get_nowarn_spec (expr);
> if (!spec)
> - return get_no_warning_bit (expr);
> + return false;
>
> const nowarn_spec_t optspec (opt);
> bool dis = *spec & optspec;
>
> or, if that's not desired, at least
>
> gcc_checking_assert (!get_no_warning_bit (expr));
>
> when there's a spec?
That would basically throw away all the warning-control.cc stuff, get us back
to
the TREE_NO_WARNING/gimple_no_warning behavior. Because the TREE_NO_WARNING
/gimple_no_warning bits are used as a flag whether at least one warning is
disabled.
Anyway, if we keep moreless the current warning-control.cc behavior, it is
unclear what should the FE call when it does:
if (!EXPR_HAS_LOCATION (t))
SET_EXPR_LOCATION (t, input_location);
Shall it do
if (!EXPR_HAS_LOCATION (t) && input_location != UNKNOWN_LOCATION)
{
if (TREE_NO_WARNING (t))
copy_warning (input_location, UNKNOWN_LOCATION);
SET_EXPR_LOCATION (t, input_location);
}
or so or some function that will do roughly that?
The copy_warning in gimple.h looks just wrong to me:
static inline void
gimple_set_location (gimple *g, location_t location)
{
/* Copy the no-warning data to the statement location. */
copy_warning (location, g->location);
g->location = location;
}
If I have some random statement which doesn't have any warning suppression
for it (i.e. !gimple_no_warning_p) and I set the location for it to some real
location, all of sudden that location will have suppressions of "all warnings"
if it is ever used somewhere else where warnings are suppressed.
Shouldn't that do:
if (__builtin_expect (g->no_warning, 0))
copy_warning (location, g->location);
instead?
Shouldn't we have some early return e.g. in the copy_warning (location_t,
location_t) if the locations are the same?
