Re: [PATCH v3 18/25] struct lock_file: declare some fields volatile

[Johannes Sixt <j.sixt@viscovery.net>]

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 <j.sixt@viscovery.net>

Not a big deal, but just in case you re-roll again and you do not forget:

  Johannes Sixt <j6t@kdbg.org>

is preferred.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  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;
>