All comments here refer to the first instance of an issue; many issues are
shared between the two ports but are only mentioned once here.
On Sat, 15 Oct 2011, Walter Lee wrote:
> +#undef SYSROOT_SUFFIX_SPEC
> +#define SYSROOT_SUFFIX_SPEC "%{mbme:/usr/lib/bme;mnewlib:/usr/lib/newlib}"
> +
> +#undef SYSROOT_HEADERS_SUFFIX_SPEC
> +#define SYSROOT_HEADERS_SUFFIX_SPEC SYSROOT_SUFFIX_SPEC
These specs seem rather unusual. Typically such specs would be generated
depending on what multilibs were being built (see t-sysroot-suffix), and
since a subdirectory sysroot itself contains directories such as usr/lib
and usr/include, you would not expect a /usr/lib path within those specs.
It appears from the documentation that these are options for alternative C
libraries. There is an existing infrastructure for options for
alternative C libraries used with the Linux kernel, with the -mbionic,
-mglibc and -muclibc options all of which use Negative so that only the
last one is considered for specs processing, all of which set a single
variable and which affect the predefined macros, settings of
TARGET_C99_FUNCTIONS and TARGET_HAS_SINCOS, etc.; you should integrate
these options properly with that infrastructure.
But I don't see any sign of multilib settings to cause GCC's libraries to
be built for these variants - although libstdc++, for example, definitely
depends on libc in various ways, so you certainly can't expect to be able
to use a different libc without having a separate multilib for it.
(I am presuming that these are for libraries used in userspace with the
Linux kernel or as part of building the Linux kernel; if they aren't used
with Linux at all then a separate -elf or -eabi target would be the normal
approach.)
> +#undef ASM_SPEC
> +#define ASM_SPEC "%{v:-V} %{Qy:} %{!Qn:-Qy} %{n} %{T} %{Ym,*} %{Yd,*} \
> + %{Wa,*:%*} %{m32:--32} %{m64:--64}"
All of this apart from %{m32:--32} %{m64:--64} is obsolete. See a series
of my patches that cleaned up obsolete pieces in specs.
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00189.html> for -Wa,;
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00192.html> for -n and -T;
<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00195.html> for -v;
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00893.html> for -Yd,;
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00913.html> for -Ym,;
<http://gcc.gnu.org/ml/gcc-patches/2011-01/msg00914.html> for -Qn and -Qy.
> + %{!dynamic-linker:-dynamic-linker \
%{!dynamic-linker:...} doesn't do what you think and is also obsolete;
again see my patches
(<http://gcc.gnu.org/ml/gcc-patches/2010-12/msg00194.html> in this case).
> +#undef STARTFILE_SPEC
> +#define STARTFILE_SPEC "\
> + %{!shared: %{pg|p|profile:gcrt1.o%s;pie:Scrt1.o%s;:crt1.o%s}} \
> + crti.o%s \
> + %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s} \
> + %{ffeedback-emit*:crtfbi.o%s}"
There are no -ffeedback-emit* or -ffeedback-generate options in FSF GCC so
there should be no specs referencing them.
Without that, this looks identical to GNU_USER_TARGET_STARTFILE_SPEC in
gnu-user.h so you shouldn't need to override that at all (but if you just
need to add to it, then appeand to GNU_USER_TARGET_STARTFILE_SPEC rather
than replicating its contents).
> +#undef LIB_SPEC
> +#define LIB_SPEC \
> + "%{ffeedback-generate*|ffeedback-emit*:-lgcov -linstr} \
> + %{pthread:-lpthread} \
> + %{shared:-lc} \
> + %{!shared:%{mieee-fp:-lieee} %{profile:-lc_p}%{!profile:-lc}}"
You don't have a -mieee-fp option in your .opt file, so shouldn't have a
spec for it.
> +#undef MCOUNT_NAME
> +#define MCOUNT_NAME "mcount"
For a new target it seems much better to define your ABI to use a name in
the reserved namespace for this - that is, starting with two underscores.
> +/* For __clear_cache in libgcc2.c. */
> +#ifdef IN_LIBGCC2
> +
> +#include <arch/icache.h>
Where does this header come from? Linux kernel, glibc, somewhere else?
In general you want to condition header includes on inhibit_libc to
facilitate bootstrapping (including building a partial static libgcc)
before the libc headers are installed, since configuring glibc to install
its headers requires a working compiler to run configure tests.
> diff -r -u -p -N
> /home/packages/gcc-4.7.0-179959/gcc/config/tilegx/mul_tables.c
> ./gcc/config/tilegx/mul_tables.c
> --- /home/packages/gcc-4.7.0-179959/gcc/config/tilegx/mul_tables.c
> 1969-12-31 19:00:00.000000000 -0500
> +++ ./gcc/config/tilegx/mul_tables.c 2011-10-14 18:14:11.524757000 -0400
> @@ -0,0 +1,19853 @@
Are you really sure that this 19853-line file is source code - "the
preferred form of the work for making modifications to it"? How was it
written? If it was generated by a program, and modifying that program
would be the preferred way of changing this file, then the genuine source
code for that program needs to be included as well. The genuine source
code may not be used at build time - the ARM port has various code
generated by O'Caml generators, for example - but at least it needs to be
documented how someone, using the shipped source code and free software
implementations of any relevant languages, can regenerate the generated
file exactly. And if the generation is easy and doesn't require unusual
tools, the rules for doing so should go in the makefiles (if for whatever
reason a generated file still needs shipping in the source tree, also
update contrib/gcc_update to avoid accidental regeneration).
> +tilegx-c.o: $(srcdir)/config/tilegx/tilegx-c.c \
> + $(CONFIG_H) $(SYSTEM_H) coretypes.h $(MACHMODE_H) \
> + $(TM_H) $(TM_P_H) $(CPPLIB_H) $(TREE_H) $(C_COMMON_H)
> + $(CC) -c $(ALL_CFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
> + $(srcdir)/config/tilegx/tilegx-c.c
> +
> +mul_tables.o: $(srcdir)/config/tilegx/mul_tables.c \
> + $(CONFIG_H) $(SYSTEM_H) coretypes.h $(EXPR_H) $(OPTABS_H) \
> + $(srcdir)/config/tilegx/tilegx-multiply.h
> + $(CC) -c $(ALL_CFLAGS) $(ALL_CPPFLAGS) $(INCLUDES) \
> + $(srcdir)/config/tilegx/mul_tables.c
When the compiler is built as C++, all source of the compiler should
normally also be built as C++. That is, you should use $(COMPILER), not
$(CC). You should also verify that if you build the compiler using a
current trunk compiler as your starting point, it passes a
--enable-werror-always build as both C and C++; if you have only tested
with cross compiler builds rather than native bootstrap, those don't use
-Werror by default, hence the need to test with --enable-werror-always and
to use a recent trunk compiler as the starting point.
> +#ifndef _TILEGX_MULTIPLY_H
> +#define _TILEGX_MULTIPLY_H
Header include guards in host-side code like this should not be in the
reserved namespace; use e.g. GCC_TILEGX_MULTIPLY_H instead.
> +#ifndef __TILEGX_PROTOS_H__
> +#define __TILEGX_PROTOS_H__
Likewise.
> +#include "errors.h"
errors.h is the header for cut-down diagnostic functions used in generator
programs. Normal compiler code should use diagnostic-core.h instead.
> +/* Implement TARGET_OPTION_OVERRIDE. */
> +static void
> +tilegx_option_override (void)
> +{
> + static const struct cpu_table {
> + const char *const name;
> + const enum processor_type processor;
> + } cpu_table[] = {
> + { "tilegx", PROCESSOR_TILEGX },
> + { NULL, PROCESSOR_MAX }
> + };
> + const char *cpu_string;
> + int i;
> +
> + /* Default to TILE-GX. */
> + cpu_string = tilegx_cpu_string;
> + if (cpu_string == NULL)
> + cpu_string = "tilegx";
> +
> + for (i = 0; cpu_table[i].name; i++)
> + if (!strcmp (cpu_string, cpu_table[i].name))
> + {
> + tilegx_cpu = cpu_table[i].processor;
> + break;
> + }
> +
> + if (!cpu_table[i].name)
> + error ("bad value %s for -mcpu switch", cpu_string);
Is there a reason all this needs to use this hook rather than the generic
.opt file Enum facility for options with enumerated arguments?
> +/* Return the simd variant of the constant NUM of mode MODE, by
> + replicating it to fill an interger of mode DImode. NUM is first
> + truncated to fit in MODE. */
> +rtx
> +tilegx_simd_int (rtx num, enum machine_mode mode)
> +{
> + HOST_WIDE_INT n = 0;
> +
> + if (CONST_INT_P (num))
> + {
> + n = INTVAL (num);
> + }
> + else
> + {
> + error ("Immediate operand to SIMD op not a constant.");
> + }
Error messages should not start with a capital letter or end with '.'.
Is this a message about a bad argument to a built-in function, or
something like that? If not (if no input should ever generate it), then
I'd think it should be an internal error, not a normal one. But if it
does come from built-in expansion then this point is probably too late to
get a good diagnostic location; ideally you'd use error_at with a location
derived from the relevant expression, and generate errors early enough for
that to be possible.
> + if (!(*insn_op->predicate) (op[opnum], insn_op->mode))
> + {
> + /* We still failed to meet the predicate even after moving
> + into a register. Assume we needed an immediate. */
> + error ("operand must be an immediate of the right size");
Here's another case where explicit locations would be better. You may at
least be able to check EXPR_HAS_LOCATION (exp) and use the expression's
location if so.
> + output_operand_lossage ("Invalid operand for 'r' specifier.");
Again, diagnostics should not start with an uppercase letter or end with a
period.
> +@smallexample
> +
> +unsigned long long __insn_op (...)
> +
> +@end smallexample
> +
> +Where ``op'' is the name of the instruction. Refer to the ISA manual
> +for the complete list of instructions.
Should use @var{op} in both the example and in the main text in place of
``op''.
> +tilegx-*-linux*)
> + tm_file="elfos.h gnu-user.h linux.h glibc-stdint.h tilegx/linux.h
> ${tm_file}"
> + tmake_file="${tmake_file} tilegx/t-tilegx"
> + extra_headers="tilegx-builtins.h tilegx-multiply.h"
> + extra_objs="mul_tables.o"
> + c_target_objs="tilegx-c.o"
> + cxx_target_objs="tilegx-c.o"
> + ;;
> +tilepro-*-linux*)
> + tm_file="elfos.h gnu-user.h linux.h glibc-stdint.h tilepro/linux.h
> ${tm_file}"
> + tmake_file="${tmake_file} tilepro/t-tilepro"
> + extra_headers="tilepro-builtins.h tilepro-multiply.h"
extra_headers is for headers for the *target*, that should be installed
with the compiler and should have the runtime library license exception,
not for headers that are part of the compiler on the *host*.
--
Joseph S. Myers
jos...@codesourcery.com
