more on missing volatile qualifications

more on missing volatile qualifications

[Jens Gustedt]

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

Hi,
I forced my compiler into reporting inconsistencies concerning the
usage (or not) of volatile, and I came up with three other spots that
I would like to patch. These are

 - the definition of pthread_once_t
 - the definition of pthread_spinlock_t
 - the handler array in sigaction.c

All three could benefit for an additional volatile qualification. All
their usages are already so, so this would just be conservative and
not risk any incompatibilities, I think.

Also, I can't think of any semantics for the three, where opitimizing
out loads or stores makes any sense, so this also should never see any
kind of performance regression.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: more on missing volatile qualifications

[Szabolcs Nagy]

* Jens Gustedt <jens.gustedt@inria.fr> [2017-06-25 10:45:16 +0200]:
> 
>  - the definition of pthread_once_t
>  - the definition of pthread_spinlock_t
>  - the handler array in sigaction.c
> 
> All three could benefit for an additional volatile qualification. All
> their usages are already so, so this would just be conservative and
> not risk any incompatibilities, I think.

pthread_once_t and pthread_spinlock_t qualifiers are
visible in the c++ name mangling if a c++ function takes
pointer to them as arguments so the change is an abi break.

> Also, I can't think of any semantics for the three, where opitimizing
> out loads or stores makes any sense, so this also should never see any
> kind of performance regression.

there was a case in glibc when volatile caused problems:
some generic atomic macro tried to create a temporary using

  __typeof(*(p)) __tmp = *(p);

but then __tmp become volatile and operations on it generated
useless load/stores to the stack. it could be worked around as

  __typeof( (__typeof(*(p))) *(p) ) __tmp = *(p);

is not volatile because the cast expression is unqualified.
(musl does not have such __typeof hacks, but it is an
example where volatile caused unexpeced regression)

Re: more on missing volatile qualifications

[Jens Gustedt]

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

Hello Szabolcs,

On Sun, 25 Jun 2017 12:17:04 +0200 Szabolcs Nagy <nsz@port70.net> wrote:

> pthread_once_t and pthread_spinlock_t qualifiers are
> visible in the c++ name mangling if a c++ function takes
> pointer to them as arguments so the change is an abi break.

too bad, so we can't change these two

There is a reading of the C standard that says that volatile only has
implications if an object itself is such qualified, having a volatile
qualified lvalue access isn't enough. I don't think that any current
compiler does such weird things, but who knows where optimisers will
go in the future.

> > Also, I can't think of any semantics for the three, where
> > opitimizing out loads or stores makes any sense, so this also
> > should never see any kind of performance regression.  
> 
> there was a case in glibc when volatile caused problems:
> some generic atomic macro tried to create a temporary using
> 
>   __typeof(*(p)) __tmp = *(p);
> 
> but then __tmp become volatile and operations on it generated
> useless load/stores to the stack. it could be worked around as
> 
>   __typeof( (__typeof(*(p))) *(p) ) __tmp = *(p);
> 
> is not volatile because the cast expression is unqualified.
> (musl does not have such __typeof hacks, but it is an
> example where volatile caused unexpeced regression)

AFAICS for the third finding in sigaction.c this would not be an
issue. Since in addition this is something dealing with signal stuff,
I still think that volatile would be in order, here.

Thanks
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: more on missing volatile qualifications

[Rich Felker]

On Sun, Jun 25, 2017 at 01:06:29PM +0200, Jens Gustedt wrote:
> Hello Szabolcs,
> 
> On Sun, 25 Jun 2017 12:17:04 +0200 Szabolcs Nagy <nsz@port70.net> wrote:
> 
> > pthread_once_t and pthread_spinlock_t qualifiers are
> > visible in the c++ name mangling if a c++ function takes
> > pointer to them as arguments so the change is an abi break.
> 
> too bad, so we can't change these two
> 
> There is a reading of the C standard that says that volatile only has
> implications if an object itself is such qualified, having a volatile
> qualified lvalue access isn't enough. I don't think that any current
> compiler does such weird things, but who knows where optimisers will
> go in the future.

Indeed. GCC seems committed to treating "accesses through volatile
lvalue" as volatile, but I'd rather not depend on it. Perhaps we
should add a primitive to atomic.h for loading the value of atomics so
that we never access them directly; then volatile would not matter.

> AFAICS for the third finding in sigaction.c this would not be an
> issue. Since in addition this is something dealing with signal stuff,
> I still think that volatile would be in order, here.

The line:

	memcpy(set, handler_set, sizeof handler_set);

is not valid if handler_set is made volatile; we'd have to write out
the code to copy it. Not a big deal though, and more correct anyway;
using memcpy to copy something that's semantically atomic is sloppy.

Unfortunately since I don't want to encode knowledge of the naming of
sigset_t internals here, we'd probably need a loop to copy to a
non-volatile array the same as handler_set, then memcpy from there to
set.

Rich

Re: more on missing volatile qualifications

[Jens Gustedt]

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

Hello Rich,

On Tue, 4 Jul 2017 17:19:09 -0400 Rich Felker <dalias@libc.org> wrote:

> On Sun, Jun 25, 2017 at 01:06:29PM +0200, Jens Gustedt wrote:
> > There is a reading of the C standard that says that volatile only
> > has implications if an object itself is such qualified, having a
> > volatile qualified lvalue access isn't enough. I don't think that
> > any current compiler does such weird things, but who knows where
> > optimisers will go in the future.  
> 
> Indeed. GCC seems committed to treating "accesses through volatile
> lvalue" as volatile, but I'd rather not depend on it. Perhaps we
> should add a primitive to atomic.h for loading the value of atomics so
> that we never access them directly; then volatile would not matter.

While I agree for the principle, identifying all these places and
replace them with a primitive might not be so easy.

Luckily most of these variables a called "lock" :)

I'll look into it and see what I can do. But I will probably not be
able to come up with sensible platform specific choices for an
"a_load" primitive and set it to "a_cas(p, 0, 0)" or so, to have a
start.

> > AFAICS for the third finding in sigaction.c this would not be an
> > issue. Since in addition this is something dealing with signal
> > stuff, I still think that volatile would be in order, here.  
> 
> The line:
> 
> 	memcpy(set, handler_set, sizeof handler_set);
> 
> is not valid if handler_set is made volatile;

Yes, so said the compiler, too.

> we'd have to write out
> the code to copy it. Not a big deal though, and more correct anyway;
> using memcpy to copy something that's semantically atomic is sloppy.

Ok, so let's do this properly.

> Unfortunately since I don't want to encode knowledge of the naming of
> sigset_t internals here, we'd probably need a loop to copy to a
> non-volatile array the same as handler_set, then memcpy from there to
> set.

Yes, unfortunately. To do that we'd also need an "a_load_l" primitive,
or we'd have to change handler_set to an array of unsigned, which is
far more intrusive.

Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]