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

[Alistair Popple]

On Wednesday, 7 March 2018 4:35:24 PM AEDT Cyril Bur wrote:
> 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.

Thanks for the reminder :-)

> 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 {
>