Patchwork [Ada] Lock-free implementation of protected objects

login
register
mail settings
Submitter Iain Sandoe
Date July 23, 2012, 2:24 p.m.
Message ID <62F94423-30A3-4A67-8F69-B64FE7A71C8E@codesourcery.com>
Download mbox | patch
Permalink /patch/172698/
State New
Headers show

Comments

Iain Sandoe - July 23, 2012, 2:24 p.m.
Hi Arnaud,

On 23 Jul 2012, at 09:02, Arnaud Charlet wrote:

> This patch implements a check in the runtime library that determines whether
> the current target supports the atomic primitives up to 64 bits.

If I understand the name of the flag, it looks like an "all or nothing" for atomic primitives?
is that a consequence of the language definition, or simply that it isn't worth spending a lot of effort on 32 bit machines?

> This should fix build failures on e.g. powerpc-darwin.

almost :-)

On a 64-bit processor, the [32 bit] powerpc-darwin kernel is capable of launching 64bit processes.
Thus, there is an m64 multi-lib for powerpc-darwin, which is built by default for GCC (and, for that multi-lib, the 64 bit locks are available).  At present, bootstrap is failing while building this multi-lib.

With the following, bootstrap completed on powerpc-apple-darwin9, and make check-ada shows no new fails.
Should I apply it?
Iain

gcc/ada: 

	* system-darwin-ppc64.ads: Add Support_Atomic_Primitives,
	set to True.
Arnaud Charlet - July 23, 2012, 2:27 p.m.
> > This patch implements a check in the runtime library that determines
> > whether
> > the current target supports the atomic primitives up to 64 bits.
> 
> If I understand the name of the flag, it looks like an "all or nothing" for
> atomic primitives?

Right.

> is that a consequence of the language definition, or simply that it isn't
> worth spending a lot of effort on 32 bit machines?

The latter for now.

> > This should fix build failures on e.g. powerpc-darwin.
> 
> almost :-)
> 
> On a 64-bit processor, the [32 bit] powerpc-darwin kernel is capable of
> launching 64bit processes.
> Thus, there is an m64 multi-lib for powerpc-darwin, which is built by
> default for GCC (and, for that multi-lib, the 64 bit locks are available).  At
> present, bootstrap is failing while building this multi-lib.
> 
> With the following, bootstrap completed on powerpc-apple-darwin9, and
> make check-ada shows no new fails.
> Should I apply it?

Looks good to me, go ahead, although I'm a bit surprised that you got an error,
can you clarify what error you got?

> Iain
> 
> gcc/ada: 
> 
> 	* system-darwin-ppc64.ads: Add Support_Atomic_Primitives,
> 	set to True.
Iain Sandoe - July 23, 2012, 2:32 p.m.
On 23 Jul 2012, at 15:27, Arnaud Charlet wrote:
>> With the following, bootstrap completed on powerpc-apple-darwin9, and
>> make check-ada shows no new fails.
>> Should I apply it?
> 
> Looks good to me, go ahead, although I'm a bit surprised that you got an error,
> can you clarify what error you got?

IIRC, that the flag was undefined. 
If it's important I can revert the fix in my local tree and re-build.
Iaim
Geert Bosch - July 23, 2012, 2:42 p.m.
On Jul 23, 2012, at 10:32, Iain Sandoe wrote:

>> Looks good to me, go ahead, although I'm a bit surprised that you got an error,
>> can you clarify what error you got?
> 
> IIRC, that the flag was undefined. 
> If it's important I can revert the fix in my local tree and re-build.
> Iaim

No need to do that. Indeed, the flag must be defined for 
all versions of system.ads. Please apply the patch, it 
really seems to be just an oversight.

I'd consider this could even have been fixed under the "obvious"
rule. Thanks very much, Iain!

  -Geert
Arnaud Charlet - July 23, 2012, 2:43 p.m.
> >> With the following, bootstrap completed on powerpc-apple-darwin9,
> >> and
> >> make check-ada shows no new fails.
> >> Should I apply it?
> > 
> > Looks good to me, go ahead, although I'm a bit surprised that you got an
> > error,
> > can you clarify what error you got?
> 
> IIRC, that the flag was undefined. 

The compiler should NOT generate an error in such case.

Vincent, can you confirm that the compiler will default to False in case
the value is not defined in system.ads?

If not, then this needs to be fixed.

> If it's important I can revert the fix in my local tree and re-build.

Given that True is a proper value for darwin x64, your change is fine,
but it shouldn't have been needed, since there should be a proper default.

Arno
Arnaud Charlet - July 23, 2012, 2:45 p.m.
> >> Looks good to me, go ahead, although I'm a bit surprised that you got an
> >> error,
> >> can you clarify what error you got?
> > 
> > IIRC, that the flag was undefined. 
> > If it's important I can revert the fix in my local tree and re-build.
> > Iaim

> No need to do that. Indeed, the flag must be defined for 
> all versions of system.ads.

No, as we agreed and discussed, the flag does NOT have to be defined for all
versions of system.ads, so this is a bug that needs to be fixed (precisely
for the issue raised here: we don't want unknown or new ports to be broken
by default).

> Please apply the patch, it 
> really seems to be just an oversight.

No, it's not just an oversight, and it's also not obvious as shown by this
discussion.

Arno
Vincent PUCCI - July 23, 2012, 2:50 p.m.
On 07/23/2012 10:43 AM, Arnaud Charlet wrote:
>>>> With the following, bootstrap completed on powerpc-apple-darwin9,
>>>> and
>>>> make check-ada shows no new fails.
>>>> Should I apply it?
>>> Looks good to me, go ahead, although I'm a bit surprised that you got an
>>> error,
>>> can you clarify what error you got?
>> IIRC, that the flag was undefined.
> The compiler should NOT generate an error in such case.
>
> Vincent, can you confirm that the compiler will default to False in case
> the value is not defined in system.ads?

The swicth is defaulted to be False in Targparm.
However, as far as I understood in Targparm, the switch must be present 
in all
system.ads packages but I may be wrong.
Arnaud Charlet - July 23, 2012, 2:57 p.m.
> The swicth is defaulted to be False in Targparm.
> However, as far as I understood in Targparm, the switch must be present in
> all system.ads packages but I may be wrong.

That sounds wrong and isn't how other flags work.

Vincent, can you please double check exactly what's happening, and in particular
verify that a missing flag in system-<target>.ads will NOT cause an error?

Also, I would remove all the default values to False, since they mainly add
noise (and create inconsistency for system files that do not have this
value set).

Arno
Iain Sandoe - July 23, 2012, 3:03 p.m.
On 23 Jul 2012, at 15:57, Arnaud Charlet wrote:

>> The swicth is defaulted to be False in Targparm.
>> However, as far as I understood in Targparm, the switch must be present in
>> all system.ads packages but I may be wrong.
> 
> That sounds wrong and isn't how other flags work.
> 
> Vincent, can you please double check exactly what's happening, and in particular
> verify that a missing flag in system-<target>.ads will NOT cause an error?
> 
> Also, I would remove all the default values to False, since they mainly add
> noise (and create inconsistency for system files that do not have this
> value set).

FWIW, I checked the build transcript for the failed case:

s-atopri.adb:40:10: "Support_Atomic_Primitives" is undefined (more references follow)
make[8]: *** [s-atopri.o] Error 1

cheers
Iain
Vincent PUCCI - July 23, 2012, 3:04 p.m.
On 07/23/2012 11:03 AM, Iain Sandoe wrote:
> On 23 Jul 2012, at 15:57, Arnaud Charlet wrote:
>
>> That sounds wrong and isn't how other flags work.
>>
>> Vincent, can you please double check exactly what's happening, and in particular
>> verify that a missing flag in system-<target>.ads will NOT cause an error?
>>
>> Also, I would remove all the default values to False, since they mainly add
>> noise (and create inconsistency for system files that do not have this
>> value set).
> FWIW, I checked the build transcript for the failed case:
>
> s-atopri.adb:40:10: "Support_Atomic_Primitives" is undefined (more references follow)
> make[8]: *** [s-atopri.o] Error 1
>
> cheers
> Iain

Just got the same error... investigating
Geert Bosch - July 23, 2012, 3:13 p.m.
On Jul 23, 2012, at 10:24, Iain Sandoe wrote:

>> This patch implements a check in the runtime library that determines whether
>> the current target supports the atomic primitives up to 64 bits.
> 
> If I understand the name of the flag, it looks like an "all or nothing" for atomic primitives?
> is that a consequence of the language definition, or simply that it isn't worth spending a lot of effort on 32 bit machines?

There is nothing related to the language definition here, as these are not standardized
packages, but part of the implementation. Attempts to use them directly from user programs
will result in warnings.

There are a few issues. We really don't want to have different versions of the spec
for different targets. Rather, we'd have functions that either raise an exception
or use a lock-based implementation if the target lacks the required capabilities.
The system-specific boolean is a mostly a method that allows us to "switch off"
our use of lock-free protected types on a per system basis. This avoids unnecessary
instability in less commonly used platforms while we're enhancing this new capability.

IIUC, all ports are supposed to implement the atomic built-ins. If they are not 
supported in hardware, there should be a library function for it that uses locking.
The problem we're trying to address is builds failing because of undefined
references to __atomic_* functions.

I could also have used __atomic_always_lock_free, which is better in many ways,
but we also need to know in the front end what expansion to use for protected
types. My initial thought was to just have the front end build some trees
calling __atomic_always_lock_free and see if they fold to True. However,
this was considered undesirable.

The alternative would be to have the front end call can_compare_and_swap_p(),
but that would have the front end depend on rtl.h, which even I understand is
bad. Probably can_compare_and_swap_p() should be moved to a better place, so
the front end can use it directly. As I felt it was important to address the 
failures quickly, we used the current approach.

  -Geert
Arnaud Charlet - July 23, 2012, 3:16 p.m.
>> FWIW, I checked the build transcript for the failed case:
>> 
>> s-atopri.adb:40:10: "Support_Atomic_Primitives" is undefined (more
>> references follow)
>> make[8]: *** [s-atopri.o] Error 1
>> 
>> cheers
>> Iain
> 
> Just got the same error... investigating

Ah, so the issue is not in the compiler/targpam, but in a direct use in the
run-time, which is wrong as per previous discussions: we cannot rely on
Support_Atomic_Primitives to be present in all system files.

Arno
Geert Bosch - July 23, 2012, 3:21 p.m.
On Jul 23, 2012, at 10:45, Arnaud Charlet wrote:
> No, as we agreed and discussed, the flag does NOT have to be defined for all
> versions of system.ads, so this is a bug that needs to be fixed (precisely
> for the issue raised here: we don't want unknown or new ports to be broken
> by default).

Having a default can't work, as s-atopri.adb needs access to the flag.
Only the front end itself can use a default, not the run time.

  -Geert
Arnaud Charlet - July 23, 2012, 3:24 p.m.
> Having a default can't work, as s-atopri.adb needs access to the flag.
> Only the front end itself can use a default, not the run time.

Well, this means the current design is broken and s-atopri.adb needs to
be modified.

As we discussed, we cannot assume that System.Support_Atomic_Primitives
exists, since there will always be some target system*.ads files around
that don't have this flag (as shown by this discussion).

For a run-time (as opposed to compiler) parameter, s-parame is probably a
better place to start from, possibly using a separate.

Arno
Geert Bosch - July 23, 2012, 5:58 p.m.
On Jul 23, 2012, at 11:21, Geert Bosch wrote:
> On Jul 23, 2012, at 10:45, Arnaud Charlet wrote:
>> No, as we agreed and discussed, the flag does NOT have to be defined for all
>> versions of system.ads, so this is a bug that needs to be fixed (precisely
>> for the issue raised here: we don't want unknown or new ports to be broken
>> by default).
> 
> Having a default can't work, as s-atopri.adb needs access to the flag.
> Only the front end itself can use a default, not the run time.

While currently the flag is needed, I agree with you that this is undesirable.
The only way to avoid this is to have a type attribute, such as 
T'Atomic_Always_Lock_Free that is statically True if T is known to be
supported for atomic operations.

This would be similar to the atomic built-in __atomic_always_lock_free
function, but initially based on the value of the system constant (as
well as size/alignment of the type). Eventually, we should be able to
query this directly from the back end.

Regardless, the situation as of now is that system.ads must define the flag,
so right now Iain's patch is correct and fixes a build failure. As soon as
the attribute is implemented, we can remove the system flags on systems
where it is set to False.

  -Geert
Richard Henderson - July 25, 2012, 5:39 p.m.
On 07/23/2012 08:13 AM, Geert Bosch wrote:
> IIUC, all ports are supposed to implement the atomic built-ins. If they are not 
> supported in hardware, there should be a library function for it that uses locking.
> The problem we're trying to address is builds failing because of undefined
> references to __atomic_* functions.

Link against libatomic, which provides those very functions?


r~

Patch

Index: gcc/ada/system-darwin-ppc64.ads
===================================================================
--- gcc/ada/system-darwin-ppc64.ads     (revision 189777)
+++ gcc/ada/system-darwin-ppc64.ads     (working copy)
@@ -137,6 +137,7 @@  private
    Stack_Check_Limits        : constant Boolean := False;
    Support_64_Bit_Divides    : constant Boolean := True;
    Support_Aggregates        : constant Boolean := True;
+   Support_Atomic_Primitives : constant Boolean := True;
    Support_Composite_Assign  : constant Boolean := True;
    Support_Composite_Compare : constant Boolean := True;
    Support_Long_Shifts       : constant Boolean := True;