lkml.org 
[lkml]   [2009]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH -tip -v12 02/11] x86: x86 instruction decoder build-time selftest
    On Thu, Jul 16, 2009 at 11:57:06AM -0400, Masami Hiramatsu wrote:
    > Add a user-space selftest of x86 instruction decoder at kernel build time.
    > When CONFIG_X86_DECODER_SELFTEST=y, Kbuild builds a test harness of x86
    > instruction decoder and performs it after building vmlinux.
    > The test compares the results of objdump and x86 instruction decoder
    > code and check there are no differences.

    Long overdue review from my side...

    > arch/x86/scripts/Makefile | 19 +++++++
    > arch/x86/scripts/distill.awk | 42 +++++++++++++++++
    > arch/x86/scripts/test_get_len.c | 99 +++++++++++++++++++++++++++++++++++++++
    > arch/x86/scripts/user_include.h | 49 +++++++++++++++++++

    Hmmm, we have two architectures that uses scripts/ and three that
    uses tools/.
    I prefer the latter name as what we have ere is beyound what
    I generally recognize as a script.

    we have scripts/ in top-level and we do not rename this
    as we have this hardcoded too many places - but no reason to
    use the wrong name here.

    > diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
    > index 01e079a..9090665 100644
    > --- a/arch/x86/include/asm/inat.h
    > +++ b/arch/x86/include/asm/inat.h
    > @@ -20,7 +20,9 @@
    > * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
    > *
    > */
    > +#ifdef __KERNEL__
    > #include <linux/types.h>
    > +#endif
    >
    > /* Instruction attributes */
    > typedef u32 insn_attr_t;

    Why this?
    If you need this to use this file from userspace then could we do some
    other trick to make this OK?

    I see it repeated several times below.
    [If this has already been discussed I have missed it - sorry].


    > diff --git a/arch/x86/scripts/Makefile b/arch/x86/scripts/Makefile
    > new file mode 100644
    > index 0000000..f08859e
    > --- /dev/null
    > +++ b/arch/x86/scripts/Makefile
    > @@ -0,0 +1,19 @@
    > +PHONY += posttest
    > +quiet_cmd_posttest = TEST $@
    > + cmd_posttest = objdump -d $(objtree)/vmlinux | awk -f $(srctree)/arch/x86/scripts/distill.awk | $(obj)/test_get_len
    > +

    You are using the native objdump here.
    But I assume this fails miserably when you build x86 on a powerpc host.
    In other words - you broke an allyesconfig build for -next...
    We have $(OBJDUMP) for this.

    > +posttest: $(obj)/test_get_len vmlinux
    > + $(call cmd,posttest)
    > +
    > +test_get_len_SRC = $(srctree)/arch/x86/scripts/test_get_len.c $(srctree)/arch/x86/lib/insn.c $(srctree)/arch/x86/lib/inat.c
    > +test_get_len_INC = $(srctree)/arch/x86/include/asm/inat.h $(srctree)/arch/x86/include/asm/insn.h $(objtree)/arch/x86/lib/inat-tables.c
    > +
    > +quiet_cmd_test_get_len = CC $@
    > + cmd_test_get_len = $(CC) -Wall $(test_get_len_SRC) -I$(objtree)/arch/x86/lib/ -I$(srctree)/arch/x86/include -include $(srctree)/arch/x86/scripts/user_include.h -o $@

    Is there a specific reason why you cannot use the standard hostprogs-y for this?
    It will take care of dependency tracking etc.
    What you have above is a hopeless incomplete list of dependencies.

    You need to use HOST_EXTRACFLAGS to set additional -I options and the -include.

    > +
    > +static void usage()
    > +{
    > + fprintf(stderr, "usage: %s < distilled_disassembly\n", prog);
    > + exit(1);
    > +}

    It would be nice to tell the user what the program is supposed to do.
    I know this is a bit unusual but no reason to copy bad practice.

    > index 0000000..3bdcc55
    > --- /dev/null
    > +++ b/arch/x86/scripts/user_include.h
    > @@ -0,0 +1,49 @@
    > +#ifndef __USER_TYPES_H
    > +#define __USER_TYPES_H
    > +
    > +/*
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License as published by
    > + * the Free Software Foundation; either version 2 of the License, or
    > + * (at your option) any later version.
    > + *
    > + * This program is distributed in the hope that it will be useful,
    > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + *
    > + * You should have received a copy of the GNU General Public License
    > + * along with this program; if not, write to the Free Software
    > + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
    > + *
    > + * Copyright (C) IBM Corporation, 2009
    > + */
    > +
    > +#include <string.h>
    > +
    > +#ifdef __x86_64__
    > +#define CONFIG_X86_64
    > +#else
    > +#define CONFIG_X86_32
    > +#endif
    > +typedef unsigned char u8;
    > +typedef unsigned short u16;
    > +typedef unsigned int u32;
    > +typedef unsigned long long u64;
    > +
    > +typedef signed char s8;
    > +typedef short s16;
    > +typedef int s32;
    > +typedef long long s64;
    > +
    > +typedef enum bool { false = 0, true } bool;
    > +
    > +/* any harmless file-scope decl */
    > +#define NOP_DECL struct __nop
    > +#define EXPORT_SYMBOL_GPL(symbol) NOP_DECL
    > +#define MODULE_LICENSE(gpl) NOP_DECL
    > +
    > +#define WARN_ON(cond) do { } while (0)
    > +#define unlikely(cond) (cond)
    > +

    So this is a file that alows you to include the other files without dragging in
    a massive amount a stuff.
    Would be nice if you wrote so in the file so it is explicit.


    I tried to look at inat.h to see why all this was needed but tht
    file is not present in my tree - neither in -tip?

    Sam


    \
     
     \ /
      Last update: 2009-07-16 18:31    [from the cache]
    ©2003-2011 Jasper Spaans. Advertise on this site