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

Jump to bottom

cmd/compile: bug in string comparison #24817

Closed
dsymonds opened this issue Apr 12, 2018 · 14 comments
Closed

cmd/compile: bug in string comparison #24817

dsymonds opened this issue Apr 12, 2018 · 14 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.11

Comments

@dsymonds
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.10.1 (playground). This also reproduces with go version devel +b1335037fa Wed Mar 7 22:03:43 2018 +0000 linux/amd64.

Does this issue reproduce with the latest release?

It does in the playground.

What did you do?

https://play.golang.org/p/qJc3MD8uB9j

package main

import (
	"fmt"
)

func main() {
	s := "123"
	const t = "123"
	fmt.Println("" < s)
	fmt.Println("" < t)
}

What did you expect to see?

The two string comparisons should be the same, yielding true and true.

What did you see instead?

false
true
@dsymonds
Copy link
Contributor Author

It's still broken at head (go version devel +2dfb423e6e Wed Apr 11 23:46:30 2018 +0000 linux/amd64).

@dsymonds
Copy link
Contributor Author

Oddly, it is not a commutative bug. If the empty string is on the RHS then it is fine (https://play.golang.org/p/TULS3BC_uuY).

@dsnet
Copy link
Member

dsnet commented Apr 12, 2018 •
edited

Interesting regression from Go1.8. Root cause: https://golang.org/cl/26758

\cc @josharian @bradfitz

@ALTree ALTree added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Apr 12, 2018
@josharian
Copy link
Contributor

Nice find!

On my phone, but I believe the offending line is:

r := nod(cmp, nod(OLEN, ncs, nil), nodintconst(int64(len(s))))

Unlike = and !=, the order of args here matters for > and <. Oops.

This only happens for the empty string.

The cleanest fix is probably to rewrite >, <, >=, and <= for “” into !=, false, true, and == respectively. I’ll send a CL first chance I get.

@bradfitz
Copy link
Contributor

Ouch. This will need some backports too.

@andybons andybons added this to the Go1.10.2 milestone Apr 12, 2018
@gopherbot
Copy link

Change https://golang.org/cl/106655 mentions this issue: cmd/compile: fix evaluation of "" < s

@josharian
Copy link
Contributor

For the record, the affected comparisons are:

"" < s
"" <= s
"" > s
"" >= s

@josharian
Copy link
Contributor

Reopening for backport.

@josharian josharian reopened this Apr 12, 2018
@andybons andybons added the CherryPickCandidate Used during the release process for point releases label Apr 18, 2018
@andybons
Copy link
Member

@josharian can you open another issue on the 1.9.6 milestone per https://golang.org/wiki/MinorReleases?

(This process is new and will be communicated more widely very soon)

@josharian
Copy link
Contributor

We keep going back and forth on this. So now we're opening new issues again? Is there something special I'm supposed to put in the new issue?

@bradfitz
Copy link
Contributor

We keep going back and forth on this.

Yes, sorry. We know that, though. We tried both ways and like the first more.

So now we're opening new issues again?

Yes.

@andybons
Copy link
Member

I'm sorry for the churn in process.

Per the Wiki doc listed above:

As soon as an interested party thinks an issue should be considered for backport, they open one or two “child” issues titled like package: title [1.9 backport]. The issue should include a link to the original issue, the CLs that need to be cherry-picked, and a short rationale about why the backport might be needed. This will soon be automated via GopherBot but for now must be done manually.

This was referenced Apr 18, 2018
Closed
Closed
@josharian
Copy link
Contributor

Glad to hear it is going to be automated.

@bradfitz bradfitz modified the milestones: Go1.10.2, Go1.11 Apr 19, 2018
@bradfitz bradfitz removed the CherryPickCandidate Used during the release process for point releases label Apr 19, 2018
@FiloSottile
Copy link
Contributor

Closing this in favor of the backport issues as it was fixed in master.

ns-codereview pushed a commit to couchbase/cbft that referenced this issue May 1, 2018
@abhinavdangeti
MB-29491: Upgrade Go version to 1.9.6
60742b7 
Addresses string comparison issue:
golang/go#24817

Change-Id: I8a8a10f2739e8b1a4884f3b74db7a6b0db78f35b
Reviewed-on: http://review.couchbase.org/93589
Well-Formed: Build Bot <build@couchbase.com>
Reviewed-by: Marty Schoch <marty.schoch@gmail.com>
Tested-by: Abhinav Dangeti <abhinav@couchbase.com>
@golang golang locked and limited conversation to collaborators Apr 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
8 participants
@bradfitz @dsymonds @josharian @andybons @FiloSottile @ALTree @dsnet @gopherbot