This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/2] Optimize generic spinlock code and use C11 like atomic macros.

On 02/28/2017 08:15 AM, Torvald Riegel wrote:

On Mon, 2017-02-27 at 17:57 +0000, Szabolcs Nagy wrote:

On 26/02/17 20:35, Torvald Riegel wrote:

On Sun, 2017-02-26 at 21:29 +0100, Florian Weimer wrote:

* Torvald Riegel:


… like this, it would change the C++ name mangling of anything related
to pthread_spinlock_t.  It is defined as a typedef, so the mangling
uses the definition to refer to the type, not the name, according to
the language rules, where typedef does not create a new type, and
typedefs with the same definition can be used interchangeably.


I'm not saying that we should change the definition to a union.  What
2.14.2 in the document cited above states is that the pointers to the
union and the individual parts are interchangeable.  So we can use a
pointer to a part (ie, non-volatile) interchangeably with the pointer to
the union that we use internally.


The relevant quote from that document (C memory object and value
semantics: the space of de facto and ISO standards) is:

| The standard says: 6.7.2.1p16 “The size of a union is
| sufficient to contain the largest of its members. The value of
| at most one of the members can be stored in a union object
| at any time. *A pointer to a union object, suitably converted,*
| *points to each of its members (or if a member is a bit-field,*
| *then to the unit in which it resides), and vice versa.*” (bold
| emphasis added).

So I think this is only valid if you actually start with a union
object.  A plain top-level definition of a volatile int is not a union
member, so this rule does not apply (in the “vice versa” part).


Then we'll have to agree to disagree.  What do others think about this
question?


i think the standard says that

volatile int x;
*(int*)&x;

is undefined and i think

int r = *p;


At least on the architectures that already use the __atomic builtins, we
never perform such an access -- everything goes through __atomic*.  On
other architectures, we already use plain, non-volatile accesses to
emulate real atomics (eg, see atomic_store_relaxed).  ISTM that we
wouldn't be making things worse in the sense that we already make
assumptions about what the compiler would do or would not (e.g., the
compiler does not store to *p again as in your third example).
Yes, architectures which use the C11 atomics passes the volatile int * to the __atomic* builtins. 

For architectures not using C11 atomics:
From the spinlock perspective, the latest patch does not access a casted pointer which discards the volatile qualifier. 

Instead in atomic_exchange_acq there is a
read from a volatile int * into a local int variable.
The volatile int * is passed to
atomic_compare_and_exchange_bool_acq macro.
The passed oldval / newval variables have type int instead
of volatile int.
atomic_load_relaxed is now using a local variable of type int instead of volatile int. Reading from the volatile int * is done by gcc via inline assembly. Afterwards the local int variable is returned to the user of this macro. 

__atomic_val_bysize is now using a local variable of type int instead
of volatile int. The return value of pre##_<sizeof(*mem)>_##post is stored in the local variable and then returned to the user of this macro. 

If this is okay, I prefer to use the changes from the latest patch.
Then this will fix - if compiled with GCC >= 5 - the usage of
the atomic macros in include/atomic.h with volatile int *
for architectures which do not define USE_ATOMIC_COMPILER_BUILTINS
to one.

If this is not okay, I would skip those changes in include/atomic.h
for this patch.
After we have a solution, we should work on a separate patch.
Even if gcc provides an extension as mentioned by Florian, I think it will only be available with new GCCs. What about the current GCCs? 

Bye
Stefan
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]