| Message ID | 62F94423-30A3-4A67-8F69-B64FE7A71C8E@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
> > 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.
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
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
> >> 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
> >> 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
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.
> 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
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
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
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
>> 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
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
> 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
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
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~
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;