You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2016/07/04 09:01:49 UTC
RE: svn commit: r1751207 - in /subversion/branches/1.9.x: ./ STATUS subversion/libsvn_subr/win32_crashrpt.c
> -----Original Message-----
> From: svn-role@apache.org [mailto:svn-role@apache.org]
> Sent: maandag 4 juli 2016 06:01
> To: commits@subversion.apache.org
> Subject: svn commit: r1751207 - in /subversion/branches/1.9.x: ./ STATUS
> subversion/libsvn_subr/win32_crashrpt.c
>
> Author: svn-role
> Date: Mon Jul 4 04:00:50 2016
> New Revision: 1751207
>
> URL: http://svn.apache.org/viewvc?rev=1751207&view=rev
> Log:
> Merge the r1663253 group from trunk:
>
> * r1663253, r1704821, r1738659, r1738828
> Fix a few instances of undefined behavior in Win32 crash reporter.
> Justification:
> Potentially crashing in a crash reporter is bad.
> Votes:
> +1: kotkov, ivan, jamessan
>
> Modified:
> subversion/branches/1.9.x/ (props changed)
> subversion/branches/1.9.x/STATUS
> subversion/branches/1.9.x/subversion/libsvn_subr/win32_crashrpt.c
> Modified: subversion/branches/1.9.x/subversion/libsvn_subr/win32_crashrpt.c
> URL:
> http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/libsvn_sub
> r/win32_crashrpt.c?rev=1751207&r1=1751206&r2=1751207&view=diff
> ================================================================
> ==============
> --- subversion/branches/1.9.x/subversion/libsvn_subr/win32_crashrpt.c
> (original)
> +++ subversion/branches/1.9.x/subversion/libsvn_subr/win32_crashrpt.c Mon
> Jul 4 04:00:50 2016
> @@ -53,9 +53,9 @@ HANDLE dbghelp_dll = INVALID_HANDLE_VALU
> #define LOGFILE_PREFIX "svn-crash-log"
>
> #if defined(_M_IX86)
> -#define FORMAT_PTR "0x%08x"
> +#define FORMAT_PTR "0x%08Ix"
> #elif defined(_M_X64)
> -#define FORMAT_PTR "0x%016I64x"
> +#define FORMAT_PTR "0x%016Ix"
> #endif
This changes the Visual C++ 64 bit specifier in the long specifier. On Visual C++ longs are 32 bit when compiling for AMD64. You would need long long in newer VC compilers or the explicit 64 bit support to properly handle this, as was done before the patch
I would say this introduces *more* unspecified behavior.
In practice 64 bit is reserved for each integer type provided in varargs in the Windows AMD64 calling convention, so it doesn't change that much and the runtime result will be +- identical....
But when we use the system sprintf(), fprintf(), anyway... why not just use "0x%p" for all calling conventions?
(Rest of the patch looks ok)
Bert
RE: svn commit: r1751207 - in /subversion/branches/1.9.x: ./ STATUS subversion/libsvn_subr/win32_crashrpt.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Evgeny Kotkov [mailto:evgeny.kotkov@visualsvn.com]
> Sent: donderdag 7 juli 2016 19:02
> To: Bert Huijben <be...@qqmail.nl>
> Cc: Subversion Development <de...@subversion.apache.org>
> Subject: Re: svn commit: r1751207 - in /subversion/branches/1.9.x: ./ STATUS
> subversion/libsvn_subr/win32_crashrpt.c
>
> Bert Huijben <be...@qqmail.nl> writes:
>
> >> #if defined(_M_IX86)
> >> -#define FORMAT_PTR "0x%08x"
> >> +#define FORMAT_PTR "0x%08Ix"
> >> #elif defined(_M_X64)
> >> -#define FORMAT_PTR "0x%016I64x"
> >> +#define FORMAT_PTR "0x%016Ix"
> >> #endif
My comments would have applied if I had correctly noticed that you removed the entire "I64" prefix.. But you actually left the "I" prefix, which is a good change.
Sorry for the confusion,
Bert
For others:
See https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx for the documentation of the Visual C++ specific prefixes.
"I" is explicitly size_t and/or ptrdiff_t sized.
Re: svn commit: r1751207 - in /subversion/branches/1.9.x: ./ STATUS subversion/libsvn_subr/win32_crashrpt.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Bert Huijben <be...@qqmail.nl> writes:
>> #if defined(_M_IX86)
>> -#define FORMAT_PTR "0x%08x"
>> +#define FORMAT_PTR "0x%08Ix"
>> #elif defined(_M_X64)
>> -#define FORMAT_PTR "0x%016I64x"
>> +#define FORMAT_PTR "0x%016Ix"
>> #endif
>
> This changes the Visual C++ 64 bit specifier in the long specifier.
> On Visual C++ longs are 32 bit when compiling for AMD64. You would need
> long long in newer VC compilers or the explicit 64 bit support to properly
> handle this, as was done before the patch
>
> I would say this introduces *more* unspecified behavior.
(Sorry that it took me a while to reply.)
I don't quite see where the behavior we have now is undefined. The values
that we pass to a FORMAT_PTR are either UINT_PTR's or DWORD_PTR's.
Both of them are unsigned 32-bit or 64-bit types, depending on whether we
compile for 32 or for 64 bits. For instance, a DWORD_PTR is defined as
follows, per the spec [1, 2]:
typedef unsigned __int3264 ULONG_PTR;
typedef ULONG_PTR DWORD_PTR;
With that in mind, an "%Ix" format specifier matches the type of what we
pass in varargs, and this behavior is defined.
Or am I missing something?
> But when we use the system sprintf(), fprintf(), anyway... why not just
> use "0x%p" for all calling conventions?
Sounds like a nice improvement that we can probably do in trunk.
[1] https://msdn.microsoft.com/en-us/library/cc230322
[2] https://msdn.microsoft.com/en-us/library/cc230394
Regards,
Evgeny Kotkov