Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile
From: Johannes Sixt <hidden>
Date: 2016-06-15 23:00:42
Am 4/14/2014 15:54, schrieb Michael Haggerty:
The function remove_lock_file_on_signal() is used as a signal handler. It is not realistic to make the signal handler conform strictly to the C standard, which is very restrictive about what a signal handler is allowed to do. But let's increase the likelihood that it will work: The lock_file_list global variable and several fields from struct lock_file are used by the signal handler. Declare those values "volatile" to increase the chance that the signal handler will see a valid object state.
Yes, it's important that the signal handler sees a valid object state, and "volatile" can help here. But I think the reason why it helps is not obvious, and it should be mentioned in the commit message: It is not so much that "volatile" forces the compiler to lay down each access of the variable coded in C in the assembly code, but more importantly, that "volatile" disallows any re-ordering of these accesses. Then: - 'lock->active = 1' must be the last assignment during setup - 'lock->active = 0' must be the first assignment during tear-down. - Ideally, all members of struct lock_file should be "volatile". The last point is important because the compiler is allowed to re-order accesses to non-"volatile" variables across "volatile" accesses. I say "ideally" because if filename were defined "volatile filename[PATH_MAX]", strcpy() could not be used anymore. OTOH, it is unlikely that a compiler re-orders a strcpy() call across other expressions, and we can get away without "volatile" in the "filename" case in practice.
Suggested-by: Johannes Sixt <redacted>
Not a big deal, but just in case you re-roll again and you do not forget: Johannes Sixt [off-list ref] is preferred.
quoted hunk ↗ jump to hunk
Signed-off-by: Michael Haggerty <redacted> --- cache.h | 6 +++--- lockfile.c | 2 +- refs.c | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-)diff --git a/cache.h b/cache.h index b7af173..9019c7d 100644 --- a/cache.h +++ b/cache.h@@ -538,10 +538,10 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct #define LOCK_SUFFIX_LEN 5 struct lock_file { - struct lock_file *next; + struct lock_file *volatile next; volatile sig_atomic_t active; - int fd; - pid_t owner; + volatile int fd; + volatile pid_t owner; char on_list; char filename[PATH_MAX]; };diff --git a/lockfile.c b/lockfile.c index 50a0541..fce53f1 100644 --- a/lockfile.c +++ b/lockfile.c@@ -69,7 +69,7 @@ * See Documentation/api-lockfile.txt for more information. */ -static struct lock_file *lock_file_list; +static struct lock_file *volatile lock_file_list; static const char *alternate_index_output; static void remove_lock_file(void)diff --git a/refs.c b/refs.c index cb2f825..db8057c 100644 --- a/refs.c +++ b/refs.c@@ -2213,15 +2213,16 @@ int commit_packed_refs(void) struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(&ref_cache); int error = 0; + int fd; if (!packed_ref_cache->lock) die("internal error: packed-refs not locked"); write_or_die(packed_ref_cache->lock->fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER)); + fd = packed_ref_cache->lock->fd; do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache), - 0, write_packed_entry_fn, - &packed_ref_cache->lock->fd); + 0, write_packed_entry_fn, &fd); if (commit_lock_file(packed_ref_cache->lock)) error = -1; packed_ref_cache->lock = NULL;