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: bug in string comparison #24817

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

Comments

Projects
None yet
8 participants
@dsymonds
Copy link
Member

commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

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

@dsymonds

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

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

\cc @josharian @bradfitz

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 12, 2018

Ouch. This will need some backports too.

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

@gopherbot

This comment has been minimized.

Copy link

commented Apr 12, 2018

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

@gopherbot gopherbot closed this in c1ed1f3 Apr 12, 2018

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

For the record, the affected comparisons are:

"" < s
"" <= s
"" > s
"" >= s
@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

Reopening for backport.

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

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.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2018

Glad to hear it is going to be automated.

@bradfitz bradfitz modified the milestones: Go1.10.2, Go1.11 Apr 19, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

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

MB-29491: Upgrade Go version to 1.9.6
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.
You can’t perform that action at this time.