code review 6005053: fmt: change to work with Go

fmt: change to work with Go

Escape analysis code in Go requires that we
restore fmt flags after a fmtprint/fmtvprint.
This change aligns fmtprint and fmtvprint
with http://codereview.appspot.com/5129057.

[akumar, 2012-04-11 19:18:37]

Hello nix-dev@googlegroups.com (cc: rminnich@gmail.com, john@jfloren.net,
0intro@gmail.com),

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

[quanstro, 2012-04-11 22:04:51]

how have you tested this?

- erik

[nemo_lsub.org, 2012-04-11 22:56:13]

Before accepting things because "they are required for go", I'd like to
see a working go port.

I understand changes have to be made to make it work, but, before we know the
port works, I think that "required for go" is not enough (at least for me) to
lgtm a change.

Of course, if the change improves things, that's a different issue.

--
using ipad keyboard. excuse any typos.


On Apr 11, 2012, at 9:18 PM, seed@mail.nanosouffle.net wrote:

> Reviewers: nix-dev_googlegroups.com,
> 
> Message:
> Hello nix-dev@googlegroups.com (cc: rminnich@gmail.com,
> john@jfloren.net, 0intro@gmail.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/nix-os/
> 
> 
> Description:
> fmt: change to work with Go
> 
> Escape analysis code in Go requires that we
> restore fmt flags after a fmtprint/fmtvprint.
> This change aligns fmtprint and fmtvprint
> with http://codereview.appspot.com/5129057.
> 
> Please review this at http://codereview.appspot.com/6005053/
> 
> Affected files:
>  M sys/src/libc/fmt/fmtprint.c
>  M sys/src/libc/fmt/fmtvprint.c
> 
> 
> Index: sys/src/libc/fmt/fmtprint.c
> ===================================================================
> --- a/sys/src/libc/fmt/fmtprint.c
> +++ b/sys/src/libc/fmt/fmtprint.c
> @@ -14,19 +14,9 @@
>    va_list va;
>    int n;
> 
> -    f->flags = 0;
> -    f->width = 0;
> -    f->prec = 0;
> -    va = f->args;
> -    va_start(f->args, fmt);
> -    n = dofmt(f, fmt);
> -    va_end(f->args);
> -    f->flags = 0;
> -    f->width = 0;
> -    f->prec = 0;
> -    f->args = va;
> -    if(n >= 0)
> -        return 0;
> +    va_start(va, fmt);
> +    n = fmtvprint(f, fmt, va);
> +    va_end(va);
>    return n;
> }
> 
> Index: sys/src/libc/fmt/fmtvprint.c
> ===================================================================
> --- a/sys/src/libc/fmt/fmtvprint.c
> +++ b/sys/src/libc/fmt/fmtvprint.c
> @@ -12,18 +12,19 @@
> fmtvprint(Fmt *f, char *fmt, va_list args)
> {
>    va_list va;
> -    int n;
> +    int n, w, p;
> +    unsigned long fl;
> 
> -    f->flags = 0;
> -    f->width = 0;
> -    f->prec = 0;
> +    w = f->width;
> +    p = f->prec;
> +    fl = f->flags;
>    va = f->args;
>    f->args = args;
>    n = dofmt(f, fmt);
> -    f->flags = 0;
> -    f->width = 0;
> -    f->prec = 0;
>    f->args = va;
> +    f->width = w;
> +    f->prec = p;
> +    f->flags = fl;
>    if(n >= 0)
>        return 0;
>    return n;
> 

[quanstro, 2012-04-11 23:03:17]

On Wed Apr 11 18:56:22 EDT 2012, nemo@lsub.org wrote:
> Before accepting things because "they are required for go", I'd like to
> see a working go port.
> 
> I understand changes have to be made to make it work, but, before we know the
> port works, I think that "required for go" is not enough (at least for me) to
lgtm a change.
> 
> Of course, if the change improves things, that's a different issue.

+1.

- erik

[ality, 2012-04-12 13:41:21]

Francisco J Ballesteros <nemo@lsub.org> once said:
> Before accepting things because "they are required for go", I'd like to
> see a working go port.

I've just updated my repo with all pending CLs
that make Go work on Nix, including Akshat's 
tsemacquire changes.  You can get it here:

https://bitbucket.org/ality/go

  Anthony

P.S.

If you want to build on vx32 you'll need to patch
in this diff (relative to Ron's repo):

diff -r 17a064eed9c2 src/libvx32/emu.c
--- a/src/libvx32/emu.c	Mon May 30 10:09:22 2011 +0200
+++ b/src/libvx32/emu.c	Thu Apr 12 06:40:13 2012 -0700
@@ -744,6 +744,7 @@
 		switch (*inp++) {
 
 		// No-operand insns
+		case 0x90: // PAUSE (not really a REP instruction)
 		case 0xa4: case 0xa5: case 0xa6: case 0xa7:	// MOVS, CMPS
 		case 0xaa: case 0xab:				// STOS
 		case 0xac: case 0xad: case 0xae: case 0xaf:	// LODS, SCAS
@@ -813,6 +814,8 @@
 		// No additional operand
 		case 0xc8: case 0xc9: case 0xca: case 0xcb:	// BSWAP
 		case 0xcc: case 0xcd: case 0xce: case 0xcf:
+		case 0x31: // RDTSC (this should be emulated)
+		case 0x77: // EMMS
 			goto notrans;
 
 		// General EA operands
@@ -830,6 +833,7 @@
 		case 0x54: case 0x55: case 0x56: case 0x57:	// ANDPS etc.
 		case 0x58: case 0x59: case 0x5a: case 0x5b:	// ADDPS etc.
 		case 0x5c: case 0x5d: case 0x5e: case 0x5f:	// SUBPS etc.
+		case 0x6f: case 0x7f:		// MOVQ
 		case 0xa3:					// BT Ev,Gv
 		case 0xab:					// BTS Ev,Gv
 		case 0xaf:					// IMUL Gv,Ev
@@ -857,6 +861,7 @@
 		case 0x17:				// MOVHPS Mq,Vps
 		case 0x2b:				// MOVNTPS
 		case 0xc3:				// MOVNTI Md,Gd
+		case 0xc7:				// CMPXCHG8B Mq
 			if (EA_MOD(*inp) == 3)	// Mem-only
 				goto invalid;
 			inp = xscan_rm(inp);

  Anthony

[akumar, 2012-04-12 18:06:04]

Hello nix-dev@googlegroups.com, quanstro@quanstro.net, nemo@lsub.org,
ality@pbrane.org (cc: rminnich@gmail.com, john@jfloren.net, 0intro@gmail.com),

Please take another look.

[quanstro, 2012-04-12 20:11:51]

still would like to see the zeroing of flags, width, prec
happen just once in _fmtdispatch.

http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall
File sys/man/2/fmtinstall (left):

http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall#oldcode269
sys/man/2/fmtinstall:269: However, these functions clear the width, precision,
and flags.
how about "will start with fresh ..."?

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint.c
File sys/src/ape/lib/fmt/fmtprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint....
sys/src/ape/lib/fmt/fmtprint.c:32: n = fmtvprint(f, fmt, va);
just call dofmt

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint.c
File sys/src/ape/lib/fmt/fmtvprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint...
sys/src/ape/lib/fmt/fmtvprint.c:34: fl = f->flags;
move this to _fmtdispatch

http://codereview.appspot.com/6005053/diff/7008/sys/src/libc/fmt/fmtvprint.c
File sys/src/libc/fmt/fmtvprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/libc/fmt/fmtvprint.c#...
sys/src/libc/fmt/fmtvprint.c:19: fl = f->flags;
move to _fmtdispatch

[akumar, 2012-04-12 22:03:41]

PTAL.

http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall
File sys/man/2/fmtinstall (left):

http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall#oldcode269
sys/man/2/fmtinstall:269: However, these functions clear the width, precision,
and flags.
On 2012/04/12 20:11:51, quanstro wrote:
> how about "will start with fresh ..."?
Or, "These functions will preserve width, precision, and flags."

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint.c
File sys/src/ape/lib/fmt/fmtprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint....
sys/src/ape/lib/fmt/fmtprint.c:32: n = fmtvprint(f, fmt, va);
On 2012/04/12 20:11:51, quanstro wrote:
> just call dofmt
Then we have to push/pop f->args and check the return value of dofmt; same stuff
that fmtvprint already does. Should we avoid code duplication here, or not?

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint.c
File sys/src/ape/lib/fmt/fmtvprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint...
sys/src/ape/lib/fmt/fmtvprint.c:34: fl = f->flags;
On 2012/04/12 20:11:51, quanstro wrote:
> move this to _fmtdispatch
Done.

http://codereview.appspot.com/6005053/diff/7008/sys/src/libc/fmt/fmtvprint.c
File sys/src/libc/fmt/fmtvprint.c (right):

http://codereview.appspot.com/6005053/diff/7008/sys/src/libc/fmt/fmtvprint.c#...
sys/src/libc/fmt/fmtvprint.c:19: fl = f->flags;
On 2012/04/12 20:11:51, quanstro wrote:
> move to _fmtdispatch

Done.

[quanstro, 2012-04-12 22:08:27]

On Thu Apr 12 18:03:49 EDT 2012, seed@mail.nanosouffle.net wrote:
> PTAL.
> 
> 
> http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall
> File sys/man/2/fmtinstall (left):
> 
>
http://codereview.appspot.com/6005053/diff/7008/sys/man/2/fmtinstall#oldcode269
> sys/man/2/fmtinstall:269: However, these functions clear the width,
> precision, and flags.
> On 2012/04/12 20:11:51, quanstro wrote:
> > how about "will start with fresh ..."?
> Or, "These functions will preserve width, precision, and flags."

that sounds very good.

> http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint.c
> File sys/src/ape/lib/fmt/fmtprint.c (right):
> 
>
http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint....
> sys/src/ape/lib/fmt/fmtprint.c:32: n = fmtvprint(f, fmt, va);
> On 2012/04/12 20:11:51, quanstro wrote:
> > just call dofmt
> Then we have to push/pop f->args and check the return value of dofmt;
> same stuff that fmtvprint already does. Should we avoid code duplication
> here, or not?

since you moved the flag saving & restoring to _fmtdispatch,
isn't all this extra code in fmtvprint redundant?

> 
>
http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint.c
> File sys/src/ape/lib/fmt/fmtvprint.c (right):
> 
>
http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtvprint...
> sys/src/ape/lib/fmt/fmtvprint.c:34: fl = f->flags;
> On 2012/04/12 20:11:51, quanstro wrote:
> > move this to _fmtdispatch
> Done.

- erik

[akumar, 2012-04-12 22:13:41]

On 12 April 2012 15:07, erik quanstrom <quanstro@quanstro.net> wrote:
>> Or, "These functions will preserve width, precision, and flags."
>
> that sounds very good.

Will resubmit with this, then (didn't add it yet).

>>
http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint.c
>>
http://codereview.appspot.com/6005053/diff/7008/sys/src/ape/lib/fmt/fmtprint....
>> sys/src/ape/lib/fmt/fmtprint.c:32: n = fmtvprint(f, fmt, va);
>> On 2012/04/12 20:11:51, quanstro wrote:
>> > just call dofmt
>> Then we have to push/pop f->args and check the return value of dofmt;
>> same stuff that fmtvprint already does. Should we avoid code duplication
>> here, or not?
>
> since you moved the flag saving & restoring to _fmtdispatch,
> isn't all this extra code in fmtvprint redundant?

In fmtvprint? I just submitted a new patchset (#6), which removes
the pushing/popping of width, prec, and flags. Would you rather
that f->args was saved in the fmtdispatch code as well?


Thanks,
Akshat

[quanstro, 2012-04-12 22:32:07]

great.  can you recompile your system and live with it
for a bit.  if you don't find any unexpected gotchas,
then lgtm.

- erik

[akumar, 2012-04-12 22:37:14]

Hello nix-dev@googlegroups.com, quanstro@quanstro.net, nemo@lsub.org,
ality@pbrane.org (cc: rminnich@gmail.com, john@jfloren.net, 0intro@gmail.com),

Please take another look.

[0intro, 2012-04-13 13:45:53]

LGTM