code review 6005053: fmt: change to work with Go (issue 6005053)

[se...@mail.nanosouffle.net]

Reviewers: nix-dev_googlegroups.com,

Message:
Hello nix...@googlegroups.com (cc: rmin...@gmail.com,
jo...@jfloren.net, 0in...@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;

[David du Colombier]

LGTM

[erik quanstrom]

how have you tested this?

- erik

[Francisco J Ballesteros]

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.

[Charles Forsyth]

I think it would be ok when changes have explanations that justified them more generally. This change

saves and restores various format settings, which might generally be good (but how?),

yet is explained as "Escape analysis code in Go requires that we restore fmt flags after a fmtprint/fmtvprint."
which I didn't understand. (I'd ordinarily suppose "escape analysis" had to do with the lifetime of references and values!

Or Alcatraz, except that's shut. Here it might be the % formats? I don't know.)

[erik quanstrom]

On Wed Apr 11 18:56:22 EDT 2012, ne...@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

[erik quanstrom]

On Wed Apr 11 19:02:50 EDT 2012, charles...@gmail.com wrote:

> I think it would be ok when changes have explanations that justified them
> more generally. This change
> saves and restores various format settings, which might generally be good
> (but how?),

sorry to reply twice.

changing fmtvprint from starting with no width, prec or flags to
inheriting the flags is a big change, and i think it's likely to break
existing code. i think we should be very wary of breaking existing
code.

- erik

[ron minnich]

On Wed, Apr 11, 2012 at 4:07 PM, erik quanstrom <quan...@quanstro.net> wrote:

> changing fmtvprint from starting with no width, prec or flags to
> inheriting the flags is a big change, and i think it's likely to break
> existing code.  i think we should be very wary of breaking existing
> code.

The process works! This fix can not be used as is, and we'll go back
to the drawing board :-)

ron

[Francisco J Ballesteros]

On Apr 12, 2012, at 1:14 AM, ron minnich wrote:

The process works! This fix can not be used as is, and we'll go back
to the drawing board :-)

I'm not that sure.

Noticed how many submits in the last days?

Can you guess why?

[David du Colombier]

> how have you tested this?

I verified it solves the problem we had on Go, then
fully recompiled one of my file servers and
checked everything was behaving fine, especially
the programs who use fmtprint and fmtvprint.
But I'm sure more testing should be done.

This change is originating from here:

http://golang.org/src/lib9/fmt/fmtprint.c
http://golang.org/src/lib9/fmt/fmtvprint.c

And initially happened in this CL :

https://codereview.appspot.com/5129057/

That said, the fact it did look good to me
doesn't mean it should look good for everyone here,
and and even less the solution is correct.

I trust you guys all to make your own test and
confirm the non-regression of this change.

For those who want to try Go, you can checkout
the current repository and apply the CL 5998048 [1].
Plan 9 requires the tsemacquire patch (part of NIX, or
available here [2]) and this current CL.

[1] https://codereview.appspot.com/5998048/
[2] http://www.9legacy.org/9legacy/patch/tsemacquire.diff

--
David du Colombier

[Anthony Martin]

Francisco J Ballesteros <ne...@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

[Anthony Martin]

It doesn't inherit them since __fmtdispatch always
clears them. This change simply saves them before
the call to dofmt and restores them afterwards.

This was changed in the Go repo so that the compiler
can print Node pointers using a recursive formatting
function.¹

This changeset LGTM.

Cheers,
Anthony

1. https://code.google.com/p/go/source/browse/src/cmd/gc/fmt.c#1055

[Charles Forsyth]

ooh! an instruction new to me. i shall use it everywhere now.

[erik quanstrom]

On Thu Apr 12 10:24:56 EDT 2012, charles...@gmail.com wrote:

> ooh! an instruction new to me. i shall use it everywhere now.
>
> On 12 April 2012 14:41, Anthony Martin <al...@pbrane.org> wrote:
>

> > + case 0x90: // PAUSE (not really a REP instruction)

rofl.

- erik

[erik quanstrom]

> It doesn't inherit them since __fmtdispatch always
> clears them. This change simply saves them before
> the call to dofmt and restores them afterwards.

do you mean _fmtdispatch? __fmtdispatch is ape only.

you're correct. _fmtdispatch does that. convoluted
to have it this way.

but, i would think if you want to go this way, _fmtdispatch
should do this work, and to clean things up Fmt
should use an unnamed structure Fmtcall as the
per-call data.

struct Fmtcall {
int width;
int prec;
uint flags;
}

then you can have in _fmtdispatch,
Fmtcall callsave;

...
callsave = f.Fmtcall;
f.Fmtcall = Zfmtcall;
n = (*fmtfmt(r))(f);
f.Fmtcall = callsave;

(fmtfmt isn't a function ptr even though the original code has
it that way.)

this sort of change would need to make it into the manual.

even for the more conservative fix here, the comment in fmtvprint
will be wrong with this change and we don't say "unsigned long".

also, if you're going to change the libc version, the ape version
must also be changed.

i would not lgtm this at this time.

- erik

[Anthony Martin]

erik quanstrom <quan...@quanstro.net> once said:

I've changed the comment. It now reads:

// PAUSE (one NOP wasn't good enough)

Anthony

[erik quanstrom]

> I've changed the comment. It now reads:
>
> // PAUSE (one NOP wasn't good enough)

one could argue that pause is useful but only in virtual machine
situations. it's supposed to be a hint that you're waiting for
a spin lock to clear. if one assumes another (virtual) machine
has it, pause is supposed to be your hint.

of course monitor and mwait seem more straightforward here.
they declare what they're trying to wait for a change in.
it would have been nice if they'd indicated a cacheline.

- erik

[Charles Forsyth]

hyperthreads use it too. it seems useful in the fairly fast stage of spinlocks.

[se...@mail.nanosouffle.net]

Hello nix...@googlegroups.com, quan...@quanstro.net, ne...@lsub.org,
al...@pbrane.org (cc: rmin...@gmail.com, jo...@jfloren.net,
0in...@gmail.com),

Please take another look.

http://codereview.appspot.com/6005053/

[erik quanstrom]

On Thu Apr 12 13:35:30 EDT 2012, charles...@gmail.com wrote:

> hyperthreads use it too. it seems useful in the fairly fast stage of
> spinlocks.

i didn't want to mention that because it's not clear that any
µarches past the p4 can really benefit from this, or if it's implemented.
what are the chances on a 32 core machine than your ht partner
is holding the lock? it seems unlikely that pause would cause something
to happen over a qpi/ht link.

- erik

[quan...@quanstro.net]

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.c#newcode32
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.c#newcode34
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#newcode19
sys/src/libc/fmt/fmtvprint.c:19: fl = f->flags;
move to _fmtdispatch

http://codereview.appspot.com/6005053/

[se...@mail.nanosouffle.net]

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.c#newcode32
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.c#newcode34
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#newcode19
sys/src/libc/fmt/fmtvprint.c:19: fl = f->flags;

On 2012/04/12 20:11:51, quanstro wrote:
> move to _fmtdispatch

Done.

http://codereview.appspot.com/6005053/

[erik quanstrom]

On Thu Apr 12 18:03:49 EDT 2012, se...@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.c#newcode32
> 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.c#newcode34
> 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

[se...@mail.nanosouffle.net]

On 12 April 2012 15:07, erik quanstrom <quan...@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.c#newcode32
>> 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

http://codereview.appspot.com/6005053/

[erik quanstrom]

here's what i was thinking for fmtdispatch (not compiled)

void*
_fmtdispatch(Fmt *f, void *fmt, int isrunes)
{
Rune rune, r;
int i, n;
long flags, width, prec;

for(;;){
if(isrunes){
r = *(Rune*)fmt;
fmt = (Rune*)fmt + 1;
}else{
fmt = (char*)fmt + chartorune(&rune, fmt);
r = rune;
}
f->r = r;
switch(r){
case '\0':
return nil;
case '.':
f->flags |= FmtWidth|FmtPrec;
continue;
case '0':
if(!(f->flags & FmtWidth)){
f->flags |= FmtZero;
continue;
}
/* fall through */
case '1': case '2': case '3': case '4':
case '5': case '6': case '7': case '8': case '9':
i = 0;
while(r >= '0' && r <= '9'){
i = i * 10 + r - '0';
if(isrunes){
r = *(Rune*)fmt;
fmt = (Rune*)fmt + 1;
}else{
r = *(char*)fmt;
fmt = (char*)fmt + 1;
}
}
if(isrunes)
fmt = (Rune*)fmt - 1;
else
fmt = (char*)fmt - 1;
numflag:
if(f->flags & FmtWidth){
f->flags |= FmtPrec;
f->prec = i;
}else{
f->flags |= FmtWidth;
f->width = i;
}
continue;
case '*':
i = va_arg(f->args, int);
if(i < 0){
/*
* negative precision =>
* ignore the precision.
*/
if(f->flags & FmtPrec){
f->flags &= ~FmtPrec;
f->prec = 0;
continue;
}
i = -i;
f->flags |= FmtLeft;
}
goto numflag;
}
flags = f->flags;
width = f->width;
prec = f->prec;
f->flags = 0;
f->width = 0;
f->prec = 0;

n = (*fmtfmt(r))(f);

f->flags = flags;
f->width = width;
f->prec = prec;

if(n < 0)
return nil;
if(n == 0)
return fmt;
}
}

- erik

[erik quanstrom]

and i did it wrong ... sorry.

void*
_fmtdispatch(Fmt *f, void *fmt, int isrunes)
{
Rune rune, r;
int i, n;
long flags, width, prec;

flags = f->flags;

width = f->width;
prec = f->prec;
f->flags = 0;
f->width = 0;
f->prec = 0;

for(;;){

if(isrunes){
r = *(Rune*)fmt;
fmt = (Rune*)fmt + 1;
}else{
fmt = (char*)fmt + chartorune(&rune, fmt);
r = rune;
}
f->r = r;
switch(r){
case '\0':

f->flags = flags;
f->width = width;
f->prec = prec;

n = (*fmtfmt(r))(f);

[Akshat Kumar]

Do you see something wrong with the
way that I have done it?

In your code, if n > 0, then the flags,
width, and precision may be off.

[Akshat Kumar]

(My patch for fmt.c is here:
http://codereview.appspot.com/6005053/patch/12002/10007

You can also check out the other
changes in the new patchset.)

[erik quanstrom]

you're right. i've got my head in another problem,
and i'm just confused. sorry.

- erik

[quan...@quanstro.net]

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

- erik

http://codereview.appspot.com/6005053/

[se...@mail.nanosouffle.net]

[Akshat Kumar]

This is the final version with the appropriate
change to the man page.

[0in...@gmail.com]

LGTM

https://codereview.appspot.com/6005053/

[erik quanstrom]

i'd like to hear verification that a full system
build with this change works as expected if only
because everything depends on print, and i can't
think of an easy way of grepping for wierd dependencies
in this case.

- erik

[John Floren]

I'll add: Akshat, could you also build a kernel with this and run your
laptop on it for a while?

John

[Akshat Kumar]

Booting as of now.