Re: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

Re: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

(Add John F. Reiser in this loop)

On 10/23/10, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 2010-10-22 at 14:10 -0700, David Daney wrote:
>> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
>> > From: Wu Zhangjin<wuzhangjin@gmail.com>
>> >
>> > In some situations(with related kernel config and gcc options), the
>> > modules may have the same address space as the core kernel space, so
>> > mcount_regex for modules should also match R_MIPS_26.
>> >
>>
>> I think Steve is rewriting this bit to be a pure C program.  Is this
>> file even used anymore?  If so for how long?
>
> It's already in mainline, but is only supported for x86 for now. Until
> we verify that it works for other archs (which is up to you guys to
> verify) the script will still be used.
>
> Also, I did not write it, John Reiser did. I just cleaned it up a bit
> and got it working with the build system.

Just applied this tmp patch to try the basic function.

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index ce8af84..3bf1c0c 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -9,6 +9,7 @@ config MIPS
        select HAVE_FUNCTION_TRACE_MCOUNT_TEST
        select HAVE_DYNAMIC_FTRACE
        select HAVE_FTRACE_MCOUNT_RECORD
+       select HAVE_C_RECORDMCOUNT
        select HAVE_FUNCTION_GRAPH_TRACER
        select HAVE_KPROBES
        select HAVE_KRETPROBES
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 26e1271..0fb200f 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -270,6 +270,7 @@ do_file(char const *const fname)
        case EM_IA_64:   reltype = R_IA64_IMM64;   gpfx = '_'; break;
        case EM_PPC:     reltype = R_PPC_ADDR32;   gpfx = '_'; break;
        case EM_PPC64:   reltype = R_PPC64_ADDR64; gpfx = '_'; break;
+       case EM_MIPS:    reltype = R_MIPS_26;      gpfx = '_'; break;
        case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
        case EM_SH:      reltype = R_SH_DIR32;                 break;
        case EM_SPARCV9: reltype = R_SPARC_64;     gpfx = '_'; break;

(Note: The above patch is not enough, for the modules with
-mlong-calls, the reltype should be R_MIPS_HI16, and we may also need
to add our specific code for sift_rel_mcount() to get the right
location of the _mcount calling site)

but failed with the following error:

$ make ARCH=mips CROSS_COMPILE=mips64el-unknown-linux-gnu-
  CHK     include/linux/version.h
  CHK     include/generated/utsrelease.h
  HOSTCC  scripts/basic/fixdep
  HOSTCC  scripts/basic/docproc
scripts/basic/docproc.c: In function ‘docsect’:
scripts/basic/docproc.c:336: warning: ignoring return value of
‘asprintf’, declared with attribute warn_unused_result
  Checking missing-syscalls for N32
  CALL    scripts/checksyscalls.sh
  Checking missing-syscalls for O32
  CALL    scripts/checksyscalls.sh
  CC      kernel/bounds.s
  GEN     include/generated/bounds.h
  CC      arch/mips/kernel/asm-offsets.s
  GEN     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CC      scripts/mod/empty.o
  HOSTCC  scripts/mod/mk_elfconfig
  MKELF   scripts/mod/elfconfig.h
  HOSTCC  scripts/mod/file2alias.o
  HOSTCC  scripts/mod/modpost.o
  HOSTCC  scripts/mod/sumversion.o
  HOSTLD  scripts/mod/modpost
  HOSTCC  scripts/kallsyms
  HOSTCC  scripts/conmakehash
  HOSTCC  scripts/bin2c
  HOSTCC  scripts/recordmcount
  CC      init/main.o
/bin/sh: line 1: 21835 Segmentation fault     scripts/recordmcount "init/main.o"
make[1]: *** [init/main.o] Error 139
make: *** [init] Error 2

I traced the problem and found it was triggered by the 201 line of
scripts/recordmcount.h:

198                 if (!mcountsym) {
199                         Elf_Sym const *const symp =
200                                 &sym0[ELF_R_SYM(_w(relp->r_info))];
*201                         char const *symname = &str0[w(symp->st_name)];*
202
203                         if ('.' == symname[0])
204                                 ++symname;  /* ppc64 hack */

Exactly, it was triggered by: symp->st_name, symp is normal address,
i.e. 0xa01831f0, but perhaps the content pointed by this address may
not exist or is not allocated before?

Did I miss something for MIPS specific support?

By the way, because MIPS need to cope with its modules
particularly(differ from the module):

for kernel:

    #     10:   03e0082d        move    at,ra
    #     14:   0c000000        jal     0
    #                    14: R_MIPS_26   _mcount           --> we
record this for MIPS kernel
    #                    14: R_MIPS_NONE *ABS*
    #                    14: R_MIPS_NONE *ABS*
    #    18:   00020021        nop


for module:

    #       c:  3c030000        lui     v1,0x0
    #                   c: R_MIPS_HI16  _mcount         --> we record
this for MIPS module
    #                   c: R_MIPS_NONE  *ABS*
    #                   c: R_MIPS_NONE  *ABS*
    #      10:  64630000        daddiu  v1,v1,0
    #                   10: R_MIPS_LO16 _mcount
    #                   10: R_MIPS_NONE *ABS*
    #                   10: R_MIPS_NONE *ABS*
    #      14:  03e0082d        move    at,ra
    #      18:  0060f809        jalr    v1
    #                     label:

But I found there was only one argument: /path/to/file.o passed to
scripts/recordmount:

scripts/Makefile.build

217 cmd_record_mcount = if [ $(@) != "scripts/mod/empty.o" ]; then
             \
218                         $(objtree)/scripts/recordmcount "$(@)";
             \
219                     fi;
220 else

So, should we pass "$(if $(part-of-module),1,0)" to the C version of
recordmcount?

Regards,
Wu Zhangjin

Re: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[Steven Rostedt]

On Sun, 2010-10-24 at 02:53 +0800, wu zhangjin wrote:
> (Add John F. Reiser in this loop)
> 
> On 10/23/10, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Fri, 2010-10-22 at 14:10 -0700, David Daney wrote:
> >> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
> >> > From: Wu Zhangjin<wuzhangjin@gmail.com>
> >> >
> >> > In some situations(with related kernel config and gcc options), the
> >> > modules may have the same address space as the core kernel space, so
> >> > mcount_regex for modules should also match R_MIPS_26.
> >> >
> >>
> >> I think Steve is rewriting this bit to be a pure C program.  Is this
> >> file even used anymore?  If so for how long?
> >
> > It's already in mainline, but is only supported for x86 for now. Until
> > we verify that it works for other archs (which is up to you guys to
> > verify) the script will still be used.
> >
> > Also, I did not write it, John Reiser did. I just cleaned it up a bit
> > and got it working with the build system.
> 
> Just applied this tmp patch to try the basic function.
> 
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index ce8af84..3bf1c0c 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -9,6 +9,7 @@ config MIPS
>         select HAVE_FUNCTION_TRACE_MCOUNT_TEST
>         select HAVE_DYNAMIC_FTRACE
>         select HAVE_FTRACE_MCOUNT_RECORD
> +       select HAVE_C_RECORDMCOUNT
>         select HAVE_FUNCTION_GRAPH_TRACER
>         select HAVE_KPROBES
>         select HAVE_KRETPROBES
> diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
> index 26e1271..0fb200f 100644
> --- a/scripts/recordmcount.c
> +++ b/scripts/recordmcount.c
> @@ -270,6 +270,7 @@ do_file(char const *const fname)
>         case EM_IA_64:   reltype = R_IA64_IMM64;   gpfx = '_'; break;
>         case EM_PPC:     reltype = R_PPC_ADDR32;   gpfx = '_'; break;
>         case EM_PPC64:   reltype = R_PPC64_ADDR64; gpfx = '_'; break;
> +       case EM_MIPS:    reltype = R_MIPS_26;      gpfx = '_'; break;
>         case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
>         case EM_SH:      reltype = R_SH_DIR32;                 break;
>         case EM_SPARCV9: reltype = R_SPARC_64;     gpfx = '_'; break;
> 
> (Note: The above patch is not enough, for the modules with
> -mlong-calls, the reltype should be R_MIPS_HI16, and we may also need
> to add our specific code for sift_rel_mcount() to get the right
> location of the _mcount calling site)
> 
> but failed with the following error:
> 
> $ make ARCH=mips CROSS_COMPILE=mips64el-unknown-linux-gnu-
>   CHK     include/linux/version.h
>   CHK     include/generated/utsrelease.h
>   HOSTCC  scripts/basic/fixdep
>   HOSTCC  scripts/basic/docproc
> scripts/basic/docproc.c: In function ‘docsect’:
> scripts/basic/docproc.c:336: warning: ignoring return value of
> ‘asprintf’, declared with attribute warn_unused_result
>   Checking missing-syscalls for N32
>   CALL    scripts/checksyscalls.sh
>   Checking missing-syscalls for O32
>   CALL    scripts/checksyscalls.sh
>   CC      kernel/bounds.s
>   GEN     include/generated/bounds.h
>   CC      arch/mips/kernel/asm-offsets.s
>   GEN     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CC      scripts/mod/empty.o
>   HOSTCC  scripts/mod/mk_elfconfig
>   MKELF   scripts/mod/elfconfig.h
>   HOSTCC  scripts/mod/file2alias.o
>   HOSTCC  scripts/mod/modpost.o
>   HOSTCC  scripts/mod/sumversion.o
>   HOSTLD  scripts/mod/modpost
>   HOSTCC  scripts/kallsyms
>   HOSTCC  scripts/conmakehash
>   HOSTCC  scripts/bin2c
>   HOSTCC  scripts/recordmcount
>   CC      init/main.o
> /bin/sh: line 1: 21835 Segmentation fault     scripts/recordmcount "init/main.o"
> make[1]: *** [init/main.o] Error 139
> make: *** [init] Error 2
> 
> I traced the problem and found it was triggered by the 201 line of
> scripts/recordmcount.h:
> 
> 198                 if (!mcountsym) {
> 199                         Elf_Sym const *const symp =
> 200                                 &sym0[ELF_R_SYM(_w(relp->r_info))];
> *201                         char const *symname = &str0[w(symp->st_name)];*

I merged the 32bit and 64bit here. It may be my fault on this one ;-)

I'll look into it on Monday. Thanks,

-- Steve

> 202
> 203                         if ('.' == symname[0])
> 204                                 ++symname;  /* ppc64 hack */
> 
> Exactly, it was triggered by: symp->st_name, symp is normal address,
> i.e. 0xa01831f0, but perhaps the content pointed by this address may
> not exist or is not allocated before?
> 
> Did I miss something for MIPS specific support?
> 
> By the way, because MIPS need to cope with its modules
> particularly(differ from the module):
> 
> for kernel:
> 
>     #     10:   03e0082d        move    at,ra
>     #     14:   0c000000        jal     0
>     #                    14: R_MIPS_26   _mcount           --> we
> record this for MIPS kernel
>     #                    14: R_MIPS_NONE *ABS*
>     #                    14: R_MIPS_NONE *ABS*
>     #    18:   00020021        nop
> 
> 
> for module:
> 
>     #       c:  3c030000        lui     v1,0x0
>     #                   c: R_MIPS_HI16  _mcount         --> we record
> this for MIPS module
>     #                   c: R_MIPS_NONE  *ABS*
>     #                   c: R_MIPS_NONE  *ABS*
>     #      10:  64630000        daddiu  v1,v1,0
>     #                   10: R_MIPS_LO16 _mcount
>     #                   10: R_MIPS_NONE *ABS*
>     #                   10: R_MIPS_NONE *ABS*
>     #      14:  03e0082d        move    at,ra
>     #      18:  0060f809        jalr    v1
>     #                     label:
> 
> But I found there was only one argument: /path/to/file.o passed to
> scripts/recordmount:
> 
> scripts/Makefile.build
> 
> 217 cmd_record_mcount = if [ $(@) != "scripts/mod/empty.o" ]; then
>              \
> 218                         $(objtree)/scripts/recordmcount "$(@)";
>              \
> 219                     fi;
> 220 else
> 
> So, should we pass "$(if $(part-of-module),1,0)" to the C version of
> recordmcount?
> 
> Regards,
> Wu Zhangjin

Re: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[John Reiser]

Hi, here is the diagnosis for SIGSEGV in recordmcount.c on MIPS64:

On 10/23/2010, wu zhangjin wrote:
>   CC      init/main.o
> /bin/sh: line 1: 21835 Segmentation fault     scripts/recordmcount "init/main.o"
> make[1]: *** [init/main.o] Error 139
> make: *** [init] Error 2
> 
> I traced the problem and found it was triggered by the 201 line of
> scripts/recordmcount.h:
> 
> 198                 if (!mcountsym) {
> 199                         Elf_Sym const *const symp =
> 200                                 &sym0[ELF_R_SYM(_w(relp->r_info))];
> *201                         char const *symname = &str0[w(symp->st_name)];*
> 202
> 203                         if ('.' == symname[0])
> 204                                 ++symname;  /* ppc64 hack */
> 
> Exactly, it was triggered by: symp->st_name, symp is normal address,
> i.e. 0xa01831f0, but perhaps the content pointed by this address may
> not exist or is not allocated before?
> 
> Did I miss something for MIPS specific support?

The layout of a MIPS structure Elf64_Rela is not described correctly
by the macros ELF64_R_SYM and ELF64_R_TYPE of <elf.h>:
-----
#define ELF64_R_SYM(i)                  ((i) >> 32)
#define ELF64_R_TYPE(i)                 ((i) & 0xffffffff)
-----

"readelf main.o" says:
-----
Relocation section '.rela.text' at offset 0x5b68 contains 59 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000000020  004200000004 R_MIPS_26         0000000000000000 _mcount + 0
                    Type2: R_MIPS_NONE
                    Type3: R_MIPS_NONE
-----

The actual bytes are [performed on a little-endian machine,
which is the same as main.o, namely ELFDATA2LSB]:
-----
$ od -Ax -tx8 -j0x5b68 main.o  |  sed 2q
005b68 0000000000000020 0400000000000042
005b78 0000000000000000
-----

So it looks like the data actually corresponds to:
-----
#define MIPS_ELF64_R_TYPE(i)  (0xff & ((i)>>56))
#define MIPS_ELF64_R_TYPE2(i) (0xff & ((i)>>48))
#define MIPS_ELF64_R_TYPE3(i) (0xff & ((i)>>40))
#define MIPS_ELF64_R_SYM(i)   (0xffffffff & (i))  /* perhaps 40 bits? */
-----

What this means for recordmcount.c is that ELF_R_SYM and ELF_R_TYPE
should become pointers to functions with default bodies given by
the macros in <elf.h>, and which EM_MIPS overrides.


> for kernel:
>     #     14:   0c000000        jal     0
>     #                    14: R_MIPS_26   _mcount

> for module:
>     #       c:  3c030000        lui     v1,0x0
>     #                   c: R_MIPS_HI16  _mcount

I suggest that the argv command line for recordcmount.c have an
optional flag -m or --module, such that the correct reltype can
be chosen when .e_machine is decoded.

> (Note: The above patch is not enough, for the modules with
> -mlong-calls, the reltype should be R_MIPS_HI16, and we may also need
> to add our specific code for sift_rel_mcount() to get the right
> location of the _mcount calling site)

"-mlong-calls" must set .e_flags, or otherwise provide enough
description to that the correct reltype can be chosen at the time
when .e_machine is decoded.  Adjusting the address for the location
of the call to _mcount should be another function pointer that is
overridden for EM_MIPS.

Regards,

-- 
John Reiser, jreiser@BitWagon.com

patch: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[John Reiser]

On 10/23/2010, wu zhangjin wrote:

>   CC      init/main.o
> /bin/sh: line 1: 21835 Segmentation fault     scripts/recordmcount "init/main.o"
> make[1]: *** [init/main.o] Error 139


64-bit EM_MIPS has weird ELF64_Rela.info.  Adjust recordmcount.[ch].

Signed-off-by: John Reiser <jreiser@BitWagon.com>

[I don't know where the git repository for recordmcount.h is,
so I have used a temporary one.]

--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -212,11 +212,26 @@ is_mcounted_section_name(char const *const txtname)
 		0 == strcmp(".text.unlikely", txtname);
 }
 +
 /* 32 bit and 64 bit are very similar */
 #include "recordmcount.h"
 #define RECORD_MCOUNT_64
 #include "recordmcount.h"
+/* 64-bit EM_MIPS has weird ELF64_Rela.r_info */
+static uint64_t MIPS64_r_sym(Elf64_Xword xword)
+{
+	/* Perhaps this should be 40 bits, but kernel isn't that big. */
+	return 0xffffffff & xword;
+}
+
+static uint64_t MIPS64_r_info(Elf64_Xword sym, unsigned type)
+{
+	/* Type2 and Type3 are assumed zero.  [See "readelf --relocs".] */
+	return (((uint64_t)type)<<56) | sym;
+}
+
+
 static void
 do_file(char const *const fname)
 {
@@ -268,6 +283,7 @@ do_file(char const *const fname)
 	case EM_386:	 reltype = R_386_32;                   break;
 	case EM_ARM:	 reltype = R_ARM_ABS32;                break;
 	case EM_IA_64:	 reltype = R_IA64_IMM64;   gpfx = '_'; break;
+	case EM_MIPS:	 /* reltype: e_class    */ gpfx = '_'; break;
 	case EM_PPC:	 reltype = R_PPC_ADDR32;   gpfx = '_'; break;
 	case EM_PPC64:	 reltype = R_PPC64_ADDR64; gpfx = '_'; break;
 	case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
@@ -291,6 +307,8 @@ do_file(char const *const fname)
 		}
 		if (EM_S390 == w2(ehdr->e_machine))
 			reltype = R_390_32;
+		if (EM_MIPS == w2(ehdr->e_machine))
+			reltype = R_MIPS_32;
 		do32(ehdr, fname, reltype);
 	} break;
 	case ELFCLASS64: {
@@ -303,6 +321,11 @@ do_file(char const *const fname)
 		}
 		if (EM_S390 == w2(ghdr->e_machine))
 			reltype = R_390_64;
+		if (EM_MIPS == w2(ghdr->e_machine)) {
+			reltype = R_MIPS_64;
+			Elf_r_sym = MIPS64_r_sym;
+			Elf_r_info = MIPS64_r_info;
+		}
 		do64(ghdr, fname, reltype);
 	} break;
 	}  /* end switch */
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -31,8 +31,12 @@
 #undef Elf_Rela
 #undef Elf_Sym
 #undef ELF_R_SYM
+#undef Elf_r_sym
 #undef ELF_R_INFO
+#undef Elf_r_info
 #undef ELF_ST_BIND
+#undef fn_ELF_R_SYM
+#undef fn_ELF_R_INFO
 #undef uint_t
 #undef _w
 #undef _align
@@ -52,8 +56,12 @@
 # define Elf_Rela		Elf64_Rela
 # define Elf_Sym		Elf64_Sym
 # define ELF_R_SYM		ELF64_R_SYM
+# define Elf_r_sym		Elf64_r_sym
 # define ELF_R_INFO		ELF64_R_INFO
+# define Elf_r_info		Elf64_r_info
 # define ELF_ST_BIND		ELF64_ST_BIND
+# define fn_ELF_R_SYM		fn_ELF64_R_SYM
+# define fn_ELF_R_INFO		fn_ELF64_R_INFO
 # define uint_t			uint64_t
 # define _w			w8
 # define _align			7u
@@ -72,14 +80,32 @@
 # define Elf_Rela		Elf32_Rela
 # define Elf_Sym		Elf32_Sym
 # define ELF_R_SYM		ELF32_R_SYM
+# define Elf_r_sym		Elf32_r_sym
 # define ELF_R_INFO		ELF32_R_INFO
+# define Elf_r_info		Elf32_r_info
 # define ELF_ST_BIND		ELF32_ST_BIND
+# define fn_ELF_R_SYM		fn_ELF32_R_SYM
+# define fn_ELF_R_INFO		fn_ELF32_R_INFO
 # define uint_t			uint32_t
 # define _w			w
 # define _align			3u
 # define _size			4
 #endif
 +/* Functions and pointers that 64-bit EM_MIPS can override. */
+static uint_t fn_ELF_R_SYM(uint_t info)
+{
+	return ELF_R_SYM(info);
+}
+static uint_t (*Elf_r_sym)(uint_t info) = fn_ELF_R_SYM;
+
+static uint_t fn_ELF_R_INFO(uint_t sym, unsigned type)
+{
+	return ELF_R_INFO(sym, type);
+}
+static uint_t (*Elf_r_info)(uint_t sym, unsigned type) = fn_ELF_R_INFO;
+
+
 /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
 static void append_func(Elf_Ehdr *const ehdr,
 			Elf_Shdr *const shstr,
@@ -197,22 +223,22 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
 	for (t = nrel; t; --t) {
 		if (!mcountsym) {
 			Elf_Sym const *const symp =
-				&sym0[ELF_R_SYM(_w(relp->r_info))];
+				&sym0[Elf_r_sym(_w(relp->r_info))];
 			char const *symname = &str0[w(symp->st_name)];
  			if ('.' == symname[0])
 				++symname;  /* ppc64 hack */
 			if (0 == strcmp((('_' == gpfx) ? "_mcount" : "mcount"),
 					symname))
-				mcountsym = ELF_R_SYM(_w(relp->r_info));
+				mcountsym = Elf_r_sym(_w(relp->r_info));
 		}
-		if (mcountsym == ELF_R_SYM(_w(relp->r_info))) {
+		if (mcountsym == Elf_r_sym(_w(relp->r_info))) {
 			uint_t const addend = _w(_w(relp->r_offset) - recval);
  			mrelp->r_offset = _w(offbase
 				+ ((void *)mlocp - (void *)mloc0));
-			mrelp->r_info = _w(ELF_R_INFO(recsym, reltype));
+			mrelp->r_info = _w(Elf_r_info(recsym, reltype));
 			if (sizeof(Elf_Rela) == rel_entsize) {
 				((Elf_Rela *)mrelp)->r_addend = addend;
 				*mlocp++ = 0;

-- 
John Reiser, jreiser@BitWagon.com

Re: patch: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[Maciej W. Rozycki]

On Sun, 24 Oct 2010, John Reiser wrote:

> @@ -212,11 +212,26 @@ is_mcounted_section_name(char const *const txtname)
>  		0 == strcmp(".text.unlikely", txtname);
>  }
>  +
>  /* 32 bit and 64 bit are very similar */
>  #include "recordmcount.h"
>  #define RECORD_MCOUNT_64
>  #include "recordmcount.h"
> +/* 64-bit EM_MIPS has weird ELF64_Rela.r_info */
> +static uint64_t MIPS64_r_sym(Elf64_Xword xword)
> +{
> +	/* Perhaps this should be 40 bits, but kernel isn't that big. */
> +	return 0xffffffff & xword;
> +}

 R_SYM is 32-bit on n64 MIPS, no need for the comment here.  This code 
looks wrong to me though (i.e. asssuming xword is the integer value of 
r_info), see below.

> +static uint64_t MIPS64_r_info(Elf64_Xword sym, unsigned type)
> +{
> +	/* Type2 and Type3 are assumed zero.  [See "readelf --relocs".] */
> +	return (((uint64_t)type)<<56) | sym;
> +}

 On n64 MIPS the bit alignment of the r_sym and r_type fields within a 
native 64-bit integer depends on the endianness of the target.  You need 
to take this into account.

 Note that on n64 MIPS R_INFO effectively is defined like this:

struct {
	Elf64_Word r_sym;
	Elf64_Byte r_ssym;
	Elf64_Byte r_type3;
	Elf64_Byte r_type2;
	Elf64_Byte r_type;
} r_info;

i.e. R_SYM is always a 32-bit value in the native endianness and it's 
followed by four bytes whose order is the same regardless of the system's 
endianness.  Search the web for SGI's "64-bit ELF Object File 
Specification" for further details.

[I wish people read the specs and did not rely on guesswork before writing 
code like this, sigh...]

  Maciej

Re: patch: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[John Reiser]

On 10/24/2010 08:59 PM, Maciej W. Rozycki wrote:

> Search the web for SGI's "64-bit ELF Object File 
> Specification" for further details.
> 
> [I wish people read the specs and did not rely on guesswork before writing 
> code like this, sigh...]

I offered a patch.  Would you care to offer a different patch?

Give the literal URL that you intend.  Include the URL in the patch!
An actual citation (author, title, URL, date) is more valuable than
"search the web", even if the URL should be come stale.

There is more than one spec.  <elf.h> is one of them, and it is available
without searching.  It is a fault on the MIPS milieu that one must search
for the spec.  The obvious candidate after searching:
   http://techpubs.sgi.com/library/manuals/4000/007-4658-001/pdf/007-4658-001.pdf
says only "Draft version 2.5".  Was it ever adopted?  When, and by whom?
Is there a more-authoritative version?  Has it been superseded?

Patch, please?

-- 
John Reiser, jreiser@BitWagon.com

patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[John Reiser]

Here's a second try [discard the first] for handling MIPS64 in recordmcount.[ch].

Signed-off-by: John Reiser <jreiser@BitWagon.com

diff --git a/recordmcount.c b/recordmcount.c
index 7f7f718..7337ee8 100644
--- a/recordmcount.c
+++ b/recordmcount.c
@@ -212,11 +212,48 @@ is_mcounted_section_name(char const *const txtname)
 		0 == strcmp(".text.unlikely", txtname);
 }

+
 /* 32 bit and 64 bit are very similar */
 #include "recordmcount.h"
 #define RECORD_MCOUNT_64
 #include "recordmcount.h"

+/* 64-bit EM_MIPS has weird ELF64_Rela.r_info.
+ * http://techpubs.sgi.com/library/manuals/4000/007-4658-001/pdf/007-4658-001.pdf
+ * We interpret Table 29 Relocation Operation (Elf64_Rel, Elf64_Rela) [p.40]
+ * to imply the order of the members; the spec does not say so.
+ *	typedef unsigned char Elf64_Byte;
+ * fails on MIPS64 because their <elf.h> already has it!
+ */
+typedef unsigned char myElf64_byte;
+typedef struct {
+	Elf64_Addr    r_offset;               /* Address */
+	struct {
+		Elf64_Word r_sym;
+		myElf64_byte r_ssym;  /* Special sym: gp-relative, etc. */
+		myElf64_byte r_type3;
+		myElf64_byte r_type2;
+		myElf64_byte r_type;
+	} r_info;
+	Elf64_Sxword  r_addend;               /* Addend */
+} MIPS64_Rela;
+
+static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
+{
+	return w(((MIPS64_Rela const *)rp)->r_info.r_sym);
+}
+
+static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
+{
+	MIPS64_Rela *const m64rp = (MIPS64_Rela *)rp;
+	m64rp->r_info.r_sym = w(sym);
+	m64rp->r_info.r_ssym = 0;
+	m64rp->r_info.r_type3 = 0;
+	m64rp->r_info.r_type2 = 0;
+	m64rp->r_info.r_type = type;
+}
+
+
 static void
 do_file(char const *const fname)
 {
@@ -268,6 +305,7 @@ do_file(char const *const fname)
 	case EM_386:	 reltype = R_386_32;                   break;
 	case EM_ARM:	 reltype = R_ARM_ABS32;                break;
 	case EM_IA_64:	 reltype = R_IA64_IMM64;   gpfx = '_'; break;
+	case EM_MIPS:	 /* reltype: e_class    */ gpfx = '_'; break;
 	case EM_PPC:	 reltype = R_PPC_ADDR32;   gpfx = '_'; break;
 	case EM_PPC64:	 reltype = R_PPC64_ADDR64; gpfx = '_'; break;
 	case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
@@ -291,6 +329,8 @@ do_file(char const *const fname)
 		}
 		if (EM_S390 == w2(ehdr->e_machine))
 			reltype = R_390_32;
+		if (EM_MIPS == w2(ehdr->e_machine))
+			reltype = R_MIPS_32;
 		do32(ehdr, fname, reltype);
 	} break;
 	case ELFCLASS64: {
@@ -303,6 +343,11 @@ do_file(char const *const fname)
 		}
 		if (EM_S390 == w2(ghdr->e_machine))
 			reltype = R_390_64;
+		if (EM_MIPS == w2(ghdr->e_machine)) {
+			reltype = R_MIPS_64;
+			Elf64_r_sym = MIPS64_r_sym;
+			Elf64_r_info = MIPS64_r_info;
+		}
 		do64(ghdr, fname, reltype);
 	} break;
 	}  /* end switch */
diff --git a/recordmcount.h b/recordmcount.h
index 7f39d09..190fd18 100644
--- a/recordmcount.h
+++ b/recordmcount.h
@@ -31,8 +31,12 @@
 #undef Elf_Rela
 #undef Elf_Sym
 #undef ELF_R_SYM
+#undef Elf_r_sym
 #undef ELF_R_INFO
+#undef Elf_r_info
 #undef ELF_ST_BIND
+#undef fn_ELF_R_SYM
+#undef fn_ELF_R_INFO
 #undef uint_t
 #undef _w
 #undef _align
@@ -52,8 +56,12 @@
 # define Elf_Rela		Elf64_Rela
 # define Elf_Sym		Elf64_Sym
 # define ELF_R_SYM		ELF64_R_SYM
+# define Elf_r_sym		Elf64_r_sym
 # define ELF_R_INFO		ELF64_R_INFO
+# define Elf_r_info		Elf64_r_info
 # define ELF_ST_BIND		ELF64_ST_BIND
+# define fn_ELF_R_SYM		fn_ELF64_R_SYM
+# define fn_ELF_R_INFO		fn_ELF64_R_INFO
 # define uint_t			uint64_t
 # define _w			w8
 # define _align			7u
@@ -72,14 +80,32 @@
 # define Elf_Rela		Elf32_Rela
 # define Elf_Sym		Elf32_Sym
 # define ELF_R_SYM		ELF32_R_SYM
+# define Elf_r_sym		Elf32_r_sym
 # define ELF_R_INFO		ELF32_R_INFO
+# define Elf_r_info		Elf32_r_info
 # define ELF_ST_BIND		ELF32_ST_BIND
+# define fn_ELF_R_SYM		fn_ELF32_R_SYM
+# define fn_ELF_R_INFO		fn_ELF32_R_INFO
 # define uint_t			uint32_t
 # define _w			w
 # define _align			3u
 # define _size			4
 #endif

+/* Functions and pointers that 64-bit EM_MIPS can override. */
+static uint_t fn_ELF_R_SYM(Elf_Rel const *rp)
+{
+	return ELF_R_SYM(_w(rp->r_info));
+}
+static uint_t (*Elf_r_sym)(Elf_Rel const *rp) = fn_ELF_R_SYM;
+
+static void fn_ELF_R_INFO(Elf_Rel *const rp, unsigned sym, unsigned type)
+{
+	rp->r_info = ELF_R_INFO(sym, type);
+}
+static void (*Elf_r_info)(Elf_Rel *const rp, unsigned sym, unsigned type) = fn_ELF_R_INFO;
+
+
 /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
 static void append_func(Elf_Ehdr *const ehdr,
 			Elf_Shdr *const shstr,
@@ -197,22 +223,22 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
 	for (t = nrel; t; --t) {
 		if (!mcountsym) {
 			Elf_Sym const *const symp =
-				&sym0[ELF_R_SYM(_w(relp->r_info))];
+				&sym0[Elf_r_sym(relp)];
 			char const *symname = &str0[w(symp->st_name)];
  			if ('.' == symname[0])
 				++symname;  /* ppc64 hack */
 			if (0 == strcmp((('_' == gpfx) ? "_mcount" : "mcount"),
 					symname))
-				mcountsym = ELF_R_SYM(_w(relp->r_info));
+				mcountsym = Elf_r_sym(relp);
 		}

-		if (mcountsym == ELF_R_SYM(_w(relp->r_info))) {
+		if (mcountsym == Elf_r_sym(relp)) {
 			uint_t const addend = _w(_w(relp->r_offset) - recval);
  			mrelp->r_offset = _w(offbase
 				+ ((void *)mlocp - (void *)mloc0));
-			mrelp->r_info = _w(ELF_R_INFO(recsym, reltype));
+			Elf_r_info(mrelp, recsym, reltype);
 			if (sizeof(Elf_Rela) == rel_entsize) {
 				((Elf_Rela *)mrelp)->r_addend = addend;
 				*mlocp++ = 0;

-- 
John Reiser, jreiser@BitWagon.com

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

On 10/26/10, John Reiser <jreiser@bitwagon.com> wrote:
> Here's a second try [discard the first] for handling MIPS64 in
> recordmcount.[ch].

Just tested the 64bit kernel, it works well.

Will test the 64bit module and the 32bit kernel and module tomorrow If
time allowed.

BTW, seems this patch can not be applied directly, suggest you
generate it by "git format" automatically ;-)

Thanks & Regards,
Wu Zhangjin

>
> Signed-off-by: John Reiser <jreiser@BitWagon.com
>
> diff --git a/recordmcount.c b/recordmcount.c
> index 7f7f718..7337ee8 100644
> --- a/recordmcount.c
> +++ b/recordmcount.c
> @@ -212,11 +212,48 @@ is_mcounted_section_name(char const *const txtname)
>  		0 == strcmp(".text.unlikely", txtname);
>  }
>
> +
>  /* 32 bit and 64 bit are very similar */
>  #include "recordmcount.h"
>  #define RECORD_MCOUNT_64
>  #include "recordmcount.h"
>
> +/* 64-bit EM_MIPS has weird ELF64_Rela.r_info.
> + *
> http://techpubs.sgi.com/library/manuals/4000/007-4658-001/pdf/007-4658-001.pdf
> + * We interpret Table 29 Relocation Operation (Elf64_Rel, Elf64_Rela)
> [p.40]
> + * to imply the order of the members; the spec does not say so.
> + *	typedef unsigned char Elf64_Byte;
> + * fails on MIPS64 because their <elf.h> already has it!
> + */
> +typedef unsigned char myElf64_byte;
> +typedef struct {
> +	Elf64_Addr    r_offset;               /* Address */
> +	struct {
> +		Elf64_Word r_sym;
> +		myElf64_byte r_ssym;  /* Special sym: gp-relative, etc. */
> +		myElf64_byte r_type3;
> +		myElf64_byte r_type2;
> +		myElf64_byte r_type;
> +	} r_info;
> +	Elf64_Sxword  r_addend;               /* Addend */
> +} MIPS64_Rela;
> +
> +static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
> +{
> +	return w(((MIPS64_Rela const *)rp)->r_info.r_sym);
> +}
> +
> +static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
> +{
> +	MIPS64_Rela *const m64rp = (MIPS64_Rela *)rp;
> +	m64rp->r_info.r_sym = w(sym);
> +	m64rp->r_info.r_ssym = 0;
> +	m64rp->r_info.r_type3 = 0;
> +	m64rp->r_info.r_type2 = 0;
> +	m64rp->r_info.r_type = type;
> +}
> +
> +
>  static void
>  do_file(char const *const fname)
>  {
> @@ -268,6 +305,7 @@ do_file(char const *const fname)
>  	case EM_386:	 reltype = R_386_32;                   break;
>  	case EM_ARM:	 reltype = R_ARM_ABS32;                break;
>  	case EM_IA_64:	 reltype = R_IA64_IMM64;   gpfx = '_'; break;
> +	case EM_MIPS:	 /* reltype: e_class    */ gpfx = '_'; break;
>  	case EM_PPC:	 reltype = R_PPC_ADDR32;   gpfx = '_'; break;
>  	case EM_PPC64:	 reltype = R_PPC64_ADDR64; gpfx = '_'; break;
>  	case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
> @@ -291,6 +329,8 @@ do_file(char const *const fname)
>  		}
>  		if (EM_S390 == w2(ehdr->e_machine))
>  			reltype = R_390_32;
> +		if (EM_MIPS == w2(ehdr->e_machine))
> +			reltype = R_MIPS_32;
>  		do32(ehdr, fname, reltype);
>  	} break;
>  	case ELFCLASS64: {
> @@ -303,6 +343,11 @@ do_file(char const *const fname)
>  		}
>  		if (EM_S390 == w2(ghdr->e_machine))
>  			reltype = R_390_64;
> +		if (EM_MIPS == w2(ghdr->e_machine)) {
> +			reltype = R_MIPS_64;
> +			Elf64_r_sym = MIPS64_r_sym;
> +			Elf64_r_info = MIPS64_r_info;
> +		}
>  		do64(ghdr, fname, reltype);
>  	} break;
>  	}  /* end switch */
> diff --git a/recordmcount.h b/recordmcount.h
> index 7f39d09..190fd18 100644
> --- a/recordmcount.h
> +++ b/recordmcount.h
> @@ -31,8 +31,12 @@
>  #undef Elf_Rela
>  #undef Elf_Sym
>  #undef ELF_R_SYM
> +#undef Elf_r_sym
>  #undef ELF_R_INFO
> +#undef Elf_r_info
>  #undef ELF_ST_BIND
> +#undef fn_ELF_R_SYM
> +#undef fn_ELF_R_INFO
>  #undef uint_t
>  #undef _w
>  #undef _align
> @@ -52,8 +56,12 @@
>  # define Elf_Rela		Elf64_Rela
>  # define Elf_Sym		Elf64_Sym
>  # define ELF_R_SYM		ELF64_R_SYM
> +# define Elf_r_sym		Elf64_r_sym
>  # define ELF_R_INFO		ELF64_R_INFO
> +# define Elf_r_info		Elf64_r_info
>  # define ELF_ST_BIND		ELF64_ST_BIND
> +# define fn_ELF_R_SYM		fn_ELF64_R_SYM
> +# define fn_ELF_R_INFO		fn_ELF64_R_INFO
>  # define uint_t			uint64_t
>  # define _w			w8
>  # define _align			7u
> @@ -72,14 +80,32 @@
>  # define Elf_Rela		Elf32_Rela
>  # define Elf_Sym		Elf32_Sym
>  # define ELF_R_SYM		ELF32_R_SYM
> +# define Elf_r_sym		Elf32_r_sym
>  # define ELF_R_INFO		ELF32_R_INFO
> +# define Elf_r_info		Elf32_r_info
>  # define ELF_ST_BIND		ELF32_ST_BIND
> +# define fn_ELF_R_SYM		fn_ELF32_R_SYM
> +# define fn_ELF_R_INFO		fn_ELF32_R_INFO
>  # define uint_t			uint32_t
>  # define _w			w
>  # define _align			3u
>  # define _size			4
>  #endif
>
> +/* Functions and pointers that 64-bit EM_MIPS can override. */
> +static uint_t fn_ELF_R_SYM(Elf_Rel const *rp)
> +{
> +	return ELF_R_SYM(_w(rp->r_info));
> +}
> +static uint_t (*Elf_r_sym)(Elf_Rel const *rp) = fn_ELF_R_SYM;
> +
> +static void fn_ELF_R_INFO(Elf_Rel *const rp, unsigned sym, unsigned type)
> +{
> +	rp->r_info = ELF_R_INFO(sym, type);
> +}
> +static void (*Elf_r_info)(Elf_Rel *const rp, unsigned sym, unsigned type) =
> fn_ELF_R_INFO;
> +
> +
>  /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations.
> */
>  static void append_func(Elf_Ehdr *const ehdr,
>  			Elf_Shdr *const shstr,
> @@ -197,22 +223,22 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
>  	for (t = nrel; t; --t) {
>  		if (!mcountsym) {
>  			Elf_Sym const *const symp =
> -				&sym0[ELF_R_SYM(_w(relp->r_info))];
> +				&sym0[Elf_r_sym(relp)];
>  			char const *symname = &str0[w(symp->st_name)];
>   			if ('.' == symname[0])
>  				++symname;  /* ppc64 hack */
>  			if (0 == strcmp((('_' == gpfx) ? "_mcount" : "mcount"),
>  					symname))
> -				mcountsym = ELF_R_SYM(_w(relp->r_info));
> +				mcountsym = Elf_r_sym(relp);
>  		}
>
> -		if (mcountsym == ELF_R_SYM(_w(relp->r_info))) {
> +		if (mcountsym == Elf_r_sym(relp)) {
>  			uint_t const addend = _w(_w(relp->r_offset) - recval);
>   			mrelp->r_offset = _w(offbase
>  				+ ((void *)mlocp - (void *)mloc0));
> -			mrelp->r_info = _w(ELF_R_INFO(recsym, reltype));
> +			Elf_r_info(mrelp, recsym, reltype);
>  			if (sizeof(Elf_Rela) == rel_entsize) {
>  				((Elf_Rela *)mrelp)->r_addend = addend;
>  				*mlocp++ = 0;
>
> --
> John Reiser, jreiser@BitWagon.com
>
>

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[Steven Rostedt]

On Tue, 2010-10-26 at 02:17 +0800, wu zhangjin wrote:
> On 10/26/10, John Reiser <jreiser@bitwagon.com> wrote:
> > Here's a second try [discard the first] for handling MIPS64 in
> > recordmcount.[ch].
> 
> Just tested the 64bit kernel, it works well.
> 
> Will test the 64bit module and the 32bit kernel and module tomorrow If
> time allowed.
> 
> BTW, seems this patch can not be applied directly, suggest you
> generate it by "git format" automatically ;-)


Great! If it all passes nicely, can I get a tested-by from you, and a
patch that enables it for mips with an Acked-by: from Ralf?

We may still be able to make this merge window.

-- Steve

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[Maciej W. Rozycki]

On Mon, 25 Oct 2010, John Reiser wrote:

> Here's a second try [discard the first] for handling MIPS64 in recordmcount.[ch].

 Thanks for your contribution -- I appreciate it as undoubtedly do the 
others -- no need to feel frustrated. :)

 Please note that the unusual relocation format comes from the fact the 
MIPS n64 ABI supplement was developed before the ELF64 gABI was finalised 
-- no surprise here given MIPS processors were the first 64-bit around.  
Then the gABI took a more generic approach to handle compound relocations.  
Even `readelf' you cited only got it right as recently as in 2005 (BFD was 
OK from the beginning though AFAIR, so code generated as well as e.g. 
`objdump' reports were right).

 Your change looks good to me overall (I haven't tried building or running 
it -- I take your word you've got it right).  I have a small nit though -- 
see below.

> +typedef unsigned char myElf64_byte;
> +typedef struct {
> +	Elf64_Addr    r_offset;               /* Address */
> +	struct {
> +		Elf64_Word r_sym;
> +		myElf64_byte r_ssym;  /* Special sym: gp-relative, etc. */
> +		myElf64_byte r_type3;
> +		myElf64_byte r_type2;
> +		myElf64_byte r_type;
> +	} r_info;
> +	Elf64_Sxword  r_addend;               /* Addend */
> +} MIPS64_Rela;
> +
> +static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
> +{
> +	return w(((MIPS64_Rela const *)rp)->r_info.r_sym);
> +}
> +
> +static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
> +{
> +	MIPS64_Rela *const m64rp = (MIPS64_Rela *)rp;
> +	m64rp->r_info.r_sym = w(sym);
> +	m64rp->r_info.r_ssym = 0;
> +	m64rp->r_info.r_type3 = 0;
> +	m64rp->r_info.r_type2 = 0;
> +	m64rp->r_info.r_type = type;
> +}

 Please try and avoid type punning, i.e. use a union to switch between the 
gABI format of r_info and the n64 MIPS psABI variation.  You'll avoid the 
risk of GCC doing weird stuff wrt aliasing.

 Thanks,

  Maciej

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

On Tue, Oct 26, 2010 at 4:24 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 2010-10-26 at 02:17 +0800, wu zhangjin wrote:
>> On 10/26/10, John Reiser <jreiser@bitwagon.com> wrote:
>> > Here's a second try [discard the first] for handling MIPS64 in
>> > recordmcount.[ch].
>>
>> Just tested the 64bit kernel, it works well.
>>
>> Will test the 64bit module and the 32bit kernel and module tomorrow If
>> time allowed.
>>
>> BTW, seems this patch can not be applied directly, suggest you
>> generate it by "git format" automatically ;-)
>
>
> Great! If it all passes nicely, can I get a tested-by from you, and a
> patch that enables it for mips with an Acked-by: from Ralf?

Based on John's patch, I just added the support(no -m or --module for
recordmcount is needed) for MIPS module and tested it, it works well.
Will send it out after the testing the 32bit support.

Regards,
Wu Zhangjin

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

On Tue, Oct 26, 2010 at 9:36 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Mon, 25 Oct 2010, John Reiser wrote:
>
>> Here's a second try [discard the first] for handling MIPS64 in recordmcount.[ch].
>
>  Thanks for your contribution -- I appreciate it as undoubtedly do the
> others -- no need to feel frustrated. :)
>
>  Please note that the unusual relocation format comes from the fact the
> MIPS n64 ABI supplement was developed before the ELF64 gABI was finalised
> -- no surprise here given MIPS processors were the first 64-bit around.
> Then the gABI took a more generic approach to handle compound relocations.
> Even `readelf' you cited only got it right as recently as in 2005 (BFD was
> OK from the beginning though AFAIR, so code generated as well as e.g.
> `objdump' reports were right).
>
>  Your change looks good to me overall (I haven't tried building or running
> it -- I take your word you've got it right).  I have a small nit though --
> see below.
>
>> +typedef unsigned char myElf64_byte;
>> +typedef struct {
>> +     Elf64_Addr    r_offset;               /* Address */
>> +     struct {
>> +             Elf64_Word r_sym;
>> +             myElf64_byte r_ssym;  /* Special sym: gp-relative, etc. */
>> +             myElf64_byte r_type3;
>> +             myElf64_byte r_type2;
>> +             myElf64_byte r_type;
>> +     } r_info;
>> +     Elf64_Sxword  r_addend;               /* Addend */
>> +} MIPS64_Rela;
>> +
>> +static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
>> +{
>> +     return w(((MIPS64_Rela const *)rp)->r_info.r_sym);
>> +}
>> +
>> +static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
>> +{
>> +     MIPS64_Rela *const m64rp = (MIPS64_Rela *)rp;
>> +     m64rp->r_info.r_sym = w(sym);
>> +     m64rp->r_info.r_ssym = 0;
>> +     m64rp->r_info.r_type3 = 0;
>> +     m64rp->r_info.r_type2 = 0;
>> +     m64rp->r_info.r_type = type;
>> +}
>
>  Please try and avoid type punning, i.e. use a union to switch between the
> gABI format of r_info and the n64 MIPS psABI variation.  You'll avoid the
> risk of GCC doing weird stuff wrt aliasing.

Hi, Maciej

will this help?

typedef struct {
        Elf64_Addr    r_offset;               /* Address */
        union {
                struct {
                        Elf64_Word r_sym;
                        myElf64_byte r_ssym;  /* Special sym:
gp-relative, etc. */
                        myElf64_byte r_type3;
                        myElf64_byte r_type2;
                        myElf64_byte r_type;
                } r_info;
                Elf64_Xword gABI_r_info;
        };
        Elf64_Sxword  r_addend;               /* Addend */
} MIPS64_Rela;

Regards,
Wu Zhangjin

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

Hi, John & Steve

I just tested this patch and another patch of mine for the module
support on 32bit and 64bit MIPS, all of them works, so, it is time to
send the whole patchset out.

To john:

Your patch is perfect except the little feedback about type punning
from Maciej, I have fixed it in my local copy.

To add the full C version of recordmcount support for MIPS, we also
need to select the HAVE_C_RECORDMCOUNT in arch/mips/Kconfig and add my
patch for the module support. So, Can I send out the whole
patchset(including yours with your signed-off-by: and my tested-by:)?

Regards,
Wu Zhangjin

On Tue, Oct 26, 2010 at 12:46 AM, John Reiser <jreiser@bitwagon.com> wrote:
> Here's a second try [discard the first] for handling MIPS64 in recordmcount.[ch].
>
> Signed-off-by: John Reiser <jreiser@BitWagon.com
>
> diff --git a/recordmcount.c b/recordmcount.c
> index 7f7f718..7337ee8 100644
> --- a/recordmcount.c
> +++ b/recordmcount.c
> @@ -212,11 +212,48 @@ is_mcounted_section_name(char const *const txtname)
>                0 == strcmp(".text.unlikely", txtname);
>  }
>
> +
>  /* 32 bit and 64 bit are very similar */
>  #include "recordmcount.h"
>  #define RECORD_MCOUNT_64
>  #include "recordmcount.h"
>
> +/* 64-bit EM_MIPS has weird ELF64_Rela.r_info.
> + * http://techpubs.sgi.com/library/manuals/4000/007-4658-001/pdf/007-4658-001.pdf
> + * We interpret Table 29 Relocation Operation (Elf64_Rel, Elf64_Rela) [p.40]
> + * to imply the order of the members; the spec does not say so.
> + *     typedef unsigned char Elf64_Byte;
> + * fails on MIPS64 because their <elf.h> already has it!
> + */
> +typedef unsigned char myElf64_byte;
> +typedef struct {
> +       Elf64_Addr    r_offset;               /* Address */
> +       struct {
> +               Elf64_Word r_sym;
> +               myElf64_byte r_ssym;  /* Special sym: gp-relative, etc. */
> +               myElf64_byte r_type3;
> +               myElf64_byte r_type2;
> +               myElf64_byte r_type;
> +       } r_info;
> +       Elf64_Sxword  r_addend;               /* Addend */
> +} MIPS64_Rela;
> +
> +static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
> +{
> +       return w(((MIPS64_Rela const *)rp)->r_info.r_sym);
> +}
> +
> +static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
> +{
> +       MIPS64_Rela *const m64rp = (MIPS64_Rela *)rp;
> +       m64rp->r_info.r_sym = w(sym);
> +       m64rp->r_info.r_ssym = 0;
> +       m64rp->r_info.r_type3 = 0;
> +       m64rp->r_info.r_type2 = 0;
> +       m64rp->r_info.r_type = type;
> +}
> +
> +
>  static void
>  do_file(char const *const fname)
>  {
> @@ -268,6 +305,7 @@ do_file(char const *const fname)
>        case EM_386:     reltype = R_386_32;                   break;
>        case EM_ARM:     reltype = R_ARM_ABS32;                break;
>        case EM_IA_64:   reltype = R_IA64_IMM64;   gpfx = '_'; break;
> +       case EM_MIPS:    /* reltype: e_class    */ gpfx = '_'; break;
>        case EM_PPC:     reltype = R_PPC_ADDR32;   gpfx = '_'; break;
>        case EM_PPC64:   reltype = R_PPC64_ADDR64; gpfx = '_'; break;
>        case EM_S390:    /* reltype: e_class    */ gpfx = '_'; break;
> @@ -291,6 +329,8 @@ do_file(char const *const fname)
>                }
>                if (EM_S390 == w2(ehdr->e_machine))
>                        reltype = R_390_32;
> +               if (EM_MIPS == w2(ehdr->e_machine))
> +                       reltype = R_MIPS_32;
>                do32(ehdr, fname, reltype);
>        } break;
>        case ELFCLASS64: {
> @@ -303,6 +343,11 @@ do_file(char const *const fname)
>                }
>                if (EM_S390 == w2(ghdr->e_machine))
>                        reltype = R_390_64;
> +               if (EM_MIPS == w2(ghdr->e_machine)) {
> +                       reltype = R_MIPS_64;
> +                       Elf64_r_sym = MIPS64_r_sym;
> +                       Elf64_r_info = MIPS64_r_info;
> +               }
>                do64(ghdr, fname, reltype);
>        } break;
>        }  /* end switch */
> diff --git a/recordmcount.h b/recordmcount.h
> index 7f39d09..190fd18 100644
> --- a/recordmcount.h
> +++ b/recordmcount.h
> @@ -31,8 +31,12 @@
>  #undef Elf_Rela
>  #undef Elf_Sym
>  #undef ELF_R_SYM
> +#undef Elf_r_sym
>  #undef ELF_R_INFO
> +#undef Elf_r_info
>  #undef ELF_ST_BIND
> +#undef fn_ELF_R_SYM
> +#undef fn_ELF_R_INFO
>  #undef uint_t
>  #undef _w
>  #undef _align
> @@ -52,8 +56,12 @@
>  # define Elf_Rela              Elf64_Rela
>  # define Elf_Sym               Elf64_Sym
>  # define ELF_R_SYM             ELF64_R_SYM
> +# define Elf_r_sym             Elf64_r_sym
>  # define ELF_R_INFO            ELF64_R_INFO
> +# define Elf_r_info            Elf64_r_info
>  # define ELF_ST_BIND           ELF64_ST_BIND
> +# define fn_ELF_R_SYM          fn_ELF64_R_SYM
> +# define fn_ELF_R_INFO         fn_ELF64_R_INFO
>  # define uint_t                        uint64_t
>  # define _w                    w8
>  # define _align                        7u
> @@ -72,14 +80,32 @@
>  # define Elf_Rela              Elf32_Rela
>  # define Elf_Sym               Elf32_Sym
>  # define ELF_R_SYM             ELF32_R_SYM
> +# define Elf_r_sym             Elf32_r_sym
>  # define ELF_R_INFO            ELF32_R_INFO
> +# define Elf_r_info            Elf32_r_info
>  # define ELF_ST_BIND           ELF32_ST_BIND
> +# define fn_ELF_R_SYM          fn_ELF32_R_SYM
> +# define fn_ELF_R_INFO         fn_ELF32_R_INFO
>  # define uint_t                        uint32_t
>  # define _w                    w
>  # define _align                        3u
>  # define _size                 4
>  #endif
>
> +/* Functions and pointers that 64-bit EM_MIPS can override. */
> +static uint_t fn_ELF_R_SYM(Elf_Rel const *rp)
> +{
> +       return ELF_R_SYM(_w(rp->r_info));
> +}
> +static uint_t (*Elf_r_sym)(Elf_Rel const *rp) = fn_ELF_R_SYM;
> +
> +static void fn_ELF_R_INFO(Elf_Rel *const rp, unsigned sym, unsigned type)
> +{
> +       rp->r_info = ELF_R_INFO(sym, type);
> +}
> +static void (*Elf_r_info)(Elf_Rel *const rp, unsigned sym, unsigned type) = fn_ELF_R_INFO;
> +
> +
>  /* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
>  static void append_func(Elf_Ehdr *const ehdr,
>                        Elf_Shdr *const shstr,
> @@ -197,22 +223,22 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
>        for (t = nrel; t; --t) {
>                if (!mcountsym) {
>                        Elf_Sym const *const symp =
> -                               &sym0[ELF_R_SYM(_w(relp->r_info))];
> +                               &sym0[Elf_r_sym(relp)];
>                        char const *symname = &str0[w(symp->st_name)];
>                        if ('.' == symname[0])
>                                ++symname;  /* ppc64 hack */
>                        if (0 == strcmp((('_' == gpfx) ? "_mcount" : "mcount"),
>                                        symname))
> -                               mcountsym = ELF_R_SYM(_w(relp->r_info));
> +                               mcountsym = Elf_r_sym(relp);
>                }
>
> -               if (mcountsym == ELF_R_SYM(_w(relp->r_info))) {
> +               if (mcountsym == Elf_r_sym(relp)) {
>                        uint_t const addend = _w(_w(relp->r_offset) - recval);
>                        mrelp->r_offset = _w(offbase
>                                + ((void *)mlocp - (void *)mloc0));
> -                       mrelp->r_info = _w(ELF_R_INFO(recsym, reltype));
> +                       Elf_r_info(mrelp, recsym, reltype);
>                        if (sizeof(Elf_Rela) == rel_entsize) {
>                                ((Elf_Rela *)mrelp)->r_addend = addend;
>                                *mlocp++ = 0;
>
> --
> John Reiser, jreiser@BitWagon.com
>
>

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[Maciej W. Rozycki]

On Wed, 27 Oct 2010, wu zhangjin wrote:

> will this help?
> 
> typedef struct {
>         Elf64_Addr    r_offset;               /* Address */
>         union {
>                 struct {
>                         Elf64_Word r_sym;
>                         myElf64_byte r_ssym;  /* Special sym:
> gp-relative, etc. */
>                         myElf64_byte r_type3;
>                         myElf64_byte r_type2;
>                         myElf64_byte r_type;
>                 } r_info;
>                 Elf64_Xword gABI_r_info;
>         };
>         Elf64_Sxword  r_addend;               /* Addend */
> } MIPS64_Rela;

 More or less, although you need to give your union a name to access its 
members. ;)  It may be simpler to refer to r_info only, e.g. something 
along these lines:

typedef uint8_t myElf64_Byte;
union mips_r_info {
	Elf64_Xword r_info;
	struct {
		Elf64_Word r_sym;
		myElf64_Byte r_ssym;
		myElf64_Byte r_type3;
		myElf64_Byte r_type2;
		myElf64_Byte r_type;
	} r_mips;
};

static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
{
	return w(((union mips_r_info){ .r_info = rp->r_info }).r_mips.r_sym);
}

static void MIPS64_r_info(Elf64_Rel *const rp,
			  unsigned int sym, unsigned int type)
{
	rp->r_info = ((union mips_r_info){
		.r_mips = { .r_sym = w(sym), .r_type = type }
	}).r_info;
}

Untested, but GCC 4.1.2 seems to turn it into decent big-endian code:

tmp.o:     file format elf64-tradbigmips

Disassembly of section .text:

0000000000000000 <MIPS64_r_sym>:
   0:	03e00008 	jr	ra
   4:	9c820008 	lwu	v0,8(a0)

0000000000000008 <MIPS64_r_info>:
   8:	30c600ff 	andi	a2,a2,0xff
   c:	0005283c 	dsll32	a1,a1,0x0
  10:	00a62825 	or	a1,a1,a2
  14:	03e00008 	jr	ra
  18:	fc850008 	sd	a1,8(a0)

and not so decent little-endian code (too many shifts):

tmpel.o:     file format elf64-tradlittlemips

Disassembly of section .text:

0000000000000000 <MIPS64_r_sym>:
   0:	dc820008 	ld	v0,8(a0)
   4:	00021000 	sll	v0,v0,0x0
   8:	0002103c 	dsll32	v0,v0,0x0
   c:	03e00008 	jr	ra
  10:	0002103e 	dsrl32	v0,v0,0x0

0000000000000018 <MIPS64_r_info>:
  18:	0005283c 	dsll32	a1,a1,0x0
  1c:	0006363c 	dsll32	a2,a2,0x18
  20:	0005283e 	dsrl32	a1,a1,0x0
  24:	00a62825 	or	a1,a1,a2
  28:	03e00008 	jr	ra
  2c:	fc850008 	sd	a1,8(a0)

GCC may have been fixed/improved since though (I'd expect so, but didn't 
have the resources to upgrade yet, so check yourself).

 Here's my sign-off mark if you'd like to use the code above.

Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>

  Maciej

Re: patch v2: [RFC 2/2] ftrace/MIPS: Add support for C version of recordmcount

[wu zhangjin]

On Wed, Oct 27, 2010 at 5:31 PM, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Wed, 27 Oct 2010, wu zhangjin wrote:
>
>> will this help?
>>
>> typedef struct {
>>         Elf64_Addr    r_offset;               /* Address */
>>         union {
>>                 struct {
>>                         Elf64_Word r_sym;
>>                         myElf64_byte r_ssym;  /* Special sym:
>> gp-relative, etc. */
>>                         myElf64_byte r_type3;
>>                         myElf64_byte r_type2;
>>                         myElf64_byte r_type;
>>                 } r_info;
>>                 Elf64_Xword gABI_r_info;
>>         };
>>         Elf64_Sxword  r_addend;               /* Addend */
>> } MIPS64_Rela;
>
>  More or less, although you need to give your union a name to access its
> members. ;)  It may be simpler to refer to r_info only, e.g. something
> along these lines:

Yeah, I like it ;) In reality, In my local copy, I have tried to use
MIPS64_Rel instead of MIPS64_Rela, but you did more, so, I will use
your method before sending the patchset out.

Thanks & Regards,
Wu Zhangjin

>
> typedef uint8_t myElf64_Byte;
> union mips_r_info {
>        Elf64_Xword r_info;
>        struct {
>                Elf64_Word r_sym;
>                myElf64_Byte r_ssym;
>                myElf64_Byte r_type3;
>                myElf64_Byte r_type2;
>                myElf64_Byte r_type;
>        } r_mips;
> };
>
> static uint64_t MIPS64_r_sym(Elf64_Rel const *rp)
> {
>        return w(((union mips_r_info){ .r_info = rp->r_info }).r_mips.r_sym);
> }
>
> static void MIPS64_r_info(Elf64_Rel *const rp,
>                          unsigned int sym, unsigned int type)
> {
>        rp->r_info = ((union mips_r_info){
>                .r_mips = { .r_sym = w(sym), .r_type = type }
>        }).r_info;
> }
>
> Untested, but GCC 4.1.2 seems to turn it into decent big-endian code:
>
> tmp.o:     file format elf64-tradbigmips
>
> Disassembly of section .text:
>
> 0000000000000000 <MIPS64_r_sym>:
>   0:   03e00008        jr      ra
>   4:   9c820008        lwu     v0,8(a0)
>
> 0000000000000008 <MIPS64_r_info>:
>   8:   30c600ff        andi    a2,a2,0xff
>   c:   0005283c        dsll32  a1,a1,0x0
>  10:   00a62825        or      a1,a1,a2
>  14:   03e00008        jr      ra
>  18:   fc850008        sd      a1,8(a0)
>
> and not so decent little-endian code (too many shifts):
>
> tmpel.o:     file format elf64-tradlittlemips
>
> Disassembly of section .text:
>
> 0000000000000000 <MIPS64_r_sym>:
>   0:   dc820008        ld      v0,8(a0)
>   4:   00021000        sll     v0,v0,0x0
>   8:   0002103c        dsll32  v0,v0,0x0
>   c:   03e00008        jr      ra
>  10:   0002103e        dsrl32  v0,v0,0x0
>
> 0000000000000018 <MIPS64_r_info>:
>  18:   0005283c        dsll32  a1,a1,0x0
>  1c:   0006363c        dsll32  a2,a2,0x18
>  20:   0005283e        dsrl32  a1,a1,0x0
>  24:   00a62825        or      a1,a1,a2
>  28:   03e00008        jr      ra
>  2c:   fc850008        sd      a1,8(a0)
>
> GCC may have been fixed/improved since though (I'd expect so, but didn't
> have the resources to upgrade yet, so check yourself).
>
>  Here's my sign-off mark if you'd like to use the code above.
>
> Signed-off-by: Maciej W. Rozycki <macro@linux-mips.org>
>
>  Maciej
>