diff mbox series

[v5,07/76] elf: Remove duplicate preprocessor constant definition

Message ID 1532967169-22265-8-git-send-email-aleksandar.markovic@rt-rk.com
State New
Headers show
Series Add nanoMIPS support to QEMU | expand

Commit Message

Aleksandar Markovic July 30, 2018, 4:11 p.m. UTC 
From: Aleksandar Markovic <amarkovic@wavecomp.com>

Remove duplicate preprocessor constant definition for EF_MIPS_ARCH.

The duplicate was introduced in commit 45506bdd.

Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/elf.h | 1 -
 1 file changed, 1 deletion(-)

Comments

Laurent Vivier July 30, 2018, 4:49 p.m. UTC | #1 
Le 30/07/2018 à 18:11, Aleksandar Markovic a écrit :
> From: Aleksandar Markovic <amarkovic@wavecomp.com>
> 
> Remove duplicate preprocessor constant definition for EF_MIPS_ARCH.
> 
> The duplicate was introduced in commit 45506bdd.
> 
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/elf.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/elf.h b/include/elf.h
> index 934dbbd..c8aaa2a 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -33,7 +33,6 @@ typedef int64_t  Elf64_Sxword;
>  
>  /* Flags in the e_flags field of the header */
>  /* MIPS architecture level. */
> -#define EF_MIPS_ARCH            0xf0000000
>  
>  /* Legal values for MIPS architecture level.  */
>  #define EF_MIPS_ARCH_1		0x00000000	/* -mips1 code.  */
> 

You should move the comment "MIPS architecture level" where the other
EF_MIPS_ARCH is defined (see glibc or system elf.h).

Thanks,
Laurent
Aleksandar Markovic Aug. 1, 2018, 6:51 p.m. UTC | #2 
Hi, Laurent.

> From: Laurent Vivier <laurent@vivier.eu>
> Sent: Monday, July 30, 2018 6:49 PM
> 
> Le 30/07/2018 à 18:11, Aleksandar Markovic a écrit :
> > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> >
> > Remove duplicate preprocessor constant definition for EF_MIPS_ARCH.
> >
> > The duplicate was introduced in commit 45506bdd.
> >
> > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  include/elf.h | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/include/elf.h b/include/elf.h
> > index 934dbbd..c8aaa2a 100644
> > --- a/include/elf.h
> > +++ b/include/elf.h
> > @@ -33,7 +33,6 @@ typedef int64_t  Elf64_Sxword;
> >
> >  /* Flags in the e_flags field of the header */
> >  /* MIPS architecture level. */
> > -#define EF_MIPS_ARCH            0xf0000000
> >
> >  /* Legal values for MIPS architecture level.  */
> >  #define EF_MIPS_ARCH_1               0x00000000      /* -mips1 code.  */
> >
> 
> You should move the comment "MIPS architecture level" where the other
> EF_MIPS_ARCH is defined (see glibc or system elf.h).
> 
> Thanks,
> Laurent

The comment "/* MIPS architecture level */" was present both before and after the commit 45506bdd. The scope of this patch is to rollback a duplicate preprocessor definition originating from the commit 45506bdd. This means that moving/modifying comments should not be in this patch. Can you please clarify for me what additional changes you suggest (but these should definitely be in a separate patch)? Do you want to sync this segment of QEMU's elf.h (copied below) with glibc corresponding section from its elf.h (also below)?

QEMU's elf.h:

/* MIPS architecture level. */
#define EF_MIPS_ARCH            0xf0000000 <----------------- duplicate

/* Legal values for MIPS architecture level.  */
#define EF_MIPS_ARCH_1		0x00000000	/* -mips1 code.  */
#define EF_MIPS_ARCH_2		0x10000000	/* -mips2 code.  */
#define EF_MIPS_ARCH_3		0x20000000	/* -mips3 code.  */
#define EF_MIPS_ARCH_4		0x30000000	/* -mips4 code.  */
#define EF_MIPS_ARCH_5		0x40000000	/* -mips5 code.  */
#define EF_MIPS_ARCH_32		0x50000000	/* MIPS32 code.  */
#define EF_MIPS_ARCH_64		0x60000000	/* MIPS64 code.  */
#define EF_MIPS_ARCH_32R2       0x70000000      /* MIPS32r2 code.  */
#define EF_MIPS_ARCH_64R2       0x80000000      /* MIPS64r2 code.  */
#define EF_MIPS_ARCH_32R6       0x90000000      /* MIPS32r6 code.  */
#define EF_MIPS_ARCH_64R6       0xa0000000      /* MIPS64r6 code.  */

/* The ABI of a file. */
#define EF_MIPS_ABI_O32		0x00001000	/* O32 ABI.  */
#define EF_MIPS_ABI_O64		0x00002000	/* O32 extended for 64 bit.  */

#define EF_MIPS_NOREORDER 0x00000001
#define EF_MIPS_PIC       0x00000002
#define EF_MIPS_CPIC      0x00000004
#define EF_MIPS_ABI2		0x00000020
#define EF_MIPS_OPTIONS_FIRST	0x00000080
#define EF_MIPS_32BITMODE	0x00000100
#define EF_MIPS_ABI		0x0000f000
#define EF_MIPS_FP64      0x00000200
#define EF_MIPS_NAN2008   0x00000400
#define EF_MIPS_ARCH      0xf0000000


glibc's elf.h:

/* Legal values for e_flags field of Elf32_Ehdr.  */

#define EF_MIPS_NOREORDER	1     /* A .noreorder directive was used.  */
#define EF_MIPS_PIC		2     /* Contains PIC code.  */
#define EF_MIPS_CPIC		4     /* Uses PIC calling sequence.  */
#define EF_MIPS_XGOT		8
#define EF_MIPS_64BIT_WHIRL	16
#define EF_MIPS_ABI2		32
#define EF_MIPS_ABI_ON32	64
#define EF_MIPS_FP64		512  /* Uses FP64 (12 callee-saved).  */
#define EF_MIPS_NAN2008	1024  /* Uses IEEE 754-2008 NaN encoding.  */
#define EF_MIPS_ARCH		0xf0000000 /* MIPS architecture level.  */

/* Legal values for MIPS architecture level.  */

#define EF_MIPS_ARCH_1		0x00000000 /* -mips1 code.  */
#define EF_MIPS_ARCH_2		0x10000000 /* -mips2 code.  */
#define EF_MIPS_ARCH_3		0x20000000 /* -mips3 code.  */
#define EF_MIPS_ARCH_4		0x30000000 /* -mips4 code.  */
#define EF_MIPS_ARCH_5		0x40000000 /* -mips5 code.  */
#define EF_MIPS_ARCH_32		0x50000000 /* MIPS32 code.  */
#define EF_MIPS_ARCH_64		0x60000000 /* MIPS64 code.  */
#define EF_MIPS_ARCH_32R2	0x70000000 /* MIPS32r2 code.  */
#define EF_MIPS_ARCH_64R2	0x80000000 /* MIPS64r2 code.  */


Regards,
Aleksandar
Aleksandar Markovic Aug. 1, 2018, 7:09 p.m. UTC | #3 
Hi again, Laurent.

Did you mean to move the comment from THE CURRENT to THE RIGHT position, as displayed here:

QEMU's elf.h:

/* MIPS architecture level. */  <--- THE CURRENT POSITION
#define EF_MIPS_ARCH            0xf0000000 <----------------- duplicate

/* Legal values for MIPS architecture level.  */
#define EF_MIPS_ARCH_1          0x00000000      /* -mips1 code.  */
#define EF_MIPS_ARCH_2          0x10000000      /* -mips2 code.  */
#define EF_MIPS_ARCH_3          0x20000000      /* -mips3 code.  */
#define EF_MIPS_ARCH_4          0x30000000      /* -mips4 code.  */
#define EF_MIPS_ARCH_5          0x40000000      /* -mips5 code.  */
#define EF_MIPS_ARCH_32         0x50000000      /* MIPS32 code.  */
#define EF_MIPS_ARCH_64         0x60000000      /* MIPS64 code.  */
#define EF_MIPS_ARCH_32R2       0x70000000      /* MIPS32r2 code.  */
#define EF_MIPS_ARCH_64R2       0x80000000      /* MIPS64r2 code.  */
#define EF_MIPS_ARCH_32R6       0x90000000      /* MIPS32r6 code.  */
#define EF_MIPS_ARCH_64R6       0xa0000000      /* MIPS64r6 code.  */

/* The ABI of a file. */
#define EF_MIPS_ABI_O32         0x00001000      /* O32 ABI.  */
#define EF_MIPS_ABI_O64         0x00002000      /* O32 extended for 64 bit.  */

#define EF_MIPS_NOREORDER 0x00000001
#define EF_MIPS_PIC       0x00000002
#define EF_MIPS_CPIC      0x00000004
#define EF_MIPS_ABI2            0x00000020
#define EF_MIPS_OPTIONS_FIRST   0x00000080
#define EF_MIPS_32BITMODE       0x00000100
#define EF_MIPS_ABI             0x0000f000
#define EF_MIPS_FP64      0x00000200
#define EF_MIPS_NAN2008   0x00000400
#define EF_MIPS_ARCH      0xf0000000 /* MIPS architecture level. */ <--- THE RIGHT POSITION



> From: Aleksandar Markovic
> Sent: Wednesday, August 1, 2018 8:51 PM
> 
> Hi, Laurent.
> 
> > From: Laurent Vivier <laurent@vivier.eu>
> > Sent: Monday, July 30, 2018 6:49 PM
> >
> > Le 30/07/2018 à 18:11, Aleksandar Markovic a écrit :
> > > From: Aleksandar Markovic <amarkovic@wavecomp.com>
> > >
> > > Remove duplicate preprocessor constant definition for EF_MIPS_ARCH.
> > >
> > > The duplicate was introduced in commit 45506bdd.
> > >
> > > Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > > ---
> > >  include/elf.h | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/include/elf.h b/include/elf.h
> > > index 934dbbd..c8aaa2a 100644
> > > --- a/include/elf.h
> > > +++ b/include/elf.h
> > > @@ -33,7 +33,6 @@ typedef int64_t  Elf64_Sxword;
> > >
> > >  /* Flags in the e_flags field of the header */
> > >  /* MIPS architecture level. */
> > > -#define EF_MIPS_ARCH            0xf0000000
> > >
> > >  /* Legal values for MIPS architecture level.  */
> > >  #define EF_MIPS_ARCH_1               0x00000000      /* -mips1 code.  */
> > >
> >
> > You should move the comment "MIPS architecture level" where the other
> > EF_MIPS_ARCH is defined (see glibc or system elf.h).
> >
> > Thanks,
> > Laurent
> 
> The comment "/* MIPS architecture level */" was present both before and after the commit 45506bdd. The scope of this patch is to rollback a duplicate preprocessor definition originating from the commit 45506bdd. This means that moving/modifying comments should not be in this patch. Can you please clarify for me what additional changes you suggest (but these should definitely be in a separate patch)? Do you want to sync this segment of QEMU's elf.h (copied below) with glibc corresponding section from its elf.h (also below)?
> 
> QEMU's elf.h:
> 
> /* MIPS architecture level. */
> #define EF_MIPS_ARCH            0xf0000000 <----------------- duplicate
> 
> /* Legal values for MIPS architecture level.  */
> #define EF_MIPS_ARCH_1          0x00000000      /* -mips1 code.  */
> #define EF_MIPS_ARCH_2          0x10000000      /* -mips2 code.  */
> #define EF_MIPS_ARCH_3          0x20000000      /* -mips3 code.  */
> #define EF_MIPS_ARCH_4          0x30000000      /* -mips4 code.  */
> #define EF_MIPS_ARCH_5          0x40000000      /* -mips5 code.  */
> #define EF_MIPS_ARCH_32         0x50000000      /* MIPS32 code.  */
> #define EF_MIPS_ARCH_64         0x60000000      /* MIPS64 code.  */
> #define EF_MIPS_ARCH_32R2       0x70000000      /* MIPS32r2 code.  */
> #define EF_MIPS_ARCH_64R2       0x80000000      /* MIPS64r2 code.  */
> #define EF_MIPS_ARCH_32R6       0x90000000      /* MIPS32r6 code.  */
> #define EF_MIPS_ARCH_64R6       0xa0000000      /* MIPS64r6 code.  */
> 
> /* The ABI of a file. */
> #define EF_MIPS_ABI_O32         0x00001000      /* O32 ABI.  */
> #define EF_MIPS_ABI_O64         0x00002000      /* O32 extended for 64 bit.  */
> 
> #define EF_MIPS_NOREORDER 0x00000001
> #define EF_MIPS_PIC       0x00000002
> #define EF_MIPS_CPIC      0x00000004
> #define EF_MIPS_ABI2            0x00000020
> #define EF_MIPS_OPTIONS_FIRST   0x00000080
> #define EF_MIPS_32BITMODE       0x00000100
> #define EF_MIPS_ABI             0x0000f000
> #define EF_MIPS_FP64      0x00000200
> #define EF_MIPS_NAN2008   0x00000400
> #define EF_MIPS_ARCH      0xf0000000
> 
> 
> glibc's elf.h:
> 
> /* Legal values for e_flags field of Elf32_Ehdr.  */
> 
> #define EF_MIPS_NOREORDER       1     /* A .noreorder directive was used.  */
> #define EF_MIPS_PIC             2     /* Contains PIC code.  */
> #define EF_MIPS_CPIC            4     /* Uses PIC calling sequence.  */
> #define EF_MIPS_XGOT            8
> #define EF_MIPS_64BIT_WHIRL     16
> #define EF_MIPS_ABI2            32
> #define EF_MIPS_ABI_ON32        64
> #define EF_MIPS_FP64            512  /* Uses FP64 (12 callee-saved).  */
> #define EF_MIPS_NAN2008 1024  /* Uses IEEE 754-2008 NaN encoding.  */
> #define EF_MIPS_ARCH            0xf0000000 /* MIPS architecture level.  */
> 
> /* Legal values for MIPS architecture level.  */
> 
> #define EF_MIPS_ARCH_1          0x00000000 /* -mips1 code.  */
> #define EF_MIPS_ARCH_2          0x10000000 /* -mips2 code.  */
> #define EF_MIPS_ARCH_3          0x20000000 /* -mips3 code.  */
> #define EF_MIPS_ARCH_4          0x30000000 /* -mips4 code.  */
> #define EF_MIPS_ARCH_5          0x40000000 /* -mips5 code.  */
> #define EF_MIPS_ARCH_32         0x50000000 /* MIPS32 code.  */
> #define EF_MIPS_ARCH_64         0x60000000 /* MIPS64 code.  */
> #define EF_MIPS_ARCH_32R2       0x70000000 /* MIPS32r2 code.  */
> #define EF_MIPS_ARCH_64R2       0x80000000 /* MIPS64r2 code.  */
> 
> 
> Regards,
> Aleksandar
Peter Maydell Aug. 1, 2018, 7:37 p.m. UTC | #4 
On 1 August 2018 at 19:51, Aleksandar Markovic <amarkovic@wavecomp.com> wrote:
> Hi, Laurent.
>
>> From: Laurent Vivier <laurent@vivier.eu>
>> Sent: Monday, July 30, 2018 6:49 PM
>>
>> Le 30/07/2018 à 18:11, Aleksandar Markovic a écrit :
>> > diff --git a/include/elf.h b/include/elf.h
>> > index 934dbbd..c8aaa2a 100644
>> > --- a/include/elf.h
>> > +++ b/include/elf.h
>> > @@ -33,7 +33,6 @@ typedef int64_t  Elf64_Sxword;
>> >
>> >  /* Flags in the e_flags field of the header */
>> >  /* MIPS architecture level. */
>> > -#define EF_MIPS_ARCH            0xf0000000
>> >
>> >  /* Legal values for MIPS architecture level.  */
>> >  #define EF_MIPS_ARCH_1               0x00000000      /* -mips1 code.  */
>> >
>>
>> You should move the comment "MIPS architecture level" where the other
>> EF_MIPS_ARCH is defined (see glibc or system elf.h).
>>
>> Thanks,
>> Laurent
>
> The comment "/* MIPS architecture level */" was present both
> before and after the commit 45506bdd. The scope of this patch
> is to rollback a duplicate preprocessor definition originating
> from the commit 45506bdd. This means that moving/modifying
> comments should not be in this patch. Can you please clarify
> for me what additional changes you suggest (but these should
> definitely be in a separate patch)? Do you want to sync this
> segment of QEMU's elf.h (copied below) with glibc corresponding
> section from its elf.h (also below)?

It seems to me that the logical place for defining EF_MIPS_ARCH
is next to the definitions EF_MIPS_ARCH_1 &c which give the
valid values for that field. It's likely because the
pre-45506bdd QEMU header didn't put the two together as
you'd expect that the duplicate got added.

We should fix the duplication not by just rolling back to
pre-45506bdd, but in a way that gets us to having the
EF_MIPS_ARCH and EF_MIPS_ARCH_1 &c in the same place in
the file. The simplest way to do that is just to delete
the second (original) EF_MIPS_ARCH #define, the one just
below the EF_MIPS_NAN2008 define.

We could line up with the order the glibc headers use, but
that seems like overkill compared to just deleting a line.

thanks
-- PMM
Aleksandar Markovic Aug. 1, 2018, 8:01 p.m. UTC | #5 
> 
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Wednesday, August 1, 2018 9:37 PM
> 
> > The simplest way to do that is just to delete
> > the second (original) EF_MIPS_ARCH #define, the one just
> > below the EF_MIPS_NAN2008 define.
> >
> > We could line up with the order the glibc headers use, but
> > that seems like overkill compared to just deleting a line.
> 

This sounds reasonable to me, and, if nobody disputes it, I will implement it this way in the next version.

Aleksandar
Laurent Vivier Aug. 1, 2018, 8:31 p.m. UTC | #6 
Le 01/08/2018 à 22:01, Aleksandar Markovic a écrit :
>>
>> From: Peter Maydell <peter.maydell@linaro.org>
>> Sent: Wednesday, August 1, 2018 9:37 PM
>>
>>> The simplest way to do that is just to delete
>>> the second (original) EF_MIPS_ARCH #define, the one just
>>> below the EF_MIPS_NAN2008 define.
>>>
>>> We could line up with the order the glibc headers use, but
>>> that seems like overkill compared to just deleting a line.
>>
> 
> This sounds reasonable to me, and, if nobody disputes it, I will implement it this way in the next version.

I agree

Laurent
diff mbox series

Patch

diff --git a/include/elf.h b/include/elf.h
index 934dbbd..c8aaa2a 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -33,7 +33,6 @@  typedef int64_t  Elf64_Sxword;
 
 /* Flags in the e_flags field of the header */
 /* MIPS architecture level. */
-#define EF_MIPS_ARCH            0xf0000000
 
 /* Legal values for MIPS architecture level.  */
 #define EF_MIPS_ARCH_1		0x00000000	/* -mips1 code.  */