[PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Richard Earnshaw
This cost me at least a couple of hours debugging at the weekend; I was
trying to work out why my file handles were all corrupted when compiling
code for Thumb-2.  It turns out that Newlib's config.h was changing the
ABI depending on the instruction set it was compiling for -- a big No
No, since code is supposed to interwork freely between the two ISAs!

Anyway, I think the following (added by Paul back in 2006) is just
wrong.  We need a more precise way to detect M-profile devices if they
must have smaller newlib footprints.

I've chosen to comment out this code rather than remove it, since that
way it acts as a clear reminder as to why we don't want to do this.  If
someone adds more precise targeting of M-profile cores, then the code
here can then go entirely.

R.

2009-06-02  Richard Earnshaw  <[hidden email]>

        * include/sys/config.h ([__thumb2__]): Don't define _REENT_SMALL.

patch (679 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Paul Brook
On Tuesday 02 June 2009, Richard Earnshaw wrote:
> This cost me at least a couple of hours debugging at the weekend; I was
> trying to work out why my file handles were all corrupted when compiling
> code for Thumb-2.  It turns out that Newlib's config.h was changing the
> ABI depending on the instruction set it was compiling for -- a big No
> No, since code is supposed to interwork freely between the two ISAs!

I'm fairly sure I tested this, and concluded that it had no effect on the
external library ABI. In fact I think I fixed bugs related to this.
i.e. this only matters if you're trying to compile part of the library as
Thumb-2 and part as ARM. Do we really care about that? Or did I miss
something?

Paul
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Richard Earnshaw
On Tue, 2009-06-02 at 14:46 +0100, Paul Brook wrote:

> On Tuesday 02 June 2009, Richard Earnshaw wrote:
> > This cost me at least a couple of hours debugging at the weekend; I was
> > trying to work out why my file handles were all corrupted when compiling
> > code for Thumb-2.  It turns out that Newlib's config.h was changing the
> > ABI depending on the instruction set it was compiling for -- a big No
> > No, since code is supposed to interwork freely between the two ISAs!
>
> I'm fairly sure I tested this, and concluded that it had no effect on the
> external library ABI. In fact I think I fixed bugs related to this.
> i.e. this only matters if you're trying to compile part of the library as
> Thumb-2 and part as ARM. Do we really care about that? Or did I miss
> something?
>
> Paul

Yes, this leaks out into user code -- at least it does for me.  Try
building the standard arm-eabi multilib tools and then running the gcc
testsuite with -mcpu=cortex-a8 and -mthumb.  Programs such as
gcc.c-torture/exectute/fprintf-1.c will then fail.

The definition of stdout is:
#define stdout  (_REENT->_stdout)

Which thus depends on the layout of _REENT.

R.
--
Richard Earnshaw             Email: [hidden email]
Engineering Manager          Phone: +44 1223 400569 (Direct + VoiceMail)
OpenSource Tools             Switchboard: +44 1223 400400
ARM Ltd                      Fax: +44 1223 400410
110 Fulbourn Rd              Web: http://www.arm.com/
Cambridge, UK. CB1 9NJ


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Paul Brook
> Yes, this leaks out into user code -- at least it does for me.  Try
> building the standard arm-eabi multilib tools and then running the gcc
> testsuite with -mcpu=cortex-a8 and -mthumb.  Programs such as
> gcc.c-torture/exectute/fprintf-1.c will then fail.
>
> The definition of stdout is:
> #define stdout  (_REENT->_stdout)

In my sources this field lives at the same location in both cases.

Paul
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Paul Brook
On Tuesday 02 June 2009, Paul Brook wrote:
> > Yes, this leaks out into user code -- at least it does for me.  Try
> > building the standard arm-eabi multilib tools and then running the gcc
> > testsuite with -mcpu=cortex-a8 and -mthumb.  Programs such as
> > gcc.c-torture/exectute/fprintf-1.c will then fail.
> >
> > The definition of stdout is:
> > #define stdout  (_REENT->_stdout)
>
> In my sources this field lives at the same location in both cases.

Further investigation indicates upstream newlib is missing the following
patch.

2009-06-02  Paul Brook  <[hidden email]>

        newlib/
        * libc/include/sys/reent.h (_reent): Adjust _REENT_SMALL to be binary
        compatible with normal layout

Index: newlib/libc/include/sys/reent.h
===================================================================
--- newlib/libc/include/sys/reent.h (revision 159552)
+++ newlib/libc/include/sys/reent.h (revision 159553)
@@ -344,14 +344,15 @@ struct _misc_reent
  * ports with 16-bit int's but 32-bit pointers, align nicely.  */
 struct _reent
 {
+  /* As an exception to the above put _errno first for binary
+     compatibility with non _REENT_SMALL targets.  */
+  int _errno; /* local copy of errno */
 
   /* FILE is a big struct and may change over time.  To try to achieve binary
      compatibility with future versions, put stdin,stdout,stderr here.
      These are pointers into member __sf defined below.  */
   __FILE *_stdin, *_stdout, *_stderr; /* XXX */
 
-  int _errno; /* local copy of errno */
-
   int  _inc; /* used by tmpnam */
 
   char *_emergency;

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Jeff Johnston
Paul Brook wrote:

> On Tuesday 02 June 2009, Paul Brook wrote:
>  
>>> Yes, this leaks out into user code -- at least it does for me.  Try
>>> building the standard arm-eabi multilib tools and then running the gcc
>>> testsuite with -mcpu=cortex-a8 and -mthumb.  Programs such as
>>> gcc.c-torture/exectute/fprintf-1.c will then fail.
>>>
>>> The definition of stdout is:
>>> #define stdout  (_REENT->_stdout)
>>>      
>> In my sources this field lives at the same location in both cases.
>>    
>
> Further investigation indicates upstream newlib is missing the following
> patch.
>
> 2009-06-02  Paul Brook  <[hidden email]>
>
> newlib/
> * libc/include/sys/reent.h (_reent): Adjust _REENT_SMALL to be binary
> compatible with normal layout
>
>  
This would require changing the initialization macro in kind or else
there will be trouble.  I'm not thrilled with leaving two gaps in the
struct for 16-bit platforms, but there are now 2 unused fields since
Corinna revamped locale support.  If the unpaired int that is caused by
the errno move was put below and the unused fields were removed, I could
certainly live with that as the struct gets smaller and there is one
left-over int anyway.  Might as well be errno.

That said, I agree with Richard's point.  If the thumb2 flag is not a
proper check for a platform that requires a small reent struct, then it
shouldn't be used as such.

Perhaps a staggered approach can be used.  Make the reent change now as
a stop-gap and work on changing the compiler so that a proper check can
be made for when reent small should really be used and a proper multilib
is created.  For now, put a comment in that the thumb2 check is wrong
and needs to be changed.

Seem reasonable?

-- Jeff J.

> Index: newlib/libc/include/sys/reent.h
> ===================================================================
> --- newlib/libc/include/sys/reent.h (revision 159552)
> +++ newlib/libc/include/sys/reent.h (revision 159553)
> @@ -344,14 +344,15 @@ struct _misc_reent
>   * ports with 16-bit int's but 32-bit pointers, align nicely.  */
>  struct _reent
>  {
> +  /* As an exception to the above put _errno first for binary
> +     compatibility with non _REENT_SMALL targets.  */
> +  int _errno; /* local copy of errno */
>  
>    /* FILE is a big struct and may change over time.  To try to achieve binary
>       compatibility with future versions, put stdin,stdout,stderr here.
>       These are pointers into member __sf defined below.  */
>    __FILE *_stdin, *_stdout, *_stderr; /* XXX */
>  
> -  int _errno; /* local copy of errno */
> -
>    int  _inc; /* used by tmpnam */
>  
>    char *_emergency;
>
>  

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Paul Brook
> That said, I agree with Richard's point.  If the thumb2 flag is not a
> proper check for a platform that requires a small reent struct, then it
> shouldn't be used as such.

Well, the whole idea that this is something that can be figured out from the
target is wrong. It should really be a user visible configure option.

Paul
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Jeff Johnston
Paul Brook wrote:

>> That said, I agree with Richard's point.  If the thumb2 flag is not a
>> proper check for a platform that requires a small reent struct, then it
>> shouldn't be used as such.
>>    
>
> Well, the whole idea that this is something that can be figured out from the
> target is wrong. It should really be a user visible configure option.
>
> Paul
>  
Do you mean the existing --enable-newlib-reent-small configure option or
something different?

-- Jeff J.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Paul Brook
> >> That said, I agree with Richard's point.  If the thumb2 flag is not a
> >> proper check for a platform that requires a small reent struct, then it
> >> shouldn't be used as such.
> >
> > Well, the whole idea that this is something that can be figured out from
> > the target is wrong. It should really be a user visible configure option.
>
> Do you mean the existing --enable-newlib-reent-small configure option or
> something different?

Yes, exactly that.  In that case I suggest removing the __thumb2__ check
altogether.  It would be nice if reent-small didn't break the library ABI, but
that's a different issue.

Paul
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH, ARM] Don't use __thumb2__ to select _REENT_SMALL

Jeff Johnston
Paul Brook wrote:

>>>> That said, I agree with Richard's point.  If the thumb2 flag is not a
>>>> proper check for a platform that requires a small reent struct, then it
>>>> shouldn't be used as such.
>>>>        
>>> Well, the whole idea that this is something that can be figured out from
>>> the target is wrong. It should really be a user visible configure option.
>>>      
>> Do you mean the existing --enable-newlib-reent-small configure option or
>> something different?
>>    
>
> Yes, exactly that.  In that case I suggest removing the __thumb2__ check
> altogether.  It would be nice if reent-small didn't break the library ABI, but
> that's a different issue.
>
> Paul
>  
Done.  I'm still willing to accept a patch to _REENT_SMALL and reclaim
the unused locale variables.
I can do this if you don't want to.

-- Jeff J.