Re: h8300-elf port: gas patches

[Joern Rennecke <amylaar at redhat dot com>]

[tc-h8300.c]
>   > 	(build_bytes): For OBJ_ELF, leave pcrel adjustment to
>   > 	MD_PCREL_FROM_SECTION; use BFD_RELOC_8 for memory-indirect
>   > 	addresses.
> What is the point behind these changes?   Why in the world would we do
> anything different for handling of PC-relative addresses between COFF
> and ELF?

AFAICS MD_PCREL_FROM_SECTION is made to account for differences between the
relocation's address and the actual base address used by pc-relative addresses.
The h8-coff code has some FIXME comments that suggest that it doesn't
use the bfd infrastructure properly.  As h8-elf is a new port, I'm trying
to do this right now.

> Similarly, using BFD_RELOC_8 seems really really really wrong.  There's
> no way that can be correct.

The idea here is that the compiler makes the memory indirect addressing
explicit, i.e. it says that the address of the function is to be placed
into the zero page, and there is a symbol in front of the zero page location
where the address is placed, and the value of that symbol is used then
for memory-indirect addressing.
I've looked into the h8300h programming manual, and AFAICT this is actually
the semantics they meant @@ to have.

> R_MEM_INDIRECT performs two distinct, but equally important actions.
> 
>   1. The address of the symbol referenced by R_MEM_INDIRECT is placed
>   into the next entry within the function vector.
> 
>   2. The address of that entry within the function vector is placed
>   into the location requested by the reloc.

Yes, I understand that.  I think of it as a kludge that was necessary
beause coff doesn't allow you to use arbitrary sections to place data
in.
Elf doesn't have that limitation; and in the interest of interoperability
of tools, we should try to limit the number of special-case relocations
we use.

> I don't see how BFD_RELOC_8 has any chance of performing both actions.

1. Is mostly done by the compiler, with a little help from the linker to
concatenate all the section contents that go into the zero page.

>   > 	Make prel relocation signed.
> What was the point behind this change?  It may be right, it may not.  Either
> way I'd like some explanation.

The overflow check complains if it sees a negative offset for an unsigned
field.  AFAICR that check was missing for h8300-coff.  Or at least it
couldn't use the generic overflow check code.

>   > 	(tc_crawl_symbol_chain, tc_headers_hook): Don't define for OBJ_ELF.
>   > 	(tc_reloc_mangle): Likewise.
> And why would we define these to do anything different for ELF?  These seem
> like gratutious changes which serve no meaningful purpose.  Please explain.

The type object_headers is not available in th elf version.

>   > 	(md_section_align): For OBJ_ELF, just return size.
> Why do something different here?   Worse yet, why do something different
> than the vast majority of ports here?  Seems like another gratutious change.

I couldn't use the coff version because the symbols were not defined.
I am open to suggestions what should go there.

>   > 	Don't clobber most significant byte for 24 bit relocations.
> Seems to me that if we don't want to do this for ELF, then we don't
> want to do it for COFF either.

Well, AFAICT there were no 24 bit relocations that would be resolved at
assembly time; the coff port just defers most relocations, no matter if
you request relaxation or not.
The code as it is wouldn't complile for h8300-coff.

>   > 	(md_pcrel_from): If OBJ_ELF, handle one and two byte cases.
> I believe this goes away if you remove some of the gratutious changes
> to build_bytes.

No, this abort-ing implementation for h8300-coff only 'works' because no
pc-relative relocations are resolved at assembly time for h8300-coff.
> 
>   > 	(tc_gen_reloc): New function for OBJ_ELF.
> Probably OK.  Though I would ask why it is only needed for COFF.
For elf.  This is because the elf gas is a BFD_ASSEMBLER one, while the
coff gas isn't.

>   > 	* tc-h8300.h (TARGET_ARCH, TC_LINKRELAX_FIXUP): Define.
> The change for TC_LINKRELAX_FIXUP seems clearly wrong since we're going
> to want the linker to relax things.

The plan was to start out with never-relaxing, and get that to work;
then progress to relaxing if that was requested.  I'm not quite done with
debugging yet.

>   > 	(RELOC_32): Don't define if OBJ_ELF.
> Why bother with this change?  Does having RELOC_32 defined cause problems
> with OBJ_ELF?  It would seem to me it's not used at all and could just
> be deleted regardless of what object file is in use.

obj-elf.c includes aout64.h, where RELOC_32 is an enumeration value.
The definition in tc-h8300.h is numeric, and hence not suitable as
an enumberation tag.

>   > 	* gas/h8300/h8300-coff.exp, gas/h8300/h8300-elf.exp: New files.
> Seems like major overkill/duplication.  If the only tests that are
> different are the branch tests, then break just those out into separate
> .exp files and keep all the others common.  It just seems silly to me
> to have so much code duplicated.

But that's what I did.