Re: memcmp-sse4.S EqualHappy bug

[Andrea Arcangeli <aarcange at redhat dot com>]

On Wed, Jun 17, 2015 at 08:59:52PM +0200, Szabolcs Nagy wrote:
> c11 has threads and a memory model that makes concurrency issues
> observable in standard c.
> 
> however you have a data race that is undefined behaviour:
> 
> objects passed to memcmp are not supposed to be modified concurrently
> without synchronization.

All right, I expected this was not a bug by the standard.

In my initial test code I had a contended pthread mutex at the start
of the page that was passed to memcmp, I wasn't flipping bits myself,
but I guess that's still undefined behavior if it's
pthread_mutex_lock/unlock that are changing the memory passed to memcmp.

> that is unfortunate but i think your test code should be fixed.

That's ok, I already fixed it my test code after realizing the
problem or it wouldn't run correctly on most x86-64 out there.

> (to avoid the observed behaviour the libc would have to guarantee
> atomic memcmp which is nontrivial to do)

The end of the memory passed to memcmp is never changing and it is
always different. I didn't expect an atomic behavior within the part
of the page that is being changed, full atomicity is not to be
expected and it was never provided by any version.

If sometime memcmp returns != 0 inside the non-atomic part of the
memory that's fine. What was unexpected is that sometime it returns 0
without checking the end of the memory that is always different and
never changes.

I made a proposal that could fix the unexpected behavior still without
requiring a full atomic memcmp, but then it's up to you. My proposed
change wouldn't affect the sse4 fast paths but it clearly would have
overhead to propagate the "length" in rdx and to check it before
returning 0, perhaps a dozen asm instruction more for each memcmp in
average (independent of the length).

If no change is made to memcmp/bcmp to avoid the unexpected behavior
in the undefined case, I would suggest to write about this detail in
memcmp and bcmp manpage, as it could save some time and it's better
this new behavior gets advertised.

Thanks,
Andrea