Remove usage of unsafe.Pointer in winc

[leaanthony]

Compiling with -race causes a standard windows app to crash. This fixes that. Potential fix for #1554

[leaanthony]

Not sure this is the correct fix, but it does fix the problem.

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Pages

Latest commit:	847857a
Status:	✅  Deploy successful!
Preview URL:	https://9367ccce.wails-website.pages.dev
Branch Preview URL:	https://bugfix-windows-race-detectio.wails-website.pages.dev

View logs

[stffabi]

The MakeIntResource which normally is a macro in win32 is very special.

It returns a pointer whose value is equal to the int value, so it does not point to an address with the int value but to an address equal to the int value. I think this might be only fixable by changing the return type to an uintptr. But since the recieving method could also be called with an string pointer it would be maybe the best to directly call the loadicon with the uintptr.

[stffabi]

So I think we now return the wrong value.

[leaanthony]

@stffabi - yeah, I understand the problem a bit more now. It appears that the use of unsafe pointer and casting to a *uint16 is how most projects on GitHub do this. Any thoughts?

[stffabi]

I think we would need to work with a uintptr type instead of a *uint16. Which would need some changes on the caller side but I think that might be the only way to satisfy the race detector.

@leaanthony If you would like I could add a proposal to this PR.

[leaanthony]

Thanks @stffabi ! I did try changing to uintptr and got reasonably far but it didn't appear to change anything. I was probably doing it wrong

[stffabi]

Thanks @stffabi ! I did try changing to uintptr and got reasonably far but it didn't appear to change anything. I was probably doing it wrong

You have still seen the race error or the missing icon?

[stffabi]

I've added a proposal, with this the icon will be displayed correctly. I wasn't able to test the --race because it seems like it's not supported on arm64 and I only have Windows ARM64.

[leaanthony]

I've just pushed a fix for EnumProc which was failing in a similar way.

[stffabi]

I think there might be still one edge case, the &monitorContainer pointer should be passed as unsafe.Pointer and not uintptr here:

succeeded := w32.EnumDisplayMonitors(dc, nil, syscall.NewCallback(EnumProc), uintptr(unsafe.Pointer(&monitorContainer)))

Otherwise go might not know that this pointer is escaping to the syscall and when growing the stack the pointer might become invalid.

[leaanthony]

Good catch. I've pushed a commit that I think fixes what you raised. Please review @stffabi 👍

[stffabi]

LGTM 👍

[stffabi]

Just for doc purposes, if one needs to just have the checkptr active without go race (which needs CGO) one can just use the following flags.

go build -gcflags "all=-N -l -d=checkptr"