code review 108360043: cmd/5c, cmd/5g, cmd/5l, liblink: nacl/arm support

cmd/5c, cmd/5g, cmd/5l, liblink: nacl/arm support

[minux, 2014-07-02 00:30:47]

Hello rsc@golang.org, iant@golang.org (cc: golang-codereviews@googlegroups.com),

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

[dave_cheney.net, 2014-07-07 12:25:32]

LGTM.

[rsc, 2014-07-09 19:44:14]

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5c/peep.c
File src/cmd/5c/peep.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5c/peep.c#newcode856
src/cmd/5c/peep.c:856: if(nacl) return 0;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5c/peep.c#newcode862
src/cmd/5c/peep.c:862: if(nacl) return 0;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5c/reg.c
File src/cmd/5c/reg.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5c/reg.c#newcode207
src/cmd/5c/reg.c:207: switch(p->as) {
/* the mod/div runtime routines smash R12 */

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5g/peep.c
File src/cmd/5g/peep.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5g/peep.c#newcode878
src/cmd/5g/peep.c:878: if(nacl) return 0;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5g/peep.c#newcode884
src/cmd/5g/peep.c:884: if(nacl) return 0;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5g/reg.c
File src/cmd/5g/reg.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/cmd/5g/reg.c#newcode226
src/cmd/5g/reg.c:226: if(p->as == ADIV || p->as == ADIVU || p->as == AMOD ||
p->as == AMODU)
/* the div/mod runtime smashes R12 */

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c
File src/liblink/asm5.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:378: static int	asmoutnacl(Link *ctxt, int32 origPC, Prog *p,
Optab *o, int32 *out);
drop names of arguments, for consistency with surrounding declarations.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:452: static int asmoutnacl(Link *ctxt, int32 origPC, Prog *p,
Optab *o, int32 *out)
\n before asmoutnacl

static int
asmoutnacl...

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:454: int size, reg;
functions begin with declarations, then blank line, then initializations.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:458: out2 = out;
blank line before this one.
can out2 be deleted now?

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:460: int32 opc = p->pc; (void)opc;
move declaration up. leave declaration here.
remove (void)opc.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:461: // instruction specific
blank line before this comment

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:464: if(out != nil) asmout(ctxt, p, o, out);
two lines.
you might be better off always passing a non-nil out, and then al these checks
go away.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:469: if(out != nil) asmout(ctxt, p, o, out);
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:471: case AUNDEF: case APLD:
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:473: if(out != nil)
indent body (or make out always non-nil)

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:483: case AB: case ABL:
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:485: if(out != nil) asmout(ctxt, p, o, out2);
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:489: if((p->pc&15) == 12) p->pc += 4;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:501: static LSym *deferreturn;
make this a global static

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:511: case AMOVH: case AMOVHU: case AMOVHS:
multiple lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:542: case 0: p->pc += 4; break;
multiple lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:544: case 2: p->pc += 8; break;
if pc is okay at 12 mod 16, and pc is currently 8 mod 16, why are we adding 8
instead of 4?

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:550: } else {
no else after if that doesn't continue.
add a blank line instead.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:563: if(p->to.type == D_OREG) a2 = &q->to;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:564: else a2 = &q->from;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:588: }
add a blank line after ths

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:591: if(p->to.type == D_OREG) a = &p->to;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:592: else a = &p->from;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:600: if((p->pc&15) == 12) p->pc += 4;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:603: if(out != nil) asmout(ctxt, p, o, out2);
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:615: if(p->to.type == D_OREG) a2 = &q->to;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:616: else a2 = &q->from;
two lines

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:642: }
add a blank line after this

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:653: if(((p->pc+size) & 15) == 0) p->pc += 4;
two lines.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:688: // can't happen: blitrl is not nil, but checkpool
doesn't flushpool
s/doesn't/didn't/

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:741: int times = 0;
move decl to top of function

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:752: if(c > p->pc) p->pc = c;
two lines

[minux, 2014-07-10 05:03:35]

Hello rsc@golang.org, iant@golang.org, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[minux, 2014-07-10 05:05:39]

it seems rietveld lost the feature that when you
hg mail a CL, all pending comments will also be
sent out. Or am I mistaken and that is never a
feature at all?

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c
File src/liblink/asm5.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:460: int32 opc = p->pc; (void)opc;
On 2014/07/09 19:44:13, rsc wrote:
> move declaration up. leave declaration here.
> remove (void)opc.
actually opc is unused debugging code that i missed when
cleaning up.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:464: if(out != nil) asmout(ctxt, p, o, out);
On 2014/07/09 19:44:13, rsc wrote:
> you might be better off always passing a non-nil out, and then al these checks
> go away.
I tried, but 5g failed at compiling runtime and i spent some
time debugging the issue, but haven't found the bug.
when i wrote the code, i made the assumption that all but the
last call will have nil out.

Instead I will properly indent all the ifs.

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:511: case AMOVH: case AMOVHU: case AMOVHS:
On 2014/07/09 19:44:14, rsc wrote:
> multiple lines
I used lines to group related operations together.
do you really want me to use one case per line?

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:544: case 2: p->pc += 8; break;
On 2014/07/09 19:44:13, rsc wrote:
> if pc is okay at 12 mod 16, and pc is currently 8 mod 16, why are we adding 8
> instead of 4?
you're right. this is rare case when the comment is more correct than the code.

[rsc, 2014-07-10 18:33:46]

On 2014/07/10 05:05:39, minux wrote:
> it seems rietveld lost the feature that when you
> hg mail a CL, all pending comments will also be
> sent out. Or am I mistaken and that is never a
> feature at all?

I have never understood what causes that to happen. Sometimes it does, sometimes
it does not.

[rsc, 2014-07-10 18:36:11]

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c
File src/liblink/asm5.c (right):

https://codereview.appspot.com/108360043/diff/40001/src/liblink/asm5.c#newcod...
src/liblink/asm5.c:511: case AMOVH: case AMOVHU: case AMOVHS:
On 2014/07/10 05:05:38, minux wrote:
> On 2014/07/09 19:44:14, rsc wrote:
> > multiple lines
> I used lines to group related operations together.
> do you really want me to use one case per line?

yes, and then sort the lines.
then it is easy for people to see if something is there or not.

[rsc, 2014-07-10 18:41:04]

LGTM

probably calling asmout has side effects we don't want, which is why always
having out != nil doesn't work. don't worry too much about it.

https://codereview.appspot.com/108360043/diff/110013/src/liblink/asm5.c
File src/liblink/asm5.c (right):

https://codereview.appspot.com/108360043/diff/110013/src/liblink/asm5.c#newco...
src/liblink/asm5.c:550: if(((p->pc+size)&15)%8 == 0)
the &15 is unnecessary

[minux, 2014-07-10 19:14:42]

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

cmd/5c, cmd/5g, cmd/5l, liblink: nacl/arm support

LGTM=dave, rsc
R=rsc, iant, dave
CC=golang-codereviews
https://codereview.appspot.com/108360043

[gobot, 2014-07-11 03:00:13]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/8159a76396189d1594555fb4c1ee09e6efacfd07

[minux, 2014-07-11 03:33:01]

On Thu, Jul 10, 2014 at 11:00 PM, <gobot@golang.org> wrote:

> This CL appears to have broken the plan9-386-cnielsen builder.
> See http://build.golang.org/log/8159a76396189d1594555fb4c1ee09e6efacfd0
> <http://build.golang.org/log/8159a76396189d1594555fb4c1ee09e6efacfd07>

# GOMAXPROCS=2 runtime -cpu=1,2,4
panic: test timed out after 4m0s

unrelated test time out.