This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: h8300-elf port: gas patches


  In message <200108290030.f7T0UZb05356@phal.cambridge.redhat.com>you write:
  > [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 addres
  > ses.
  > 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.
OK.  I understand your motivation now.  However, if I was going to try and
clean up the H8 port, I would clean up both the coff and elf ports together.

Regardless, I believe this issue has been addressed by fixing the documentation
for MD_PCREL_FROM_SECTION to match reality, so this specific issue has been
addressed.


  > > 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.
That's not the semantics as I have always understood them.  Plus if you go
this direction, then you have to add more code to the compiler, assembler,
etc which is specific to the ELF port.  I think that's a bad idea.
 
  > > 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.
Actually, it does.  It just has limitations on the total number of
sections you can use.

  > 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 think this is necessary or even wise.  R_MEM_INDIRECT is basically
the same as most PIC related relocations -- are you arguing that all the
GOT/PLT relocs that most ports use should be special cased in the compiler?
I didn't think so.

And again, I don't see how making the ELF and COFF ports handle this stuff
in a different way buys us anything in terms of long term maintenance.

  > >   > 	Make prel relocation signed.
  > > What was the point behind this change?  It may be right, it may not.  Eit
  > her
  > > 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.
OK.  Consider this change approved.

  > 
  > >   > 	(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 se
  > em
  > > like gratutious changes which serve no meaningful purpose.  Please explai
  > n.
  > 
  > The type object_headers is not available in th elf version.
OK.  So is object_headers a property of ELF/COFF or a property of 
BFD_ASSEMBLER or !BFD_ASSEMBLER?

  > 
  > >   > 	(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 chan
  > ge.
  > 
  > I couldn't use the coff version because the symbols were not defined.
  > I am open to suggestions what should go there.
OK.  I see the problem here.

What I think would be better:

#ifdef BFD_ASSEMBLER
valueT
md_section_align (segment, size)
     segT segment;
     valueT size;
{
  int align = bfd_get_section_alignment (stdoutput, segment);
  return ((size + (1 << align) - 1) & (-1 << align));
}
#else
alueT
md_section_align (seg, size)
     segT seg;
     valueT size;
{
  return ((size + (1 << section_alignment[(int) seg]) - 1)
          & (-1 << section_alignment[(int) seg]));
}
#endif



  > >   > 	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.
And the ELF port is going to defer most relocations just like the COFF
port.  Anything else is silly.

So if there aren't ever any 24bit relocs, then have both ports abort
or something like that if presented with a 24bit reloc that the 
assembler is expected to resolve.

  > >   > 	(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.
OK.  So treat this just like COFF.  THe set of relocs resolved by both
ports at assembly time should be the same.  Thus, if the abort is safe for
the h8300-coff, then it should be safe for h8300-elf.

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


  > >   > 	* 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.
It seems to me like doing things this way just made more work for yourself.
Please make the coff and elf ports work in the same manner.


  > 
  > >   > 	(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.
Ugh.  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.
  > 
  > But that's what I did.
Err, no.  Keep all the existing tests which can be shared in h8300.exp.  Then
create h8300-coff.exp and h8300-elf.exp for those which can not be shared.
If you look at h8300-coff.exp you clearly didn't do that.  You'll find stuff
like arithmetic, logical, bit, etc tests instead of just the branching
tests.

jeff




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]