code review 11107044: cmd/gc: avoid passing unevaluated constant expressions ...

cmd/gc: avoid passing unevaluated constant expressions to backends.

Backends do not exactly expect receiving binary operators with
constant operands or use workarounds to move them to
register/stack in order to handle them.

Fixes issue 5841.

[remyoudompheng, 2013-07-11 07:18:19]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[DMorsing, 2013-07-11 08:20:50]

This change is weird. I think you're papering over some deeper issue in the
compilers.

https://codereview.appspot.com/11107044/diff/5005/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

https://codereview.appspot.com/11107044/diff/5005/src/cmd/6g/cgen.c#newcode1240
src/cmd/6g/cgen.c:1240: if(!nl->addable) {
I'm not sure about this. It shouldn't matter for evaluating nl whether it is
addable or not.

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c#newcode1382
src/cmd/gc/walk.c:1382: evconst(n);
All constants should be fully evaluated in typecheck.

[DMorsing, 2013-07-11 08:45:04]

Ok, I think the compiler is trying to optimize y%1 too eagerly. For example,
this doesn't panic:

var y *int
if *y%1 == 0 {
}

[rsc, 2013-07-11 18:00:32]

I agree with Daniel. The fix should probably be in the front end.

[remyoudompheng, 2013-07-11 21:27:27]

https://codereview.appspot.com/11107044/diff/5005/src/cmd/6g/cgen.c
File src/cmd/6g/cgen.c (right):

https://codereview.appspot.com/11107044/diff/5005/src/cmd/6g/cgen.c#newcode1240
src/cmd/6g/cgen.c:1240: if(!nl->addable) {
On 2013/07/11 08:20:51, DMorsing wrote:
> I'm not sure about this. It shouldn't matter for evaluating nl whether it is
> addable or not.

This change is not mandatory and only aims at bringing 6g closer to 8g (don't
generate unnecessary moving).

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c#newcode1382
src/cmd/gc/walk.c:1382: evconst(n);
On 2013/07/11 08:20:51, DMorsing wrote:
> All constants should be fully evaluated in typecheck.

walk transforms y%1 in a constant 0. As you point out it seems to miss some side
effects of y, but that explains why unevaluated constants can appear here.

Do you suggest an alternative solution?

[remyoudompheng, 2013-07-20 10:19:47]

On 2013/07/11 08:45:04, DMorsing wrote:
> Ok, I think the compiler is trying to optimize y%1 too eagerly. For example,
> this doesn't panic:
> 
> var y *int
> if *y%1 == 0 {
> }

I cannot reproduce this issue. Can you post a full program?

Rémy.

[DMorsing, 2013-07-20 12:42:46]

On 2013/07/20 10:19:47, remyoudompheng wrote:
> On 2013/07/11 08:45:04, DMorsing wrote:
> > Ok, I think the compiler is trying to optimize y%1 too eagerly. For example,
> > this doesn't panic:
> > 
> > var y *int
> > if *y%1 == 0 {
> > }
> 
> I cannot reproduce this issue. Can you post a full program?
> 
> Rémy.

Weird, I can't reproduce as well.

Maybe I was wrong. Reading the -W output suggests that we put in a noop read
before inserting the 0 constant.

[rsc, 2013-07-22 23:58:42]

Can you please make this CL only include the change in cmd/gc?
That's the part I'm comfortable putting into Go 1.1.2. We can do the other
changes separately.

[rsc, 2013-07-23 00:01:23]

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c
File src/cmd/gc/walk.c (right):

https://codereview.appspot.com/11107044/diff/5005/src/cmd/gc/walk.c#newcode1382
src/cmd/gc/walk.c:1382: evconst(n);
// Expressions that are constant at run time but not
// considered const by the language spec are not turned into
// constants until walk. For example, if n is y%1 == 0, the
// walk of y%1 may have replaced it by 0.
// Check whether n with its updated args is itself now a constant.
evconst(n);
<blank line>
ullmancalc(n);

[remyoudompheng, 2013-07-24 06:46:52]

Hello golang-dev@googlegroups.com, daniel.morsing@gmail.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2013-07-25 13:41:26]

LGTM

[rsc, 2013-07-25 13:42:06]

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

cmd/gc: avoid passing unevaluated constant expressions to backends.

Backends do not exactly expect receiving binary operators with
constant operands or use workarounds to move them to
register/stack in order to handle them.

Fixes issue 5841.

R=golang-dev, daniel.morsing, rsc
CC=golang-dev
https://codereview.appspot.com/11107044

Committer: Russ Cox <rsc@golang.org>