Bug of intrin functions

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug of intrin functions

David Chisnall via cfe-dev
I just find a serious bug. Windows prebuilt binary.
-------------------------------------------------------------------------------------------------------
#include <stdio.h>
#include <intrin.h>

//this function is just copyed from `intrin.h': __movsb(), __x86_64__
static __inline__ void __attribute__((__always_inline__, __nodebug__))
__movsb_buggy(unsigned char *__dst, unsigned char const *__src, size_t __n) {
//for(size_t i=0; i<__n; i++) __dst[i] = __src[i]; //OK
__asm__("rep movsb" : : "D"(__dst), "S"(__src), "c"(__n)); //FAIL
}

int main(int argc, const char ** argv)
{
unsigned char dst[4]={'a', 'b', 'c', '\0'};
unsigned char src[4]={'1', '2', '3', '\0'};
__movsb_buggy(dst, src, 1);
puts((char*)dst); //expect "1bc", but get "bc"
puts((char*)src); //expect "123", but get "23"
return 0;
}
------------------------------------------------------------------------------------------------


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bug of intrin functions

David Chisnall via cfe-dev
On 2017/11/1 22:08, LIANG Xingliang via cfe-dev wrote:

> I just find a serious bug. Windows prebuilt binary.
> -------------------------------------------------------------------------------------------------------
> #include <stdio.h>
> #include <intrin.h>
>
> //this function is just copyed from `intrin.h': __movsb(), __x86_64__
> static __inline__ void __attribute__((__always_inline__, __nodebug__))
> __movsb_buggy(unsigned char *__dst, unsigned char const *__src, size_t
> __n) {
> //for(size_t i=0; i<__n; i++) __dst[i] = __src[i]; //OK
> __asm__("rep movsb" : : "D"(__dst), "S"(__src), "c"(__n)); //FAIL
> }
>
> int main(int argc, const char ** argv)
> {
> unsigned char dst[4]={'a', 'b', 'c', '\0'};
> unsigned char src[4]={'1', '2', '3', '\0'};
> __movsb_buggy(dst, src, 1);
> puts((char*)dst); //expect "1bc", but get "bc"
> puts((char*)src); //expect "123", but get "23"
> return 0;
> }
> ------------------------------------------------------------------------------------------------
>
This implementation looks wrong to me.

First, `movsb` alters RDI, RSI and RCX, so all three parameters must be
in-out ones. Second, the memory area offset from RDI is written to and
the one offset from RSI is read from, but this __asm__ statement fails
to say that.

AFAICT the correct implementation should be:
```
static __inline__ void __attribute__((__always_inline__, __nodebug__))
__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n)
{
   __asm__ ("rep movsb"
     : "+D"(__dst), "+S"(__src), "+c"(__n), "=m"(*(char (*)[])__dst)
     : "m"(*(char (*)[])__src)
   );
}
```


--
Best regards,
LH_Mouse

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Bug of intrin functions

David Chisnall via cfe-dev
Can you file a bug on this? I suspect it was broken in r290539 when the clobber list was removed because it was made an error to clobber an input or output.

~Craig

On Wed, Nov 1, 2017 at 11:05 PM, Liu Hao via cfe-dev <[hidden email]> wrote:
On 2017/11/1 22:08, LIANG Xingliang via cfe-dev wrote:
I just find a serious bug. Windows prebuilt binary.
-------------------------------------------------------------------------------------------------------
#include <stdio.h>
#include <intrin.h>

//this function is just copyed from `intrin.h': __movsb(), __x86_64__
static __inline__ void __attribute__((__always_inline__, __nodebug__))
__movsb_buggy(unsigned char *__dst, unsigned char const *__src, size_t __n) {
//for(size_t i=0; i<__n; i++) __dst[i] = __src[i]; //OK
__asm__("rep movsb" : : "D"(__dst), "S"(__src), "c"(__n)); //FAIL
}

int main(int argc, const char ** argv)
{
unsigned char dst[4]={'a', 'b', 'c', '\0'};
unsigned char src[4]={'1', '2', '3', '\0'};
__movsb_buggy(dst, src, 1);
puts((char*)dst); //expect "1bc", but get "bc"
puts((char*)src); //expect "123", but get "23"
return 0;
}
------------------------------------------------------------------------------------------------

This implementation looks wrong to me.

First, `movsb` alters RDI, RSI and RCX, so all three parameters must be in-out ones. Second, the memory area offset from RDI is written to and the one offset from RSI is read from, but this __asm__ statement fails to say that.

AFAICT the correct implementation should be:
```
static __inline__ void __attribute__((__always_inline__, __nodebug__))
__movsb(unsigned char *__dst, unsigned char const *__src, size_t __n)
{
  __asm__ ("rep movsb"
    : "+D"(__dst), "+S"(__src), "+c"(__n), "=m"(*(char (*)[])__dst)
    : "m"(*(char (*)[])__src)
  );
}
```


--
Best regards,
LH_Mouse

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev