On 04/23/2016 05:30 PM, Christian Ullrich wrote:
> * Andrew Dunstan wrote:
>
>> On 04/22/2016 01:21 AM, Michael Paquier wrote:
>
>>>>> 5. It also complains about us casting a pid_t to a HANDLE in
>>>>> pg_basebackup.c. Not sure what to do about that.
>>>> The thing that's being cast is not a PID, but a HANDLE to a process.
>>>> pid_t is a typedef for int (in port/win32.h), therefore is always 32
>>>> bits, while HANDLE is actually void*. However, Microsoft guarantees
>>>> that kernel32 HANDLEs (this includes those to threads and processes)
>>>> fit into 32 bits on AMD64.
>
>>> Yes, when casting things this way I think that a comment would be fine
>>> in the code. We could do that as separate patches actually.
>>
>> We are already casting the pid_t to HANDLE and still getting a warning.
>> Apparently we need to do something on win64 like
>>
>> (HANDLE) ((int64) bgchild)
>
> Ah, OK, it warns about a cast to a larger type because the value might
> get sign extended. Not unreasonable.
>
> In this case, I would prefer this:
>
> diff --git a/src/include/port/win32.h b/src/include/port/win32.h
> index ba8cf9d..b4086f1 100644
> --- a/src/include/port/win32.h
> +++ b/src/include/port/win32.h
> @@ -256,7 +256,7 @@ typedef int gid_t;
> typedef long key_t;
>
> #ifdef WIN32_ONLY_COMPILER
> -typedef int pid_t;
> +typedef intptr_t pid_t;
> #endif
>
> /*
>
> With this change, pg_basebackup -X stream works the same when built
> for 64 and 32 bits.
>
That's a change that will have a pretty wide effect. Everything up to
now has been pretty low risk, but this worries me rather more. Maybe
it's safe, but I'd like to hear others' comments.
cheers
andrew