Re: [PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

[Alexandre Julliard]

Kirill Smirnov  writes:
> @@ -92,7 +92,8 @@ static void X11DRV_ImmSetInternalString(DWORD dwOffset,
>      ptr_new = CompositionString + byte_offset;
>      memmove(ptr_new + byte_length, ptr_new + byte_selection,
>              dwCompStringLength - byte_offset - byte_selection);
> - memcpy(ptr_new, lpComp, byte_length);
> - /* Do not pass NULL even if the length == 0 */
> - if (lpComp) memcpy(ptr_new, lpComp, byte_length);
> dwCompStringLength += byte_expansion;

Why would that be a problem?

[PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

[Kirill Smirnov]

According to the standard, memcpy() and its friends
should never accept NULL as argument, otherwise
the behavior is undefined and optimizer takes this
into account.

Below in the same file (line 206) caller passes
NULL as lpComp argument triggering this case.

This patch is of the same nature as the following:

commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0
Author: Kirill Smirnov 
Date:   Mon May 9 18:56:14 2016 +0300

winhlp32: Do not pass NULL to strchr() (spotted by clang).

Signed-off-by: Kirill K. Smirnov 
     Signed-off-by: Alexandre Julliard

On 08.07.2016 06:16, Alexandre Julliard wrote:
> Kirill Smirnov  writes:
> > @@ -92,7 +92,8 @@ static void X11DRV_ImmSetInternalString(DWORD dwOffset,
> >      ptr_new = CompositionString + byte_offset;
> >      memmove(ptr_new + byte_length, ptr_new + byte_selection,
> >              dwCompStringLength - byte_offset - byte_selection);
> > - memcpy(ptr_new, lpComp, byte_length);
> > - /* Do not pass NULL even if the length == 0 */
> > - if (lpComp) memcpy(ptr_new, lpComp, byte_length);
> > dwCompStringLength += byte_expansion;
>
> Why would that be a problem?

[PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

[Alexandre Julliard]

Kirill Smirnov  writes:
> According to the standard, memcpy() and its friends
> should never accept NULL as argument, otherwise
> the behavior is undefined and optimizer takes this
> into account.
>
> Below in the same file (line 206) caller passes
> NULL as lpComp argument triggering this case.
>
> This patch is of the same nature as the following:
>
> commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0
> Author: Kirill Smirnov 
> Date:   Mon May 9 18:56:14 2016 +0300
> winhlp32: Do not pass NULL to strchr() (spotted by clang).

strchr has to access the pointer so obviously it can't take NULL, but
memcpy with a zero length shouldn't, so it's not very useful compiler
behavior IMO. Sadly it seems the standard does allow it...

[PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)

[Matteo Bruni]

2016-07-09 8:42 GMT+02:00 Alexandre Julliard :
> Kirill Smirnov  writes:
> > According to the standard, memcpy() and its friends
> > should never accept NULL as argument, otherwise
> > the behavior is undefined and optimizer takes this
> > into account.
> >
> > Below in the same file (line 206) caller passes
> > NULL as lpComp argument triggering this case.
> >
> > This patch is of the same nature as the following:
> >
> > commit 5a3ad0ecf08c00fc0b7b2f24e55be27b7c3f5da0
> > Author: Kirill Smirnov 
> > Date:   Mon May 9 18:56:14 2016 +0300
> > winhlp32: Do not pass NULL to strchr() (spotted by clang).
>
> strchr has to access the pointer so obviously it can't take NULL, but
> memcpy with a zero length shouldn't, so it's not very useful compiler
> behavior IMO. Sadly it seems the standard does allow it...

Yeah and gcc has also been exploiting that for more aggressive
optimizations for a while (see
https://gcc.gnu.org/gcc-4.9/porting_to.html).
deb274226783ab886bdb44876944e156757efe2b was probably the one
significant precedent for Wine.