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

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

Open
vadim-za opened this issue Apr 6, 2024 · 7 comments
Open

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

vadim-za opened this issue Apr 6, 2024 · 7 comments
Labels
bug Observed behavior contradicts documented or intended behavior os-windows standard library This issue involves writing Zig code for the standard library.
Milestone

Comments

@vadim-za
Copy link

vadim-za commented Apr 6, 2024

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

  • CreateWindowExW (passing an ATOM value instead of string as lpClassName argument),
  • LoadIconW (passing an integer identifier, such as IDI_APPLICATION, instead of a string as lpIconName argument)
  • etc.

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;
@vadim-za vadim-za added the bug Observed behavior contradicts documented or intended behavior label Apr 6, 2024
@expikr
Copy link
Contributor

expikr commented Apr 6, 2024

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
Copy link
Collaborator

squeek502 commented Apr 6, 2024

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 (#4426).

@vadim-za
Copy link
Author

vadim-za commented Apr 6, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

expikr commented Apr 6, 2024

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
Copy link
Author

vadim-za commented Apr 6, 2024

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
Copy link
Author

vadim-za commented Apr 7, 2024

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.

@andrewrk andrewrk added standard library This issue involves writing Zig code for the standard library. os-windows labels Aug 16, 2024
@andrewrk andrewrk added this to the unplanned milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior os-windows standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

No branches or pull requests

4 participants