Re: [PATCH] Add new target h8300-*-linux

[Jeff Law <law@redhat.com>]

On 01/22/15 01:38, Yoshinori Sato wrote:
> Add h8300-*-linux target for h8300 linux kernel and userland.
>
> h8300-*-elf is some difference of standard elf.
> h8300-*-linux is compatible of standard elf rules.
It seems to me that you should have linux.h just define things that are 
specific to the linux port rather than copying huge amounts of h8300.h.

> diff --git a/gcc/config/h8300/h8300.c b/gcc/config/h8300/h8300.c
> index e7ed03a..4ec8516 100644
> --- a/gcc/config/h8300/h8300.c
> +++ b/gcc/config/h8300/h8300.c
> @@ -359,11 +359,13 @@ h8300_option_override (void)
>         target_flags |= MASK_H8300S_1;
>       }
>
> +#ifndef __h8300_linux__
>     if (TARGET_H8300 && TARGET_INT32)
>      {
>         error ("-mint32 is not supported for H8300 and H8300L targets");
>         target_flags ^= MASK_INT32;
>      }
> +#endif
On precisely what targets is Linux going to be supported on the H8 
series?  Presumably it's going to be some H8/S variants and newer?


> --- /dev/null
> +++ b/gcc/config/h8300/linux.h
This really should just be overriding any generic definitions in h8300.h 
rather than copying most of that file.  I didn't read through this in 
any detail as it's unacceptable as-is.


Presumably you're using the standard ABI (parameter passing, register 
usage, etc).

Presumably you're requiring 32 bit integers?



> diff --git a/gcc/config/h8300/t-linux b/gcc/config/h8300/t-linux
> new file mode 100644
> index 0000000..ac8b0cb
> --- /dev/null
> +++ b/gcc/config/h8300/t-linux
> @@ -0,0 +1,20 @@
> +# Copyright (C) 2015 Free Software Foundation, Inc.
> +#
> +# This file is part of GCC.
> +#
> +# GCC is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3, or (at your option)
> +# any later version.
> +#
> +# GCC is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with GCC; see the file COPYING3.  If not see
> +# <http://www.gnu.org/licenses/>.
> +
> +MULTILIB_OPTIONS = mh/ms/msx
> +MULTILIB_DIRNAMES = h8300h h8300s h8sx
So are you really going to support Linux on the old H8/300H?  Is there 
enough address space on that target for it to work (my recollection of 
these processors is fading, but IIRC the H8/300H had a rather limited 
address space.

> index 4bfa7be..e3abc89 100644
> --- a/libgcc/config/h8300/lib1funcs.S
> +++ b/libgcc/config/h8300/lib1funcs.S
> @@ -84,6 +84,12 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>   #define A3E	e3
>   #endif
>
> +#define CONCAT(A,B)     A##B
> +#define LABEL0(U,X)    CONCAT(U,__##X)
> +#define LABEL0_DEF(U,X)    CONCAT(U,__##X##:)
> +#define LABEL_DEF(X)       LABEL0_DEF(__USER_LABEL_PREFIX__,X)
> +#define LABEL(X)       LABEL0(__USER_LABEL_PREFIX__,X)
> +
>   #ifdef __H8300H__
>   #ifdef __NORMAL_MODE__
>   	.h8300hn
> @@ -111,8 +117,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
>   #ifdef __H8300__
>   	.section .text
>   	.align 2
> -	.global ___cmpsi2
> -___cmpsi2:
> +	.global LABEL(cmpsi2)
> +LABEL_DEF(cmpsi2)
Presumably this is to get consistency in the namespace and ensure that 
USER_LABEL_PREFIX is honored?


You're going to need a copyright assignment with the FSF.  You should 
get that process started so that when the next stage1 development cycle 
opens up, you'll be in a position where we can legally accept your 
contributions.

The patch will need a ChangeLog entry.  See gcc/ChangeLog for examples. 
  Remember the ChangeLog merely describes what changed, not why.  If 
something needs a "why" explanation, then that explanation belongs in 
the code itself.


Jeff