From: Stafford Horne via Libc-alpha <libc-alpha@sourceware.org>
To: Joseph Myers <joseph@codesourcery.com>
Cc: Openrisc <openrisc@lists.librecores.org>,
GLIBC patches <libc-alpha@sourceware.org>,
Christian Svensson <blue@cmd.nu>
Subject: Re: [PATCH 1/1] Initial support for OpenRISC
Date: Sat, 23 May 2020 06:31:23 +0900 [thread overview]
Message-ID: <20200522213123.GB75760@lianli.shorne-pla.net> (raw)
In-Reply-To: <alpine.DEB.2.21.2005222027180.10437@digraph.polyomino.org.uk>
Hello,
Thanks for the review.
On Fri, May 22, 2020 at 08:56:08PM +0000, Joseph Myers wrote:
> On Fri, 22 May 2020, Stafford Horne via Libc-alpha wrote:
>
> > This patch includes the OpenRISC glibc support for linux.
>
> This is missing a build-many-glibcs.py update. You always need to include
> such an update with a new port, which must build at least one
> configuration for each ABI supported by the port (and may build any major
> non-ABI variants that seem relevant). build-many-glibcs.py results for
> the port must be clean (i.e. no failures in the compilation part of the
> testsuite), at least when building with GCC and binutils master (it's OK
> if you need features or bug fixes not in the most recent release branches,
> although distributors might prefer such fixes to be backported).
> Similarly, there should be NEWS and README updates.
OK.
> New 32-bit ports should preferably be set up to support only 64-bit time
> and 64-bit offsets (see ARC and RV32 patch postings for examples).
OK.
> > diff --git a/sysdeps/or1k/bits/atomic.h b/sysdeps/or1k/bits/atomic.h
>
> There should be no such file. bits/ is only for installed headers, not
> internal ones. See commit de071d199a8578055edf2722114788ae749823aa ("Move
> bits/atomic.h to atomic-machine.h (bug 14912).", 11 Sep 2015).
OK.
> > diff --git a/sysdeps/or1k/nptl/bits/semaphore.h b/sysdeps/or1k/nptl/bits/semaphore.h
>
> Do you really need this file? See commit
> 1270fbaaeebe642db335fccaaf98c82e6647cc0d ("semaphore: consolidate arch
> headers into a generic one", 5 May 2020).
Likely, not, I will review.
> > + # Needed for relro detection
> > + libc_commonpagesize=0x2000
> > + libc_relro_required=yes
>
> See commit cb403c34c6f6e1cce5018864485958cfc2e28906 ("Remove relro
> configure test.", 27 Jun 2014).
OK.
> When you have an old out-of-tree port, it's a good idea to go through
> cross-architecture commits from when the port was first created onwards to
> identify such global changes that need to be applied to that port.
I did try to do this, but it started in 2014 it was hard to pick up everything.
I also payed close attention to the riscv and csky ports.
> > diff --git a/sysdeps/unix/sysv/linux/or1k/bits/mman.h b/sysdeps/unix/sysv/linux/or1k/bits/mman.h
>
> You shouldn't have this file. See the comment in
> sysdeps/unix/sysv/linux/bits/mman.h (and commit
> d3a43e49f342c4663af0fff9d95000cfe26867c3, "Unify many bits/mman.h
> headers.", 18 Sep 2018, and commit
> 61d8b5feeed36e242a043befe9b11f7f8c294f58, "Share MAP_* flags between more
> architectures.", 26 Sep 2018):
>
> /* These definitions are appropriate for architectures that, in the
> Linux kernel, either have no uapi/asm/mman.h, or have one that
> includes asm-generic/mman.h without any changes or additions
> relevant to glibc. If there are additions relevant to glibc, an
> architecture-specific bits/mman.h is needed. */
OK.
> > diff --git a/sysdeps/unix/sysv/linux/or1k/configure.ac b/sysdeps/unix/sysv/linux/or1k/configure.ac
> > new file mode 100644
> > index 0000000000..505530d5c3
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/or1k/configure.ac
> > @@ -0,0 +1,4 @@
> > +GLIBC_PROVIDES dnl See aclocal.m4 in the top level source directory.
> > +# Local configure fragment for sysdeps/unix/sysv/linux/or1k.
> > +
> > +arch_minimum_kernel=3.4.0
>
> You'll probably need a newer minimum kernel when requiring 64-bit time
> support, until all the fallback for 64-bit time on 32-bit kernels without
> the 64-bit-time syscalls is implemented.
OK, noted.
> > +#undef __ASSUME_SET_ROBUST_LIST
>
> Are you sure this is never supported on this architecture?
> arch/openrisc/include/asm/futex.h appears to have gained
> futex_atomic_cmpxchg_inatomic in 4.11, so I'd expect the #undef only to be
> for kernels older than that.
Sorry, I overlooked this thanks for catching. I added the atomic support to the
kernel.
> > diff --git a/sysdeps/unix/sysv/linux/or1k/sys/procfs.h b/sysdeps/unix/sysv/linux/or1k/sys/procfs.h
>
> See commit d62f9ec0cce26a275ec68f4564814041a33395b1 ("Complete
> sys/procfs.h unification.", 25 Sep 2018). You shouldn't have an
> architecture-specific version of this file; add whatever bits/ headers are
> needed instead.
OK.
> In general, each architecture-specific file needs a justification that
> either no generic version exists or the generic one is unsuitable or
> suboptimal for this architecture; the default is that we want to use
> unified implementations of as many files as possible.
This has also been my goal for OpenRISC (use as much generic code as possible),
but when going over the original port I missed some of these. I noticed in the
last few months since I started fixing up the port that many more architecture
specific parts were moved to generic code and I did those moves. I seem to
missed everything from before 2019 when I started the port.
Thank you for pointing these out.
I will work on cleaning this up, testing and posting a new version in a week or
two.
-Stafford
next prev parent reply other threads:[~2020-05-22 21:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 11:36 [PATCH 0/1] OpenRISC port Stafford Horne via Libc-alpha
2020-05-22 11:36 ` [PATCH 1/1] Initial support for OpenRISC Stafford Horne via Libc-alpha
2020-05-22 12:56 ` Florian Weimer via Libc-alpha
2020-05-22 21:59 ` Stafford Horne via Libc-alpha
2020-05-22 22:02 ` Joseph Myers
2020-05-22 22:32 ` Stafford Horne via Libc-alpha
2020-06-10 20:17 ` Stafford Horne via Libc-alpha
2020-05-22 20:56 ` Joseph Myers
2020-05-22 21:31 ` Stafford Horne via Libc-alpha [this message]
2020-05-22 18:52 ` [PATCH 0/1] OpenRISC port Joseph Myers
2020-05-22 21:01 ` Stafford Horne via Libc-alpha
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
List information: https://www.gnu.org/software/libc/involved.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200522213123.GB75760@lianli.shorne-pla.net \
--to=libc-alpha@sourceware.org \
--cc=blue@cmd.nu \
--cc=joseph@codesourcery.com \
--cc=openrisc@lists.librecores.org \
--cc=shorne@gmail.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).