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

windows/arm: callback regression in upcoming 1.16 #42591

Closed
zx2c4 opened this issue Nov 13, 2020 · 9 comments
Closed

windows/arm: callback regression in upcoming 1.16 #42591

zx2c4 opened this issue Nov 13, 2020 · 9 comments
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 13, 2020

https://go-review.googlesource.com/c/go/+/258938 prevents windows/arm from working, with just a small TODO added to the code indicating that it's broken. That's a big problem. This issue is to track fixing that before 1.16 lands.

https://go-review.googlesource.com/c/go/+/269037 has additional cleanups of that code.

I've got an arm builder ready to test CLs that address this.

@cherrymui @aclements @alexbrainman @bradfitz

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 13, 2020

@gopherbot set tag release-blocker

@ALTree
Copy link
Member

ALTree commented Nov 13, 2020

@zx2c4 If this actually only breaks windows/arm (and won't cause any issue on other platforms), I don't think it should be given release-blocker status.

windows/arm is not a first class port ("Broken builds block releases"). In fact, I would argue it's not even a second-class port: we don't have a builder at the moment (#38607: add a Windows ARM builder). windows/arm is basically in a "do we even support this" status.

We can still tentatively put this in the 1.16 milestone (which I did) but I don't think it'll block the 1.16 release.

@ALTree ALTree added this to the Go1.16 milestone Nov 13, 2020
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 13, 2020
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 13, 2020

Ooof. FWIW, real software is about to ship using windows/arm, because real hardware that uses arm on windows has been successfully commercialized. Hopefully we can get Go into shape for 1.16.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 13, 2020

@aclements As discussed on that CL, I look forward to testing/polishing some things on the hardware. I thought I'd paste here the sample programs (from @rozmansi) that I've been using to test this:

C reference:

#include <windows.h>
#include <stdio.h>

VOID CALLBACK EventCallback(HWINEVENTHOOK hWinEventHook, DWORD event, HWND hwnd, LONG idObject, LONG idChild, DWORD idEventThread, DWORD dwmsEventTime)
{
    fprintf(stdout, "%p %x %p %x %x %x %x\n", hWinEventHook, event, hwnd, idObject, idChild, idEventThread, dwmsEventTime);
}

int main(void)
{
    HWINEVENTHOOK Hook = SetWinEventHook(EVENT_OBJECT_CREATE, EVENT_OBJECT_CREATE, NULL, EventCallback, 0, 0, WINEVENT_SKIPOWNPROCESS|WINEVENT_OUTOFCONTEXT);
    if (!Hook) {
        fprintf(stderr, "Error setting win event hook: 0x%x\n", GetLastError());
        return 1;
    }
    for (;;) {
        MSG Msg;
        if (GetMessageW(&Msg, 0, 0, 0)) {
            TranslateMessage(&Msg);
            DispatchMessageW(&Msg);
        }
    }
    UnhookWinEvent(Hook);
    return 0;
}

Go crasher:

package main

import (
	"fmt"
	"github.com/lxn/win"
)

func EventCallback(hWinEventHook win.HWINEVENTHOOK, event uint32, hwnd win.HWND, idObject int32, idChild int32, idEventThread uint32, dwmsEventTime uint32) uintptr {
	fmt.Printf("%x %x %x %x %x %x %x\n", hWinEventHook, event, hwnd, idObject, idChild, idEventThread, dwmsEventTime)
	return 0
}

func main() {
	hook, err := win.SetWinEventHook(win.EVENT_OBJECT_CREATE, win.EVENT_OBJECT_CREATE, 0, EventCallback, 0, 0, win.WINEVENT_SKIPOWNPROCESS|win.WINEVENT_OUTOFCONTEXT)
	defer win.UnhookWinEvent(hook)
	if err != nil {
		fmt.Printf("Error setting win event hook: %v\n", err)
		return
	}
	for {
		var msg win.MSG
		if m := win.GetMessage(&msg, 0, 0, 0); m != 0 {
			win.TranslateMessage(&msg)
			win.DispatchMessage(&msg)
		}
	}
}

With some minor differences in the format string, these should produce the same output on the console, which one can generate by clicking between various other windows on the system:

image

And here are some (working) pre-compiled binaries: tester.zip

@gopherbot
Copy link

Change https://golang.org/cl/270077 mentions this issue: runtime: simplify C -> Go callbacks on windows/arm

@networkimprov
Copy link

networkimprov commented Nov 18, 2020

Just in case...

@gopherbot add release-blocker
@gopherbot add OS-Windows

@gopherbot
Copy link

Change https://golang.org/cl/271178 mentions this issue: runtime: support new callbackasm1 calling convention on windows/arm

@networkimprov
Copy link

Shouldn't regressions be release-blocker by default?

@ALTree
Copy link
Member

ALTree commented Nov 19, 2020

Shouldn't regressions be release-blocker by default?

@networkimprov No, not at all. Serious regressions in first class ports, maybe. I already explained above that this is not the case.

@ALTree ALTree added the arch-arm Issues solely affecting the 32-bit arm architecture. label Nov 19, 2020
@golang golang locked and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm Issues solely affecting the 32-bit arm architecture. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

4 participants