Re: [PATCH 3/5] setupapi: Fix compiler warnings when converting between pointers and integers on 64-bit systems
Hugh McMaster hugh.mcmaster@outlook.com writes:
Signed-off-by: Hugh McMaster hugh.mcmaster@outlook.com
dlls/setupapi/devinst.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
That's only hiding the bug.
On Tuesday, 8 August 2017 9:04 PM, Alexandre Julliard wrote:
That's only hiding the bug.
The basic problem is the use of DWORD in the following structure. On 64-bit systems we then see warnings about the sizeof(DWORD) vs sizeof(void *).
/* Pointed to by SP_DEVINFO_DATA's Reserved member */ struct DeviceInfo { struct DeviceInfoSet *set; HKEY key; BOOL phantom; DWORD devId; LPWSTR instanceId; struct list interfaces; };
There are several ways to fix the core problem. One option is to replace the DWORD in that struct with DWORD_PTR or ULONG_PTR. Both types are identical, with DWORD_PTR being a typedef'd ULONG_PTR.
Another option is to typedef a QWORD, and then use C preprocessing: #ifdef _WIN64 QWORD devId; #else DWORD devID; #endif
Unfortunately, however we choose to fix this part, using (multiple) casts will still be necessary to prevent the compiler warning about mismatched size conversion between pointers and integers.
For example, in line 471:
devInfo->devId = (DWORD)devInst;
devInst is a HANDLE (i.e. void *).
Line 4027 in dlls/setupapi/devinst.c: struct DeviceInfo *ppdevInfo = GlobalLock((HANDLE)dnDevInst); dnDevInst is type DEVINST, which is a typedef'd DWORD.
How do you want to proceed?
-- Hugh McMaster
On 08/09/2017 04:26 AM, Hugh McMaster wrote:
On Tuesday, 8 August 2017 9:04 PM, Alexandre Julliard wrote:
That's only hiding the bug.
The basic problem is the use of DWORD in the following structure. On 64-bit systems we then see warnings about the sizeof(DWORD) vs sizeof(void *).
/* Pointed to by SP_DEVINFO_DATA's Reserved member */ struct DeviceInfo { struct DeviceInfoSet *set; HKEY key; BOOL phantom; DWORD devId; LPWSTR instanceId; struct list interfaces; };
There are several ways to fix the core problem. One option is to replace the DWORD in that struct with DWORD_PTR or ULONG_PTR. Both types are identical, with DWORD_PTR being a typedef'd ULONG_PTR.
Another option is to typedef a QWORD, and then use C preprocessing: #ifdef _WIN64 QWORD devId; #else DWORD devID; #endif
Unfortunately, however we choose to fix this part, using (multiple) casts will still be necessary to prevent the compiler warning about mismatched size conversion between pointers and integers.
For example, in line 471:
devInfo->devId = (DWORD)devInst;
devInst is a HANDLE (i.e. void *).
Line 4027 in dlls/setupapi/devinst.c: struct DeviceInfo *ppdevInfo = GlobalLock((HANDLE)dnDevInst); dnDevInst is type DEVINST, which is a typedef'd DWORD.
How do you want to proceed?
Your proposals will keep truncating a pointer to the lower 32 bits for Win64. That's what Alexandre meant with just masking the error.
As DevId is a DWORD (struct SP_DEVINFO_DATA from the public API) wouldn't the correct fix be to move away from DevId being just a pointer? https://msdn.microsoft.com/en-us/library/windows/hardware/ff552344(v=vs.85).... It could be an index or a hash, something that fits into 32 bits.
Aric might have some insight here as he looked at the Plug&Play manager as well as at the Device Tree.
bye michael
Hello,
On 8/9/17 5:36 AM, Michael Stefaniuc wrote:
On 08/09/2017 04:26 AM, Hugh McMaster wrote:
On Tuesday, 8 August 2017 9:04 PM, Alexandre Julliard wrote:
That's only hiding the bug.
The basic problem is the use of DWORD in the following structure. On 64-bit systems we then see warnings about the sizeof(DWORD) vs sizeof(void *).
/* Pointed to by SP_DEVINFO_DATA's Reserved member */ struct DeviceInfo { struct DeviceInfoSet *set; HKEY key; BOOL phantom; DWORD devId; LPWSTR instanceId; struct list interfaces; };
There are several ways to fix the core problem. One option is to replace the DWORD in that struct with DWORD_PTR or ULONG_PTR. Both types are identical, with DWORD_PTR being a typedef'd ULONG_PTR.
Another option is to typedef a QWORD, and then use C preprocessing: #ifdef _WIN64 QWORD devId; #else DWORD devID; #endif
Unfortunately, however we choose to fix this part, using (multiple) casts will still be necessary to prevent the compiler warning about mismatched size conversion between pointers and integers.
For example, in line 471:
devInfo->devId = (DWORD)devInst;devInst is a HANDLE (i.e. void *).
Quick correction here being a handle does not mean it is a HANDLE (i.e. void *). My work on windows seems to show that devnode handles tend to just be small incremental numbers (1, 2, 3, 4. etc) clearly they are references to some internal structure or such.
Line 4027 in dlls/setupapi/devinst.c: struct DeviceInfo *ppdevInfo = GlobalLock((HANDLE)dnDevInst); dnDevInst is type DEVINST, which is a typedef'd DWORD.
How do you want to proceed?
Your proposals will keep truncating a pointer to the lower 32 bits for Win64. That's what Alexandre meant with just masking the error.
As DevId is a DWORD (struct SP_DEVINFO_DATA from the public API) wouldn't the correct fix be to move away from DevId being just a pointer? https://msdn.microsoft.com/en-us/library/windows/hardware/ff552344(v=vs.85).... It could be an index or a hash, something that fits into 32 bits.
Aric might have some insight here as he looked at the Plug&Play manager as well as at the Device Tree.
I do no have as much insight into the setupapi parts of this as I had just grazed the surface there with my HID work. However I do know that on windows devnodes are NOT pointers, they are handles. Using pointers instead of handles is a fast and easy thing done in many places in wine which bites us as we move to 64-bit. As I said I just scratched the surface of setupapi so I do not know the correct path forward here, but expanding the structure would not be, in my opinion.
Likely we would need to build those handles, maybe based on user handles or something as they would need to be global.
-aric
-
Alexandre Julliard -
Aric Stewart -
Hugh McMaster -
Michael Stefaniuc