]> granicus.if.org Git - graphviz/commit
sfio: remove 'SF_MTSAFE'
authorMatthew Fernandez <matthew.fernandez@gmail.com>
Mon, 25 Apr 2022 04:30:44 +0000 (21:30 -0700)
committerMatthew Fernandez <matthew.fernandez@gmail.com>
Fri, 29 Apr 2022 00:01:33 +0000 (17:01 -0700)
commit64edb09accf11480adf9452b317bfa25129a09be
tree1f3c1ec9296cd0eabb420b1117dac4a225adca80
parent96703e462f10cb798272e9820a05df956096fca9
sfio: remove 'SF_MTSAFE'

This constant was performing two overlapping roles:

  1. A flag callers could use to indicate they anticipated calling into sfio
     functions with the same object from multiple threads.

  2. A flag used by the library itself to indicate whether thread-safety was
     available.

To deal with both macros having the same name, sfhdr.h was forcing the value
seen internally to zero. This mechanism has numerous shortcomings:

  1. It was possible for a caller to use the external value of `SF_MTSAFE`
     (010000) that, thanks to the overriding described above, sfio itself would
     fail to understand.

  2. Including sfio.h within sfio sources without including sfhdr.h was a latent
     foot gun that would result in part of sfio believing the `SF_MTSAFE` flag
     _did_ have meaning.

  3. sfio functions are not thread-safe, with or without `SF_MTSAFE` defined,
     and regardless of what value it has.

To take an example to illustrate 3, `sfopen` does something like double-checked
locking¹ to anticipate another thread modifying `f` in-between its first check
and the point at which it calls `SFMTXSTART`. Except `SFMTXSTART` is not a lock.
There is no synchronization point here, and in fact an optimizing compiler will
simply coalesce both the first and second checks into one. It is not clear to me
whether this code was written when multicore machines were uncommon (though this
design is also unsafe on a unicore machine) or if `SFMTXSTART` is some kind of
placeholder that was meant to one day grow a locking mechanism.

It seems safer and more honest to remove this constant, and with it the
implication that thread-safety is toggle-able. Migrating to C stdio (#1998) will
render all of this moot as it is thread-safe by default.

¹ https://en.wikipedia.org/wiki/Double-checked_locking
lib/sfio/sfextern.c
lib/sfio/sfhdr.h
lib/sfio/sfio.h
lib/sfio/sfopen.c