Re: Update newlib to support efficient string operation functions for Thumb.

[Jeff Johnston <jjohnstn at redhat dot com>]

----- Original Message -----
> From: "Corinna Vinschen" <vinschen@redhat.com>
> To: newlib@sourceware.org
> Sent: Tuesday, September 2, 2014 5:05:36 AM
> Subject: Re: Update newlib to support efficient string operation functions for Thumb.
> 
> Hi Hale,
> 
> On Aug 26 17:44, Hale Wang wrote:
> > Hi,
> > 
> > This patch is used to resubmit the previous newlib patch. And at the same
> > time, this patch can also clean up the arm backend in newlib.
> > 
> > The memchr-stub.c and memcpy-stub.c are redundant. There is no need to
> > explicitely include the corresponding source files(../../string/*.c).
> > 
> > Jeff Johnston provided a cleaner way to realize this. This patch use this
> > solution to clean up all the functions in newlib/libc/machine/arm.
> > 
> > By the way, the previous issue fixed in the previous patch is also included
> > in this patch.
> 
> The patch looks ok, but I have two comments/questions:
> 
> > --- a/newlib/libc/machine/arm/Makefile.am
> > +++ b/newlib/libc/machine/arm/Makefile.am
> > @@ -6,13 +6,60 @@ INCLUDES = $(NEWLIB_CFLAGS) $(CROSS_CFLAGS)
> > $(TARGET_CFLAGS)
> >  
> >  AM_CCASFLAGS = $(INCLUDES)
> >  
> > +if HAVE_THUMB1
> > +if OPT_SIZE
> > +STRCMP_SRC=strcmp.S
> > +STRCMP_OBJ=$(lpfx)strcmp.o
> > +STRLEN_SRC=strlen.c
> > +STRLEN_OBJ=$(lpfx)strlen.o
> > +else
> > +STRCMP_SRC=
> > +STRCMP_OBJ=
> > +STRLEN_SRC=
> > +STRLEN_OBJ=
> > +endif
> > +else
> > +STRCMP_SRC=strcmp.S
> > +STRCMP_OBJ=$(lpfx)strcmp.o
> > +STRLEN_SRC=strlen.c
> > +STRLEN_OBJ=$(lpfx)strlen.o
> > +endif
> > +
> > +if HAVE_ARMV7
> > +MEMCHR_SRC=memchr.S
> > +MEMCHR_OBJ=$(lpfx)memchr.o
> > +else
> > +MEMCHR_SRC=
> > +MEMCHR_OBJ=
> > +endif
> > +
> > +if OPT_SIZE
> > +MEMCPY_SRC=
> > +MEMCPY_OBJ=
> > +else
> > +if HAVE_ARMV7A
> > +MEMCPY_SRC=memcpy.S
> > +MEMCPY_OBJ=$(lpfx)memcpy.o
> > +else
> > +if HAVE_ARMV7M
> > +MEMCPY_SRC=memcpy.S
> > +MEMCPY_OBJ=$(lpfx)memcpy.o
> > +else
> > +MEMCPY_SRC=
> > +MEMCPY_OBJ=
> > +endif !HAVE_ARMV7M
> > +endif !HAVE_ARMV7A
> > +endif !OPT_SIZE
> 
> Your configure script sets the variables HAVE_THUMB1, OPT_SIZE,
> HAVE_ARMV7, HAVE_ARMV7A, and HAVE_ARMV7M for the sake of configuration.
> But the actual configuration is done here in Makefile.am, rather than in
> configure.in.  These variables are not used anywhere else, for instance,
> in the sources.
> 
> What you really need in Makefile.am are only the variables STRCMP_SRC/OBJ,
> STRLEN_SRC/OBJ, MEMCHR_SRC/OBJ and MEMCPY_SRC/OBJ.
> 
> So, instead of defining the HAVE_foo vars in configure.in just to set
> the SRC/LIB vars in Makefile.am, wouldn't it make more sense to set the
> SRC/OBJ variables directly in configure.in rather than to spread this
> configuration over two file?

I specified it in my sample.  The whole reason this is being done is to exclude and include
files based on the processor and compilation options.  IMO, it is a little clearer
to have the reasoning in the Makefile.am where you are piecing together which
files are included/excluded but your way does get to use boolean operators which makes
it much more concise.

> 
> > diff --git a/newlib/libc/machine/arm/configure.in
> > b/newlib/libc/machine/arm/configure.in
> > index 6236338..edf9222 100644
> > --- a/newlib/libc/machine/arm/configure.in
> > +++ b/newlib/libc/machine/arm/configure.in
> > @@ -10,5 +10,133 @@ AC_CONFIG_AUX_DIR(../../../..)
> >  
> >  NEWLIB_CONFIGURE(../../..)
> >  
> > +dnl Check for Thumb1 supported.
> > +AC_CACHE_CHECK(whether we are using thumb1,
> > +	       acnewlib_cv_thumb1_processor, [dnl
> > +cat > conftest.c <<EOF
> > +
> > +#if defined (__thumb__) && !defined (__thumb2__)
> > +  #define _THUMB1
> > + #else
> > +  #error "not thumb1"
> > +#endif
> > +int main () {
> > +  return 0;
> > +}
> > +EOF
> > +if AC_TRY_COMMAND([${CC} $CFLAGS $CPPFLAGS -c -o conftest.o conftest.c
> > +							1>&AS_MESSAGE_LOG_FD])
> > +then
> > +  acnewlib_cv_thumb1_processor=yes;
> > +else
> > +  acnewlib_cv_thumb1_processor=no;
> > +fi
> > +rm -f conftest*])
> > [...]
> 
> Here I was wondering, do you really have to check for thumb1, armv7,
> armv7a, armv7m by running conftests?  Isn't this information easily
> available from the target triplet?  Sorry if that's a dumb question,
> but I'm not in ARM development so I really don't know.
> 

Thumb is specified via -mthumb and is a multilib in gcc.  I don't know if thumb is even used in
configuration triplets anymore.  AFAICT it looks like the arches may not always be used in
triplets either, looking at the arm toolsets out there.

-- Jeff J.

> Thanks
> Corinna
> 
> --
> Corinna Vinschen
> Cygwin Maintainer
> Red Hat
>