This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: GCC 5 Status Report (2015-01-19), Trunk in Stage 4


On Tue, Jan 27, 2015 at 10:12:15AM +0100, Richard Biener wrote:
> On Tue, 27 Jan 2015, Jakub Jelinek wrote:
> > On Tue, Jan 27, 2015 at 10:04:38AM +0100, Richard Biener wrote:
> > > On Tue, 27 Jan 2015, Andreas Krebbel wrote:
> > > > [PATCH] S/390: -mhotpatch v2
> > > > https://gcc.gnu.org/ml/gcc-patches/2015-01/msg02370.html
> > > > 
> > > > It is a backend only change to our existing -mhotpatch feature
> > > > requested by the Linux kernel guys for the ftrace implementation:
> > > > https://lkml.org/lkml/2015/1/26/320
> > > > 
> > > > They need it in an upstream GCC asap. If we don't get it into 5.0 we
> > > > probably would need to commit it onto 5.1 branch right after the
> > > > release. I would rather try to avoid this since it would make the
> > > > hotpatch feature incompatible between 5.0 and 5.1.

> > > Did you consider using an alternate option name instead of changing
> > > it in an incompatible way?  I realize SUSE will need to backport this
> > 
> > Yeah, the option incompatibility worries me.  Can't -mhotpatch without =
> > stand for the old behavior?  Does it map to some -mhotpatch=X,Y value,
> > or is it not worth to support both?
> 
> It maps to -mhotpatch=12.

The new expression for the old behaviour would be -mhotpatch=12,2
(or attribute((hotpatch(12,2))), and -mno-notpatch is expressed as
-mhotpatch=0,0 now.

> The old one had one argument while the new
> one has two...  so eventually you can distinguish them this way,
> though for the inlining I'd have added -minline-hotpatched and
> if you switch the arguments of the new hotpatch then -mhotpatch=12
> would trivially become supported again...

The reasons for the incompatible changes are:

 - With the old code it was really weird that you requested n
   halfwords to be patched before the label and silently got two
   halfwords patched after the label too.

 - The interaction between inlining and hotpatching was really
   awkward to code and led to behaviour hard to understand for the
   user.  Furthermore hotpatching is now compatible with
   always_inline (which generated an error in the old code).

 - With the simplification to the interaction between inlining and
   hotpatching (needed by the kernel), the new code would only look
   the same on the command line but actually do something else.

 - The old code had already three options (-mhotpatch,
   -mno-hotpatch and -mhotpatch=) and adding another one to define
   the size to be patched after the label would have made an even
   bigger mess of the already bad usability.

 - The user who requested that feature does not use it yet and
   will get the updated patch with the new semantics before he
   starts to use it.

 - We believe that nobody has tried to use the old semantics
   (except the colleague next door who requested the new
   functionality for use in the kernel).

To sum it up, given that we don't expect any current users, making
an incompatible change and thus avoiding the extra hassle in
favour of a clean design seemed acceptable.  It would certainly be
possible to keep the interfaces compatible, but we think it is not
worth the effort.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]