This is the mail archive of the libc-ports@sources.redhat.com mailing list for the libc-ports project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] am33: bring port up to date with NPTL support


On Mon, 18 Jun 2012, Mark Salter wrote:

> This is a first cut at bringing the AM33 port up to date. The basis of
> the NPTL support comes from an older working implementation based on
> glibc 2.9. Additional changes have been made to deal with glibc changes
> and a newer gcc. Currently, this is untested pending some gcc tls support
> fixes.

General observations:

* New files should use a URL instead of an FSF postal address; that change 
was made globally in libc and ports.

* There should be an ABI test baseline in 
sysdeps/unix/sysv/linux/am33/nptl/.

* There should be a C++ types baseline in data/ (and a localplt one if the 
generic one doesn't suffice).

* sysdeps/unix/sysv/linux/am33/configure.in should change 
arch_minimum_kernel to 2.6.25, the version where am33 support went into 
kernel.org kernels, and you should have a kernel-features.h unless the 
libc version is fully correct about when features were supported on am33.

* Use copyright ranges (<year>-2012) in all new or changed files with more 
than one copyright year.  Some files appear to be missing copyright date 
updates.

Specific comments:

> diff --git a/sysdeps/am33/atomicity.h b/sysdeps/am33/atomicity.h
> index b0ba43d..b3ba5a4 100644
> --- a/sysdeps/am33/atomicity.h
> +++ b/sysdeps/am33/atomicity.h

You're patching a file that nothing (in libc, ports or this patch) uses.  
Remove it instead.  It's probably a good idea to check all am33 port files 
(after this patch) to make sure they are actually used.

> diff --git a/sysdeps/am33/bits/setjmp.h b/sysdeps/am33/bits/setjmp.h
> index 6dd87cb..8d0c11b 100644
> --- a/sysdeps/am33/bits/setjmp.h
> +++ b/sysdeps/am33/bits/setjmp.h

> +/* Test if longjmp to JMPBUF would unwind the frame
> +   containing a local variable at ADDRESS.  */
> +#define _JMPBUF_UNWINDS(jmpbuf, address) \
> +  ((void *) (address) < (void *) (jmpbuf[__JMP_BUF_SP]))

This belongs in jmpbuf-unwind.h, not bits/setjmp.h.  The other definitions 
you add belong in jmpbuf-offsets.h, not bits/setjmp.h.

> +#if !defined RTLD_BOOTSTRAP || USE___THREAD

USE___THREAD is an obsolete macro.

> +#if (!defined RTLD_BOOTSTRAP || USE___THREAD) \

Likewise.

> diff --git a/sysdeps/am33/elf/configure.in b/sysdeps/am33/elf/configure.in

/elf/ sysdeps directories are no longer used, so this belongs directly in 
sysdeps/am33/.

> +if test "$usetls" != no; then

Obsolete conditional.

> +if test $libc_cv_am33_tls = yes; then
> +  AC_DEFINE(HAVE_TLS_SUPPORT)
> +fi

Obsolete macro.  Instead, use AC_MSG_ERROR if TLS support is missing.

> diff --git a/sysdeps/am33/fpu/feholdexcpt.c b/sysdeps/am33/fpu/feholdexcpt.c
> +libm_hidden_def (feholdexcept)
> diff --git a/sysdeps/am33/fpu/fesetround.c b/sysdeps/am33/fpu/fesetround.c
> +libm_hidden_def (fesetround)

I think you need such changes to fegetenv.c, fesetenv.c, feupdateenv.c, 
fraiseexcpt.c and ftestexcept.c as well.

> diff --git a/sysdeps/am33/fpu/libm-test-ulps b/sysdeps/am33/fpu/libm-test-ulps
> new file mode 100644
> index 0000000..ad50fcb
> --- /dev/null
> +++ b/sysdeps/am33/fpu/libm-test-ulps
> @@ -0,0 +1,605 @@

That looks too short to have been recently generated ... once you have 
testing working you should regenerate this for current glibc, starting 
with an empty file.

> diff --git a/sysdeps/am33/init-misc.c b/sysdeps/am33/init-misc.c

I do wonder whether overriding this file is really the best way to 
initialize something architecture-specific ... maybe someone else has a 
better way to do it?

> +#ifdef HAVE_ELF
> +#define L(name)		.L##name
> +#else

HAVE_ELF is obsolete.

> diff --git a/sysdeps/arm/dl-machine.h b/sysdeps/arm/dl-machine.h

You should not be modifying files for another machine when submitting a 
port.  If you wish to submit this patch, you need to do so separately, 
with its own rationale.

> diff --git a/sysdeps/unix/sysv/linux/am33/bits/fcntl.h b/sysdeps/unix/sysv/linux/am33/bits/fcntl.h
> index 33b8bcd..588e5fa 100644
> --- a/sysdeps/unix/sysv/linux/am33/bits/fcntl.h
> +++ b/sysdeps/unix/sysv/linux/am33/bits/fcntl.h

Do you need changes to O_SYNC and O_DSYNC to match 2.6.33+ kernels (see 
<http://sourceware.org/ml/libc-ports/2009-12/msg00013.html>)?

You'll want to compare with other architectures - it looks like there are 
various other ways this patch does not update bits/fcntl.h to be fully in 
sync regarding what macros are defined under what conditions, what 
functions are declared, etc.

> diff --git a/sysdeps/unix/sysv/linux/am33/bits/mman.h b/sysdeps/unix/sysv/linux/am33/bits/mman.h
> index 763b060..ff759b5 100644
> --- a/sysdeps/unix/sysv/linux/am33/bits/mman.h
> +++ b/sysdeps/unix/sysv/linux/am33/bits/mman.h

Again, does not appear to be a complete update.

> diff --git a/sysdeps/unix/sysv/linux/am33/nptl/Versions b/sysdeps/unix/sysv/linux/am33/nptl/Versions
> new file mode 100644
> index 0000000..435c921
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/am33/nptl/Versions
> @@ -0,0 +1,8 @@
> +libc {
> +  GLIBC_PRIVATE {
> +    # A copy of sigaction lives in NPTL, and needs these.
> +    __default_sa_restorer; __default_rt_sa_restorer;
> +    __default_sa_restorer_v1; __default_rt_sa_restorer_v1;
> +    __default_sa_restorer_v2; __default_rt_sa_restorer_v2;
> +  }
> +}

Too much copying from other ports - these are ARM-specific and don't exist 
for am33.

> +strong_alias (__pthread_once, __pthread_once_internal)

We removed those aliases, replacing them by hidden_def.

There may be more issues with the port not being up to date with global 
changes, but they would probably be easier to see once the above issues 
are fixed and the fixed patch checked in, rather than trying to compare 
the combination of the old port and this patch against other ports.

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]