| Message ID | 20131120090429.GT30563@lug-owl.de |
|---|---|
| State | New |
| Headers | show |
Jan-Benedict Glaw <jbglaw@lug-owl.de> writes: > 2013-11-20 Jan-Benedict Glaw <jbglaw@lug-owl.de> > > * config/mips/mips.c (r10k_simplify_address): Eliminate macro usage. OK. And thanks for the catch. Richard
On Wed, Nov 20, 2013 at 10:04 AM, Jan-Benedict Glaw wrote: > 2013-11-20 Jan-Benedict Glaw <...> > > * config/mips/mips.c (r10k_simplify_address): Eliminate macro usage. > > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > index 82ca719..d06d574 100644 > --- a/gcc/config/mips/mips.c > +++ b/gcc/config/mips/mips.c > @@ -14842,7 +14842,7 @@ r10k_simplify_address (rtx x, rtx insn) > /* Replace the incoming value of $sp with > virtual_incoming_args_rtx. */ > if (x == stack_pointer_rtx > - && DF_REF_BB (def) == ENTRY_BLOCK_PTR) > + && DF_REF_BB (def) == ENTRY_BLOCK_PTR_FOR_FN (cfun)) > newx = virtual_incoming_args_rtx; > } > else if (dominated_by_p (CDI_DOMINATORS, DF_REF_BB (use), > > > Ok? > > MfG, JBG This patch is obvious and it fixes breakage. Please go ahead and commit it. I wonder if there are any more cases like this missed... Could you please check that? Something like: egrep -w "ENTRY_BLOCK_PTR|EXIT_BLOCK_PTR" gcc/*.[ch] gcc/config/*.[ch] gcc/config/*/*.{c,h,md} should do the trick. (I'd do it myself if I had access to a Linux box right now...) Ciao! Steven
On Wed, 2013-11-20 10:08:45 +0100, Steven Bosscher <stevenb.gcc@gmail.com> wrote: [...] > I wonder if there are any more cases like this missed... Could you > please check that? Something like: > > egrep -w "ENTRY_BLOCK_PTR|EXIT_BLOCK_PTR" gcc/*.[ch] gcc/config/*.[ch] > gcc/config/*/*.{c,h,md} No more uses, but 21 comment lines contain references to the macros. MfG, JBG
Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR
On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote:
> This patch is obvious and it fixes breakage. Please go ahead and commit it.
Sorry to pick on you here Steven, but this doesn't meet gcc's
definition of an obvious patch. Don't believe me? See
http://gcc.gnu.org/svnwrite.html#policies
Allowed as obvious in the gcc sources are typo fixes for comments or
similar, or reverting a bad patch you made. That's it. The power to
change anything else is reserved to the relevant maintainer.
Last I checked, you're not a MIPS maintainer.. Am I rebuking you?
No, not at all! You just gave me a perfect lead in for the
following.. :)
/rant
It's utterly ridiculous that gcc doesn't have a reasonable obvious
patch rule. Only comments? In that all non-maintainers can be
trusted with? What a poor lot of contributors we have.
Should a maintainer not even be able to authorise simple patches out
of their area as Steven just did? That's what a strict interpretation
of the current rules in MAINTAINERS plus the current obvious patch
rule implies. Or does the obvious patch rule just apply to
non-maintainers?
/no-rant
Can I recommend gdb's obvious patch policy? It even tickles my sense
of humour. "will the person who hates my work the most be able to
find fault with the change" - if so, then it's not obvious..
On Tue, Nov 26, 2013 at 6:17 AM, Alan Modra wrote: > Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR > On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: >> This patch is obvious and it fixes breakage. Please go ahead and commit it. > > Sorry to pick on you here Steven, but this doesn't meet gcc's > definition of an obvious patch. Don't believe me? See > http://gcc.gnu.org/svnwrite.html#policies Hmm.... I guess the patch will have to be reverted, then :-) Or maybe this would be under the banner of "We don't want to get overly anal-retentive about checkin policies." In any case, it's not unprecedented that obviously obvious patches get checked in even if they're not obvious according to that policy. To list a few from just this month: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02989.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02975.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02970.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02972.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02496.html http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02331.html So perhaps the policy should include a line about fixing trivial breakage from recent check-ins. Ciao! Steven
On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> wrote: > Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR > On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: >> This patch is obvious and it fixes breakage. Please go ahead and commit it. > > Sorry to pick on you here Steven, but this doesn't meet gcc's > definition of an obvious patch. Don't believe me? See > http://gcc.gnu.org/svnwrite.html#policies > > Allowed as obvious in the gcc sources are typo fixes for comments or > similar, or reverting a bad patch you made. That's it. The power to > change anything else is reserved to the relevant maintainer. Huh. That's silly. It allows nothing interesting! > Can I recommend gdb's obvious patch policy? It even tickles my sense > of humour. "will the person who hates my work the most be able to > find fault with the change" - if so, then it's not obvious.. I like this one much better. Anyone else opposed to changing the obvious-commit policy to something along these lines? Diego.
On 11/26/13 08:21, Diego Novillo wrote: > On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> wrote: >> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR >> On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: >>> This patch is obvious and it fixes breakage. Please go ahead and commit it. >> >> Sorry to pick on you here Steven, but this doesn't meet gcc's >> definition of an obvious patch. Don't believe me? See >> http://gcc.gnu.org/svnwrite.html#policies >> >> Allowed as obvious in the gcc sources are typo fixes for comments or >> similar, or reverting a bad patch you made. That's it. The power to >> change anything else is reserved to the relevant maintainer. > > Huh. That's silly. It allows nothing interesting! As I've stated within the last few months, I'm certainly open to revisiting that policy. I believe we put that policy in place in circa 1998 as we started up egcs. > >> Can I recommend gdb's obvious patch policy? It even tickles my sense >> of humour. "will the person who hates my work the most be able to >> find fault with the change" - if so, then it's not obvious.. > > I like this one much better. Anyone else opposed to changing the > obvious-commit policy to something along these lines? Seems reasonable to me. jeff
> -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > owner@gcc.gnu.org] On Behalf Of Jeff Law > Sent: Tuesday, November 26, 2013 11:31 AM > To: Diego Novillo; Steven Bosscher; gcc-patches > Subject: Re: gcc's obvious patch policy > > On 11/26/13 08:21, Diego Novillo wrote: > > On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> > wrote: > >> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On > >> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: > >>> This patch is obvious and it fixes breakage. Please go ahead and commit > it. > >> > >> Sorry to pick on you here Steven, but this doesn't meet gcc's > >> definition of an obvious patch. Don't believe me? See > >> http://gcc.gnu.org/svnwrite.html#policies > >> > >> Allowed as obvious in the gcc sources are typo fixes for comments or > >> similar, or reverting a bad patch you made. That's it. The power to > >> change anything else is reserved to the relevant maintainer. > > > > Huh. That's silly. It allows nothing interesting! > As I've stated within the last few months, I'm certainly open to revisiting that > policy. I believe we put that policy in place in circa > 1998 as we started up egcs. > > > > >> Can I recommend gdb's obvious patch policy? It even tickles my sense > >> of humour. "will the person who hates my work the most be able to > >> find fault with the change" - if so, then it's not obvious.. > > > > I like this one much better. Anyone else opposed to changing the > > obvious-commit policy to something along these lines? > Seems reasonable to me. > Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in. Thanks, Balaji V. Iyer. > jeff
> Can I make a suggestion that if someone is making an "obvious" change (with > the exception of changing non-working code (comments, things inside #if 0, > etc)), have a patch on the mailing list for 12-24 hrs. before putting it > in? Maybe they could say something like, I will check this in by X time > <TIMEZONE> tomorrow since this looks obvious to me. This way if the change > hurts someone who is working on a feature in their local machine that is > using the existing framework can chime in. I disagree, obvious patches cannot reasonably invalidate the work of others, or else they are simply not obvious. At worst they can privatize a public function or remove an unused one, but then it's easy to undo that later.
On Tue, Nov 26, 2013 at 05:14:22PM +0000, Iyer, Balaji V wrote: > > > > -----Original Message----- > > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- > > owner@gcc.gnu.org] On Behalf Of Jeff Law > > Sent: Tuesday, November 26, 2013 11:31 AM > > To: Diego Novillo; Steven Bosscher; gcc-patches > > Subject: Re: gcc's obvious patch policy > > > > On 11/26/13 08:21, Diego Novillo wrote: > > > On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> > > wrote: > > >> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On > > >> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: > > >>> This patch is obvious and it fixes breakage. Please go ahead and commit > > it. > > >> > > >> Sorry to pick on you here Steven, but this doesn't meet gcc's > > >> definition of an obvious patch. Don't believe me? See > > >> http://gcc.gnu.org/svnwrite.html#policies > > >> > > >> Allowed as obvious in the gcc sources are typo fixes for comments or > > >> similar, or reverting a bad patch you made. That's it. The power to > > >> change anything else is reserved to the relevant maintainer. > > > > > > Huh. That's silly. It allows nothing interesting! > > As I've stated within the last few months, I'm certainly open to revisiting that > > policy. I believe we put that policy in place in circa > > 1998 as we started up egcs. > > > > > > > >> Can I recommend gdb's obvious patch policy? It even tickles my sense > > >> of humour. "will the person who hates my work the most be able to > > >> find fault with the change" - if so, then it's not obvious.. > > > > > > I like this one much better. Anyone else opposed to changing the > > > obvious-commit policy to something along these lines? > > Seems reasonable to me. > > > > Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in. > This would seem to exclude the most useful type of obvious fixes, trivial changes which are currently preventing a bootstrap. What benefit does waiting 24 hours to add a missing 'unsigned' give? Surely If the change were likely to impact someone else in a non-trivial way, it cannot be considered an obvious change and thus, this rule would not apply. James
On 26/11/13 17:14, Iyer, Balaji V wrote: > > >> -----Original Message----- >> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches- >> owner@gcc.gnu.org] On Behalf Of Jeff Law >> Sent: Tuesday, November 26, 2013 11:31 AM >> To: Diego Novillo; Steven Bosscher; gcc-patches >> Subject: Re: gcc's obvious patch policy >> >> On 11/26/13 08:21, Diego Novillo wrote: >>> On Tue, Nov 26, 2013 at 12:17 AM, Alan Modra <amodra@gmail.com> >> wrote: >>>> Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR On >>>> Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: >>>>> This patch is obvious and it fixes breakage. Please go ahead and commit >> it. >>>> >>>> Sorry to pick on you here Steven, but this doesn't meet gcc's >>>> definition of an obvious patch. Don't believe me? See >>>> http://gcc.gnu.org/svnwrite.html#policies >>>> >>>> Allowed as obvious in the gcc sources are typo fixes for comments or >>>> similar, or reverting a bad patch you made. That's it. The power to >>>> change anything else is reserved to the relevant maintainer. >>> >>> Huh. That's silly. It allows nothing interesting! >> As I've stated within the last few months, I'm certainly open to revisiting that >> policy. I believe we put that policy in place in circa >> 1998 as we started up egcs. >> >>> >>>> Can I recommend gdb's obvious patch policy? It even tickles my sense >>>> of humour. "will the person who hates my work the most be able to >>>> find fault with the change" - if so, then it's not obvious.. >>> >>> I like this one much better. Anyone else opposed to changing the >>> obvious-commit policy to something along these lines? >> Seems reasonable to me. >> > > Can I make a suggestion that if someone is making an "obvious" change (with the exception of changing non-working code (comments, things inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. before putting it in? Maybe they could say something like, I will check this in by X time <TIMEZONE> tomorrow since this looks obvious to me. This way if the change hurts someone who is working on a feature in their local machine that is using the existing framework can chime in. > There may be times when that is undesirable. For example, the obvious fix to something that prevents the compiler from building. I think we have enough checks and balances in place. Anyone repeatedly/gratuitously abusing the obvious rule is likely to lose their commit bit pretty quickly. We're a community that works by co-operation, so lets co-operate as much as possible. R.
> -----Original Message----- > From: Eric Botcazou [mailto:ebotcazou@adacore.com] > Sent: Tuesday, November 26, 2013 12:33 PM > To: Iyer, Balaji V > Cc: gcc-patches@gcc.gnu.org; Diego Novillo; Jeff Law; Steven Bosscher > Subject: Re: gcc's obvious patch policy > > > Can I make a suggestion that if someone is making an "obvious" change > > (with the exception of changing non-working code (comments, things > > inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. > > before putting it in? Maybe they could say something like, I will > > check this in by X time <TIMEZONE> tomorrow since this looks obvious > > to me. This way if the change hurts someone who is working on a > > feature in their local machine that is using the existing framework can > chime in. > > I disagree, obvious patches cannot reasonably invalidate the work of others, > or else they are simply not obvious. At worst they can privatize a public > function or remove an unused one, but then it's easy to undo that later. > Those at worst examples you have mentioned is the ones that scare me. Sometimes when a function becomes private, making it public back again is sometimes an uphill battle. I am not saying they shouldn't commit it, but at least give a heads-up. This being said, I am Ok with either decision. > -- > Eric Botcazou
On Tue, Nov 26, 2013 at 12:40 PM, Iyer, Balaji V <balaji.v.iyer@intel.com> wrote: > > >> -----Original Message----- >> From: Eric Botcazou [mailto:ebotcazou@adacore.com] >> Sent: Tuesday, November 26, 2013 12:33 PM >> To: Iyer, Balaji V >> Cc: gcc-patches@gcc.gnu.org; Diego Novillo; Jeff Law; Steven Bosscher >> Subject: Re: gcc's obvious patch policy >> >> > Can I make a suggestion that if someone is making an "obvious" change >> > (with the exception of changing non-working code (comments, things >> > inside #if 0, etc)), have a patch on the mailing list for 12-24 hrs. >> > before putting it in? Maybe they could say something like, I will >> > check this in by X time <TIMEZONE> tomorrow since this looks obvious >> > to me. This way if the change hurts someone who is working on a >> > feature in their local machine that is using the existing framework can >> chime in. >> >> I disagree, obvious patches cannot reasonably invalidate the work of others, >> or else they are simply not obvious. At worst they can privatize a public >> function or remove an unused one, but then it's easy to undo that later. >> > > Those at worst examples you have mentioned is the ones that scare me. Sometimes when a function becomes private, making it public back > again is sometimes an uphill battle. I am not saying they shouldn't commit it, but at least give a heads-up. Nah. Simple patches like that are always easy to pinpoint and address afterwards. Obvious patches will always be on the small side, after all. Diego.
On Nov 25, 2013, at 9:17 PM, Alan Modra <amodra@gmail.com> wrote: > Was Re: [buildrobot] [PATCH] mips: Really remove ENTRY_BLOCK_PTR > On Wed, Nov 20, 2013 at 10:08:45AM +0100, Steven Bosscher wrote: >> This patch is obvious and it fixes breakage. Please go ahead and commit it. > > Sorry to pick on you here Steven, but this doesn't meet gcc's > definition of an obvious patch. Don't believe me? No. I don't, let me quote from the policy: We don't want to get overly anal-retentive about checkin policies. Nit picking the fine lines from a lawyer perspective as to the exact meaning of the word is, is best suited for lawyers and presidents. We are above that around here. We work through co-operation and a shared desire to push gcc in the direction of being the best it can be. The more rules one writes about what is and is not acceptable and the more capriciousness one uses when applying those rules, the worse off I think we are. I don't favor that direction. Merely mentioning that a fix that is checked in to resolve a build issue doesn't meet your understanding of the letter of the law, I think is already takes us in a non-desirable direction. You discourage lots of people by making them have second thoughts about checking in any fix to any build issue. I personally favor a compiler that builds. If you have a substantive disagreement as to the exact formation of the patch that was checked in under the obvious rule, I think the right approach is to point out the issues that you have with the patch and if you feel strongly enough that the source tree is better without it, to ask for the person to revert it (or fix it to more your liking). In the end, we can trust that a reviewer will settle any disagreements. They can approve the patch as is, insist that it be removed, explain how it should be fixed instead… If we change the text of the policy, I think we should resist the temptation to make it more strict. We can explain that patches checked in under the obvious rule are free to be post-checkin reviewed by a reviewer and that review can include any/all of the usual review responses and can include a please revert. If anything, we should list more categories of what obvious is, or say, including, but not limited to, so that people don't form an overly restrictive view of the policy as you have done. A example of a category not listed is unbreaking the build. > It's utterly ridiculous that gcc doesn't have a reasonable obvious > patch rule. Maybe we already do, and you never knew it. :-) I already have a reasonable obvious patch rule. > Only comments? In the parts I review for, the rule isn't restricted to comments. The history of gcc is replete with examples that are non-comment changes under the obvious rule and the maintainers of those areas that think, at least in part, as I do as well.
>> Sorry to pick on you here Steven, but this doesn't meet gcc's >> definition of an obvious patch. Don't believe me? > No. I don't, let me quote from the policy: I find this whole thread a rather sad and pathetic bikeshed discussion. Regardless of the formal policy, the basic concept is to use common sense. Common sense about the context of the code being changed, common sense about the patch itself, and common sense about the maintenance area and the maintainers. Anything more than that is people trying to create / change rules as a stick to hit each other over the head or a straight jacket to tie each other up. Thanks, David
To me the issue is not what is written down about the policy, but whether the policy works in practice, and it seems like it does, so what's the problem? This just seems to be making a problem where none exists.
On Tue, Nov 26, 2013 at 04:30:50PM -0500, David Edelsohn wrote: > >> Sorry to pick on you here Steven, but this doesn't meet gcc's > >> definition of an obvious patch. Don't believe me? > > > No. I don't, let me quote from the policy: > > I find this whole thread a rather sad and pathetic bikeshed > discussion. Regardless of the formal policy, the basic concept is to > use common sense. Common sense about the context of the code being > changed, common sense about the patch itself, and common sense about > the maintenance area and the maintainers. > > Anything more than that is people trying to create / change rules as a > stick to hit each other over the head or a straight jacket to tie each > other up. I find this a bit rich coming from you, David. On the weekend I committed a patch as obvious, for which you "hit me over the head", stating in no uncertain terms that I should not bypass you and commit patches like that as "obvious". I still think the substance of the patch was obvious for anyone who has worked on the powerpc backend for as long as I have, but after some discussion I backed down because technically, you were within your rights and I had transgressed the rules. You have the stick *now*. And wield it. I'm trying to take it away from you..
On Tue, Nov 26, 2013 at 04:56:26PM -0500, Robert Dewar wrote: > To me the issue is not what is written down about > the policy, but whether the policy works in practice, > and it seems like it does, so what's the problem? > > This just seems to be making a problem where > none exists. I gave some background in my email to David over why I'm stirring the pot here. The thing about written policy is that it sets the tone for a project. A restrictive policy tends to authoritarian rule by maintainers, it seems to me. A less restricive policy ought to ease some of the nonsense that goes on currently, for instance, port maintainers thinking they need to get global maintainer permission for trivial patches outside their area of responsibility. As an example, for http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02793.html I was told I'd need global maintainer approval.. Well, maybe I do in the current climate. I hope I haven't offended the review gods too much here. I'm sure other people have noticed the issues I'm raising but have more wisely than I, kept quiet.
> The thing about written policy is that it sets the tone for a project. > A restrictive policy tends to authoritarian rule by maintainers, it > seems to me. And a too little restrictive policy runs the risk of creating a feeling that the rules aren't necessarily to be taken too seriously. Neither outcome is good.
On Tue, Nov 26, 2013 at 8:37 PM, Alan Modra <amodra@gmail.com> wrote: >> I find this whole thread a rather sad and pathetic bikeshed >> discussion. Regardless of the formal policy, the basic concept is to >> use common sense. Common sense about the context of the code being >> changed, common sense about the patch itself, and common sense about >> the maintenance area and the maintainers. >> >> Anything more than that is people trying to create / change rules as a >> stick to hit each other over the head or a straight jacket to tie each >> other up. > > I find this a bit rich coming from you, David. On the weekend I > committed a patch as obvious, for which you "hit me over the head", > stating in no uncertain terms that I should not bypass you and commit > patches like that as "obvious". I still think the substance of the > patch was obvious for anyone who has worked on the powerpc backend for > as long as I have, but after some discussion I backed down because > technically, you were within your rights and I had transgressed the > rules. > > You have the stick *now*. And wield it. I'm trying to take it away > from you.. Alan, I privately asked you not to commit obvious patches to the port because there was no reason to rush the patch as "obvious". There are a lot of changes happening to the port from many directions and I am trying to prevent unintended conflict from destabilizing the port. You're the one turning this into a public issue and making it personal. - David
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 82ca719..d06d574 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -14842,7 +14842,7 @@ r10k_simplify_address (rtx x, rtx insn) /* Replace the incoming value of $sp with virtual_incoming_args_rtx. */ if (x == stack_pointer_rtx - && DF_REF_BB (def) == ENTRY_BLOCK_PTR) + && DF_REF_BB (def) == ENTRY_BLOCK_PTR_FOR_FN (cfun)) newx = virtual_incoming_args_rtx; } else if (dominated_by_p (CDI_DOMINATORS, DF_REF_BB (use),