Re: [RFC PATCH 1/2] Preserve datatype information when parsing dts

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 16012 bytes --]

On Tue, Nov 28, 2017 at 11:57:09AM +0000, Grant Likely wrote:
> The current code throws away all the data type and grouping information
> when parsing the DTS source file, which makes it difficult to
> reconstruct the data format when emitting a format that can express data
> types (ie. dts and yaml). Use the marker list to mark the beginning and
> end of each integer array block (<> and []), the datatype contained in
> each (8, 16, 32 & 64 bit widths), and the start of each string.
> 
> At the same time, factor out the heuristic code used to guess a property
> type at emit time. It is a pretty well defined standalone block that
> could be used elsewhere, for instance, when emitting YAML source. Factor
> it out into a separate function so that it can be reused, and also to
> simplify the write_propval() function.
> 
> When emitting, group integer output back into the same groups as the
> original source and use the REF_PATH and REF_PHANDLE markers to emit the
> the node reference instead of a raw path or phandle.
> 
> Signed-off-by: Grant Likely <grant.likely-5wv7dgnIgG8@public.gmane.org>

I'm a bit dubious how well forcing the marker mechanism to do all this
stuff it was never intended for can work in the long term.  Still,
it's an interesting experiment.

> ---
>  data.c       |   4 +-
>  dtc-parser.y |  21 +++--
>  dtc.h        |   9 +++
>  livetree.c   |  49 ++++++++++++
>  treesource.c | 249 ++++++++++++++++++++++++++++++++---------------------------
>  5 files changed, 212 insertions(+), 120 deletions(-)
> 
> diff --git a/data.c b/data.c
> index aa37a16..0aef0b5 100644
> --- a/data.c
> +++ b/data.c
> @@ -74,7 +74,8 @@ struct data data_copy_escape_string(const char *s, int len)
>  	struct data d;
>  	char *q;
>  
> -	d = data_grow_for(empty_data, len + 1);
> +	d = data_add_marker(empty_data, MARKER_STRING, NULL);
> +	d = data_grow_for(d, len + 1);
>  
>  	q = d.val;
>  	while (i < len) {
> @@ -94,6 +95,7 @@ struct data data_copy_file(FILE *f, size_t maxlen)
>  {
>  	struct data d = empty_data;
>  
> +	d = data_add_marker(d, MARKER_BLOB, NULL);
>  	while (!feof(f) && (d.len < maxlen)) {
>  		size_t chunksize, ret;
>  
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 44af170..41fbd3f 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -266,11 +266,13 @@ propdata:
>  		}
>  	| propdataprefix arrayprefix '>'
>  		{
> -			$$ = data_merge($1, $2.data);
> +			$1 = data_merge($1, $2.data);
> +			$$ = data_add_marker($1, MARKER_NONE, NULL);
>  		}
>  	| propdataprefix '[' bytestring ']'
>  		{
> -			$$ = data_merge($1, $3);
> +			$1 = data_merge($1, $3);
> +			$$ = data_add_marker($1, MARKER_NONE, NULL);
>  		}
>  	| propdataprefix DT_REF
>  		{
> @@ -327,22 +329,27 @@ arrayprefix:
>  	DT_BITS DT_LITERAL '<'
>  		{
>  			unsigned long long bits;
> +			enum markertype type = MARKER_UINT32;
>  
>  			bits = $2;
>  
> -			if ((bits !=  8) && (bits != 16) &&
> -			    (bits != 32) && (bits != 64)) {
> +			switch (bits) {
> +			case 8: type = MARKER_UINT8; break;
> +			case 16: type = MARKER_UINT16; break;
> +			case 32: type = MARKER_UINT32; break;
> +			case 64: type = MARKER_UINT64; break;
> +			default:
>  				ERROR(&@2, "Array elements must be"
>  				      " 8, 16, 32 or 64-bits");
>  				bits = 32;
>  			}
>  
> -			$$.data = empty_data;
> +			$$.data = data_add_marker(empty_data, type, NULL);
>  			$$.bits = bits;
>  		}
>  	| '<'
>  		{
> -			$$.data = empty_data;
> +			$$.data = data_add_marker(empty_data, MARKER_UINT32, NULL);
>  			$$.bits = 32;
>  		}
>  	| arrayprefix integer_prim
> @@ -486,7 +493,7 @@ integer_unary:
>  bytestring:
>  	  /* empty */
>  		{
> -			$$ = empty_data;
> +			$$ = data_add_marker(empty_data, MARKER_UINT8, NULL);
>  		}
>  	| bytestring DT_BYTE
>  		{
> diff --git a/dtc.h b/dtc.h
> index 3b18a42..32a7655 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -74,9 +74,17 @@ typedef uint32_t cell_t;
>  
>  /* Data blobs */
>  enum markertype {
> +	MARKER_NONE = 0,
>  	REF_PHANDLE,
>  	REF_PATH,
>  	LABEL,
> +	MARKER_UINT8,
> +	MARKER_UINT16,
> +	MARKER_UINT32,
> +	MARKER_UINT64,
> +	MARKER_BLOB,
> +	MARKER_STRING,
> +	NUM_MARKERS,
>  };
>  
>  struct  marker {
> @@ -198,6 +206,7 @@ struct property *build_property(char *name, struct data val);
>  struct property *build_property_delete(char *name);
>  struct property *chain_property(struct property *first, struct property *list);
>  struct property *reverse_properties(struct property *first);
> +enum markertype guess_propval_type(struct property *prop);
>  
>  struct node *build_node(struct property *proplist, struct node *children);
>  struct node *build_node_delete(void);
> diff --git a/livetree.c b/livetree.c
> index 57b7db2..1bbdaf8 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -336,6 +336,55 @@ void append_to_property(struct node *node,
>  	}
>  }
>  
> +static bool isstring(char c)
> +{
> +	return (isprint((unsigned char)c)
> +		|| (c == '\0')
> +		|| strchr("\a\b\t\n\v\f\r", c));
> +}
> +
> +enum markertype guess_propval_type(struct property *prop)
> +{
> +	int len = prop->val.len;
> +	const char *p = prop->val.val;
> +	struct marker *m = prop->val.markers;
> +	int nnotstring = 0, nnul = 0;
> +	int nnotstringlbl = 0, nnotcelllbl = 0;
> +	int i;
> +
> +	/* Check if the property is type annotated */
> +	while (m && (m->type == LABEL || m->type == REF_PHANDLE))
> +		m = m->next;
> +	if (m && m->offset == 0)
> +		return MARKER_NONE;
> +	m = prop->val.markers;
> +
> +	/* data type information missing, need to guess */
> +	for (i = 0; i < len; i++) {
> +		if (! isstring(p[i]))
> +			nnotstring++;
> +		if (p[i] == '\0')
> +			nnul++;
> +	}
> +
> +	for_each_marker_of_type(m, LABEL) {
> +		if ((m->offset > 0) && (prop->val.val[m->offset - 1] != '\0'))
> +			nnotstringlbl++;
> +		if ((m->offset % sizeof(cell_t)) != 0)
> +			nnotcelllbl++;
> +	}
> +
> +	if ((p[len-1] == '\0') && (nnotstring == 0) && (nnul < (len-nnul))
> +	    && (nnotstringlbl == 0)) {
> +		return MARKER_STRING;
> +	} else if (((len % sizeof(cell_t)) == 0) && (nnotcelllbl == 0)) {
> +		return MARKER_UINT32;
> +	} else {
> +		return MARKER_UINT8;
> +	}
> +}
> +
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>  {
>  	struct reserve_info *new = xmalloc(sizeof(*new));
> diff --git a/treesource.c b/treesource.c
> index 2461a3d..cc3b223 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -54,29 +54,15 @@ static void write_prefix(FILE *f, int level)
>  		fputc('\t', f);
>  }
>  
> -static bool isstring(char c)
> +static void write_propval_string(FILE *f, const char *str, size_t len)
>  {
> -	return (isprint((unsigned char)c)
> -		|| (c == '\0')
> -		|| strchr("\a\b\t\n\v\f\r", c));
> -}
> -
> -static void write_propval_string(FILE *f, struct data val)
> -{
> -	const char *str = val.val;
>  	int i;
> -	struct marker *m = val.markers;
>  
> -	assert(str[val.len-1] == '\0');
> +	assert(str[len-1] == '\0');
>  
> -	while (m && (m->offset == 0)) {
> -		if (m->type == LABEL)
> -			fprintf(f, "%s: ", m->ref);
> -		m = m->next;
> -	}
>  	fprintf(f, "\"");
>  
> -	for (i = 0; i < (val.len-1); i++) {
> +	for (i = 0; i < (len-1); i++) {
>  		char c = str[i];
>  
>  		switch (c) {
> @@ -108,15 +94,7 @@ static void write_propval_string(FILE *f, struct data val)
>  			fprintf(f, "\\\"");
>  			break;
>  		case '\0':
> -			fprintf(f, "\", ");
> -			while (m && (m->offset <= (i + 1))) {
> -				if (m->type == LABEL) {
> -					assert(m->offset == (i+1));
> -					fprintf(f, "%s: ", m->ref);
> -				}
> -				m = m->next;
> -			}
> -			fprintf(f, "\"");
> +			fprintf(f, "\", \"");
>  			break;
>  		default:
>  			if (isprint((unsigned char)c))
> @@ -126,114 +104,161 @@ static void write_propval_string(FILE *f, struct data val)
>  		}
>  	}
>  	fprintf(f, "\"");
> -
> -	/* Wrap up any labels at the end of the value */
> -	for_each_marker_of_type(m, LABEL) {
> -		assert (m->offset == val.len);
> -		fprintf(f, " %s:", m->ref);
> -	}
>  }
>  
> -static void write_propval_cells(FILE *f, struct data val)
> +static void write_propval_ref(FILE *f, struct node *np, const char *hint)
>  {
> -	void *propend = val.val + val.len;
> -	fdt32_t *cp = (fdt32_t *)val.val;
> -	struct marker *m = val.markers;
> -
> -	fprintf(f, "<");
> -	for (;;) {
> -		while (m && (m->offset <= ((char *)cp - val.val))) {
> -			if (m->type == LABEL) {
> -				assert(m->offset == ((char *)cp - val.val));
> -				fprintf(f, "%s: ", m->ref);
> -			}
> -			m = m->next;
> -		}
> +	struct label *l;
> +	struct node *root = np;
>  
> -		fprintf(f, "0x%x", fdt32_to_cpu(*cp++));
> -		if ((void *)cp >= propend)
> -			break;
> -		fprintf(f, " ");
> -	}
> +	while (root->parent)
> +		root = root->parent;
>  
> -	/* Wrap up any labels at the end of the value */
> -	for_each_marker_of_type(m, LABEL) {
> -		assert (m->offset == val.len);
> -		fprintf(f, " %s:", m->ref);
> +	/* If possible, use the hint string as the reference */
> +	if (hint && np == get_node_by_ref(root, hint)) {
> +		fprintf(f, hint[0] == '/' ? " &{%s}" : " &%s", hint);
> +		return;
>  	}
> -	fprintf(f, ">");
> -}
> -
> -static void write_propval_bytes(FILE *f, struct data val)
> -{
> -	void *propend = val.val + val.len;
> -	const char *bp = val.val;
> -	struct marker *m = val.markers;
> -
> -	fprintf(f, "[");
> -	for (;;) {
> -		while (m && (m->offset == (bp-val.val))) {
> -			if (m->type == LABEL)
> -				fprintf(f, "%s: ", m->ref);
> -			m = m->next;
> -		}
>  
> -		fprintf(f, "%02hhx", (unsigned char)(*bp++));
> -		if ((const void *)bp >= propend)
> -			break;
> -		fprintf(f, " ");
> +	/* Check if the node has a usable label */
> +	for_each_label(np->labels, l) {
> +		fprintf(f, " &%s", l->label);
> +		return;
>  	}
>  
> -	/* Wrap up any labels at the end of the value */
> -	for_each_marker_of_type(m, LABEL) {
> -		assert (m->offset == val.len);
> -		fprintf(f, " %s:", m->ref);
> -	}
> -	fprintf(f, "]");
> +	fprintf(f, " &{%s}", np->fullpath);
>  }
>  
> -static void write_propval(FILE *f, struct property *prop)
> +const char * delim_start[NUM_MARKERS] = {
> +	[MARKER_BLOB] = "[",
> +	[MARKER_UINT8] = "[",
> +	[MARKER_UINT16] = "/bits/ 16 <",
> +	[MARKER_UINT32] = "<",
> +	[MARKER_UINT64] = "/bits/ 64 <",
> +};
> +const char * delim_end[NUM_MARKERS] = {
> +	[MARKER_BLOB] = " ]",
> +	[MARKER_UINT8] = " ]",
> +	[MARKER_UINT16] = " >",
> +	[MARKER_UINT32] = " >",
> +	[MARKER_UINT64] = " >",
> +};
> +static void write_propval(FILE *f, struct node *root, struct property *prop)
>  {
> -	int len = prop->val.len;
> -	const char *p = prop->val.val;
> +	size_t len = prop->val.len;
> +	size_t chunk_len;
>  	struct marker *m = prop->val.markers;
> -	int nnotstring = 0, nnul = 0;
> -	int nnotstringlbl = 0, nnotcelllbl = 0;
> -	int i;
> +	struct marker dummy_marker = {
> +		.offset = 0, .type = MARKER_NONE, .next = m, .ref = NULL
> +	};
> +	enum markertype emit_type = MARKER_NONE;
> +	struct node *np;
> +	cell_t phandle;
>  
>  	if (len == 0) {
>  		fprintf(f, ";\n");
>  		return;
>  	}
>  
> -	for (i = 0; i < len; i++) {
> -		if (! isstring(p[i]))
> -			nnotstring++;
> -		if (p[i] == '\0')
> -			nnul++;
> -	}
> +	fprintf(f, " = ");
>  
> -	for_each_marker_of_type(m, LABEL) {
> -		if ((m->offset > 0) && (prop->val.val[m->offset - 1] != '\0'))
> -			nnotstringlbl++;
> -		if ((m->offset % sizeof(cell_t)) != 0)
> -			nnotcelllbl++;
> -	}
> +	dummy_marker.type = guess_propval_type(prop);
> +	if (dummy_marker.type != MARKER_NONE)
> +		m = &dummy_marker;
>  
> -	fprintf(f, " = ");
> -	if ((p[len-1] == '\0') && (nnotstring == 0) && (nnul < (len-nnul))
> -	    && (nnotstringlbl == 0)) {
> -		write_propval_string(f, prop->val);
> -	} else if (((len % sizeof(cell_t)) == 0) && (nnotcelllbl == 0)) {
> -		write_propval_cells(f, prop->val);
> -	} else {
> -		write_propval_bytes(f, prop->val);
> +	for_each_marker(m) {
> +		chunk_len = (m->next ? m->next->offset : len) - m->offset;
> +		const char *p = &prop->val.val[m->offset];
> +		const char *end = p + chunk_len;
> +
> +		switch(m->type) {
> +		case MARKER_NONE:
> +			if (emit_type != MARKER_NONE)
> +				fprintf(f, "%s", delim_end[emit_type]);
> +			emit_type = m->type;
> +			break;
> +		case MARKER_STRING:
> +		case MARKER_BLOB:
> +		case MARKER_UINT8:
> +		case MARKER_UINT16:
> +		case MARKER_UINT32:
> +		case MARKER_UINT64:
> +			emit_type = m->type;
> +			fprintf(f, m->offset ? ", %s" : "%s",
> +			           delim_start[emit_type] ? : "");
> +			break;
> +		case LABEL:
> +			fprintf(f, " %s:", m->ref);
> +			break;
> +		case REF_PHANDLE:
> +			assert(emit_type == MARKER_UINT32);
> +			assert(chunk_len >= sizeof(fdt32_t));
> +			phandle = fdt32_to_cpu(*(const fdt32_t*)p);
> +			np = get_node_by_phandle(root, phandle);
> +			if (np) {
> +				write_propval_ref(f, np, NULL);
> +				chunk_len -= sizeof(fdt32_t);
> +				p += sizeof(fdt32_t);
> +			}
> +			break;
> +		case REF_PATH:
> +			assert(emit_type == MARKER_NONE);
> +			assert(strnlen(p, chunk_len) == chunk_len - 1);
> +			np = get_node_by_ref(root, p);
> +			if (np) {
> +				write_propval_ref(f, np, m->ref);
> +				p += chunk_len;
> +				chunk_len = 0;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (chunk_len <= 0)
> +			continue;
> +
> +		switch (emit_type) {
> +		case MARKER_BLOB:
> +		case MARKER_UINT8:
> +			for (; p < end; p++)
> +				fprintf(f, " %02"PRIx8, *(const uint8_t *)p);
> +			break;
> +		case MARKER_UINT16:
> +			assert((chunk_len % sizeof(fdt16_t)) == 0);
> +			for (; p < end; p += sizeof(fdt16_t))
> +				fprintf(f, " 0x%02"PRIx16,
> +				        fdt16_to_cpu(*(const fdt16_t*)p));
> +			break;
> +		case MARKER_UINT32:
> +			assert((chunk_len % sizeof(fdt32_t)) == 0);
> +			for (; p < end; p += sizeof(fdt32_t))
> +				fprintf(f, " 0x%02"PRIx32,
> +				        fdt32_to_cpu(*(const fdt32_t*)p));
> +			break;
> +		case MARKER_UINT64:
> +			assert((chunk_len % sizeof(fdt64_t)) == 0);
> +			for (; p < end; p += sizeof(fdt64_t))
> +				fprintf(f, " 0x%02"PRIx64,
> +				        fdt64_to_cpu(*(const fdt64_t*)p));
> +			break;
> +		case MARKER_STRING:
> +			assert(p[chunk_len-1] == '\0');
> +			write_propval_string(f, p, chunk_len);
> +			break;
> +		default:
> +			/* Default to a block of raw bytes */
> +			fprintf(f, "[ ");
> +			for (; p < end; p++)
> +				fprintf(f, " %02"PRIx8, *p);
> +			fprintf(f, "]");
> +		}
>  	}
>  
> -	fprintf(f, ";\n");
> +	fprintf(f, "%s;\n", delim_end[emit_type] ? : "");
>  }
>  
> -static void write_tree_source_node(FILE *f, struct node *tree, int level)
> +static void write_tree_source_node(FILE *f, struct node *root, struct node *tree, int level)
>  {
>  	struct property *prop;
>  	struct node *child;
> @@ -252,11 +277,11 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  		for_each_label(prop->labels, l)
>  			fprintf(f, "%s: ", l->label);
>  		fprintf(f, "%s", prop->name);
> -		write_propval(f, prop);
> +		write_propval(f, root, prop);
>  	}
>  	for_each_child(tree, child) {
>  		fprintf(f, "\n");
> -		write_tree_source_node(f, child, level+1);
> +		write_tree_source_node(f, root, child, level+1);
>  	}
>  	write_prefix(f, level);
>  	fprintf(f, "};\n");
> @@ -279,6 +304,6 @@ void dt_to_source(FILE *f, struct dt_info *dti)
>  			(unsigned long long)re->size);
>  	}
>  
> -	write_tree_source_node(f, dti->dt, 0);
> +	write_tree_source_node(f, dti->dt, dti->dt, 0);
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]