[PATCH] missing volatile in __get_locale

[PATCH] missing volatile in __get_locale

[Jens Gustedt]

All int that are used with atomic operations, should be volatile.

The calls to __lock & Co hid this bug because they implicitly add the
qualifier to the target. But intelligent lto could have bitten us one
day.
---
 src/locale/locale_map.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
index c3e59174..188fcf39 100644
--- a/src/locale/locale_map.c
+++ b/src/locale/locale_map.c
@@ -26,7 +26,7 @@ static const char envvars[][12] = {
 
 const struct __locale_map *__get_locale(int cat, const char *val)
 {
-	static int lock[2];
+	static volatile int lock[2];
 	static void *volatile loc_head;
 	const struct __locale_map *p;
 	struct __locale_map *new = 0;
-- 
2.11.0

Re: [PATCH] missing volatile in __get_locale

[Rich Felker]

On Sat, Jun 24, 2017 at 11:54:25AM +0200, Jens Gustedt wrote:
> All int that are used with atomic operations, should be volatile.
> 
> The calls to __lock & Co hid this bug because they implicitly add the
> qualifier to the target. But intelligent lto could have bitten us one
> day.
> ---
>  src/locale/locale_map.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/locale/locale_map.c b/src/locale/locale_map.c
> index c3e59174..188fcf39 100644
> --- a/src/locale/locale_map.c
> +++ b/src/locale/locale_map.c
> @@ -26,7 +26,7 @@ static const char envvars[][12] = {
>  
>  const struct __locale_map *__get_locale(int cat, const char *val)
>  {
> -	static int lock[2];
> +	static volatile int lock[2];
>  	static void *volatile loc_head;
>  	const struct __locale_map *p;
>  	struct __locale_map *new = 0;
> -- 
> 2.11.0

Thanks, applying.

Rich

Re: [PATCH] missing volatile in __get_locale

[Alexander Monakov]

On Tue, 4 Jul 2017, Rich Felker wrote:

> Thanks, applying.

Can you please document the rationale for using bare ints instead of 
an explicit struct for internal locks?

TIA.
Alexander

Re: [PATCH] missing volatile in __get_locale

[Rich Felker]

On Wed, Jul 05, 2017 at 01:24:23AM +0300, Alexander Monakov wrote:
> On Tue, 4 Jul 2017, Rich Felker wrote:
> 
> > Thanks, applying.
> 
> Can you please document the rationale for using bare ints instead of 
> an explicit struct for internal locks?

I don't think there was/is any good one, it was just the choice that
was made at the time. At one point there might have been places it
avoided need for including a header to define the type, or where being
explicit about the storage needed for the lock mattered (think stdio
FILE layout, but it uses its own lock anyway), but I think it was
mostly unjustified. I wouldn't be opposed to changing it.

Rich

move to a proper __lock_t type

[Jens Gustedt]

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

Hello,

On Tue, 4 Jul 2017 18:28:42 -0400 Rich Felker <dalias@libc.org> wrote:

> On Wed, Jul 05, 2017 at 01:24:23AM +0300, Alexander Monakov wrote:
> > On Tue, 4 Jul 2017, Rich Felker wrote:
> >   
> > > Thanks, applying.  
> > 
> > Can you please document the rationale for using bare ints instead
> > of an explicit struct for internal locks?  
> 
> I don't think there was/is any good one, it was just the choice that
> was made at the time. At one point there might have been places it
> avoided need for including a header to define the type, or where being
> explicit about the storage needed for the lock mattered (think stdio
> FILE layout, but it uses its own lock anyway), but I think it was
> mostly unjustified. I wouldn't be opposed to changing it.

So what would you think of the following migration path:

 (1) Finish the volatile/atomic cleanup by adding a_load, a_load_l and
 perhaps corresponding store primitives.

 (2) Replace all occurences of "volatile int[2]" by "__lock_t" that
 encapsulates just that in a struct.

 (3) Replace the __lock/__unlock pair by the new algorithm

 (4) Reduce "__lock_t" to a struct that simply has a "volatile int" as
 sole member.

In all of that, would we need the "__" in the name of the structure? I
think this name will never be exported.

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: move to a proper __lock_t type

[Rich Felker]

On Wed, Jul 05, 2017 at 08:30:14AM +0200, Jens Gustedt wrote:
> Hello,
> 
> On Tue, 4 Jul 2017 18:28:42 -0400 Rich Felker <dalias@libc.org> wrote:
> 
> > On Wed, Jul 05, 2017 at 01:24:23AM +0300, Alexander Monakov wrote:
> > > On Tue, 4 Jul 2017, Rich Felker wrote:
> > >   
> > > > Thanks, applying.  
> > > 
> > > Can you please document the rationale for using bare ints instead
> > > of an explicit struct for internal locks?  
> > 
> > I don't think there was/is any good one, it was just the choice that
> > was made at the time. At one point there might have been places it
> > avoided need for including a header to define the type, or where being
> > explicit about the storage needed for the lock mattered (think stdio
> > FILE layout, but it uses its own lock anyway), but I think it was
> > mostly unjustified. I wouldn't be opposed to changing it.
> 
> So what would you think of the following migration path:
> 
>  (1) Finish the volatile/atomic cleanup by adding a_load, a_load_l and
>  perhaps corresponding store primitives.

I don't think there are any non-atomic stores to atomic objects; if so
they're either mistakes or they happen synchronized before any
concurrent access, so that the way the store happens does not matter.

Regarding a_load, I see it as a change unrelated to using a lock type;
as long as the object is volatile, direct loads still work fine.

Regarding a_load_l, I'd probably like to avoid it. I'm aiming to phase
out the long/64-bit atomic primitives -- the 64-bit ones are misnomers
as written, even, and there have always been sketchy aliasing issues
going on with their implementations, which I'm not sure we ever fixed.
Right now there are only a few users of them, and if we don't need to
memcpy the signal mask one, there's no reason it needs to even use the
same type/representation as a normal sigset_t array. There's actually
a larger goal to get rid of as much atomic use as possible, outside of
core sync primitive implementations. Many places they're used as
dubious speed or size optimizations where a lock would work perfectly
fine, and would be obviously correct rather than requiring difficult
correctness analysis. The only places that should really be using
atomics outside of implementing sync primitives are places where
there's an AS-safety need that could not be met using locks except
with a huge hammer (sigprocmask around critical section).

>  (2) Replace all occurences of "volatile int[2]" by "__lock_t" that
>  encapsulates just that in a struct.
> 
>  (3) Replace the __lock/__unlock pair by the new algorithm
> 
>  (4) Reduce "__lock_t" to a struct that simply has a "volatile int" as
>  sole member.

Can we avoid cosmetics/refactoring like this until the new algorithm
goes in? I just have a backlog of stuff to review and commit, and the
new algorithm is something I really want to get included for release
since it fixes a visible bug (even if hard to hit the race), and
having to go back and start again with refactored versions sets that
back.

> In all of that, would we need the "__" in the name of the structure? I
> think this name will never be exported.

I don't think so, but it might be nice to signify to someone reading
the code that it's not a public type. It might also be mildly nicer in
the debugger if the application also happens to define a type named
lock_t. I'm open to other names too.

Rich

Re: move to a proper __lock_t type

[Jens Gustedt]

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

Hello,

On Wed, 5 Jul 2017 12:07:44 -0400 Rich Felker <dalias@libc.org> wrote:

> On Wed, Jul 05, 2017 at 08:30:14AM +0200, Jens Gustedt wrote:
> > So what would you think of the following migration path:
> > 
> >  (1) Finish the volatile/atomic cleanup by adding a_load, a_load_l
> > and perhaps corresponding store primitives.  
> 
> I don't think there are any non-atomic stores to atomic objects; if so
> they're either mistakes or they happen synchronized before any
> concurrent access, so that the way the store happens does not matter.
> 
> Regarding a_load, I see it as a change unrelated to using a lock type;
> as long as the object is volatile, direct loads still work fine.
> 
> Regarding a_load_l, I'd probably like to avoid it. I'm aiming to phase
> out the long/64-bit atomic primitives -- the 64-bit ones are misnomers
> as written, even, and there have always been sketchy aliasing issues
> going on with their implementations, which I'm not sure we ever fixed.
> Right now there are only a few users of them, and if we don't need to
> memcpy the signal mask one, there's no reason it needs to even use the
> same type/representation as a normal sigset_t array. There's actually
> a larger goal to get rid of as much atomic use as possible, outside of
> core sync primitive implementations. Many places they're used as
> dubious speed or size optimizations where a lock would work perfectly
> fine, and would be obviously correct rather than requiring difficult
> correctness analysis. The only places that should really be using
> atomics outside of implementing sync primitives are places where
> there's an AS-safety need that could not be met using locks except
> with a huge hammer (sigprocmask around critical section).

I am not much a fan of the a_..._l primitives either. I came up with
it here, because we would need it in the transformation of the memcpy
logic that we discussed, where the atomic type to be copied is
implemented as "volatile unsigned long[something]".

> >  (2) Replace all occurences of "volatile int[2]" by "__lock_t" that
> >  encapsulates just that in a struct.
> > 
> >  (3) Replace the __lock/__unlock pair by the new algorithm
> > 
> >  (4) Reduce "__lock_t" to a struct that simply has a "volatile int"
> > as sole member.  
> 
> Can we avoid cosmetics/refactoring like this until the new algorithm
> goes in?

what is "this" here, (2) and (4) ?

> I just have a backlog of stuff to review and commit, and the
> new algorithm is something I really want to get included for release
> since it fixes a visible bug (even if hard to hit the race), and
> having to go back and start again with refactored versions sets that
> back.

Ok, no problem that can wait. Do you need more input for me on the new
algorithm?

> > In all of that, would we need the "__" in the name of the
> > structure? I think this name will never be exported.  
>
> I don't think so, but it might be nice to signify to someone reading
> the code that it's not a public type. It might also be mildly nicer in
> the debugger if the application also happens to define a type named
> lock_t.

right

> I'm open to other names too.

I personally don't like the "..._t" naming convention too much, but
"__lock" is already taken for the function. "_Lock", "_Lck" ?


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: move to a proper __lock_t type

[Rich Felker]

On Wed, Jul 05, 2017 at 10:53:06PM +0200, Jens Gustedt wrote:
> I am not much a fan of the a_..._l primitives either. I came up with
> it here, because we would need it in the transformation of the memcpy
> logic that we discussed, where the atomic type to be copied is
> implemented as "volatile unsigned long[something]".

It's not needed as long as volatile remains there. You can just use a
for loop doing direct reads of the volatile long array.

> > >  (2) Replace all occurences of "volatile int[2]" by "__lock_t" that
> > >  encapsulates just that in a struct.
> > > 
> > >  (3) Replace the __lock/__unlock pair by the new algorithm
> > > 
> > >  (4) Reduce "__lock_t" to a struct that simply has a "volatile int"
> > > as sole member.  
> > 
> > Can we avoid cosmetics/refactoring like this until the new algorithm
> > goes in?
> 
> what is "this" here, (2) and (4) ?

Yes, that sounds right.

> > I just have a backlog of stuff to review and commit, and the
> > new algorithm is something I really want to get included for release
> > since it fixes a visible bug (even if hard to hit the race), and
> > having to go back and start again with refactored versions sets that
> > back.
> 
> Ok, no problem that can wait. Do you need more input for me on the new
> algorithm?

I think it's okay. Going to read again. It strikes me as a bit
over-optimized for locks that are minimal contention and not
bottlenecks, but if it ends up also being used for malloc it makes
sense having it optimized.

> > > In all of that, would we need the "__" in the name of the
> > > structure? I think this name will never be exported.  
> >
> > I don't think so, but it might be nice to signify to someone reading
> > the code that it's not a public type. It might also be mildly nicer in
> > the debugger if the application also happens to define a type named
> > lock_t.
> 
> right
> 
> > I'm open to other names too.
> 
> I personally don't like the "..._t" naming convention too much, but
> "__lock" is already taken for the function. "_Lock", "_Lck" ?

I don't like stepping into _[A-Z] much since it seems to be the naming
convention that the committee has chosen for expending the language
itself. I'll think about whether there are other ideas I like.

One minor concern: if we do make it a struct type, there are possibly
formal aliasing issues with having it embedded in any of the public
pthread types (cond vars, maybe barriers?) without a public
definition. Perhaps it should be an int array typedef (length 1) or
just a typedef'd int?

Rich

Re: move to a proper __lock_t type

[Jens Gustedt]

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

Hello,

On Wed, 5 Jul 2017 17:41:50 -0400 Rich Felker <dalias@libc.org> wrote:

> One minor concern: if we do make it a struct type, there are possibly
> formal aliasing issues with having it embedded in any of the public
> pthread types (cond vars, maybe barriers?) without a public
> definition. Perhaps it should be an int array typedef (length 1) or
> just a typedef'd int?

Right, I already have looked into cond vars and have a patch for
that. At the time we were doing the C11 thrd stuff, we already
discussed that, and in fact I think now would be the time to have that
improved, too. The current implementation there, is already somewhat
similar in strategy to the new algorithm.

Hm, this would again imply that we wouldn't be able to stick a
volatile into the type, and so we would even loose the volatile that
we have now for all the "lock" variables.

So, I would go for

typedef volatile int __lock_t[1];

after you merged the new algorithm.

For cond variables the __lock/__unlock interfaces then would still
"work", modulo the problem that the underlying int is not a veritable
volatile. But that is the situation for this code now, anyhow.

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 --]