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

core: support for Go 1.15 #1293

Merged
merged 8 commits into from
Aug 17, 2020
Merged

core: support for Go 1.15 #1293

merged 8 commits into from
Aug 17, 2020

Conversation

deadprogram
Copy link
Member

This PR makes the needed changes so that TinyGo will work with the new Go 1.15 release. It also adds Go 1.15 to the tests that are run by CircleCI and MS Azure.

Signed-off-by: deadprogram <ron@hybridgroup.com>
…tion

Signed-off-by: deadprogram <ron@hybridgroup.com>
Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

https://dev.azure.com/tinygo/tinygo/_build/results?buildId=1964&view=logs&j=572904f9-79b1-51c4-99c0-531b4ffd4297&t=a5f187cc-e880-5285-3ea6-8c475049272d

C:/ProgramData/Chocolatey/lib/mingw/tools/install/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/8.1.0/../../../../x86_64-w64-mingw32/bin/ld.exe: Error: export ordinal too large: 90506
collect2.exe: error: ld returned 1 exit status

FAIL	github.com/tinygo-org/tinygo [build failed]

Looks like we hit the limit on number of exported symbols in Windows? http://lists.llvm.org/pipermail/llvm-dev/2019-June/133204.html

@deadprogram deadprogram mentioned this pull request Aug 15, 2020
builder/config.go Outdated Show resolved Hide resolved
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

I googled a bit but am not sure what is causing the link error here. Note that 90506 is significantly larger than the maximum of 65535 so something changed probably, probably in Go 1.15.

.circleci/config.yml Show resolved Hide resolved
builder/config.go Outdated Show resolved Hide resolved
@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

Actually I'm surprised how few changes are needed to get this working. Often the diff is much larger.

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

Somehow people online talk about this issue as if it's an issue with DLLs, but we're not building a DLL right?

@deadprogram
Copy link
Member Author

I think the DLL comments are still referring to the number of exported functions.

In our case, might they be debug functions being exported?

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

That's possible, at least some people on the Internet say they fixed the issue by disabling debug symbols. However, I think that would be debug symbols in Go as LLVM didn't change. Maybe there's something in the release notes (I didn't check).

Signed-off-by: deadprogram <ron@hybridgroup.com>
Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

ld issue: encountering ld: error: export ordinal too large needs the compilation flag -O1

according to https://ginkgo-project.github.io/ginkgo/doc/develop/install_ginkgo.html

@deadprogram
Copy link
Member Author

I'm not exactly sure where to try putting that flag so it gets included in the Windows build, however.

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

That doesn't make much sense to me. -O1 means with light optimizations enabled. We are already building LLVM with full optimizations enabled, I doubt reducing optimizations is going to help (it will just make TinyGo slower on Windows). Additionally, LLVM didn't change, what changed was Go (so it shouldn't be an issue with how LLVM was built).

@deadprogram
Copy link
Member Author

Looks like it is not just us: golang/go#40795

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

Great, maybe we should wait until that issue is resolved.

From the release notes:

Go now generates Windows ASLR executables when -buildmode=pie cmd/link flag is provided. Go command uses -buildmode=pie by default on Windows.

This might be it. I don't know how ASLR works on Windows but on Linux it works by building the exectuable as a shared object. If the same is true for Windows, disabling PIE might work around the issue.

@deadprogram
Copy link
Member Author

I was just reading that. Looks like we need -buildmode=exe where should that be put, LDFLAGS?

@deadprogram
Copy link
Member Author

setting the buildmode (-buildmode=exe) when running "go run" or "go build" resolves the issue.

according to https://groups.google.com/g/golang-nuts/c/kLgEb2kxI6A/m/jVArMVO5BQAJ

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

Probably in these places:
https://github.com/tinygo-org/tinygo/blob/release/Makefile#L182
https://github.com/tinygo-org/tinygo/blob/release/Makefile#L185

For example, replacing $(GO) test -v with $(GO) test -v -buildmode=exe.

@aykevl
Copy link
Member

aykevl commented Aug 15, 2020

setting the buildmode (-buildmode=exe) when running "go run" or "go build" resolves the issue.

according to https://groups.google.com/g/golang-nuts/c/kLgEb2kxI6A/m/jVArMVO5BQAJ

That's for a different issue (unrecognized option --high-entropy-va).

Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

OK that did it. All tests passing now on all architectures! I think ready to merge.

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

builder/config.go Outdated Show resolved Hide resolved
Signed-off-by: deadprogram <ron@hybridgroup.com>
@deadprogram
Copy link
Member Author

Thanks everyone for the feedback, and special thanks to @QuLogic who got this process started.

Now merging.

@deadprogram deadprogram merged commit ecda88b into dev Aug 17, 2020
@deadprogram deadprogram deleted the go-1.15-build branch September 17, 2020 04:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants