Re: [RFC PATCH V2 00/10] port C-SKY to glibc

[Joseph Myers <joseph@codesourcery.com>]

When maintaining or submitting a new glibc port, it is strongly advisable 
to read recent reviews of other glibc ports, because there are many issues 
relating to current good practice in glibc ports that are relevant to any 
new port submission, and so by studying the past reviews you can avoid 
your port suffering from the same problems as the previous ports.  Thus, 
you should read all the reviews in the past year or so of the ARC port and 
of many versions of the RISC-V port, and address the issues in the next 
version of this port, where relevant to it.

In addition, it is important when maintaining a port out of tree to ensure 
that you keep monitoring global glibc changes made across ports and keep 
your port up to date for such global changes.  Such fixes to update the 
port for such global changes are an important part of the work Sandra has 
done to prepare the binutils and GCC ports for upstream submission.

The following are a very non-exhaustive list of such generic issues that I 
believe are applicable to this port.  I expect there are others that can 
be found by a more detailed study of the ARC and RISC-V reviews.


1. Any glibc port submission needs to include the results of running the 
glibc testsuite, for all the main supported configurations of the port.  
This means the full testsuite, both compilation and execution tests.  It 
may be run natively, or you may use cross testing using test-wrapper.

The final part of the "make check" output, listing all non-PASS/XFAIL 
tests and statistics of different test results, needs to be included in 
the port submission.  It's desirable to provide a tarball of .out / 
.test-result files for all such tests, and the full "make check" output, 
somewhere external from where people can download it to study the results 
in more detail - but the basic summary, for each of the main supported 
configurations, needs to be included in the port submission.

Furthermore, the results that are relevant are, as I said in ARC port 
discussions, results with *upstream* versions of the other components, not 
some version that may be on GitHub or elsewhere.  That means upstream 
Linux, binutils, GCC - or, until the ports are upstream, the latest 
versions submitted upstream or being prepared for upstream.

Since all the ABIv1 support is being removed as part of preparing the GCC 
port for upstream, that means no ABIv1 support should be included anywhere 
in the glibc port.  It's OK to have #if or similar conditionals with 
#error in the ABIv1 case, but not anything more than errors for ABIv1.  
This is exactly analogous to the case of 32-bit RISC-V: because the kernel 
support for 32-bit RISC-V was not ready when the glibc port went in, all 
such support was removed in the glibc port or turned into #errors.  If in 
future the kernel support for 32-bit RISC-V is good enough to run the 
glibc testsuite with reasonable results, glibc support can be added at 
that point (and much the same would apply if ABIv1 support were added to 
GCC for C-SKY in future - of course, for upstream it would need to be 
cleanly implemented with appropriate -m options for selecting the ABI).

Furthermore, for a glibc port to be considered in a reasonable state for 
upstream, we expect no more than about 20 architecture-specific failures 
(you can disregard generic cross-testing failures such as listed at 
<https://sourceware.org/glibc/wiki/Release/2.27>).


2. Any glibc port needs to add appropriate configurations to 
build-many-glibcs.py.  These should cover all the different ABIs supported 
by the port, and any other significant variations.  When using the latest 
upstream versions of all components, these configurations must pass 
compilers / glibcs builds - there must be no compilation test failures for 
the new configurations whatever.

I don't believe the present port would pass the compilation parts of the 
testsuite.  For example, your sys/ucontext.h headers include headers they 
should not include and define names that are not permitted to be defined, 
without any __USE_MISC etc. conditionals as in other such headers.  (This 
is also an example of something that would have shown up in monitoring 
global glibc changes to keep your port up to date with them.)


3. The minimum symbol version needs to be GLIBC_2.28 at present.  If the 
kernel port isn't upstream in the next few months, then the glibc port 
can't go upstream in that time either, so you'll need a corresponding 
later minimum symbol version.


4. arch_minimum_kernel has to reflect the kernel version in which the 
support goes upstream.  That's certainly not 2.6.25.  Since the support 
didn't get into 4.17-rc1, presumably it needs to be 4.18.0 or later, and 
to keep being updated until the kernel support is upstream.


5. New ports should have ABI-specific dynamic linker names - a different 
name for each supported ABI variant, different from those used on all 
other architectures, to support systems using multi-arch directory 
arrangements.  For example, ld-linux-csky-{be,le}.so.1 (since you seem to 
support both endiannesses) might be appropriate, but not ld.so.1 which you 
currently have in shlib-versions (and not ld-linux.so.2 which you 
currently have in a bogus entry in ldconfig.h).  If there are other 
relevant ABI variants beyond endianness, those should be allowed for in 
the dynamic linker names.  You should give a more detailed explanation of 
what variants are or are not ABI compatible to justify the particular 
choice of what gets a separate dynamic linker name.

This of course needs coordinating with the GCC port so it passes 
appropriate -dynamic-linker options to ld.


6. Many files are missing the one-line description on the first line of 
the file before the copyright notice.


The following are more specific to this port:


7. There should be no abiv2_* files.  If ABIv1 were supported, the sysdeps 
mechanism would need to be used with abiv1/ and abiv2/ subdirectories to 
select appropriate files.


8. You have *_vfp* function names and references to VFP in comments.  Is 
VFP really the name of the floating-point unit for this architecture?  If 
not, this is cargo-culted from ARM and needs to be fixed (likewise, the 
comments in math-tests.h may not be correct for this architecture).


9. You have __csky_hard_float__ conditionals in sysdeps/csky/fpu/ files.  
That makes no sense.  If you have a proper with_fp_cond setting (see about 
monitoring global changes, again), then fpu/ files shouldn't be used at 
all for soft float.  That means you can remove those conditionals and just 
rely on the default math/ fallback implementations of fenv.h functions.

Such conditionals are still relevant in *installed* headers, but not in 
fpu/ files.


10. Existing practice in glibc is that where you have e.g. FE_DENORMAL, 
such architecture-specific values are only defined in the implementation 
namespace (e.g. __FE_DENORM in sysdeps/x86/fpu/bits/fenv.h) and are *not* 
included in FE_ALL_EXCEPT.

-- 
Joseph S. Myers
joseph@codesourcery.com