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/cgo: overflow error on int64_t #21708

Closed
reaperhulk opened this issue Aug 31, 2017 · 12 comments

Comments

@reaperhulk
Copy link

commented Aug 31, 2017

Under go 1.9 the following code no longer compiles due to an overflow:

package main

/*
#include <stdint.h>
#define AV_NOPTS_VALUE   ((int64_t)UINT64_C(0x8000000000000000))
*/
import "C"

func main() {
	if C.int64_t(0) == C.AV_NOPTS_VALUE {
		// irrelevant
	}
}

./main.go:10: constant 9223372036854775808 overflows C.int64_t

Under go 1.8.3 and below this code compiles and works without error. Obviously 0x8000000000000000 does overflow an int64_t, but it seems like the (int64_t) cast in the C code should already have overflowed it to the value -9223372036854775808 before cgo sees it?

@reaperhulk reaperhulk changed the title cmd/cgo cmd/cgo: overflow error on int64_t Aug 31, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

@hirochachacha

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2017

This is a serious bug, could be a go1.9.1 candidate.

package main

/*
#include <stdint.h>
#define AV_NOPTS_VALUE   (int64_t)(-1)
*/
import "C"
import "fmt"

func main() {
	fmt.Println(C.AV_NOPTS_VALUE)
}
./x.go:11: constant 18446744073709551615 overflows int
$ go tool cgo -debug-gcc x.go
...
not-signed-int-const:1:11: error: token is not a valid binary operator in a preprocessor subexpression
#if 0 < (AV_NOPTS_VALUE)
          ^~~~~~~~~~~~~~
/Users/hiro/x.go:5:35: note: expanded from macro 'AV_NOPTS_VALUE'
#define AV_NOPTS_VALUE   (int64_t)(-1)
                         ~~~~~~~~~^
...

The problem is that current uint detection is too optimistic.
I'm not sure about the right solution here. But at least if the detection is failed,
it should fallback to int64, not uint64.

This is the second time I broke in this season, I'm really sorry about that.

@reaperhulk

This comment has been minimized.

Copy link
Author

commented Aug 31, 2017

@hirochachacha It's okay, I appreciate the rapid diagnosis. It's easy to stay on go1.8.3 until this is resolved. Thanks for all that you do!

@gopherbot

This comment has been minimized.

Copy link

commented Aug 31, 2017

Change https://golang.org/cl/60510 mentions this issue: cmd/cgo: support large unsigned macro again

@hirochachacha

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2017

@reaperhulk Thank you. I sent the fix. I hope that solve the problem

@ianlancetaylor ianlancetaylor added this to the Go1.9.1 milestone Aug 31, 2017

@gopherbot gopherbot closed this in f74b52c Sep 1, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Reopening for cherry picking for Go1.9.1

@odeke-em odeke-em reopened this Sep 1, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Sep 1, 2017

Change https://golang.org/cl/60810 mentions this issue: [release-branch.go1.9] cmd/cgo: support large unsigned macro again

gopherbot pushed a commit that referenced this issue Sep 9, 2017
[release-branch.go1.9] cmd/cgo: support large unsigned macro again
The approach of https://golang.org/cl/43476 turned out incorrect.
The problem is that the sniff introduced by the CL only work for simple
expression. And when it fails it fallback to uint64, not int64, which
breaks backward compatibility.
In this CL, we use DWARF for guessing kind instead. That should be more
reliable than previous approach. And importanly, it fallbacks to int64 even
if it fails to guess kind.

Fixes #21708

Change-Id: I39a18cb2efbe4faa9becdcf53d5ac68dba180d46
Reviewed-on: https://go-review.googlesource.com/60510
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/60810
Reviewed-by: Hiroshi Ioka <hirochachacha@gmail.com>
Reviewed-by: Chris Broadfoot <cbro@golang.org>

@rsc rsc modified the milestones: Go1.9.1, Go1.9.2 Oct 4, 2017

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

CL 60810 OK for Go 1.9.2. (cherry-pick, want to record original if it applies cleanly)
CL 60510 OK for Go 1.9.2.

I also tested that the new test in misc/cgo/test works with (int64_t)(0xFFFFFFFFFFFFFFFF) as well as it does with (int64_t)(-1), just in case it didn't.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 14, 2017

Thank you @reaperhulk for reporting it, @hirochachacha for the fix and @rsc for testing and evaluating for the cherry pick.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 15, 2017

Change https://golang.org/cl/70970 mentions this issue: [release-branch.go1.9] cmd/cgo: support large unsigned macro again

@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2017

CL 70970 OK for Go 1.9.2 (after CL 70849).

(Gerrit believes that a cherry-pick of 60810 has already happened on release-branch.go1.9 and will not allow a second. It doesn't appreciate that we wiped that from the Git history. CL 70970 is a straight cherry-pick but with the Change-Id incremented to break the cherry-pick association on Gerrit.)

gopherbot pushed a commit that referenced this issue Oct 25, 2017
[release-branch.go1.9] cmd/cgo: support large unsigned macro again
The approach of https://golang.org/cl/43476 turned out incorrect.
The problem is that the sniff introduced by the CL only work for simple
expression. And when it fails it fallback to uint64, not int64, which
breaks backward compatibility.
In this CL, we use DWARF for guessing kind instead. That should be more
reliable than previous approach. And importanly, it fallbacks to int64 even
if it fails to guess kind.

Fixes #21708

Change-Id: I39a18cb2efbe4faa9becdcf53d5ac68dba180d47
Reviewed-on: https://go-review.googlesource.com/60510
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/60810
Reviewed-by: Hiroshi Ioka <hirochachacha@gmail.com>
Reviewed-by: Chris Broadfoot <cbro@golang.org>
Reviewed-on: https://go-review.googlesource.com/70970
Run-TryBot: Russ Cox <rsc@golang.org>
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2017

go1.9.2 has been packaged and includes:

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Oct 26 21:09:11 UTC

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.