RE: [PATCH 2/3 elf] Add MicroBlaze support to elf.h

[David Holsgrove <david.holsgrove@xilinx.com>]

Hi Andreas,

> -----Original Message-----
> From: Andreas Jaeger [mailto:aj@suse.com]
> Sent: Thursday, 29 November 2012 5:16 pm
> To: David Holsgrove
> Cc: libc-alpha@sourceware.org; libc-ports@sourceware.org; John Williams; Edgar
> E. Iglesias; Vinod Kathail; Tom Shui; Vidhumouli Hunsigida; Nagaraju Mekala
> Subject: Re: [PATCH 2/3 elf] Add MicroBlaze support to elf.h
> 
> On 11/29/2012 06:28 AM, David Holsgrove wrote:
> > d MicroBlaze relocations to elf/elf.h
> >
> > 2012-11-29  David Holsgrove<david.holsgrove@xilinx.com>
> >
> >          * elf/elf.h: Add support for MicroBlaze arch
> >
> >
> > 0002-Adding-MicroBlaze-support-to-elf-elf.h.patch
> >
> 
> David, thanks for your patch. Could you please review the Contribution
> checklist at http://sourceware.org/glibc/wiki/Contribution%20checklist?
> 
> Your patch misses a valid ChangeLog entry and that's needed for such
> changes.

Thanks, will update in next version.

> 
> >  From 02a0759b648b6aed4538cd0cde88babf5468585a Mon Sep 17 00:00:00 2001
> > From: David Holsgrove<david.holsgrove@petalogix.com>
> > Date: Wed, 4 Jan 2012 13:56:48 +1000
> > Subject: [PATCH] Adding MicroBlaze support to elf/elf.h
> >
> > Signed-off-by: David Holsgrove<david.holsgrove@petalogix.com>
> > ---
> >   elf/elf.h |   33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> >
> > diff --git a/elf/elf.h b/elf/elf.h
> > index b07e6ad..6a4a4cc 100644
> > --- a/elf/elf.h
> > +++ b/elf/elf.h
> > @@ -259,6 +259,8 @@ typedef struct
> >      chances of collision with official or non-GNU unofficial values.  */
> >
> >   #define EM_ALPHA	0x9026
> > +#define EM_NEW_MICROBLAZE   0xbd /* Xilinx MicroBlaze */
> > +#define EM_MICROBLAZE 0xbaab
> 
> Are any of these official values? The comment above asks for hig numbers
> and 0xbd is not height.
> 

Yes they are, but could be renamed to better match upstream binutils. I'll revise
in this patch for elf.h and also my patch to libc-ports thanks.

> >
> >   /* Legal values for e_version (version).  */
> >
> > @@ -2847,6 +2849,37 @@ typedef Elf32_Addr Elf32_Conflict;
> >   #define R_M32R_GOTOFF_LO	64	/* Low 16 bit offset to GOT */
> >   #define R_M32R_NUM		256	/* Keep this the last entry. */
> >
> > +/* microblaze relocations */
> > +#define R_MICROBLAZE_NONE	0
> 
> I suggest to be consistent with the writing of the name - so is it
> microblaze or MicroBlaze?

MicroBlaze :-) Comment updated.

> 
> > +#define R_MICROBLAZE_32		1
> > +#define R_MICROBLAZE_32_PCREL	2
> > +#define R_MICROBLAZE_64_PCREL	3
> > +#define R_MICROBLAZE_32_PCREL_LO	4
> > +#define R_MICROBLAZE_64		5
> > +#define R_MICROBLAZE_32_LO	6
> > +#define R_MICROBLAZE_SRO32	7
> > +#define R_MICROBLAZE_SRW32	8
>  > [...]
> 
> Please add comments for all of them,
> 

Thanks for the review, I'll send v2 of my patch shortly.

regards,
David

> Thanks,
> Andreas
> --
>   Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
>    SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>     GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
>      GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126