On Tue, May 7, 2019 at 11:55 PM Jeff Law <l...@redhat.com> wrote:
>
> On 5/7/19 3:45 AM, Richard Biener wrote:
> > On Tue, May 7, 2019 at 11:13 AM Aldy Hernandez <al...@redhat.com> wrote:
> >>
> >> Hi.
> >>
> >> We seem to have numerous copies of the same EH propagation cleanups
> >> scattered throughout the compiler. The attached patch moves all the
> >> logic into one class that allows for easy marking of statements and
> >> automatic cleanup once it goes out of scope.
> >>
> >> Tested on x86-64 Linux.
> >>
> >> OK for trunk? (*)
> >
> > Ugh :/
> >
> > First of all I don't like the fact that the actual cleanup is done
> > upon constructor execution. Please make it explicit
> > and in the constructor assert that nothing is to be done.
> I'm of a mixed mind here. I have railed against implicit code being run
> behind my back for decades.
>
> However as I've had to debug locking issues and the like in other C++
> codebases I've become more and more of a fan of RAII and its basic
> concepts. This has made me more open to code running behind my back
> like this implicitly when the object gets destroyed.
>
> There's something to be said for embedding this little class into other
> objects like Aldy has done and just letting things clean up
> automatically as the object goes out of scope. No more missing calls to
> run the cleanup bits, it "just works".
>
> But I won't object if you want it to be more explicit. I've been there
> and understand why one might want the cleanup step to be explicit.
>
>
>
> >
> > Then I'm not sure this is a 1:1 transform since for example
> >
> > @@ -1061,8 +1173,6 @@
> > substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
> > }
> >
> > gimple *old_stmt = stmt;
> > - bool was_noreturn = (is_gimple_call (stmt)
> > - && gimple_call_noreturn_p (stmt));
> >
> > /* Replace real uses in the statement. */
> > did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
> > @@ -1110,25 +1220,7 @@
> > substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
> > /* Now cleanup. */
> > if (did_replace)
> > {
> > ...
> > + fixups.record_change (old_stmt, stmt);
> >
> > here we no longer can reliably determine whether old_stmt was noreturn since
> > we substitute into stmt itself. It's no longer a correctness issue if
> > we do _not_
> > fixup_noreturn since we now have GF_CALL_CTRL_ALTERING, it's merely
> > an optimization issue. So there may be no testcase for this (previously
> > such
> > cases ICEd).
> But AFAICT we don't care in the case Aldy is changing. If we really
> want to know if the old statement was a noreturn we can test prior to
> queing it.
The code isn't doing what it did before after the change. That's a bug.
To be more explicit here - adding this kind of new and fancy C++ stuff
just for the sake of it, thereby adding yet _another_ way of handling these
things instead of forcing it a new way (remove the other APIs this
encapsulates) is very bad(TM) IMHO, both for maintainance and for
new people coming around trying to understand GCC.
Not to say that all the places that are refactored with this patch
look sligtly different and thus the pattern doesnt' exactly match
(leading to issues like the one I pointed out above).
So - please no.
Richard.
>
> >
> > I'm also not sure I like to put all these (unrelated) things into a
> > single class,
> > it really also hides the details of what is performed immediately and what
> > delayed and what kind of changes - this makes understanding of pass
> > transforms hard.
> On the other hand this class defines a contract for what it can and will
> do for us and allows us to bring consistency in that handling. We
> declare manual management of this stuff verboten. Ideally we'd declare
> the class final and avoid derivation, but I doubt we can do that
> immediately.
>
> Jeff
