code review 61370043: cmd/5g: fix print format in peephole debugging

cmd/5g: fix print format in peephole debugging

Fixes issue 7294.

[ality, 2014-02-10 02:16:00]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[minux1, 2014-02-10 02:16:43]

LGTM.

[dave_cheney.net, 2014-02-10 02:25:48]

Nice. Thanks for the quick fix. Is it possible to have a testing test/fixedbugs/
?

[ality, 2014-02-10 02:45:33]

dave@cheney.net once said:
> Nice. Thanks for the quick fix.

I have lib9/fmt fresh in my mind and this failure mode is
easy to spot. A crash in one of the print routines is very
likely due to a bad format verb.

> Is it possible to have a testing test/fixedbugs/ ?

Not easily. There doesn't seem to be way to give compiler
options to test/run.go.

  Anthony

[dave_cheney.net, 2014-02-10 02:51:37]

Does

// $G -P $D/$F.go

Still work ?

On Mon, Feb 10, 2014 at 1:45 PM, Anthony Martin <ality@pbrane.org> wrote:

> dave@cheney.net once said:
> > Nice. Thanks for the quick fix.
>
> I have lib9/fmt fresh in my mind and this failure mode is
> easy to spot. A crash in one of the print routines is very
> likely due to a bad format verb.
>
> > Is it possible to have a testing test/fixedbugs/ ?
>
> Not easily. There doesn't seem to be way to give compiler
> options to test/run.go.
>
>   Anthony
>

[ality, 2014-02-10 03:23:14]

Dave Cheney <dave@cheney.net> once said:
> Does
> 
> // $G -P $D/$F.go
> 
> Still work ?

Yes but only when manually running the test/run shell
script. Does anyone do that? I forgot it even existed.

I can add a test if you think it's worth it. This bug
was just a result of poor search and replace.

  Anthony

[minux1, 2014-02-10 03:49:59]

On Feb 9, 2014 10:23 PM, "Anthony Martin" <ality@pbrane.org> wrote:
>
> Dave Cheney <dave@cheney.net> once said:
> > Does
> >
> > // $G -P $D/$F.go
> >
> > Still work ?
>
> Yes but only when manually running the test/run shell
> script. Does anyone do that? I forgot it even existed.
>
> I can add a test if you think it's worth it. This bug
> was just a result of poor search and replace.
I don't think this worth a test.

-S is much important than this, yet even -S doesn't have a test, and it has
been broken in the past.

[dave_cheney.net, 2014-02-10 03:59:44]

Yeah, good point. 

LGTM. 

If you wanted to open a bug to add a test for -S and -P that would also be good.


> On 10 Feb 2014, at 14:49, minux <minux.ma@gmail.com> wrote:
> 
> 
> On Feb 9, 2014 10:23 PM, "Anthony Martin" <ality@pbrane.org> wrote:
> >
> > Dave Cheney <dave@cheney.net> once said:
> > > Does
> > >
> > > // $G -P $D/$F.go
> > >
> > > Still work ?
> >
> > Yes but only when manually running the test/run shell
> > script. Does anyone do that? I forgot it even existed.
> >
> > I can add a test if you think it's worth it. This bug
> > was just a result of poor search and replace.
> I don't think this worth a test.
> 
> -S is much important than this, yet even -S doesn't have a test, and it has
been broken in the past.

[minux1, 2014-02-10 04:05:22]

On Feb 9, 2014 10:59 PM, "Dave Cheney" <dave@cheney.net> wrote:
> Yeah, good point.
>
> If you wanted to open a bug to add a test for -S and -P that would also
be good.
when we migrated the toolchain to Go, all these will have dedicated tests.

[ality, 2014-02-10 09:19:34]

I usually try to wait for a LGTM from someone at Google before
submitting. Is it always necessary, even for small fixes like
this? (Are the rules written down anywhere?)

Thanks,
  Anthony

[bradfitz, 2014-02-10 16:03:49]

The rules aren't written down or even well-defined.

Feel free to submit once you get an LGTM from somebody who seems to
know/own/co-own the area. But if it's contentious or large, wait until
everybody appears haply. There's no rule about Googlers blessing things,
but it so happens that Google people are the "owners" (primary authors,
etc) of much of the code.
 On Feb 10, 2014 1:19 AM, <ality@pbrane.org> wrote:

> I usually try to wait for a LGTM from someone at Google before
> submitting. Is it always necessary, even for small fixes like
> this? (Are the rules written down anywhere?)
>
> Thanks,
>   Anthony
>
> https://codereview.appspot.com/61370043/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[dave_cheney.net, 2014-02-12 23:27:27]

On 2014/02/10 16:03:49, bradfitz wrote:
> The rules aren't written down or even well-defined.
> 
> Feel free to submit once you get an LGTM from somebody who seems to
> know/own/co-own the area. But if it's contentious or large, wait until
> everybody appears haply. There's no rule about Googlers blessing things,
> but it so happens that Google people are the "owners" (primary authors,
> etc) of much of the code.
>  On Feb 10, 2014 1:19 AM, <mailto:ality@pbrane.org> wrote:
> 
> > I usually try to wait for a LGTM from someone at Google before
> > submitting. Is it always necessary, even for small fixes like
> > this? (Are the rules written down anywhere?)
> >
> > Thanks,
> >   Anthony
> >
> > https://codereview.appspot.com/61370043/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >

Ality, I think you can submit this one. It's not contentious, is the correct
fix, and has been reviewed by several people now.

[ality, 2014-02-13 01:03:36]

*** Submitted as https://code.google.com/p/go/source/detail?r=51d9b0cf3b8d ***

cmd/5g: fix print format in peephole debugging

Fixes issue 7294.

LGTM=minux.ma, dave, bradfitz
R=golang-codereviews, minux.ma, dave, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/61370043

[rsc, 2014-02-13 03:57:27]

Why have the Plan 9 format checks not found the %.4ud mismatches that I
just fixed in src/cmd/?g/peep.c?

[0intro, 2014-02-13 07:38:58]

> Why have the Plan 9 format checks not found the %.4ud mismatches
> that I just fixed in src/cmd/?g/peep.c?

It worked. The CL 49170043 "cmd/cc, cmd/gc, cmd/ld: consolidate
print format routines" replaced prog->loc by prog->pc.

Just after this change was submitted, the Plan 9 builder reported:

warning: src/cmd/8g/reg.c:1142 format mismatch .4ud VLONG, arg 2

Then you fixed it in CL 62730043 "cmd/5g: fix regopt bug in copyprop".

See http://build.golang.org/log/35375fa705d6c1d0a97ced23f096dd81e8a2a99b

-- 
David du Colombier