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

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

cmd/cgo: overflow error on int64_t #21708

reaperhulk opened this issue Aug 31, 2017 · 12 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone
Go1.9.2

Comments

@reaperhulk
Copy link

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

/cc @ianlancetaylor @hirochachacha

@hirochachacha
Copy link
Contributor

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

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

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

@hirochachacha
Copy link
Contributor

@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
@odeke-em
Copy link
Member

odeke-em commented Sep 1, 2017

Reopening for cherry picking for Go1.9.1

@odeke-em odeke-em reopened this Sep 1, 2017
@gopherbot
Copy link

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
@hirochachacha @broady
[release-branch.go1.9] cmd/cgo: support large unsigned macro again
0993369 
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
Copy link
Contributor

rsc commented Oct 13, 2017 •
edited

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.

@rsc rsc added the CherryPickApproved Used during the release process for point releases label Oct 14, 2017
@odeke-em
Copy link
Member

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

@rsc rsc mentioned this issue Oct 15, 2017
Closed
@gopherbot
Copy link

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

@rsc
Copy link
Contributor

rsc commented Oct 15, 2017 •
edited

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
@hirochachacha @rsc
[release-branch.go1.9] cmd/cgo: support large unsigned macro again
dffc931 
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
Copy link
Contributor

rsc 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

@rsc rsc closed this as completed Oct 26, 2017
@mkrufky mkrufky mentioned this issue Nov 1, 2017
Merged
@ruflin ruflin mentioned this issue Dec 1, 2017
Closed
@andrewkroh andrewkroh mentioned this issue Dec 4, 2017
Closed
@karalabe karalabe mentioned this issue Dec 21, 2017
Merged
@holiman holiman mentioned this issue Jan 25, 2018
Closed
@golang golang locked and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Milestone
Go1.9.2
Development

No branches or pull requests
6 participants
@rsc @reaperhulk @hirochachacha @ianlancetaylor @odeke-em @gopherbot