[Pdbg] [PATCH 1/4] libpdbg: Validate hardware units at build time

[Cyril Bur]

I'll send this to the list otherwise I'll forget.

Al and I decided to send our Core HTM patches and get them merged
first, I'll rebase this series onto those.

I don't expect any other changes in v2 unless someone wants to jump in.


On Tue, 2018-02-27 at 19:23 +1100, Cyril Bur wrote:
> Hardware unit structs such as struct htm contain a struct pdbg_target
> which can be passed around to create an abstraction. Currently there is
> a bit of extra runtime code used to guard against developers not
> declaring the struct pdbg_target member as the first member. If we can
> enforce this at build time then a some runtime can be removed.
> 
> This patch enforces that the struct pdbg_target member of hardware
> units is always the first member at build time.
> 
> The only caveat is that compiling with -O0 will result in a false
> negative - the build succeeding when it should not. We should be using
> litterally any other optimisation level so that isn't really a problem.
> 
> Signed-off-by: Cyril Bur <cyrilbur at gmail.com>
> ---
>  Makefile.am      |  2 +-
>  libpdbg/target.c | 10 ++++------
>  libpdbg/target.h | 16 ++++++++++++----
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 4156b21..099a035 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -8,7 +8,7 @@ include libfdt/Makefile.libfdt
>  bin_PROGRAMS = pdbg
>  
>  ACLOCAL_AMFLAGS = -Im4
> -AM_CFLAGS = -I$(top_srcdir)/ccan/array_size -Wall -Werror
> +AM_CFLAGS = -I$(top_srcdir)/ccan/array_size -Wall -Werror -O2
>  
>  pdbg_SOURCES = \
>  	src/main.c src/cfam.c src/scom.c src/reg.c src/mem.c src/thread.c \
> diff --git a/libpdbg/target.c b/libpdbg/target.c
> index 1eb85bb..07159d2 100644
> --- a/libpdbg/target.c
> +++ b/libpdbg/target.c
> @@ -254,7 +254,7 @@ struct hw_unit_info *find_compatible_target(const char *compat)
>  	struct pdbg_target *target;
>  
>  	for (p = &__start_hw_units; p < (struct hw_unit_info **) &__stop_hw_units; p++) {
> -		target = (*p)->hw_unit + (*p)->struct_target_offset;
> +		target = (*p)->hw_unit;
>  		if (!strcmp(target->compatible, compat))
>  			return *p;
>  	}
> @@ -268,7 +268,6 @@ void pdbg_targets_init(void *fdt)
>  	const struct dt_property *p;
>  	struct pdbg_target_class *target_class;
>  	struct hw_unit_info *hw_unit_info;
> -	void *new_hw_unit;
>  	struct pdbg_target *new_target;
>  	uint32_t index;
>  
> @@ -283,10 +282,9 @@ void pdbg_targets_init(void *fdt)
>  		hw_unit_info = find_compatible_target(p->prop);
>  		if (hw_unit_info) {
>  			/* We need to allocate a new target */
> -			new_hw_unit = malloc(hw_unit_info->size);
> -			assert(new_hw_unit);
> -			memcpy(new_hw_unit, hw_unit_info->hw_unit, hw_unit_info->size);
> -			new_target = new_hw_unit + hw_unit_info->struct_target_offset;
> +			new_target = malloc(hw_unit_info->size);
> +			assert(new_target);
> +			memcpy(new_target, hw_unit_info->hw_unit, hw_unit_info->size);
>  			new_target->dn = dn;
>  			dn->target = new_target;
>  			index = dt_prop_get_u32_def(dn, "index", -1);
> diff --git a/libpdbg/target.h b/libpdbg/target.h
> index 0a618e2..2bc7dba 100644
> --- a/libpdbg/target.h
> +++ b/libpdbg/target.h
> @@ -18,6 +18,7 @@
>  
>  #include <stdint.h>
>  #include <ccan/list/list.h>
> +#include <ccan/str/str.h>
>  #include <ccan/container_of/container_of.h>
>  #include "compiler.h"
>  #include "device.h"
> @@ -60,16 +61,23 @@ extern struct list_head empty_list;
>  struct hw_unit_info {
>  	void *hw_unit;
>  	size_t size;
> -	size_t struct_target_offset;
>  };
>  
>  /* We can't pack the structs themselves directly into a special
>   * section because there doesn't seem to be any standard way of doing
>   * that due to alignment rules. So instead we pack pointers into a
> - * special section. */
> + * special section.
> + *
> + * If this macro fails to compile for you, you've probably not
> + * declared the struct pdbg_target as the first member of the
> + * container struct. Not doing so will break other assumptions.
> + * */
>  #define DECLARE_HW_UNIT(name)						\
> -	const struct hw_unit_info __used name ##_hw_unit = \
> -	{ .hw_unit = &name, .size = sizeof(name), .struct_target_offset = container_off(typeof(name), target) }; \
> +	static inline void name ##_hw_unit_check(void) {		\
> +		((void)sizeof(char[1 - 2 * (container_off(typeof(name), target) != 0)])); \
> +	}								\
> +	const struct hw_unit_info __used name ##_hw_unit =              \
> +	{ .hw_unit = &name, .size = sizeof(name) }; 	                \
>  	const struct hw_unit_info __used __section("hw_units") *name ##_hw_unit_p = &name ##_hw_unit
>  
>  struct htm {
> -- 
> 2.16.2
>