[Pdbg] [PATCH 28/29] main: Attempt to automatically discover target

[Alistair Popple]

So this was implemented a little differently to what I was originally thinking
which was pure runtime checking, but doing it at build time works and should
allow us to do some more optimisations like not linking unneeded device trees.

Couple of comments below though...
  
>  pdbg_SOURCES = \
>  	src/main.c src/cfam.c src/scom.c src/reg.c src/mem.c src/thread.c \
> -	src/htm.c
> +	src/htm.c src/options.c
> +
>  pdbg_LDADD = fake.dtb.o p8-fsi.dtb.o p8-i2c.dtb.o p9w-fsi.dtb.o	p8-host.dtb.o \
>  	p9z-fsi.dtb.o p9r-fsi.dtb.o p9-kernel.dtb.o libpdbg.la libfdt.la \
>  	p9-host.dtb.o \
> diff --git a/configure.ac b/configure.ac
> index 43348d7..7fb4393 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -8,4 +8,12 @@ AC_CONFIG_MACRO_DIR([m4])
>  AC_CONFIG_HEADERS([config.h])
>  AC_CONFIG_FILES([Makefile])
>  AC_LANG(C)
> +
> +case "$host" in
> +	arm*-*-*)       ARCH="arm" ;;
> +	powerpc*-*-*)   ARCH="ppc" ;;
> +	*)              ARCH="def" ;;
> +esac
> +AC_CONFIG_LINKS([src/options.c:src/options_$ARCH.c])

I'm not a big fan of creating symlinks to select which source files to build. It
always seems to have a way of going wrong, such as when building out-of-tree
which a lot of the various build systems that consume pdbg do (eg. yocto).

I wonder if it would be better to do something like:

AC_SUBST([ARCH])

and in the Makefile:

> +	src/htm.c src/options_ at ARCH@.c

Instead? This would also allow fairly easy selective linking of the device
trees.

>  
> +	if (!backend_is_possible(backend)) {
> +		fprintf(stderr, "Backend not possible\n");
> +		/* Probs say something about bad backend */

It would be really nice if we could print the possible backends, although I
guess if the auto-detection code works properly you should never see this unless
a user has manually specified things in which case tough I guess.

> +		print_usage(argv[0]);
> +		return 1;
> +	}
> +
> +	if (!target_is_possible(backend, device_node)) {
> +		fprintf(stderr, "Target not possible\n");

ditto.

> +		/* Probs say something about bad target */
> +		print_usage(argv[0]);
> +		return 1;
> +	}
> +
>  	if (optind >= argc) {
>  		print_usage(argv[0]);
>  		return 1;

> +	/*
> +	 * "This should never be the default" - Apopple 2017
> +	 * Unfortunately if both of those files don't exist we don't
> +	 * know what to do, we'll have to try this and hope
> +	 */
> +	fprintf(stderr, "Couldn't locate a good backend, using fallback!\n");
> +	return FSI;

aPOPPLE still doesn't like this in 2018 :-)

I think we'd be better off just failing with an error message hinting that a
user might like to try manually specifying the FSI backend. The problem with
just trying FSI is that it's important the right target is selected
(Witherspoon, Romulus, etc.) and if the autodetection is failing here why would
it work later? Although I guess we could fail later instead but there is still a
chance the FSI backend could leave the system in an unstable state.

> +const char *default_target(enum backend backend)
> +{
> +	FILE *dt_compatible;
> +	char line[256];
> +
> +	if (backend == I2C || backend == KERNEL) /* No target nessesary */
> +		return NULL;
> +
> +	dt_compatible = fopen("/proc/device-tree/compatible", "r");
> +	if (!dt_compatible)
> +		return NULL;
> +
> +	fgets(line, sizeof(line), dt_compatible);
> +	fclose(dt_compatible);
> +
> +	if (strstr(witherspoon, line))
> +		return witherspoon;
> +
> +	if (strstr(romulus, line))
> +		return romulus;
> +
> +	if (strstr(zaius, line))
> +		return zaius;
> +
> +	return NULL;
> +}
> +
> +bool target_is_possible(enum backend backend, const char *target)
> +{
> +	const char *def;
> +
> +	if (!backend_is_possible(backend))
> +		return false;
> +
> +	def = default_target(backend);
> +	return strcmp(def, target) == 0;

Of course we probably will detect failure to find a default target.
Unfortunately a segfault due to a NULL dereference is generally not considered
the nicest way of telling the user that :-P

> +const char *default_target(enum backend backend)
> +{
> +	const char *pos = NULL;
> +	char line[256];
> +	FILE *cpuinfo;
> +
> +	cpuinfo = fopen("/proc/cpuinfo", "r");
> +	if (!cpuinfo)
> +		return NULL;
> +
> +	while (fgets(line, sizeof(line), cpuinfo))
> +		if (strncmp(line, "cpu", 3) == 0)
> +			break;
> +	fclose(cpuinfo);
> +
> +	pos = strchr(line, ':');
> +	if (!pos)
> +		return NULL;

Is this sufficient to catch the case when "cpu" is not found before EOF?

- Alistair

> +
> +	if (*(pos + 1) == '\0')
> +		return NULL;
> +	pos += 2;
> +
> +	if (strncmp(pos, "POWER8", 6) == 0)
> +		return p8;
> +
> +	if (strncmp(pos, "POWER9", 6) == 0)
> +		return p9;
> +
> +	return NULL;
> +}
> +
> +bool target_is_possible(enum backend backend, const char *target)
> +{
> +	const char *def;
> +
> +	if (!backend_is_possible(backend))
> +		return false;
> +
> +	def = default_target(backend);
> +	return strcmp(def, target) == 0;
> +}
>