cmd/compile: rethink temporary names

josharian commented 10 years ago

package p

func conv() {
    var f float64
    _ = float64(uint64(f))
}

go version devel +b0443478e712 Thu May 01 16:29:34 2014 -0400 darwin/386

go tool 8g -S conv.go

The assembly listing includes:

    0x00b7 00183 (conv.go:5)    FMOVDP  F0,"".autotmp_0001+20(SP)
    0x00bb 00187 (conv.go:5)    JMP ,130
    0x00bd 00189 (conv.go:5)    FSTCW   ,""..utotmp_0004+2(SP)
    0x00c1 00193 (conv.go:5)    MOVW    $3967,""..utotmp_0005+0(SP)
    0x00c7 00199 (conv.go:5)    FLDCW   ""..utotmp_0005+0(SP),
    0x00ca 00202 (conv.go:5)    FMOVD   $f64.43e0000000000000+0(SB),F0
    0x00d0 00208 (conv.go:5)    FUCOMIP F0,F1
    0x00d2 00210 (conv.go:5)    JLS ,225
    0x00d4 00212 (conv.go:5)    FMOVVP  F0,"".autotmp_0003+12(SP)
    0x00d8 00216 (conv.go:5)    FLDCW   ""..utotmp_0004+2(SP),

Note the variable names ""..utotmp_0004 and ""..utotmp_0005. There
is downstream code that checks for the "autotmp" prefix, so this name
corruption is potentially significant. Not all autotmp names are corrupted (see
"".autotmp_0001).

I've not seen name corruption with either 5g or 6g.

josharian commented 10 years ago

Comment 1:

Urk. Actually, it looks like this is done intentionally in memname (8g/gsubr.c):
    namebuf[0] = '.';   // keep optimizer from registerizing
The question is now: Is this safe, given the downstream users of this information? I see
at least two non-optimizer clients of the "autotmp" name -- racewalk and dwarf
generation.

ianlancetaylor commented 10 years ago

Comment 3:

Labels changed: added repo-main, release-go1.3maybe.

rsc commented 10 years ago

Comment 4:

racewalk and dwarf both seem to be working okay.
I would like to change these names, though. In particular I'd like to use the shorter
construct '%c.%d' where the first character can vary according to the kind of name. t.%d
for most temps, m.%d for these memory temps in 8g, maybe p.%d for the parameter shadow
variables, maybe something different for order.c-introduced temps vs others (because the
former are cleaned up better).
But that can't happen until 1.4.

Labels changed: added release-go1.4, removed release-go1.3maybe.

Status changed to Accepted.

rsc commented 9 years ago

Comment 5:

Labels changed: added release-go1.5, removed release-go1.4.

rsc commented 9 years ago

We can rethink what's left of this once Keith is done with SSA.

golang / go

cmd/compile: rethink temporary names #7923