[PATCH 1/3] protect some clobbered variables with volatile

[PATCH 1/3] protect some clobbered variables with volatile

[Jens Gustedt]

When switching optimization to higher levels (-O3) and enable link time
optimization (-flto) gcc finds two variables that might be clobbered
accross longjmp (orig_tail in dynlink) or vfork (f in popen):

src/ldso/dynlink.c:1014:27: warning: variable ‘orig_tail’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
src/stdio/popen.c:21:8: warning: variable ‘f’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]

Trust the analysis of the compiler and protect these variables with
volatile. Both variables are only loaded once or twice, so this should
never cause a performance penalty.

1	1	src/ldso/dynlink.c
1	1	src/stdio/popen.c

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index efbec8f..e19a21f 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -1011,7 +1011,7 @@ void __init_ldso_ctors(void)
 
 void *dlopen(const char *file, int mode)
 {
-	struct dso *volatile p, *orig_tail, *next;
+	struct dso *volatile p, *volatile orig_tail, *next;
 	size_t orig_tls_cnt, orig_tls_offset, orig_tls_align;
 	size_t i;
 	int cs;
diff --git a/src/stdio/popen.c b/src/stdio/popen.c
index ed20f5a..e5fbc4f 100644
--- a/src/stdio/popen.c
+++ b/src/stdio/popen.c
@@ -18,7 +18,7 @@ FILE *popen(const char *cmd, const char *mode)
 {
 	int p[2], op, i;
 	pid_t pid;
-	FILE *f;
+	FILE *volatile f;
 	sigset_t old;
 	const char *modes = "rw", *mi = strchr(modes, *mode);
 
-- 
1.7.9.5

Re: [PATCH 1/3] protect some clobbered variables with volatile

[Rich Felker]

On Sun, Feb 10, 2013 at 11:31:41PM +0100, Jens Gustedt wrote:
> When switching optimization to higher levels (-O3) and enable link time
> optimization (-flto) gcc finds two variables that might be clobbered
> accross longjmp (orig_tail in dynlink) or vfork (f in popen):

This patch looks correct. BTW, popen is about to be replaced to avoid
vfork (using the new posix_spawn instead), but I don't mind applying
this patch first anyway as motivation for why vfork is a bad idea. :-)

Rich

Re: [PATCH 1/3] protect some clobbered variables with volatile

[Rich Felker]

On Sun, Feb 10, 2013 at 11:31:41PM +0100, Jens Gustedt wrote:
> When switching optimization to higher levels (-O3) and enable link time
> optimization (-flto) gcc finds two variables that might be clobbered
> accross longjmp (orig_tail in dynlink) or vfork (f in popen):
> 
> src/ldso/dynlink.c:1014:27: warning: variable ‘orig_tail’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
> src/stdio/popen.c:21:8: warning: variable ‘f’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
> 
> Trust the analysis of the compiler and protect these variables with
> volatile. Both variables are only loaded once or twice, so this should
> never cause a performance penalty.
> 
> 1	1	src/ldso/dynlink.c
> 1	1	src/stdio/popen.c
> 
> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
> index efbec8f..e19a21f 100644
> --- a/src/ldso/dynlink.c
> +++ b/src/ldso/dynlink.c
> @@ -1011,7 +1011,7 @@ void __init_ldso_ctors(void)
>  
>  void *dlopen(const char *file, int mode)
>  {
> -	struct dso *volatile p, *orig_tail, *next;
> +	struct dso *volatile p, *volatile orig_tail, *next;

As far as I can tell, this is a false positive. orig_tail is never
modified between setjmp and longjmp. Static analysis is probably
failing due to subsequent modification to orig_tail after the last
possible point at which a longjmp could occur.

> diff --git a/src/stdio/popen.c b/src/stdio/popen.c
> index ed20f5a..e5fbc4f 100644
> --- a/src/stdio/popen.c
> +++ b/src/stdio/popen.c
> @@ -18,7 +18,7 @@ FILE *popen(const char *cmd, const char *mode)
>  {
>  	int p[2], op, i;
>  	pid_t pid;
> -	FILE *f;
> +	FILE *volatile f;
>  	sigset_t old;
>  	const char *modes = "rw", *mi = strchr(modes, *mode);

Could you explain what the issue is here? I'm not following it. I
intend to remove the vfork usage soon anyway, but I'd like to
understand (and commit a patch with a commit-message documenting what
the problem was) if it's wrong right now for reasons other than the
fact that vfork is wrong to begin with. But on the other hand, I don't
want to commit a cargo-cult patch with a message like "because the
compiler warnings said so"...

Rich

Re: [PATCH 1/3] protect some clobbered variables with volatile

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

Hello Rich,
AFAIR these both occurred when compiling with -flto. I would not trust
the static analysis of the compiler at 100 % either, but this is just
an argument to have the volatile in.

The compiler is telling us that here he thinks that an optimization
might hide a intermittent write, but that by the aliasing rules it
would be OK to save a read. So better be careful and trust the
compiler that he is willing to do bad things :)

Maybe LTO is broken, yet, in that it might do some optimizations that
it shouldn't. Or maybe this exposes possible problems that were
previously hidden because the write was in a different compilation
unit than the read. Who knows.

In any case, the performance hit should be limited, and my personal
preference would tend towards safety and calming down the compiler.

Jens

-- 
:: INRIA Nancy Grand Est :: http://www.loria.fr/~gustedt/   ::
:: AlGorille ::::::::::::::: office Nancy : +33 383593090   ::
:: ICube :::::::::::::: office Strasbourg : +33 368854536   ::
:: ::::::::::::::::::::::::::: gsm France : +33 651400183   ::
:: :::::::::::::::::::: gsm international : +49 15737185122 ::




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]