Re: New port: Renesas RL78

[DJ Delorie]

> Ah.  All validity issues aside, then "Dereferencing unaligned
> pointers yields a compile-time error" or "pointers with unknown
> alignment" would be much less cryptic: the "Dereferencing -1"
> just sounds like *(char *) -1 or a cut

I put a more explanatory comment in there.

I'm not exactly sure what to do about that code, though - the whole
point of the code is to make the app crash, but you can't crash
embedded that way, and anything I do to "make it work" makes it *less*
useful.  Not that I've tried libssp with RL78 yet, either.

Re: New port: Renesas RL78

[DJ Delorie]

> This is unnecessary with my just applied series of libgcc patches:
> it's the default for all *-*-elf targets.

Moving targets!  Fixed.

> This is not only unnecessary, as Joseph already noted, but doesn't
> work for quite some time since fp-bit.c has been moved to libgcc.

Fixed.

Re: New port: Renesas RL78

[DJ Delorie]

> > -  unsigned int __max_iter = 100;
> > +  unsigned int __max_iter = 65536U;
> 
> Doesn't that need to be 65535U for your purpose?

Yup.  The other three did.

> Should have the runtime license exception.

Added.

> This should not be needed; just using t-fdpbit should suffice for
> almost all targets.

Done.

> > \ No newline at end of file
> 
> As already noted, avoid this.

Fixed.

> > +static int register_sizes[] =
> 
> const?

Yup.

> There are a lot of functions and variables without comments explaining 
> them and their parameters.  (If a function just implements a standard 
> hook, a comment identifying which hook suffices without needing to explain 
> the parameters of that hook again.)

I went through and commented everything in rl78.c at least; I think
the .h and .md are fairly self-documenting.  I put a big comment at
the start of the devirtualization functions giving an overview of the
real/virtual thing.

> > +  if (letter == 'q' || letter == 'Q')
> > +   error ("q/Q modifiers invalid for symbol referencs");
> 
> "referencs" should be "reference" or "references" or similar.  I think 
> this should call output_operand_lossage not error, so that it becomes an 
> error or an ICE as appropriate depending on whether the source of the 
> error is the machine description or an asm.

Done.

> cross compiler configured with --enable-werror-always, on both 32-bit and 
> 64-bit hosts if possible.

I always build on 64 bit :-)

> I think the target-independent changes outside the back end (the ones that 
> are meant as separate bug fixes required by the port) should be posted in 
> separate messages, each with its own rationale.

Will do.

Re: New port: Renesas RL78

[DJ Delorie]

> Wouldn't some tools behave better if the asm files had ELF
> decorations on the functions?

If you mean .type and .size, I added those.

Re: New port: Renesas RL78

[DJ Delorie]

Nobody has asked the obvious question: why does libssp use "*(int *)(-1)
= 0;" in the first place?

Re: New port: Renesas RL78

[DJ Delorie]

> Ah.  All validity issues aside, then "Dereferencing unaligned
> pointers yields a compile-time error" or "pointers with unknown
> alignment" would be much less cryptic: the "Dereferencing -1"
> just sounds like *(char *) -1 or a cut

Good point.

Re: New port: Renesas RL78

[Hans-Peter Nilsson]

On Wed, 2 Nov 2011, DJ Delorie wrote:
> > > Index: configure.ac
> > > +# Dereferencing -1 is a compile-time error
> >
> > This (those lines) look a little cryptic (and lack punctuation ;)
> > Wild improvement guess: "Too small 'int'?".
>
> No, the compiler specifically tests for unaligned accesses and gives a
> compile-time error, at Renesas's requrest.  The chip can't do
> unaligned accesses correctly, but it doesn't fault - it just silently
> accesses the wrong memory.  So the compiler errors instead.

Ah.  All validity issues aside, then "Dereferencing unaligned
pointers yields a compile-time error" or "pointers with unknown
alignment" would be much less cryptic: the "Dereferencing -1"
just sounds like *(char *) -1 or a cut

brgds, H-P

Re: New port: Renesas RL78

[DJ Delorie]

> deduce that this path is unreachable is to generate an abort and output an 
> informative notice with inform ().

Hmmm... I'll see if I can catch it early enough to do something more
meaningful, then.

> (I don't see what actually generates the error, since there are only two 

Actually, it's in binutils.  GCC passes along the unaligned absolute
address, which causes an error.

Re: New port: Renesas RL78

[Rainer Orth]

DJ Delorie  writes:

> Index: libgcc/config.host
> ===
> --- libgcc/config.host(revision 180758)
> +++ libgcc/config.host(working copy)
> @@ -683,12 +683,16 @@ rs6000-ibm-aix5.1.* | powerpc-ibm-aix5.1
>   tmake_file="t-fdpbit rs6000/t-ppc64-fp rs6000/t-ibm-ldouble"
>   ;;
>  rs6000-ibm-aix[56789].* | powerpc-ibm-aix[56789].*)
>   md_unwind_header=rs6000/aix-unwind.h
>   tmake_file="t-fdpbit rs6000/t-ppc64-fp rs6000/t-ibm-ldouble"
>   ;;
> +rl78-*-elf)
> + extra_parts="crtbegin.o crtend.o"

This is unnecessary with my just applied series of libgcc patches: it's
the default for all *-*-elf targets.

> Index: libgcc/config/rl78/t-rl78
[...]
> +FPBIT = fp-bit.c
> +$(gcc_objdir)/fp-bit.c: $(gcc_srcdir)/config/fp-bit.c
> + echo '#define FLOAT' > $@
> + cat $(gcc_srcdir)/config/fp-bit.c   >> $@
> +
> +DPBIT = dp-bit.c
> +$(gcc_objdir)/dp-bit.c: $(gcc_srcdir)/config/fp-bit.c
> + cat $(gcc_srcdir)/config/fp-bit.c   >> $@

This is not only unnecessary, as Joseph already noted, but doesn't work
for quite some time since fp-bit.c has been moved to libgcc.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: New port: Renesas RL78

[Joseph S. Myers]

On Wed, 2 Nov 2011, DJ Delorie wrote:

> > > Index: configure.ac
> > > +# Dereferencing -1 is a compile-time error
> > 
> > This (those lines) look a little cryptic (and lack punctuation ;)
> > Wild improvement guess: "Too small 'int'?".
> 
> No, the compiler specifically tests for unaligned accesses and gives a
> compile-time error, at Renesas's requrest.  The chip can't do
> unaligned accesses correctly, but it doesn't fault - it just silently
> accesses the wrong memory.  So the compiler errors instead.

That's not permitted by standard C for code that might not be executed.  
The standard GCC practice if you can determine at compile time that a 
given path will result in undefined behavior at runtime but don't want to 
deduce that this path is unreachable is to generate an abort and output an 
informative notice with inform ().  This is done when float is passed to 
va_arg, for example, or for calling a function through a function pointer 
cast to an incompatible type.

(I don't see what actually generates the error, since there are only two 
calls to error () in the port, both of which I've commented on 
separately.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: New port: Renesas RL78

[Joseph S. Myers]

On Wed, 2 Nov 2011, DJ Delorie wrote:

> -  unsigned int __max_iter = 100;
> +  unsigned int __max_iter = 65536U;

Doesn't that need to be 65535U for your purpose?

> Index: libgcc/config/rl78/lib2shift.c
> ===
> --- libgcc/config/rl78/lib2shift.c(revision 0)
> +++ libgcc/config/rl78/lib2shift.c(revision 0)
> @@ -0,0 +1,103 @@
> +/* Shift functions for the GCC support library for the Renesas RL78 
> processors.
> +   Copyright (C) 2011 Free Software Foundation, Inc.
> +   Contributed by Red Hat.
> +
> +   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
> +   .  */

Should have the runtime license exception.

> +FPBIT = fp-bit.c
> +$(gcc_objdir)/fp-bit.c: $(gcc_srcdir)/config/fp-bit.c
> + echo '#define FLOAT' > $@
> + cat $(gcc_srcdir)/config/fp-bit.c   >> $@
> +
> +DPBIT = dp-bit.c
> +$(gcc_objdir)/dp-bit.c: $(gcc_srcdir)/config/fp-bit.c
> + cat $(gcc_srcdir)/config/fp-bit.c   >> $@

This should not be needed; just using t-fdpbit should suffice for almost 
all targets.

> \ No newline at end of file

As already noted, avoid this.

> +static int register_sizes[] =

const?

There are a lot of functions and variables without comments explaining 
them and their parameters.  (If a function just implements a standard 
hook, a comment identifying which hook suffices without needing to explain 
the parameters of that hook again.)

> +  if (letter == 'q' || letter == 'Q')
> + error ("q/Q modifiers invalid for symbol referencs");

"referencs" should be "reference" or "references" or similar.  I think 
this should call output_operand_lossage not error, so that it becomes an 
error or an ICE as appropriate depending on whether the source of the 
error is the machine description or an asm.

> +  if (letter == 'q' || letter == 'Q')
> + error ("q/Q modifiers invalid for symbol referencs");

Likewise.

I don't see updates to md.texi, contrib.texi or contrib/config-list.mk.  
(As regards the last, you should also check that the back end does build 
cleanly starting from a current native trunk compiler used to build a 
cross compiler configured with --enable-werror-always, on both 32-bit and 
64-bit hosts if possible.  contrib/config-list.mk is used for automatic 
checks of that sort of thing across all targets.)

I think the target-independent changes outside the back end (the ones that 
are meant as separate bug fixes required by the port) should be posted in 
separate messages, each with its own rationale.

The web site changes listed in sourcebuild.texi, "Back End", will also be 
needed.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: New port: Renesas RL78

[DJ Delorie]

> > Index: configure.ac
> > +# Dereferencing -1 is a compile-time error
> 
> This (those lines) look a little cryptic (and lack punctuation ;)
> Wild improvement guess: "Too small 'int'?".

No, the compiler specifically tests for unaligned accesses and gives a
compile-time error, at Renesas's requrest.  The chip can't do
unaligned accesses correctly, but it doesn't fault - it just silently
accesses the wrong memory.  So the compiler errors instead.

> Also spotted a:
> > Index: libgcc/config/rl78/trampoline.S
> > \ No newline at end of file
> 
> Wouldn't some tools behave better if the asm files had ELF
> decorations on the functions?

Hmmm... perhaps, but this code is from an internal port that already
has gdb, and the gdb engineer hasn't complained yet, and he's usually
the one that points those out to me ;-)

> Also spotted some
> > +#if 1
> and
> > +  /*print_simple_rtl (file, op);*/
> and

I'll fix those.

> > +#if DEBUG0
> and
> > +#if DEBUG_PEEP
> and
> > +#define DEBUG_ALLOC 0

These are intentional, and left in for future debugging.  There are
other ports like that.

Re: New port: Renesas RL78

[Hans-Peter Nilsson]

On Wed, 2 Nov 2011, DJ Delorie wrote:
> The Renesas RL78 is a new low-power 8/16 bit microcontroller, with an
> architecture much like the original Z80.

Just some random spottings.

> Index: configure.ac
> +# Dereferencing -1 is a compile-time error

This (those lines) look a little cryptic (and lack punctuation ;)
Wild improvement guess: "Too small 'int'?".

Also spotted a:
> Index: libgcc/config/rl78/trampoline.S
> \ No newline at end of file

Wouldn't some tools behave better if the asm files had ELF
decorations on the functions?

Also spotted some
> +#if 1
and
> +  /*print_simple_rtl (file, op);*/
and
> +#if DEBUG0
and
> +#if DEBUG_PEEP
and
> +#define DEBUG_ALLOC 0
and I'll stop right there.

brgds, H-P