[Crash-utility] [PATCH v3 1/4] [ppc] Support PPC32 Core analysis on PPC64

Mon Jan 9 20:13:00 UTC 2012

Hi Suzuki,

I've got a few issues with this patch-set, as inlined below:

----- Original Message -----
> This patch enables crash on PPC64 to analyze the cores generated
> on a PPC32 machine. I plan to look at adding support for PPC32 Kdump
> core in the following patches. Running crash on a PPC32 board is not
> a good thing to do. Hence using a PPC64 machine to do the same.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki at in.ibm.com>
> ---
> 
>  Makefile        |    6 +++---
>  configure.c     |   14 +++++++++++++-
>  gdb-7.3.1.patch |    4 ++--
>  netdump.c       |    6 ++++++
>  4 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 923844e..eee5aee 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -27,7 +27,7 @@ TARGET=
>  GDB_CONF_FLAGS=
>  
>  ARCH := $(shell uname -m | sed -e s/i.86/i386/ -e s/sun4u/sparc64/
>  -e s/arm.*/arm/ -e s/sa110/arm/)
> -ifeq ($(ARCH), ppc64)
> +ifeq (${TARGET}, ppc64)
>  CONF_FLAGS = -m64
>  endif

I understand what you're attempting to do, but as written I don't
seen how this can possibly work.  The TARGET will be either
uppercase "PPC" or "PPC64", so CONF_FLAGS will never be set 
to -m64.  In fact, I just tested it doing a ppc64 native build, 
and it doesn't pass the -m64 to the configure.c build.

> @@ -256,8 +256,8 @@ gdb_merge: force
>  	@if [ ! -f ${GDB}/config.status ]; then \
>  	  (cd ${GDB}; ./configure ${GDB_CONF_FLAGS}
>  	  --with-separate-debug-dir=/usr/lib/debug \
>  	    --with-bugurl="" --with-expat=no --with-python=no; \
> -	  make --no-print-directory; echo ${TARGET} > crash.target) \
> -	else (cd ${GDB}/gdb; make --no-print-directory;); fi
> +	  make --no-print-directory TARGET=${TARGET}; echo ${TARGET} > crash.target) \
> +	else (cd ${GDB}/gdb; make --no-print-directory TARGET=${TARGET};); fi
>  	@if [ ! -f ${GDB}/gdb/libgdb.a ]; then \
>  	  echo; echo "gdb build failed: ${GDB}/gdb/libgdb.a does not
>  	  exist"; \
>  	  echo; exit 1; fi

I'd prefer not to use "TARGET" as the Makefile define being passed 
to the gdb build process.  It's just too common a name, and I don't 
want to run the risk that it interferes with anything built below it.

Can you change it to something that's guaranteed to be unique throughout
the whole build process?  You know, something like "CRASH_CROSS_ARCH_TARGET",
and change gdb-7.3.1/Makefile.in accordingly?  

Thanks,
  Dave

> diff --git a/configure.c b/configure.c
> index 00e7bb2..a1858a4 100755
> --- a/configure.c
> +++ b/configure.c
> @@ -134,11 +134,13 @@ char *get_extra_flags(char *);
>  #define TARGET_CFLAGS_ARM_ON_X86
>      "TARGET_CFLAGS=-D_FILE_OFFSET_BITS=64"
>  #define TARGET_CFLAGS_ARM_ON_X86_64  "TARGET_CFLAGS=-m32
>  -D_FILE_OFFSET_BITS=64"
>  #define TARGET_CFLAGS_X86_ON_X86_64  "TARGET_CFLAGS=-m32
>  -D_FILE_OFFSET_BITS=64"
> +#define TARGET_CFLAGS_PPC_ON_PPC64   "TARGET_CFLAGS=-m32
> -D_FILE_OFFSET_BITS=64 -fPIC"
>  
>  #define GDB_TARGET_DEFAULT        "GDB_CONF_FLAGS="
>  #define GDB_TARGET_ARM_ON_X86
>      "GDB_CONF_FLAGS=--target=arm-elf-linux"
>  #define GDB_TARGET_ARM_ON_X86_64
>   "GDB_CONF_FLAGS=--target=arm-elf-linux CFLAGS=-m32"
>  #define GDB_TARGET_X86_ON_X86_64
>   "GDB_CONF_FLAGS=--target=i686-pc-linux-gnu CFLAGS=-m32"
> +#define GDB_TARGET_PPC_ON_PPC64
>   "GDB_CONF_FLAGS=--target=ppc-elf-linux CFLAGS=-m32"
>  
>  /*
>   *  The original plan was to allow the use of a particular version
> @@ -361,6 +363,12 @@ get_current_configuration(struct
> supported_gdb_version *sp)
>  			 *  Build an X86 crash binary on an X86_64 host.
>  			 */
>  			target_data.target = X86;
> +		} else if ((target_data.target == PPC64) &&
> +			(name_to_target((char *)target_data.target_as_param) == PPC)) {
> +			/*
> +			 *  Build an PPC crash binary on an PPC64 host.
> +			 */
> +			target_data.target = PPC;
>  		} else if (name_to_target((char *)target_data.target_as_param) ==
>  			target_data.host) {
>  			if ((target_data.initial_gdb_target != UNKNOWN) &&
> @@ -556,7 +564,11 @@ build_configure(struct supported_gdb_version
> *sp)
>  		break;
>  	case PPC:
>  		target = TARGET_PPC;
> -		target_CFLAGS = TARGET_CFLAGS_PPC;
> +		if (target_data.host == PPC64) {
> +                        target_CFLAGS = TARGET_CFLAGS_PPC_ON_PPC64;
> +			gdb_conf_flags = GDB_TARGET_PPC_ON_PPC64;
> +		} else
> +			target_CFLAGS = TARGET_CFLAGS_PPC;
>  		break;
>  	case IA64:
>  		target = TARGET_IA64;
> diff --git a/gdb-7.3.1.patch b/gdb-7.3.1.patch
> index 013ed8f..7a8d824 100644
> --- a/gdb-7.3.1.patch
> +++ b/gdb-7.3.1.patch
> @@ -1209,7 +1209,7 @@
>   AS_FOR_BUILD = @AS_FOR_BUILD@
>   CC_FOR_BUILD = @CC_FOR_BUILD@
>   CFLAGS_FOR_BUILD = @CFLAGS_FOR_BUILD@
> -+ifeq ($(shell arch), ppc64)
> ++ifeq (${TARGET}, ppc64)
>  +CFLAGS_FOR_BUILD += -m64 -fPIC
>  +endif
>   CXXFLAGS_FOR_BUILD = @CXXFLAGS_FOR_BUILD@
> @@ -1219,7 +1219,7 @@
>   GNATMAKE = @GNATMAKE@
>   
>   CFLAGS = @CFLAGS@
> -+ifeq ($(shell arch), ppc64)
> ++ifeq (${TARGET}, ppc64)
>  +CFLAGS += -m64 -fPIC
>  +endif
>   LDFLAGS = @LDFLAGS@
> diff --git a/netdump.c b/netdump.c
> index 4011f36..c0ee9ae 100644
> --- a/netdump.c
> +++ b/netdump.c
> @@ -183,6 +183,12 @@ is_netdump(char *file, ulong source_query)
>  				goto bailout;
>  			break;
>  
> +		case EM_PPC:
> +			if (machine_type_mismatch(file, "PPC", NULL,
> +			    source_query))
> +				goto bailout;
> +			break;
> +
>  		default:
>  			if (machine_type_mismatch(file, "(unknown)", NULL,
>  			    source_query))
> 
> --
> Crash-utility mailing list
> Crash-utility at redhat.com
> https://www.redhat.com/mailman/listinfo/crash-utility
> 