Remove Cgo dependency on Windows #102

hajimehoshi · 2018-11-12T04:13:58Z

Fixes go-gl/gl#109

Design Document

https://docs.google.com/document/d/1mqquznil9fR2amtb3eTC2ObCx3A8Af_5INqKjO-pKDg/edit?usp=sharing

tl;dr

I propose to eliminate Cgo usages on Windows from go-gl/gl by fixing go-gl/glow to generate code using syscall.Syscall.

hajimehoshi · 2018-11-12T04:20:18Z

Please take a look: @slimsag @dmitshur

functions.go

hajimehoshi · 2018-11-14T07:27:36Z

friendly ping

hajimehoshi · 2018-12-04T02:53:04Z

That's really sad that there has not been any feedback in the last 22 days. This change is really important (at least to me) e.g. in order to make compiling time shorten on Windows.

pwaller · 2018-12-04T19:38:51Z

Hi @hajimehoshi, thanks very much for your contribution. Sorry it is taking time, but please be aware that no one is paid to maintain these packages. It's a best effort in our spare time and we can only give so much. Please have patience and faith that we will get to it. Feel free to continue to send pings!

tmpl/conversions.tmpl

This is a temporal fix until go-gl/glow#102 is applied.

hajimehoshi · 2019-01-05T07:26:07Z

type.go

+			return fmt.Sprintf("uintptr(math.Float64bits(%s))", name)
+		}
+	case "GLDEBUGPROC", "GLDEBUGPROCARB", "GLDEBUGPROCKHR":
+		return fmt.Sprintf("syscall.NewCallback(%s)", name)


Should this be NewCallbackCDecl?

silbinarywolf · 2019-01-27T23:45:39Z

@pwaller @kjozic or @errcw

Are any of you able to set aside some time and review this?
Being able to not rely on CGo on Windows would be amazing and help with simplifying the build process of my game engine for less experienced users.

errcw

Just a quick first pass...

errcw · 2019-02-01T00:46:02Z

functions.go

+}
+
+// Syscall returns a syscall expression for Windows.
+func (f Function) Syscall() string {


I wonder if this would be more idiomatic/clear if it returned an error when !IsImplementedForSyscall (rather than having a separate func that needs to be called)?

This is called at templates to generate Syscall calling, then returning error here would not make sense.

errcw · 2019-02-01T00:53:31Z

tmpl/conversions_windows.tmpl

@@ -0,0 +1,110 @@
+//glow:keepspace


It's not clear to me why this conversion code, or the debug code, needs to be forked across Windows/not-Windows.

#102 (comment) is the reason: using this conversions_windows.tmpl version at Cgo caused errors.

Is there any way we can reduce the duplication by factoring out the parts that are the same? (Or am I misreading and all the implementations are subtly different?)

Done (created the common conversion.tmpl)

Getting back to this now (finally, sorry for the delay)...

Are the changes to conversions.tmpl platform agnostic and should be used everywhere?

I still don't quite understand why debug needs to be forked.

Quick reply: go-gl/gl#80 might be related, but I am not sure. I'll take a look later.

I feel like if we can make conversions.tmpl pure Go first, we should do this before merging this PR. Wdyt?

Yeah, I'm all for updating conversions to work across the board.

errcw · 2019-02-01T00:53:41Z

package.go

@@ -68,14 +68,29 @@ func (pkg *Package) GeneratePackage(dir string) error {
 	if err := pkg.generateFile("package", dir); err != nil {
 		return err
 	}
-	if err := pkg.generateFile("conversions", dir); err != nil {
+	if err := pkg.generateFile("package_notwindows", dir); err != nil {


Having all these files is super unfortunate, but I'm not sure whether there is anything to be done for it.

Perhaps something like:

for _, v := range []string{"package", "package_notwindows", "package_windows", "conversions", "conversions_notwindows", "conversions_windows", "procaddr_notwindows", "procaddr_windows"} { if err := pkg.generateFile(v, dir); err != nil { return err } }

errcw · 2019-02-01T00:55:00Z

tmpl/package_notwindows.tmpl

+// #cgo linux freebsd LDFLAGS: -lGL
+// #cgo windows       LDFLAGS: -lopengl32
+//
+// #if defined(_WIN32) && !defined(APIENTRY) && !defined(__CYGWIN__) && !defined(__SCITECH_SNAP__)


presumably these windows-specific bits are no longer necessary in the not-windows files?

hajimehoshi · 2019-03-12T02:15:00Z

I think I have addressed all the issues. Please take a look. Thank you!

hajimehoshi · 2019-03-25T05:18:44Z

Oops, I need to rebase this

hajimehoshi · 2019-03-25T14:22:46Z

Done. PTAL

hajimehoshi · 2019-04-03T17:55:24Z

tmpl/conversions_windows.tmpl

+// in order to free the memory.
+//
+// If no strings are provided as a parameter this function will panic.
+func Strs(strs ...string) (cstrs **uint8, free func()) {


@errcw My concern was that **uint8 would be a Go pointer to a Go pointer, which is not allowed to pass to C world. (This is fine to pass this to Syscall on Windows as long as we care the lifetime of the object). That's why I separated the implementation for Windows and non-Windows. Do you have any suggestions?

I'm so sorry for the slow responses here. The amount of free time I have is vanishingly small these days. The cstrs **unit8 return value is actually a set of pointers into malloc'ed C memory, which I believe is safe?

Thank you for responding! Our plan is to make this function in pure Go (no cgo in any environment), right? If so, we would not be able to use C allocator.

Right, right, of course. So I'm clear, on Windows, we're using Syscall which is inherently safe to accept values that are Go pointers to Go pointers? Whereas on other platforms those same values are unsafe because they cross the Cgo boundary? If so, sounds good.

on Windows, we're using Syscall which is inherently safe to accept values that are Go pointers to Go pointers?

Yes, there is no warning, and we can assure the lifetime of the objects (by runtime.KeepAlive).

Whereas on other platforms those same values are unsafe because they cross the Cgo boundary?

This is unsafe, and unfortunately Go runtime suspends the execution with warning messages. That's the problem.

Then, I think it would be inevitable to split the implementation for Windows and non-Windows. My suggestion is to split the allocation part to minimize the split code. What do you think?

If so, sounds good.

So, you are fine with splitting the code? :-)

neclepsio · 2020-06-19T10:06:55Z

@hajimehoshi thank you very much for this. I can't understand why it has been abandoned.

I created a pull request on your nocgo branch to sync with master, implementing overloads.

Moreover, I found a bug, not linked to overloads, in function:

func DebugMessageCallback(callback DebugProc, userParam unsafe.Pointer) {
	syscall.Syscall(gpDebugMessageCallback, 2, syscall.NewCallbackCDecl(callback), uintptr(userParam), 0)
}

panic: compileCallback: expected function with one uintptr-sized result

This happens while creating a debug context with glfw 3.3; it works fine with cgo version.

If I succeed in fixing it, I will make another pull request on your branch.

Zyl9393 · 2020-10-03T14:12:14Z

I would like everyone to take notice that using syscall will double the call overhead, because syscall always calls GetLastError() on Windows, even if you use it to invoke non-WINAPI functions. If your CPU is bottlenecking (which is likely because we already experience a ~70ns overhead due to CGO) it would then bottleneck doubly, without gaining anything in the process. This will then further cement the fact that Golang projects which use OpenGL must be able to make do with a fraction of the calls that a C program could use.

zwang · 2021-03-05T19:04:22Z

Does this change limit glow only to generating Golang GL bindings using desktop OpenGL? Thanks

hajimehoshi · 2021-10-23T13:12:31Z

thank you very much for this. I can't understand why it has been abandoned.

Oh I forgot I created this PR... I was waiting for the reviews but reviewers seemed pretty busy.

If there are reviewers, I'm fine to continue this PR.

Jacalz · 2021-10-24T14:58:07Z

We at Fyne would certainly be interested at testing this once it is ready. It has been suggested a few times in the past that we should remove CGo on Windows (see fyne-io/fyne#911).

Jacalz

Not really much of a review. Just a small note on a comment :)

Jacalz · 2021-10-24T15:16:05Z

functions.go

@@ -10,6 +13,33 @@ type Function struct {
 	Return     Type
 }

+// IsImplementedForSyscall reports whether the function is implemented for syscall or not.
+func (f Function) IsImplementedForSyscall() bool {
+	// TODO: Use syscall.Syscall18 when Go 1.12 is the minimum supported version.


Looks like Go 1.12 is the lowest supported version now.

At least in glow. The gl package's mod file still says 1.9. Also, in 1.18 all the syscall functions except SyscallN are deprecated.

Sure, go-gl/gl#144

MatejMagat305 · 2024-02-20T10:56:59Z

Friedly ping ...

hajimehoshi · 2024-02-20T11:29:57Z

I'm quite busy to work on this. I'm fine if someone takes over this

hajimehoshi added 4 commits October 27, 2018 03:57

Remove importing C from conversions.go

09b7832

Fix procaddress.go for Windows

82c14b3

Fix debug.go for Windows

5e0daf8

Fix package.go for Windows

11618d0

dmitshur reviewed Nov 12, 2018

View reviewed changes

functions.go Outdated Show resolved Hide resolved

functions.go Outdated Show resolved Hide resolved

Address on reviews

cabcdf4

hajimehoshi mentioned this pull request Nov 23, 2018

Add No-Brain Jogging to examples faiface/pixel#145

Merged

hajimehoshi commented Dec 8, 2018

View reviewed changes

tmpl/conversions.tmpl Outdated Show resolved Hide resolved

Split conversions.tmpl into Cgo and non-Cgo versions

8f47bcd

hajimehoshi added a commit to hajimehoshi/ebiten that referenced this pull request Dec 8, 2018

graphicsdriver/opengl: Use non-cgo gl package on Windows (#171)

e8a1be7

This is a temporal fix until go-gl/glow#102 is applied.

niaow mentioned this pull request Dec 25, 2018

Adds dependencies to the README.md fyne-io/fyne#52

Merged

Use windows.NewLazySystemDLL for security reason

b844e0e

hajimehoshi commented Jan 5, 2019

View reviewed changes

errcw reviewed Feb 1, 2019

View reviewed changes

hajimehoshi added 3 commits March 12, 2019 10:49

Merge branch 'master' into nocgo

ecaf743

Add common conversions.tmpl

97a0597

Remove Windows macros from package_notwindows.tmpl

471a800

Use NewCallbackCDecl on Windows

2143010

Merge branch 'master' into nocgo

522711e

pwaller mentioned this pull request Mar 30, 2019

Semantic versioning tag for go modules go-gl/gl#102

Open

hajimehoshi commented Apr 3, 2019

View reviewed changes

windexlight mentioned this pull request May 3, 2020

Remove CGo on Windows fyne-io/fyne#911

Open

Jacalz reviewed Oct 24, 2021

View reviewed changes

hajimehoshi closed this Feb 20, 2024

hajimehoshi reopened this Feb 20, 2024

hajimehoshi closed this Feb 20, 2024

Remove Cgo dependency on Windows #102

Remove Cgo dependency on Windows #102

Conversation

hajimehoshi commented Nov 12, 2018 • edited Loading

Design Document

tl;dr

hajimehoshi commented Nov 12, 2018 • edited Loading

hajimehoshi commented Nov 14, 2018

hajimehoshi commented Dec 4, 2018 • edited Loading

pwaller commented Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silbinarywolf commented Jan 27, 2019

errcw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hajimehoshi Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hajimehoshi commented Mar 12, 2019

hajimehoshi commented Mar 25, 2019

hajimehoshi commented Mar 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hajimehoshi Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neclepsio commented Jun 19, 2020

Zyl9393 commented Oct 3, 2020

zwang commented Mar 5, 2021

hajimehoshi commented Oct 23, 2021

Jacalz commented Oct 24, 2021

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatejMagat305 commented Feb 20, 2024

hajimehoshi commented Feb 20, 2024

hajimehoshi commented Nov 12, 2018 •

edited

Loading

hajimehoshi commented Nov 12, 2018 •

edited

Loading

hajimehoshi commented Dec 4, 2018 •

edited

Loading

pwaller commented Dec 4, 2018 •

edited

Loading

hajimehoshi Apr 2, 2019 •

edited

Loading

hajimehoshi Apr 9, 2019 •

edited

Loading