ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
34.65k stars 2.53k forks source link

std.os.windows.LPCWSTR has wrong alignment requirements #19560

Open vadim-za opened 6 months ago

vadim-za commented 6 months ago

Zig Version

0.12.0-dev.3193+4ba4f94c9

Steps to Reproduce and Observed Behavior

Occasionally, in Windows API, integer values need to be passed as LPCWSTR parameters to functions. Examples include

These values are not required to be even. However casting them to std.os.windows.LPCWSTR causes compile errors or runtime panics if the values are not even. E.g.

pub extern "user32" fn LoadIconW(
    ?std.os.windows.HINSTANCE,
    std.os.windows.LPCWSTR,
) callconv(os.WINAPI) ?std.os.windows.HICON;

pub fn main() !void {
    const IDI_HAND = 32513;
    _ = LoadIconW(null, @as(std.os.windows.LPCWSTR, @ptrFromInt(IDI_HAND)));
}

causes compilation error pointer type '[*:0]const u16' requires aligned address.

Expected Behavior

Being able to cast odd integer values to std.os.windows.LPCWSTR. Probably also being able to do the same for std.os.windows.LPWSTR.

Proposed solution: change the definition(s) of LPCWSTR (and LPWSTR) to

pub const LPCWSTR = [*:0]align(1) const WCHAR;
expikr commented 6 months ago

For extern functions that take multipurposed pointers, perhaps anyopaque would be more appropriate.

const std = @import("std");

pub extern "user32" fn LoadIconW(
    ?std.os.windows.HINSTANCE,
    *const anyopaque,
) callconv(std.os.windows.WINAPI) ?std.os.windows.HICON;

pub fn main() !void {
    const IDI_HAND: usize = 32513;
    _ = LoadIconW(null, @ptrFromInt(IDI_HAND));
}
squeek502 commented 6 months ago

I tend to agree that this should probably be dealt with on the function binding side rather than changing LPCWSTR. I've outlined some options here:

https://ziggit.dev/t/how-to-reimplement-win32-makeintresource-macro-in-zig/3359/4

Maybe there's a place for a [*:0]align(1) const WCHAR; type in the standard library but I'm not sure it'd be used internally by Zig so it might be rejected (https://github.com/ziglang/zig/issues/4426).

vadim-za commented 6 months ago

I've outlined some options here:

I'm already doing this as a workaround in my own code. However it seems to me that Windows API, being essentially C/C++-centric, doesn't impose any alignment requirements on the pointers in general, at the very least not on the type level. That is, C/C++'s const WCHAR * translates to [*:0]align(1) const WCHAR rather than [*:0]const WCHAR in Zig.

squeek502 commented 6 months ago

That's a fair argument, align(1) is definitely more generally practical. I'm not sure where the standard library Windows definitions intend to lie on the practicality vs strictness scale.

expikr commented 6 months ago

https://stackoverflow.com/questions/77944010/win32-wide-character-string-alignment-requirements#comment137621493_77944010

Unless otherwise specified, the Windows ABI requires that all data types are naturally aligned for their type. Some processors crash on misaligned data. – Raymond Chen Feb 26 at 22:21

vadim-za commented 6 months ago

Unless otherwise specified, the Windows ABI requires that all data types are naturally aligned for their type.

Fair enough. I guess then it's a matter of changing the LPCWSTR type in a selected subset of API method arguments to another type, like in @squeek502's link. I'd leave it to the core team members then to decide, whether to simply close this bug.

vadim-za commented 6 months ago

For extern functions that take multipurposed pointers, perhaps anyopaque would be more appropriate.

It seems to me anyopaque would be too relaxed, @squeek502's approach seems to be better to me. Or one could consider using an extern union listing exactly the allowed types.

Upd: tried unions, obviously they don't work since their argument passing conversion is different. Doh.

Upd2: actually they do work, the convention is the same, at least on x64, but there is a pitfall. The following does not work:

pub const ClassIdW = extern union {
    name: std.os.windows.LPCWSTR,
    atom: std.os.windows.ATOM,
};

The problem is not obvious at the first sight. One cannot use ATOM type for the second field, it must be usize, otherwise the upper bytes will be undefined. This could be fine though for passing ClassIDs to WINAPI, a bit more questionable for receiving them.