Re: h8300-elf port: gas patches

[law at redhat dot com]

  In message <200108282230.f7SMUYB04610@phal.cambridge.redhat.com>you write:
  > gas toplevel:
  > 
  > Tue Aug 28 22:34:44 2001  J"orn Rennecke <amylaar@redhat.com>
  > 
  > 	* configure.in: Add case for h8300-*-elf.
  > 	* configure: Regenerate.
OK.

  > 	* tc-h8300.c: If OBJ_ELF, include elf/h8.h, and define
  > 	assorted coff relocations to the corresponding elf relocations.
OK.

  > 	(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?

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

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.

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


Remember, unless you have a good reason, the code for the ELF and the
COFF port should be the same.  Thus any difference is going to be looked
at very very closely.



  > 	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.


  > 	(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.


  > 	(md_convert_frag): First argument has type bfd* for OBJ_ELF.
Seems reasonable.

  > 	(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.




  > 	(md_apply_fix): Returns int, secod parameter is valueT * for
  > 	BFD_ASSEMBLER.
OK.

  > 	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.

  > 	(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.

  > 	(tc_gen_reloc): New function for OBJ_ELF.
Probably OK.  Though I would ask why it is only needed for COFF.

  > 	* 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.

  > 	(TC_MD_PCREL_FROM_SECTION_FIXED): Likewise.
Not OK until you've explained the related changes to write.c btter.

  > 	(TARGET_FORMAT): DEfine if OBJ_ELF.
OK

  > 	(TC_CONS_RELOC): Provide alternate definition for OBJ_ELF.
OK.

  > 	(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.


  > 
  > gas/testsuite:
  > 
  > 	* gas/h8300/h8300.exp (do_h8300_cbranch): Remove invocation.
  > 	(do_h8300_branch, do_h8300h_cbranch, do_h8300h_branch): Likewise.
  > 	(do_h8300s_cbranch, do_h8300s_branch, ffxx1): Likwise.
OK.

  > 	* 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.

  > 	* lib/gas-defs.exp: (regexp_diff) At verbosity 3, also show
  > 	where mismatch occurs due to regexp mismatch.
OK.


jeff