diff mbox

[Ada] Fix bootstrapping on darwin9/10 (PR ada/64349).

Message ID 7F948249-CEE4-4468-82F6-29E58AB5797F@codesourcery.com
State New
Headers show

Commit Message

Iain Sandoe Jan. 8, 2015, 11:42 p.m. UTC
On 8 Jan 2015, at 13:52, Tristan Gingold wrote:

> 
>> On 08 Jan 2015, at 13:49, Iain Sandoe <iain@codesourcery.com> wrote:
>> 
>> Hi Tristan,
>> 
>> On 7 Jan 2015, at 10:15, Arnaud Charlet wrote:
>> 
>>> Use _NSGetEnviron to get environment.
>>> 
>>> Tested on x86_64-pc-linux-gnu, committed on trunk
>>> 
>>> 2015-01-07  Tristan Gingold  <gingold@adacore.com>
>>> 
>>> 	PR ada/64349
>>> 	* env.c (__gnat_environ): Adjust for darwin9/darwin10.
>>> 
>>> <difs.txt>
>> 
>> So my original patch assumed that, while it was not legal to use environ from a shlib, it is legal to use _NSGetEnviron () from an application ...
>> 
>> .. and, OK fine, I see the point about ! defined (__arm__) .. but a few other comments.
>> 
>> ISTM that there's a partial implementation to distinguish between IN_RTS and application?
> 
> Yes you're right.  The added code should have been added after the #endif for IN_RTS.

How about this?
It uses the interface where needed, avoids it for main exes and gets rid of the negative conditional (which IMO makes the code a little more readable).

Iain

P.S. this is not Darwin9/10 - specific the only reason it doesn't fail on Darwin >= 11 is because they default to -undefined dynamic_lookup .. and so find the symbol from the exe.

Comments

Tristan Gingold Jan. 14, 2015, 9:03 a.m. UTC | #1
> On 09 Jan 2015, at 00:42, Iain Sandoe <iain@codesourcery.com> wrote:
> 
> 
> On 8 Jan 2015, at 13:52, Tristan Gingold wrote:
> 
>> 
>>> On 08 Jan 2015, at 13:49, Iain Sandoe <iain@codesourcery.com> wrote:
>>> 
>>> Hi Tristan,
>>> 
>>> On 7 Jan 2015, at 10:15, Arnaud Charlet wrote:
>>> 
>>>> Use _NSGetEnviron to get environment.
>>>> 
>>>> Tested on x86_64-pc-linux-gnu, committed on trunk
>>>> 
>>>> 2015-01-07  Tristan Gingold  <gingold@adacore.com>
>>>> 
>>>> 	PR ada/64349
>>>> 	* env.c (__gnat_environ): Adjust for darwin9/darwin10.
>>>> 
>>>> <difs.txt>
>>> 
>>> So my original patch assumed that, while it was not legal to use environ from a shlib, it is legal to use _NSGetEnviron () from an application ...
>>> 
>>> .. and, OK fine, I see the point about ! defined (__arm__) .. but a few other comments.
>>> 
>>> ISTM that there's a partial implementation to distinguish between IN_RTS and application?
>> 
>> Yes you're right.  The added code should have been added after the #endif for IN_RTS.
> 
> How about this?
> It uses the interface where needed, avoids it for main exes and gets rid of the negative conditional (which IMO makes the code a little more readable).
> 
> Iain
> 
> P.S. this is not Darwin9/10 - specific the only reason it doesn't fail on Darwin >= 11 is because they default to -undefined dynamic_lookup .. and so find the symbol from the exe.

Sorry for the late answer.  We did something slightly different: always #include crt_externs.h on no-arm Darwin.

Tristan.
Iain Sandoe Jan. 20, 2015, 10:45 a.m. UTC | #2
On 14 Jan 2015, at 09:03, Tristan Gingold wrote:

> 
>> On 09 Jan 2015, at 00:42, Iain Sandoe <iain@codesourcery.com> wrote:
>> 
>> 
>> On 8 Jan 2015, at 13:52, Tristan Gingold wrote:
>> 
>>> 
>>>> On 08 Jan 2015, at 13:49, Iain Sandoe <iain@codesourcery.com> wrote:
>>>> 
>>>> Hi Tristan,
>>>> 
>>>> On 7 Jan 2015, at 10:15, Arnaud Charlet wrote:
>>>> 
>>>>> Use _NSGetEnviron to get environment.
>>>>> 
>>>>> Tested on x86_64-pc-linux-gnu, committed on trunk
>>>>> 
>>>>> 2015-01-07  Tristan Gingold  <gingold@adacore.com>
>>>>> 
>>>>> 	PR ada/64349
>>>>> 	* env.c (__gnat_environ): Adjust for darwin9/darwin10.
>>>>> 
>>>>> <difs.txt>
>>>> 
>>>> So my original patch assumed that, while it was not legal to use environ from a shlib, it is legal to use _NSGetEnviron () from an application ...
>>>> 
>>>> .. and, OK fine, I see the point about ! defined (__arm__) .. but a few other comments.
>>>> 
>>>> ISTM that there's a partial implementation to distinguish between IN_RTS and application?
>>> 
>>> Yes you're right.  The added code should have been added after the #endif for IN_RTS.
>> 
>> How about this?
>> It uses the interface where needed, avoids it for main exes and gets rid of the negative conditional (which IMO makes the code a little more readable).
>> 
>> Iain
>> 
>> P.S. this is not Darwin9/10 - specific the only reason it doesn't fail on Darwin >= 11 is because they default to -undefined dynamic_lookup .. and so find the symbol from the exe.
> 
> Sorry for the late answer.  We did something slightly different: always #include crt_externs.h on no-arm Darwin.

Any news on when this might hit trunk?
 - it is a bootstrap issue (although on older targets).
thanks
Iain
Arnaud Charlet Jan. 20, 2015, 10:53 a.m. UTC | #3
> Any news on when this might hit trunk?
>  - it is a bootstrap issue (although on older targets).

Right, and you have local patches/a work around.

I have been on paternity leave, so with no time to sync our changes (and
with other priorities :-)).

My next sync won't be before next week.

Let us know if you'd like to see Tristan's patch before that, we can send it
to you in the mean.

Arno
Iain Sandoe Jan. 20, 2015, 10:59 a.m. UTC | #4
On 20 Jan 2015, at 10:53, Arnaud Charlet wrote:

>> Any news on when this might hit trunk?
>> - it is a bootstrap issue (although on older targets).
> 
> Right, and you have local patches/a work around.
> 
> I have been on paternity leave, so with no time to sync our changes (and
> with other priorities :-)).

indeed :-) 

> My next sync won't be before next week.
> 
> Let us know if you'd like to see Tristan's patch before that, we can send it
> to you in the mean.

That's fine - as you say we have a wrok-around in the meantime,
thanks
Iain
Dominique d'Humières Jan. 20, 2015, 12:08 p.m. UTC | #5
> Le 20 janv. 2015 à 11:59, Iain Sandoe <iain@codesourcery.com> a écrit :
> 
> 
> On 20 Jan 2015, at 10:53, Arnaud Charlet wrote:
> 
>>> Any news on when this might hit trunk?
>>> - it is a bootstrap issue (although on older targets).
>> 
>> Right, and you have local patches/a work around.
>> 
>> I have been on paternity leave, so with no time to sync our changes (and
>> with other priorities :-)).
> 
> indeed :-) 
> 
>> My next sync won't be before next week.
>> 
>> Let us know if you'd like to see Tristan's patch before that, we can send it
>> to you in the mean.
> 
> That's fine - as you say we have a wrok-around in the meantime,
> thanks
> Iain

Could you please post (or mail me) Tristan's patch? I’ld like to test it before it is committed (chat échaudé craint l’eau froide!).

TIA

Dominique
diff mbox

Patch

====

ada:

	PR ada/64349
	* env.c (__gnat_environ): Adjust environ access for Darwin.

Index: gcc/ada/env.c
===================================================================
--- gcc/ada/env.c	(revision 219325)
+++ gcc/ada/env.c	(working copy)
@@ -215,12 +215,12 @@ 
 #elif defined (sun)
   extern char **_environ;
   return _environ;
-#elif ! (defined (__vxworks))
-  extern char **environ;
+#elif defined (IN_RTS) && defined (__APPLE__) && !defined (__arm__)
+  return *_NSGetEnviron ();
+#elif defined (__vxworks)
   return environ;
-#elif defined (__APPLE__) && !defined (__arm__)
-  return *_NSGetEnviron ();
 #else
+  extern char **environ;
   return environ;
 #endif
 }