From: "Joseph S. Myers" <joseph@codesourcery.com>
To: Mark Salter <msalter@redhat.com>
Cc: libc-ports@sourceware.org
Subject: Re: [PATCH] am33: bring port up to date with NPTL support
Date: Mon, 18 Jun 2012 15:23:00 -0000 [thread overview]
Message-ID: <Pine.LNX.4.64.1206181450580.4626@digraph.polyomino.org.uk> (raw)
In-Reply-To: <1340030785-32581-1-git-send-email-msalter@redhat.com>
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
next prev parent reply other threads:[~2012-06-18 15:23 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-18 14:47 Mark Salter
2012-06-18 15:23 ` Joseph S. Myers [this message]
2012-06-18 15:41 ` Joseph S. Myers
2012-06-18 17:00 ` Joseph S. Myers
2014-01-21 22:53 ` Joseph S. Myers
2014-01-21 23:40 ` Mark Salter
2014-01-22 0:02 ` Joseph S. Myers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.1206181450580.4626@digraph.polyomino.org.uk \
--to=joseph@codesourcery.com \
--cc=libc-ports@sourceware.org \
--cc=msalter@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).