This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/4] OpenRISC binutils updates and new relocs


Hi Stafford,

>> There are a few minor formatting glitches, but nothing serious.
> 
> Will you be able to point them out?  Even just some hints?

Sure...

So for example in patch 1/4 there is:

  +enum {
  +  RTYPE_LO = 0,

when really it should be:

  +enum 
  +{
  +  RTYPE_LO = 0,

(And similarly in other places.  Basically, try to avoid ending a line
with an opening curly brace, unless that brace is the only character on
the line).

Then there is:

  +static int
  +parse_reloc(const char **strp)

Which ought to be:

  +static int
  +parse_reloc (const char **strp)

Ie - a single space between the function name and its parameters.
(I did say that these were minor formatting nits...)  In a similar
vein there is:

  +  return parse_imm16(cd, strp, opindex, (long *) valuep, 0);
  +}
 
which also needs a space between the function name and its arguments.

There are a few other cases of the above issues in the other patches,
but nothing else of note.


One other thing:  There are several places where you add calls to 
abort().  Now this is not wrong, and certainly not a reason to 
reject the patch, but I consider it to be unhelpful.  To my mind
a library, or tool, should generate an error message when something
goes wrong and not leave the user wondering why they have suddenly
got a segmentation fault.

Plus if you have a call to abort() in the code you can bet that some 
enterprising person with a binary fuzzer will find a way to trigger 
it, and then file a CVE about it.  (Fixing CVEs is the bane of my 
life as they involve lots of extra administrivia).



>> I do not see any need to add extra document for the new relocs, unless you
>> have created new assembler pseudo-ops to generate them.

> As Richard mentioned we have added a few, see PATCH 3/4 in cpu/or1k.opc the
> change:
> 
> 	(parse_reloc): Add new po(), gotpo() and gottppo() for LO13 relocations.
> 
> Is this what you mean?  I will look into adding the documentation for these.

Please do.  Most likely you will want to create a gas/doc/c-or1k.c file,
(copying the contents from another, similar file and modifying as needed), and
then patch the gas/doc/as.texi file to include it and the gas/doc/all.texi file
to define a macro for it.


>> I do have one question though.  Is there a need to be able to distinguish 
>> between binaries that use the new l.adrp instruction and those that don't.

> As Richard mentioned we don't handle this.
> 
> We have cases like this right now as well, i.e. binaries generated with `l.mul`
> or `l.div` instructions will link fine into an executable that assume those
> instrunctions should be emulated.  That doesn't throw an error and I don't think
> it has been a problem.

OK, well it is your target, so if you are OK with this then so be it.
I would recommend however thinking about a solution for the future, should the
openRISC architecture gain more variants.  My suggestion would be to make use
of ELF notes, as has been done with other ports.

Cheers
  Nick



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