On Fri, 25 Feb 2022, Jakub Jelinek wrote:
> On Fri, Feb 25, 2022 at 08:11:40AM +0100, Richard Biener via Gcc-patches
> wrote:
> > On Thu, 24 Feb 2022, Qing Zhao wrote:
> >
> > > I briefly checked all the usages of suppress_warning within the current
> > > gcc,
> > > and see that most of them are not guarded by any condition.
> > >
> > > So, the current change should be fine without introducing new issues. -:)
> > >
> > > Another thing is, if we use “warning_enable_at” to guard, I just checked,
> > > this routine is not used by any routine right now, so it might be
> > > possible that this
> > > routine has some bug itself. And now it’s the stage 4, we might need to
> > > be
> > > conservative.
> > >
> > > Based on this, I think that it might be better to put the change in as it
> > > right now.
> > > If we think that all the “suppress_warning” need to be guarded by a
> > > specific
> > > condition, we can do it in gcc13 earlier stage.
> > >
> > > What’s your opinion?
> >
> > I would agree here. Maybe a compromise would be to simply
> > set gimple_set_no_warning () on the actual stmt. That shouldn't
>
> I think it is set_no_warning_bit but it isn't exported from
> warning-control.cc.
I meant
/* Set the no_warning flag of STMT to NO_WARNING. */
static inline void
gimple_set_no_warning (gimple *stmt, bool no_warning)
{
stmt->no_warning = (unsigned) no_warning;
}
on the artificial stmt.
> One can still use TREE_NO_WARNING though, but seems Martin converted
> all usages of that to the suppress_warning APIs.
Yep, which is why I didn't suggest that. Note that we don't have
a great location to use - the other patch suggests DECL_LOCATION
which will then silence all other uninit warnings on the decl
which would be IMHO bad. Since the load is artificial we won't
ever have a more specific location for that unless there's a way
to invent it. Thus my suggestion to set the no-warning bit
on the GIMPLE stmt itself ...
Richard.
> I thought the hash tables warning-control.cc use are based on the gimple *
> and tree pointers, but apparently they aren't, they are location_t based.
> So if we keep using suppress_warning (tmp_dst, OPT_Wuninitialized);
> there, it actually means just that one entry will be added to the hash table
> because all loads in one clear_padding_flush use the same buf->loc.
> It is true that every suppress_warning will need to look it up in the hash
> table.
> But you're right that because the tmp_dst we've created is artificial, I
> don't see a reason for any warning. Unfortunately, even with
> suppress_warning (tmp_dst, all_warnings); it looks up the hash tables
> and notes stuff in there rather than just making sure the TREE_NO_WARNING
> bit is set.
> So let's go with the unconditional
> suppress_warning (tmp_dst, OPT_Wuninitialized);
> for now.
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
