Re: [Qemu-devel] [PATCH] copy, dd: simplify and optimize NUL bytes detection

[Pádraig Brady]

On 24/10/15 03:24, Pádraig Brady wrote:
> On 23/10/15 12:15, Pádraig Brady wrote:
>> On 22/10/15 20:47, Paolo Bonzini wrote:
>>>
>>>
>>> On 22/10/2015 19:39, Radim Krčmář wrote:
>>>> 2015-10-22 18:14+0200, Paolo Bonzini:
>>>>> On 22/10/2015 18:02, Eric Blake wrote:
>>>>>> I see a bug in there:
>>>>>
>>>>> Of course.  You shouldn't have told me what the bug was, I deserved
>>>>> to look for it myself. :)
>>>>
>>>> It rather seems that you don't want spoilers, :)
>>>>
>>>> I see two bugs now.
>>>
>>> Me too. :)  But Rusty surely has some testcases in case he wants to
>>> adopt some of the ideas here. O:-)
>>
>> For completeness this should address the bugs I think?
>>
>> bool memeqzero4_paolo(const void *data, size_t length)
>> {
>>     const unsigned char *p = data;
>>     unsigned long word;
> 
> Note the original coreutils code accessed a word at a time,
> but only on aligned buffers. The code below may generate
> unaligned accesses for unaligned sizes or buffers.
> You can avoid that by auto degenerating back to byte
> at a time access using:
> 
> #if _STRING_ARCH_unaligned
>   unsigned long word;
> #else
>   unsigned char word;
> #endif

Note _STRING_ARCH_unaligned being defined at the glibc level
doesn't guarantee the compiler wont do something "defined"
when vectorizing code, or perhaps removing sanity checks like:
  #define ALIGNED_POINTER(ptr, type) ((size_t) (ptr) % alignof (type) == 0)
by assuming that the lower bits of the pointer are zero
as that's the only defined operation?

One gets stronger guarantees from a compiler define, like
__ARM_FEATURE_UNALIGNED which gcc -munaligned-access defines for arm,
though we're still in undefined territory and -fsanitize=undefined
will warn about such accesses.  You'd have to explicitly avoid
such warnings with something like:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=5760532

Given coreutils only uses is_nul for relatively large buffers,
I'm going to avoid the above complexity, since the focus of this
coreutils patch was simplification, and only enable the byte at a time access.

cheers,
Pádraig.