Source code comment justifying thread sanitizer failure in walCheckpoint is wrong

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

Source code comment justifying thread sanitizer failure in walCheckpoint is wrong

Valentin
In the function `int walCheckpoint(...)` in file `wal.c` at line 1820
there is the following comment

/* Thread-sanitizer reports that the following is an unsafe read,
** as some other thread may be in the process of updating the value
** of the aReadMark[] slot. The assumption here is that if that is
** happening, the other client may only be increasing the value,
** not decreasing it. So assuming either that either the "old" or
** "new" version of the value is read, and not some arbitrary value
** that would never be written by a real client, things are still
** safe. */
u32 y = pInfo->aReadMark[i];

> assuming either that either the "old" or  "new" version of the value is read, and not some arbitrary value that would never be written by a real client

This  assumption is false according to cpprefeference [1].
Specifically, what happens here is a data race which is undefined
behavior.

I do not know whether the code is unsafe in practice. It is possible
that it is deemed safe because synchronization happens elsewhere or
because common compilers on common architectures do not exhibit
undefined behavior in this case. Either way, the comment lacks detail
because it does not agree with the standard.

Even if the code is safe in practice today, it should still avoid
undefined behavior in order to avoid future problems.

[1] https://en.cppreference.com/w/c/language/memory_model
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|

Re: Source code comment justifying thread sanitizer failure in walCheckpoint is wrong

Richard Hipp-3
On 11/27/19, Valentin <[hidden email]> wrote:
> In the function `int walCheckpoint(...)` in file `wal.c` at line 1820
> there is the following comment

Comment now enhanced.

--
D. Richard Hipp
[hidden email]
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users