Re: C6X port 10/11: The port

[Bernd Schmidt <bernds at codesourcery dot com>]

On 05/10/2011 09:53 PM, Joseph S. Myers wrote:
> General comment: there are lots of new files here that are built for the 
> target, and if possible it's preferable for such sources to be under 
> libgcc/config/ with associated build rules also located there not in the 
> gcc/ directory.

I've made an attempt to move over the floating-point library functions,
but this doesn't work since the include paths aren't set up properly -
sfp-machine.h isn't found regardless of where it is placed. So I've left
things in place for now.

I'm also unsure what to do about t-c6x-softfp. I've noticed that e.g.
moxie has two copies of the corresponding file, but I don't know why.

>> +tic6x-*-uclinux)
>> +	tm_file="elfos.h ${tm_file} dbxelf.h c6x/elf.h tm-dwarf2.h c6x/uclinux-elf.h"
> 
> All targets based on the Linux kernel (with or without MMU) should now be 
> using gnu-user.h and linux.h

Changed (or added rather). I've split the elf.h header file into two to
make better use of this.

>> +	tm_file="$tm_file glibc-stdint.h"
>> +       	tmake_file="c6x/t-c6x c6x/t-c6x-elf c6x/t-c6x-uclinux c6x/t-opts"
> 
> Indentation seems odd here.

Tabs vs spaces, corrected.

> You're using t-slibgcc-elf-ver, so presumably building shared libgcc with 
> symbol versioning - but I don't see any C6X-specific version map to assign 
> symbol versions to C6X-specific symbols (both __c6xabi_*, and the __gnu_* 
> renamings of some GCC symbols to put them in a proper vendor namespace).  

So far using --enable-shared isn't recommended anyway, since nothing
sets the DSBT index in the resulting shared libraries. Hence it hasn't
been necessary up to this point.
However, I've now made use of the 9.5/11 patch I sent earlier which
enables __gnu_ prefixes for libgcc functions, and added a .ver file for
the c6xabi functions. So far the .ver file is hardly tested (Paul,
please check the unwind symbols - copied from the ARM version of the
file. Is restore_core_regs supposed to be hidden?).

>> +		case ${with_arch} in
>> +		"" | c64x+ | c674x)
> 
> That list is inconsistent with the full set of six CPU variants supported 
> by the port; I'd think all should be handled for --with-arch

Changed.

>> +	  /* Treat any failure as the end of unwinding, to cope more
>> +	     gracefully with missing EH information.  Mixed EH and
>> +	     non-EH within one object will usually result in failure,
>> +	     because the .ARM.exidx tables do not indicate the end
>> +	     of the code to which they apply; but mixed EH and non-EH
> 
> Bogus ARM reference in a comment (and, Paul implemented linker support for 
> closing address ranges for both ARM and C6X).

Changed the ARM thing, but I'll have to defer to Paul to correctly
modify the rest of the comment when he submits the rest of the EH work.

>> +/* Common implementation for ARM ABI defined personality routines.
> 
> Another bogus ARM reference.

Changed.

>> --- /dev/null
>> +++ config/c6x/t-opts

> I don't think it makes sense for this to be separate from t-c6x.

Changed.

>> +#ifndef UNWIND_ARM_H
>> +#define UNWIND_ARM_H

Changed.

>> +#undef CC1_SPEC
>> +#define CC1_SPEC "%{G*}"
> 
> You're not using g.opt, so the -G option doesn't exist for this target, so 
> this spec is suspicious.

Removed. We were using -G at some point but it doesn't work with this ABI.

>> +/* Make __c6xabi_AEABI_NAME an alias for __GCC_NAME.  */

> All this code now needs to go in a header in libgcc/config/, listed in 
> libgcc_tm_file.

Changed. However, there's no libgcc_tm_file as far as I can see and
other ports add the header to tm_file.

> Generated files in the source tree should be accompanied by 
> timestamp-updating rules on contrib/gcc_update (this also applies to the 
> file generated by t-opts).

Fixed.

> It's desirable for --target-help to show the list of valid values, either 
> in the help for the individual option (as for various Enum options in 
> common.opt, using the TAB-separated form of help text) or by adding a help 
> text to the Enum definition.

I've done the latter. Text added to the EnumValues appears to be
ignored, is that correct?

>> Index: config/c6x/c6x.c
> 
>> +#include "tm.h"
> 
>> +#include "toplev.h"
> 
>> +#include "c6x.h"
> 
>> +#include "c6x-protos.h"

Includes fixed up.

>> +#if 0 /* FIXME: Reenable when TI's tools are fixed.  */
>> +  /* ??? Ideally we'd check flag_short_wchar somehow.  */
>> +  asm_fprintf (asm_out_file, "\t.c6xabi_attribute Tag_ABI_wchar_t, %d\n", 2);
>> +#endif
> 
> ARM has arm_output_c_attributes in arm-c.c.

Thanks. Will revisit later once we can actually generate the attribute.

>> Index: config/c6x/c6x-sched.md
>> ===================================================================
>> --- /dev/null
>> +++ config/c6x/c6x-sched.md
>> @@ -0,0 +1,838 @@
>> +;; -*- buffer-read-only: t -*-
>> +;; Generated automatically from c6x-sched.md.in by gensched.sh
>> +
> 
> Should have copyright/license notice (probably copied from the input 
> file).

I've removed the bit that strips out comments.

> These look like generic comments (which should be avoided in target 
> headers) instead of saying anything C6X-specific.

All removed.

>> +#define FUNCTION_PROFILER(file, labelno) \
>> +  fatal_error("Profiling is not yet implemented for this architecture.")
> 
> Diagnostics should not start with a capital letter or end with "." (and 
> note missing space before "(" in function call).

Fixed.

>> Index: config/c6x/libunwind.S
>> Index: config/c6x/crti.s
>> Index: config/c6x/crtn.s
> Appears to need standard libgcc copyright/license notice.

Added.

> * contrib.texi should be updated.

Not sure what I should do here, but I've modified my own entry a bit. I
suppose Paul can add himself for the EH stuff. There appear to be
entries only for individuals, not companies (with one exception).

> * There should probably be something in install.texi.  (Noting 2.22 as the 
> minimum binutils version, supposing that features not in 2.21 are in fact 
> required by the port?)

I guess it's probably better to require 2.22. I couldn't come up with
more than a one-liner to write here though.

> * Add the two new targets to contrib/config-list.mk (and confirm they do 
> pass --enable-werror-always builds with current trunk GCC).

Added. I've tried to change my builds to use that option, but there are
lots of errors that appear unrelated to the C6X port. c6x.o compiles
without warnings.

Also added: a C6X version of longlong.h, and some recent changes from
our 4.5 tree.

This version has been tested with the complete set of targets on
c6x-elf. Results are the same for big- and little-endian in all cases.
Cleanup tests fail (they require Paul's EH work), and there seem to be a
few non-C6X related failures that don't appear with our 4.5 tree:
 - if an address is truncated to HImode or QImode, the compiler
   appears not to use legitimate_address_p and crashes with -mdsbt.
   Two testcases.
 - gcc.dg/march.c fails due to extra help text
 - gcc.dg/pr45259.c produces unexpected errors
 - gcc.dg/pr48335-2.c crashes in expand_expr_addr_expr_1
Other than that it all seems as expected.


Bernd

Attachment: c6xport.diff.gz
Description: GNU Zip compressed data