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

GH-34784: [Go] Fix 32-bit build #35767

Merged
merged 1 commit into from
May 26, 2023
Merged

GH-34784: [Go] Fix 32-bit build #35767

merged 1 commit into from
May 26, 2023

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented May 25, 2023

Rationale for this change

Two locations in the code cause issues when building with GOARCH=386 (e.g. 32-bit systems).

What changes are included in this PR?

In the compute package we assume a 64-bit system when using an untyped int to hold math.MaxInt64 which overflows on a 32-bit system. So we just explicitly identify it as an int64

In the cdata package we use the older *(*[maxlen]*C.void)(unsafe.Pointer(.....))[:] syntax to retrieve the void** for the buffers, with maxlen set to a very large constant. Unfortunately on a 32-bit system this is larger than the address space. Instead we switch to using the unsafe.Slice method that was added in go1.17.

@github-actions
Copy link

@zeroshade
Copy link
Member Author

@raulcd @assignUser would it be okay to get this into the v12.0.1 patch release?

@github-actions
Copy link

⚠️ GitHub issue #34784 has been automatically assigned in GitHub to PR creator.

Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like eventually we'll want to fix up the int/int64 inconsistencies.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels May 25, 2023
@zeroshade
Copy link
Member Author

@lidavidm only in the spots where it's possible we might be dealing with values that will actually go near the overflow/underflow boundaries. I think it's fine to play wack-a-mole with them when users find issues for now as it doesn't come up often and as long as new code is developed properly.

@lidavidm
Copy link
Member

Yup - I don't think we need to change everything immediately

@assignUser
Copy link
Member

@raulcd @assignUser would it be okay to get this into the v12.0.1 patch release?

This seems to be an atomic change with no links to other commits and it fixes an important issue for 32bit users so +1 from me.

@zeroshade
Copy link
Member Author

Thanks @assignUser I'll merge this and have it marked as milestone 12.0.1

@zeroshade zeroshade merged commit 9eaee2a into apache:main May 26, 2023
@zeroshade zeroshade deleted the x86-32-bit branch May 26, 2023 15:40
raulcd pushed a commit that referenced this pull request May 30, 2023
### Rationale for this change
Two locations in the code cause issues when building with `GOARCH=386` (e.g. 32-bit systems).

### What changes are included in this PR?
In the `compute` package we assume a 64-bit system when using an untyped `int` to hold `math.MaxInt64` which overflows on a 32-bit system. So we just explicitly identify it as an `int64`

In the `cdata` package we use the older `*(*[maxlen]*C.void)(unsafe.Pointer(.....))[:]` syntax to retrieve the `void**` for the buffers, with maxlen set to a very large constant. Unfortunately on a 32-bit system this is larger than the address space. Instead we switch to using the `unsafe.Slice` method that was added in go1.17.

* Closes: #34784

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@ursabot
Copy link

ursabot commented May 31, 2023

Benchmark runs are scheduled for baseline = 77a7130 and contender = 9eaee2a. 9eaee2a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.27% ⬆️0.0%] test-mac-arm
[Failed ⬇️9.14% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️1.12% ⬆️0.06%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 9eaee2a5 ec2-t3-xlarge-us-east-2
[Finished] 9eaee2a5 test-mac-arm
[Finished] 9eaee2a5 ursa-i9-9960x
[Finished] 9eaee2a5 ursa-thinkcentre-m75q
[Finished] 77a71305 ec2-t3-xlarge-us-east-2
[Failed] 77a71305 test-mac-arm
[Failed] 77a71305 ursa-i9-9960x
[Failed] 77a71305 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented May 31, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to build Go project for Windows x86
4 participants