Problems with the new `windows-bindgen` in the rust standard library

[ChrisDenton]

Summary of current issues with updating the standard library to the new windows-bindgen:

• PUNICODE_STRING aliases to *mut LSA_UNICODE_STRING instead of *mut UNICODE_STRING (fixed in windows-clang avoid macro expansion #4759)
• FILE_DISPOSITION_INFO single field is called DeleteFileA instead of DeleteFile (fixed in windows-clang avoid macro expansion #4759)
• INVALID_SOCKET is a u32 on x86 and u64 otherwise on 64-bit. The type would be better as usize. (fixed in windows-clang avoid macro expansion #4759)
• For NtCreateFile and NtOpenFile parameters, mutable pointers (*mut) are used where the SAL annotation is IN only (*const). This is not particularly essential but it is nice to have (it's likely also true of other functions). (fixed in )
• NTSTATUS codes are sometimes u32 instead of NTSTATUS, See STATUS_PENDING, STATUS_INVALID_PARAMETER, STATUS_INVALID_HANDLE (fixed in windows-clang STATUS_* constants now come from ntstatus.h and are correctly typed as NTSTATUS #4760)
• The generator uses \r\n line endings whereas before it was \n (fixed in windows-bindgen now explicitly defaults to newline_style=Unix #4761)
• FARPROC is defined to return i32 on x86 and isize otherwise. It would be better if this was always isize. (fixed in windows-clang normalize unmerged scalar signatures #4763)
• There are a lot of cases where i32 values need to be converted to u32 to use them in functions or structures, see windows-bindgen (trunk) / SW_NORMAL / u32 vs i32 #4725

Working as intended

• _PROC_THREAD_ATTRIBUTE_LIST has a leading underscore (Problems with the new windows-bindgen in the rust standard library #4758 (comment))
• Some IPPROTO constants are i32 instead of IPPROTO. See IPPROTO_IP and IPPROTO_RM (Problems with the new windows-bindgen in the rust standard library #4758 (comment))
• The winsock AF_ constants are 32-bit and not 16-bit as the metadata does not know they're used as ADDRESS_FAMILY types.

Experimental PR to the rust-lang/rust repo: rust-lang/rust#160185

[ChrisDenton]

Also the generator uses \r\n line endings whereas before it was \n.

[kennykerr]

#4760 fixes the NTSTATUS inconsistency.

[kennykerr]

#4759 fixes the macro expansion issue you mentioned earlier.

[ChrisDenton]

Thanks! I confirmed that #4759 fixes the DeleteFileA field as well as the definitions of INVALID_SOCKET and PUNICODE_STRING.

[kennykerr]

#4761 fixes the newline issue.

[kennykerr]

#4762 adds support for the older style annotations used by NtCreateFile and friends.

[kennykerr]

Regarding _PROC_THREAD_ATTRIBUTE_LIST it is defined as follows:

typedef struct _PROC_THREAD_ATTRIBUTE_LIST *PPROC_THREAD_ATTRIBUTE_LIST, *LPPROC_THREAD_ATTRIBUTE_LIST;

This is unlike other typedefs (that windows-clang handles) like typedef enum _THREAD_INFORMATION_CLASS {...} THREAD_INFORMATION_CLASS; where the leading underscore appears to be removed.

The difference is that that THREAD_INFORMATION_CLASS directly names the enum type so the name can be replaced, but for _PROC_THREAD_ATTRIBUTE_LIST there is no other name. The only other names are those used for pointer type aliases. Those parameter types are typically expanded to raw pointers so that the various SAL annotations for constness can be expressed/applied.

[kennykerr]

Regarding IPPROTO, some of the constants are defined within the enum and some are not. See for example ws2def.h where you'll see something like this:

//
// N.B. required for backwards compatability to support 0 = IP for the
// level argument to get/setsockopt.
//
#define IPPROTO_IP              0

//
// Protocols.  The IPv6 defines are specified in RFC 2292.
//
typedef enum {
#if(_WIN32_WINNT >= 0x0501)
    IPPROTO_HOPOPTS       = 0,  // IPv6 Hop-by-Hop options
#endif//(_WIN32_WINNT >= 0x0501)
    IPPROTO_ICMP          = 1,
    IPPROTO_IGMP          = 2,
    IPPROTO_GGP           = 3,
.
.
.
} IPPROTO, *PIPROTO;

But it should not matter since IPPROTO ends up being defined as an i32 type alias by windows-bindgen like so:

pub type IPPROTO = i32;
pub const IPPROTO_IP: i32 = 0;
pub const IPPROTO_IPV4: IPPROTO = 4;

[ChrisDenton]

Ah, that's fair. I'll file _PROC_THREAD_ATTRIBUTE_LIST and IPPROTO under working as intended (in that they match the C headers).

[ChrisDenton]

Additionally:

• The winsock AF_ constants would ideally be of type ADDRESS_FAMILY (e.g. AF_INET, AF_UNIX, etc)
• It would be nice if the Win32 errors were u32 because GetLastError returns a DWORD.

[kennykerr]

It would be nice if the Win32 errors were u32 because GetLastError returns a DWORD.

Yes, I tend to agree with you and why I tilted it in that direction but the pushback in #4725 is the question. I'll try to re-summarize that issue here with some more concrete findings.

[kennykerr]

#4763 fixes the FARPROC normalization.

[kennykerr]

Regarding ADDRESS_FAMILY, this is much the same problem as these are just loose constants with not discernable association in ws2def.h:

typedef USHORT ADDRESS_FAMILY;
.
.
.
.
#define AF_UNSPEC       0               // unspecified
#define AF_UNIX         1               // local to host (pipes, portals)
#define AF_INET         2               // internetwork: UDP, TCP, etc.
#define AF_IMPLINK      3               // arpanet imp addresses

I think this would be improved with the signed/unsigned normalization though.

[ChrisDenton]

The fact that ADDRESS_FAMILY is 16-bit instead of 32-bit does make it different from other cases. So in this specific case signed/unsigned normalization would only be a marginal improvement. Though it does seem to be a general problem that there's no association within the code (at best it can be inferred from textual grouping or comments).

[kennykerr]

Open to suggestions but this is about as faithful/accurate as we can get without going down the same rabbit trail that the Win32metadata project did with all of its editorializing. I don't mind general normalization like the signed/unsigned thing, but beyond that we're kinda stuck until the Windows SDK headers provide some more clarity.