Upgrade Go to 1.18 #7

eduardvintila · 2023-07-26T13:00:25Z

This PR is part of a series of PRs that work in conjunction for the update of Go support to 1.18:

For testing, make sure to pass to QEMU the -cpu host argument (this is necessary because the paging API and ukvmem are required by libgo as part of Virtual Address space management, and somehow the default CPU just won't handle 1GB pages).

Only GCC12 (with gccgo >=12) and x86 is supported for the moment.

StefanJum · 2023-08-05T17:58:16Z

@eduardvintila What gccgo version should we use? I'm using gccgo-11 and I get the following error

workdir/apps/helloworld-go/build/libgo/origin/gcc-12.1.0/libgo/go/internal/abi/abi.go:20:19: error: use of undefined type ‘any’                                   
   20 | func FuncPCABI0(f any) uintptr {

All prs mentioned in the description are merged.

eduardvintila · 2023-08-05T19:49:28Z

What gccgo version should we use?

@StefanJum, you should use version 12, as stated in the description (though indeed I did not mention explicitly gccgo, I'll have to edit that). Obviously this should be officially documented, so I'll update the README.md file as well

eduardvintila · 2023-08-06T15:50:41Z

I've updated the documentation.

StefanJum

@eduardvintila new issue, any idea on this one?

CRITICAL:root:2
workdir/libs/libgo/Makefile.build:3: workdir/apps/helloworld-go/build/go/apphelloworldgo/Makefile: No such file or directory
make[3]: *** No rule to make target 'workdir/apps/helloworld-go/build/go/apphelloworldgo/Makefile'.  Stop.

eduardvintila · 2023-08-06T17:33:23Z

@StefanJum hmm, did you also update app-helloworld-go?

StefanJum · 2023-08-06T17:46:45Z

@StefanJum hmm, did you also update app-helloworld-go?

Yes, it's weird, I've built it in a docker container and everything builds and runs fine, but if I try to build locally it fails.
It might be some version stuff, I'll keep looking.

razvand · 2023-08-06T18:45:33Z

@StefanJum , you need golang package installed.

RaduNichita

Nice work, @eduardvintila

I have a question: could we reduce the number of files modified by this PR by not including the autogenerated files and generating them on the fly?

RaduNichita · 2023-08-06T19:39:47Z

libgo/libcalls.go

@@ -1,11 +1,12 @@
-// This file was automatically generated by mksyscall.awk
+// Code generated by mksyscall.awk. DO NOT EDIT.


If this file was automatically generated, could we add a rule in the makefile for that generating it of including it per se?

mksyscall.awk is from the upstream gcc build system. It's not integrated with Unikraft, and I don't see an easy way to add a rule in the Makefile for the moment. Ofc it would be ideal if we could automate this, but I don't think it's feasible for this release :).

Ok, let's stick with this. Maybe you should open a issue regarding this so we don't lose it in the comments.

RaduNichita · 2023-08-06T19:40:22Z

libgo/Makefile.runtime.uk

@@ -0,0 +1,3548 @@
+# This file has been auto-generated for go1.18 gccgo (GCC) 12.1.0.


This one is generated by the helper scripts, found in the scripts/ directory (also introduced with this PR). In a similar manner, lib-musl also includes generated Makefiles for each component (i.e. Makefile.uk.musl.complex, Makefile.uk.musl.signal etc.), providing also helper scripts to generate them. So I think we should keep this approach for consistency.

RaduNichita

Nice work, @eduardvintila

Reviewed-by: Radu Nichita radunichita99@gmail.com

StefanJum

@eduardvintila everything seems to work fine.

One comment I have it's that everything is done very x86-targeted (i.e. adding -D__x86_64__, x86 headers and source files, setting x86 in config files, etc.), and this might be an issue if we want to add arm support in the future.
We tried (with @razvand) to compile this for arm and we eventually made it to the linking step, but I would make everything that is architecture specific depend on $(ARCH) being set to x86_64.

razvand · 2023-08-07T16:30:52Z

@StefanJum, @eduardvintila, we will have only x86_64 for Go this release. I had made progress with the AArch64 part, but it requires further work. We'll do it after the release and have a sub-release with Go support for AArch64.

eduardvintila · 2023-08-08T08:13:38Z

@StefanJum, you're right. I've refactored the makefiles and some source files to emphasize which parts are architecture dependent.

@razvand, for an ARM build to work, the ukvmem library is necessary, which is not yet adapted to aarch64.

StefanJum · 2023-08-08T14:35:17Z

One more thing @eduardvintila, I think we should add compiler version checks in the Makefile.uk (similar to what we have in the Unikraft Makefile). I think it's the best way to document the minimum required compiler version.

This commit upgrades the library version to 1.18 by using the upstream GO runtime library provided by GCC12. New Makefile rules are introduced to compartimentalize GO pacakge bulding. Helper scripts are also added to assist with creation of such Makefiles. Glue code was also added to help with compatibility, and additional headers from `libffi` and `libbacktrace` were also introduced. Co-authored-by: Marc Rittinghaus <marc.rittinghaus@unikraft.io> Co-authored-by: Eduard Vintilă <eduard.vintila47@gmail.com> Signed-off-by: Marc Rittinghaus <marc.rittinghaus@unikraft.io> Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>

Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>

eduardvintila · 2023-08-10T20:53:20Z

Thank you, @StefanJum - I've included your suggestion! Also updated the PR based on the changes introduced by this commit from Unikraft core.

StefanJum · 2023-08-11T08:04:53Z

@eduardvintila I get these errors, I remember I've got them before but I can't figure out what I did to get pass them.

/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:321:9: error: duplicate case value                                                                           
  321 |         case SYS_madvise:                                                                                                                                                                                  
      |         ^~~~                                                                                     
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:81:9: note: previously used here                                                                             
   81 |         case SYS_madvise:                                                                                                                                                                                  
      |         ^~~~             
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:331:9: error: duplicate case value                                                                           
  331 |         case SYS_mprotect:                                                                                                                                                                                 
      |         ^~~~                                                                                                                                                                                               
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:76:9: note: previously used here                                                                             
   76 |         case SYS_mprotect:

Any unmerged pr that I need or some extra config options?

eduardvintila · 2023-08-11T11:16:23Z

@eduardvintila I get these errors, I remember I've got them before but I can't figure out what I did to get pass them.

/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:321:9: error: duplicate case value                                                                           
  321 |         case SYS_madvise:                                                                                                                                                                                  
      |         ^~~~                                                                                     
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:81:9: note: previously used here                                                                             
   81 |         case SYS_madvise:                                                                                                                                                                                  
      |         ^~~~             
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:331:9: error: duplicate case value                                                                           
  331 |         case SYS_mprotect:                                                                                                                                                                                 
      |         ^~~~                                                                                                                                                                                               
/home/unikraft/SJ/maintainer-tools/workdir/apps/helloworld-go/build/libsyscall_shim/uk_syscall_r_fn.c:76:9: note: previously used here                                                                             
   76 |         case SYS_mprotect:

Any unmerged pr that I need or some extra config options?

@StefanJum Nope, no additional PR is needed. It looks like the same syscalls are provided by two seperate libraries. By any chance, do you have both ukmmap and posix-mmap selected? This obviously should not happen, but sometimes it's influenced by the order in which you select the libraries... However, this commit should've solved the problem; I did not encounter it after it was merged

StefanJum · 2023-08-11T11:24:27Z

@StefanJum Nope, no additional PR is needed. It looks like the same syscalls are provided by two seperate libraries. By any chance, do you have both ukmmap and posix-mmap selected? This obviously should not happen, but sometimes it's influenced by the order in which you select the libraries... However, this commit should've solved the problem; I did not encounter it after it was merged

Yes, that was it. It's really messed up, it was selected even if it did not show up in the config menu (due to the depends on statement). I had to unselect everything and get it to show up, deselect it end select everything else again 🥲

StefanJum

Anyway this works, we need to soon get rid of ukmmap tho.
Thanks a lot for the work @eduardvintila 🎉 🎉

StefanJum

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com> Reviewed-by: Radu Nichita <radunichita99@gmail.com> Reviewed-by: Stefan Jumarea <stefanjumarea02@gmail.com> Approved-by: Razvan Deaconescu <razvand@unikraft.io> Tested-by: Unikraft CI <monkey@unikraft.io> GitHub-Closes: #7

eduardvintila force-pushed the go-updated-gcc12 branch 3 times, most recently from 2816c57 to 6029e95 Compare July 26, 2023 21:07

This was referenced Jul 26, 2023

Add Makefile and update Makefile.uk unikraft/app-helloworld-go#9

Closed

Makefile: Allow external Makefile.build and Provide GO 1.18 support unikraft/unikraft#1005

Closed

patches: Add syscall function prototype unikraft/lib-musl#67

Closed

razvand requested review from RaduNichita and StefanJum August 5, 2023 06:23

razvand self-assigned this Aug 5, 2023

razvand added the enhancement New feature or request label Aug 5, 2023

eduardvintila force-pushed the go-updated-gcc12 branch from 6029e95 to 79a26b7 Compare August 6, 2023 15:49

StefanJum reviewed Aug 6, 2023

View reviewed changes

StefanJum self-requested a review August 6, 2023 19:26

RaduNichita suggested changes Aug 6, 2023

View reviewed changes

eduardvintila requested a review from RaduNichita August 6, 2023 20:02

RaduNichita approved these changes Aug 7, 2023

View reviewed changes

StefanJum reviewed Aug 7, 2023

View reviewed changes

eduardvintila force-pushed the go-updated-gcc12 branch 2 times, most recently from 1199562 to ae29375 Compare August 8, 2023 08:09

eduardvintila requested a review from StefanJum August 8, 2023 08:15

README: Update documentation

77ec7f6

Signed-off-by: Eduard Vintilă <eduard.vintila47@gmail.com>

eduardvintila force-pushed the go-updated-gcc12 branch from ae29375 to 77ec7f6 Compare August 10, 2023 20:50

StefanJum approved these changes Aug 11, 2023

View reviewed changes

razvand approved these changes Aug 11, 2023

View reviewed changes

unikraft-bot closed this in 20b7203 Aug 11, 2023

unikraft-bot added the ci/merged label Aug 11, 2023

RaduNichita mentioned this pull request Aug 11, 2023

Port upstream NumPy 1.25.0 unikraft/lib-python-numpy#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Go to 1.18 #7

Upgrade Go to 1.18 #7

eduardvintila commented Jul 26, 2023 •

edited

Loading

StefanJum commented Aug 5, 2023

eduardvintila commented Aug 5, 2023 •

edited

Loading

eduardvintila commented Aug 6, 2023

StefanJum left a comment

eduardvintila commented Aug 6, 2023

StefanJum commented Aug 6, 2023

razvand commented Aug 6, 2023

RaduNichita left a comment

RaduNichita Aug 6, 2023

eduardvintila Aug 6, 2023

RaduNichita Aug 6, 2023

RaduNichita Aug 6, 2023

eduardvintila Aug 6, 2023

RaduNichita left a comment

StefanJum left a comment

razvand commented Aug 7, 2023

eduardvintila commented Aug 8, 2023

StefanJum commented Aug 8, 2023

eduardvintila commented Aug 10, 2023

StefanJum commented Aug 11, 2023 •

edited

Loading

eduardvintila commented Aug 11, 2023 •

edited

Loading

StefanJum commented Aug 11, 2023

StefanJum left a comment

StefanJum left a comment

razvand left a comment

		@@ -1,11 +1,12 @@
		// This file was automatically generated by mksyscall.awk
		// Code generated by mksyscall.awk. DO NOT EDIT.

		@@ -0,0 +1,3548 @@
		# This file has been auto-generated for go1.18 gccgo (GCC) 12.1.0.

Upgrade Go to 1.18 #7

Upgrade Go to 1.18 #7

Conversation

eduardvintila commented Jul 26, 2023 • edited Loading

StefanJum commented Aug 5, 2023

eduardvintila commented Aug 5, 2023 • edited Loading

eduardvintila commented Aug 6, 2023

StefanJum left a comment

Choose a reason for hiding this comment

eduardvintila commented Aug 6, 2023

StefanJum commented Aug 6, 2023

razvand commented Aug 6, 2023

RaduNichita left a comment

Choose a reason for hiding this comment

RaduNichita Aug 6, 2023

Choose a reason for hiding this comment

eduardvintila Aug 6, 2023

Choose a reason for hiding this comment

RaduNichita Aug 6, 2023

Choose a reason for hiding this comment

RaduNichita Aug 6, 2023

Choose a reason for hiding this comment

eduardvintila Aug 6, 2023

Choose a reason for hiding this comment

RaduNichita left a comment

Choose a reason for hiding this comment

StefanJum left a comment

Choose a reason for hiding this comment

razvand commented Aug 7, 2023

eduardvintila commented Aug 8, 2023

StefanJum commented Aug 8, 2023

eduardvintila commented Aug 10, 2023

StefanJum commented Aug 11, 2023 • edited Loading

eduardvintila commented Aug 11, 2023 • edited Loading

StefanJum commented Aug 11, 2023

StefanJum left a comment

Choose a reason for hiding this comment

StefanJum left a comment

Choose a reason for hiding this comment

razvand left a comment

Choose a reason for hiding this comment

eduardvintila commented Jul 26, 2023 •

edited

Loading

eduardvintila commented Aug 5, 2023 •

edited

Loading

StefanJum commented Aug 11, 2023 •

edited

Loading

eduardvintila commented Aug 11, 2023 •

edited

Loading