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

[Corinna Vinschen <vinschen at redhat dot com>]

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?

> 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.


Thanks
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: pgpomUCvqwz7D.pgp
Description: PGP signature