cmd/compile: "0"[0] should not be a constant

[griesemer]

The following program: http://play.golang.org/p/Xz1-Us2Cwl is rejected with:
/tmp/sandbox748824652/main.go:4: constant -48 overflows uint8

However, indexing a string constant with a constant index does not produce a constant, and thus the program should be valid.

See also #11368.

[anthonycanino1]

I've probed around a little and I believe "0"[0] is turned into a const byte during walkexpr. It seems like this is where the bug is and that it should remain a non-constant byte. Looking in to a fix.

Edit: After looking further into things and understanding a bit about the compile it appears that the code in cgen.go expects "0"[0] to have been handled by the frontend. Maybe a fix should generate code similar to the way a non-constant string is indexed? Apologies if I am off, still trying to learn the compiler code.

[gopherbot]

CL https://golang.org/cl/14800 mentions this issue.

[anthonycanino1]

After digging around for a while I think I found the problem and a potential fix (for my architecture).

"0"[0] is converted into a constant during walkexpr, which of course will trigger an error for -"0"[0]; however, codegen expects this to be handled on the front end in the code for agenr

// ...
if Isconst(nr, CTINT) {
  if Isconst(nl, CTSTR) {
    Fatalf("constant string constant index") // front end should handle
  }
  v := uint64(Mpgetfix(nr.Val().U.(*Mpint)))
  if Isslice(nl.Type) || nl.Type.Etype == TSTRING {
    if Debug['B'] == 0 && !n.Bounded {
      p1 := Thearch.Ginscmp(OGT, Types[Simtype[TUINT]], &nlen, Nodintconst(int64(v)), +1)
      Ginscall(Panicindex, -1)
      Patch(p1, Pc)
    }
  // ...
  }
}
// ...

I was able to write code that correctly fixed this by loading the string into a register, and then loading the byte at that correct index but this seemed inefficient since the byte that needs to be loaded is known at compile time. To address this I've added this code to cgen_wb that checks for this case and loads the byte directly into the register.

At cgen.go:542

// In ODOT, ODOTPTR, OINDEX, OIND, ONAME case
  if n.Op == OINDEX && Isconst(n.Left, CTSTR) && Isconst(n.Right, CTINT) {
    ind := Mpgetfix(n.Right.Val().U.(*Mpint))
    Nodconst(n, n.Type, int64(n.Left.Val().U.(string)[ind]))

    var n1 Node
    Regalloc(&n1, n.Type, res)
    Cgen(n, &n1)

    cgen_wb(&n1, res, wb)
    Regfree(&n1)
    return
  }

Once I add this code to another place it passes all tests on my architecture and OS. Can I get some feedback before I move further with this fix? Is this approach valid?

[gopherbot]

CL https://golang.org/cl/15740 mentions this issue.

[rsc]

There's a CL pending, but it's scary, and there's no real harm in the current behavior. Leaving for Go 1.7.

[rsc]

Still scary, still too late. Regrettable that we didn't get to this early in the Go 1.7 cycle. We are going to try to do better next round.