Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
griesemer opened this issue Jun 23, 2015 · 6 comments
Closed

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

griesemer opened this issue Jun 23, 2015 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@griesemer
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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

@anthonycanino1
Copy link
Contributor

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
Copy link

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

@rsc
Copy link
Contributor

rsc commented Dec 14, 2015

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 rsc modified the milestones: Go1.7, Go1.6 Dec 14, 2015
@rsc
Copy link
Contributor

rsc commented May 17, 2016

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.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 17, 2016
@griesemer griesemer modified the milestones: Go1.8Early, Go1.8 May 18, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 6, 2016
@golang golang locked and limited conversation to collaborators Oct 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants