[PATCH v4 0/3] Support character literals

[PATCH v4 0/3] Support character literals

[Anton Staaf]

    These patches add simple and escaped character literal parsing support
to the dtc for cell lists and bytestrings.  The first patch refactors the
string parsing code in data.c to expose the more primitive character parsing
code for use in the later patches.  I have left the bytestring support in
place but my hope is that it is now separate enough to be discussed
independantly of the refactor and cell list changes.

    Thanks,
        Anton

Changes in v4:
- Make character literal regex more permissive
- Check for empty character literal in eval_char_literal
- Allow lexing of character literals in all contexts
- Remove special case lexing of character literals in bytestrings

Changes in v3:
- Make octal and hex parsing code static to util.c
- Add test cases for both cell lists and bytestings w/ character literals
- Better error handling and simpler lexing
- Removed changes to out of date manual.txt
- Simplify use of get_escape_char in data_copy_escape_string
- Remove get_escape_char_exact

Changes in v2:
- Move the refactor of data.c to a separate patch
- Add support for character literals in cell lists
- Merge normal and escaped literal support for bytestrings into single patch

Anton Staaf (3):
  dtc: Refactor character literal parsing code
  dtc: Support character literals in cell lists
  dtc: Support character literals in bytestrings

 Documentation/dts-format.txt |    7 ++-
 data.c                       |   85 ++----------------------------------
 dtc-lexer.l                  |    8 +++
 dtc-parser.y                 |   36 +++++++++++++++
 tests/.gitignore             |    1 +
 tests/Makefile.tests         |    1 +
 tests/char_literal.c         |   58 ++++++++++++++++++++++++
 tests/char_literal.dts       |    6 +++
 tests/run_tests.sh           |    3 +
 tests/testdata.h             |    6 +++
 util.c                       |   99 ++++++++++++++++++++++++++++++++++++++++++
 util.h                       |    8 +++
 12 files changed, 234 insertions(+), 84 deletions(-)
 create mode 100644 tests/char_literal.c
 create mode 100644 tests/char_literal.dts

-- 
1.7.3.1

[PATCH v4 1/3] dtc: Refactor character literal parsing code

[Anton Staaf]

Move the parsing of hex, octal and escaped characters from data.c
to util.c where it can be used for character literal parsing within
strings as well as for stand alone C style character literals.

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
---
 data.c |   85 ++----------------------------------------------------
 util.c |   99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 util.h |    8 +++++
 3 files changed, 111 insertions(+), 81 deletions(-)

diff --git a/data.c b/data.c
index fe555e8..b5f3066 100644
--- a/data.c
+++ b/data.c
@@ -68,40 +68,6 @@ struct data data_copy_mem(const char *mem, int len)
 	return d;
 }
 
-static char get_oct_char(const char *s, int *i)
-{
-	char x[4];
-	char *endx;
-	long val;
-
-	x[3] = '\0';
-	strncpy(x, s + *i, 3);
-
-	val = strtol(x, &endx, 8);
-
-	assert(endx > x);
-
-	(*i) += endx - x;
-	return val;
-}
-
-static char get_hex_char(const char *s, int *i)
-{
-	char x[3];
-	char *endx;
-	long val;
-
-	x[2] = '\0';
-	strncpy(x, s + *i, 2);
-
-	val = strtol(x, &endx, 16);
-	if (!(endx  > x))
-		die("\\x used with no following hex digits\n");
-
-	(*i) += endx - x;
-	return val;
-}
-
 struct data data_copy_escape_string(const char *s, int len)
 {
 	int i = 0;
@@ -114,53 +80,10 @@ struct data data_copy_escape_string(const char *s, int len)
 	while (i < len) {
 		char c = s[i++];
 
-		if (c != '\\') {
-			q[d.len++] = c;
-			continue;
-		}
-
-		c = s[i++];
-		assert(c);
-		switch (c) {
-		case 'a':
-			q[d.len++] = '\a';
-			break;
-		case 'b':
-			q[d.len++] = '\b';
-			break;
-		case 't':
-			q[d.len++] = '\t';
-			break;
-		case 'n':
-			q[d.len++] = '\n';
-			break;
-		case 'v':
-			q[d.len++] = '\v';
-			break;
-		case 'f':
-			q[d.len++] = '\f';
-			break;
-		case 'r':
-			q[d.len++] = '\r';
-			break;
-		case '0':
-		case '1':
-		case '2':
-		case '3':
-		case '4':
-		case '5':
-		case '6':
-		case '7':
-			i--; /* need to re-read the first digit as
-			      * part of the octal value */
-			q[d.len++] = get_oct_char(s, &i);
-			break;
-		case 'x':
-			q[d.len++] = get_hex_char(s, &i);
-			break;
-		default:
-			q[d.len++] = c;
-		}
+		if (c == '\\')
+			c = get_escape_char(s, &i);
+
+		q[d.len++] = c;
 	}
 
 	q[d.len++] = '\0';
diff --git a/util.c b/util.c
index 994436f..6d07292 100644
--- a/util.c
+++ b/util.c
@@ -25,6 +25,7 @@
 #include <stdlib.h>
 #include <stdarg.h>
 #include <string.h>
+#include <assert.h>
 
 #include "util.h"
 
@@ -85,3 +86,101 @@ int util_is_printable_string(const void *data, int len)
 
 	return 1;
 }
+
+/*
+ * Parse a octal encoded character starting at index i in string s.  The
+ * resulting character will be returned and the index i will be updated to
+ * point at the character directly after the end of the encoding, this may be
+ * the '\0' terminator of the string.
+ */
+static char get_oct_char(const char *s, int *i)
+{
+	char x[4];
+	char *endx;
+	long val;
+
+	x[3] = '\0';
+	strncpy(x, s + *i, 3);
+
+	val = strtol(x, &endx, 8);
+
+	assert(endx > x);
+
+	(*i) += endx - x;
+	return val;
+}
+
+/*
+ * Parse a hexadecimal encoded character starting at index i in string s.  The
+ * resulting character will be returned and the index i will be updated to
+ * point at the character directly after the end of the encoding, this may be
+ * the '\0' terminator of the string.
+ */
+static char get_hex_char(const char *s, int *i)
+{
+	char x[3];
+	char *endx;
+	long val;
+
+	x[2] = '\0';
+	strncpy(x, s + *i, 2);
+
+	val = strtol(x, &endx, 16);
+	if (!(endx  > x))
+		die("\\x used with no following hex digits\n");
+
+	(*i) += endx - x;
+	return val;
+}
+
+char get_escape_char(const char *s, int *i)
+{
+	char	c = s[*i];
+	int	j = *i + 1;
+	char	val;
+
+	assert(c);
+	switch (c) {
+	case 'a':
+		val = '\a';
+		break;
+	case 'b':
+		val = '\b';
+		break;
+	case 't':
+		val = '\t';
+		break;
+	case 'n':
+		val = '\n';
+		break;
+	case 'v':
+		val = '\v';
+		break;
+	case 'f':
+		val = '\f';
+		break;
+	case 'r':
+		val = '\r';
+		break;
+	case '0':
+	case '1':
+	case '2':
+	case '3':
+	case '4':
+	case '5':
+	case '6':
+	case '7':
+		j--; /* need to re-read the first digit as
+		      * part of the octal value */
+		val = get_oct_char(s, &j);
+		break;
+	case 'x':
+		val = get_hex_char(s, &j);
+		break;
+	default:
+		val = c;
+	}
+
+	(*i) = j;
+	return val;
+}
diff --git a/util.h b/util.h
index cc68933..f251480 100644
--- a/util.h
+++ b/util.h
@@ -64,4 +64,12 @@ extern char *join_path(const char *path, const char *name);
  * @return 1 if a valid printable string, 0 if not */
 int util_is_printable_string(const void *data, int len);
 
+/*
+ * Parse an escaped character starting at index i in string s.  The resulting
+ * character will be returned and the index i will be updated to point at the
+ * character directly after the end of the encoding, this may be the '\0'
+ * terminator of the string.
+ */
+char get_escape_char(const char *s, int *i);
+
 #endif /* _UTIL_H */
-- 
1.7.3.1

Re: [PATCH v4 1/3] dtc: Refactor character literal parsing code

[Jon Loeliger]

> Move the parsing of hex, octal and escaped characters from data.c
> to util.c where it can be used for character literal parsing within
> strings as well as for stand alone C style character literals.
> 
> Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
> Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Applied.  Thanks.

jdl

[PATCH v4 2/3] dtc: Support character literals in cell lists

[Anton Staaf]

With this patch the following property assignment:

    property = <0x12345678 'a' '\r' 100>;

is equivalent to:

    property = <0x12345678 0x00000061 0x0000000D 0x00000064>

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
---
 Documentation/dts-format.txt |    2 +-
 dtc-lexer.l                  |    8 ++++++
 dtc-parser.y                 |   32 ++++++++++++++++++++++++++
 tests/.gitignore             |    1 +
 tests/Makefile.tests         |    1 +
 tests/char_literal.c         |   50 ++++++++++++++++++++++++++++++++++++++++++
 tests/char_literal.dts       |    5 ++++
 tests/run_tests.sh           |    3 ++
 tests/testdata.h             |    6 +++++
 9 files changed, 107 insertions(+), 1 deletions(-)
 create mode 100644 tests/char_literal.c
 create mode 100644 tests/char_literal.dts

diff --git a/Documentation/dts-format.txt b/Documentation/dts-format.txt
index a655b87..eae8b76 100644
--- a/Documentation/dts-format.txt
+++ b/Documentation/dts-format.txt
@@ -33,7 +33,7 @@ Property values may be defined as an array of 32-bit integer cells, as
 NUL-terminated strings, as bytestrings or a combination of these.
 
 * Arrays of cells are represented by angle brackets surrounding a
-  space separated list of C-style integers
+  space separated list of C-style integers or character literals.
 
 	e.g. interrupts = <17 0xc>;
 
diff --git a/dtc-lexer.l b/dtc-lexer.l
index e866ea5..494e342 100644
--- a/dtc-lexer.l
+++ b/dtc-lexer.l
@@ -29,6 +29,7 @@ PROPNODECHAR	[a-zA-Z0-9,._+*#?@-]
 PATHCHAR	({PROPNODECHAR}|[/])
 LABEL		[a-zA-Z_][a-zA-Z0-9_]*
 STRING		\"([^\\"]|\\.)*\"
+CHAR_LITERAL	'([^']|\\')*'
 WS		[[:space:]]
 COMMENT		"/*"([^*]|\*+[^*/])*\*+"/"
 LINECOMMENT	"//".*\n
@@ -109,6 +110,13 @@ static int pop_input_file(void);
 			return DT_LITERAL;
 		}
 
+<*>{CHAR_LITERAL}	{
+			yytext[yyleng-1] = '\0';
+			yylval.literal = xstrdup(yytext+1);
+			DPRINT("Character literal: %s\n", yylval.literal);
+			return DT_CHAR_LITERAL;
+		}
+
 <*>\&{LABEL}	{	/* label reference */
 			DPRINT("Ref: %s\n", yytext+1);
 			yylval.labelref = xstrdup(yytext+1);
diff --git a/dtc-parser.y b/dtc-parser.y
index 5e84a67..554f11a 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -34,6 +34,7 @@ extern struct boot_info *the_boot_info;
 extern int treesource_error;
 
 static unsigned long long eval_literal(const char *s, int base, int bits);
+static unsigned char eval_char_literal(const char *s);
 %}
 
 %union {
@@ -57,6 +58,7 @@ static unsigned long long eval_literal(const char *s, int base, int bits);
 %token DT_MEMRESERVE
 %token <propnodename> DT_PROPNODENAME
 %token <literal> DT_LITERAL
+%token <literal> DT_CHAR_LITERAL
 %token <cbase> DT_BASE
 %token <byte> DT_BYTE
 %token <data> DT_STRING
@@ -265,6 +267,10 @@ cellval:
 		{
 			$$ = eval_literal($1, 0, 32);
 		}
+	| DT_CHAR_LITERAL
+		{
+			$$ = eval_char_literal($1);
+		}
 	;
 
 bytestring:
@@ -343,3 +349,29 @@ static unsigned long long eval_literal(const char *s, int base, int bits)
 		print_error("bad literal");
 	return val;
 }
+
+static unsigned char eval_char_literal(const char *s)
+{
+	int i = 1;
+	char c = s[0];
+
+	if (c == '\0')
+	{
+		print_error("empty character literal");
+		return 0;
+	}
+
+	/*
+	 * If the first character in the character literal is a \ then process
+	 * the remaining characters as an escape encoding. If the first
+	 * character is neither an escape or a terminator it should be the only
+	 * character in the literal and will be returned.
+	 */
+	if (c == '\\')
+		c = get_escape_char(s, &i);
+
+	if (s[i] != '\0')
+		print_error("malformed character literal");
+
+	return c;
+}
diff --git a/tests/.gitignore b/tests/.gitignore
index f4e58b2..a3e9bd1 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -4,6 +4,7 @@
 /add_subnode_with_nops
 /asm_tree_dump
 /boot-cpuid
+/char_literal
 /del_node
 /del_property
 /dtbs_equal_ordered
diff --git a/tests/Makefile.tests b/tests/Makefile.tests
index c564e72..e718b63 100644
--- a/tests/Makefile.tests
+++ b/tests/Makefile.tests
@@ -5,6 +5,7 @@ LIB_TESTS_L = get_mem_rsv \
 	node_offset_by_prop_value node_offset_by_phandle \
 	node_check_compatible node_offset_by_compatible \
 	get_alias \
+	char_literal \
 	notfound \
 	setprop_inplace nop_property nop_node \
 	sw_tree1 \
diff --git a/tests/char_literal.c b/tests/char_literal.c
new file mode 100644
index 0000000..150f2a0
--- /dev/null
+++ b/tests/char_literal.c
@@ -0,0 +1,50 @@
+/*
+ * libfdt - Flat Device Tree manipulation
+ *	Testcase for character literals in dtc
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ * Copyright (C) 2011 The Chromium Authors. All rights reserved.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public License
+ * as published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "tests.h"
+#include "testdata.h"
+
+int main(int argc, char *argv[])
+{
+	void *fdt;
+	uint32_t expected_cells[5];
+
+	expected_cells[0] = cpu_to_fdt32((unsigned char)TEST_CHAR1);
+	expected_cells[1] = cpu_to_fdt32((unsigned char)TEST_CHAR2);
+	expected_cells[2] = cpu_to_fdt32((unsigned char)TEST_CHAR3);
+	expected_cells[3] = cpu_to_fdt32((unsigned char)TEST_CHAR4);
+	expected_cells[4] = cpu_to_fdt32((unsigned char)TEST_CHAR5);
+
+	test_init(argc, argv);
+	fdt = load_blob_arg(argc, argv);
+
+	check_getprop(fdt, 0, "char-literal-cells",
+		      sizeof(expected_cells), expected_cells);
+
+	PASS();
+}
diff --git a/tests/char_literal.dts b/tests/char_literal.dts
new file mode 100644
index 0000000..22e17ed
--- /dev/null
+++ b/tests/char_literal.dts
@@ -0,0 +1,5 @@
+/dts-v1/;
+
+/ {
+	char-literal-cells = <'\r' 'b' '\0' '\'' '\xff'>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 72dda32..1246df1 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -206,6 +206,9 @@ dtc_tests () {
     run_dtc_test -I dts -O dtb -o dtc_escapes.test.dtb escapes.dts
     run_test string_escapes dtc_escapes.test.dtb
 
+    run_dtc_test -I dts -O dtb -o dtc_char_literal.test.dtb char_literal.dts
+    run_test char_literal dtc_char_literal.test.dtb
+
     run_dtc_test -I dts -O dtb -o dtc_extra-terminating-null.test.dtb extra-terminating-null.dts
     run_test extra-terminating-null dtc_extra-terminating-null.test.dtb
 
diff --git a/tests/testdata.h b/tests/testdata.h
index 5b5a9a3..d4c6759 100644
--- a/tests/testdata.h
+++ b/tests/testdata.h
@@ -19,6 +19,12 @@
 #define TEST_STRING_2	"nastystring: \a\b\t\n\v\f\r\\\""
 #define TEST_STRING_3	"\xde\xad\xbe\xef"
 
+#define TEST_CHAR1	'\r'
+#define TEST_CHAR2	'b'
+#define TEST_CHAR3	'\0'
+#define TEST_CHAR4	'\''
+#define TEST_CHAR5	'\xff'
+
 #ifndef __ASSEMBLY__
 extern struct fdt_header _test_tree1;
 extern struct fdt_header _truncated_property;
-- 
1.7.3.1

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[David Gibson]

On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> With this patch the following property assignment:
> 
>     property = <0x12345678 'a' '\r' 100>;
> 
> is equivalent to:
> 
>     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> 
> Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Jon Loeliger]

> On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> > With this patch the following property assignment:
> > 
> >     property = <0x12345678 'a' '\r' 100>;
> > 
> > is equivalent to:
> > 
> >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> > 
> > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

So, I *think* we want to wait until the question of size
is resolved some more, right?  Or, take this in any event
as "without a type indicator they are all 32-bit values"?

jdl

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[David Gibson]

On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> > > With this patch the following property assignment:
> > > 
> > >     property = <0x12345678 'a' '\r' 100>;
> > > 
> > > is equivalent to:
> > > 
> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> > > 
> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> So, I *think* we want to wait until the question of size
> is resolved some more, right?  Or, take this in any event
> as "without a type indicator they are all 32-bit values"?

No this patch is fine to take without changing the cell size
semantics.  It's just that it becomes a lot more useful when we do get
those.

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Anton Staaf]

On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
>> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
>> > > With this patch the following property assignment:
>> > >
>> > >     property = <0x12345678 'a' '\r' 100>;
>> > >
>> > > is equivalent to:
>> > >
>> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
>> > >
>> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >
>> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>
>> So, I *think* we want to wait until the question of size
>> is resolved some more, right?  Or, take this in any event
>> as "without a type indicator they are all 32-bit values"?
>
> No this patch is fine to take without changing the cell size
> semantics.  It's just that it becomes a lot more useful when we do get
> those.

Yup, I'm working on a size patch by the way.  Any comments on my
previous post about it would be helpful.  But in the mean time I'm
going ahead with a solution where the current cell size is stored in
the "struct data" type references are not allowed in cell lists of
size other than 32 bits.

Thanks,
    Anton

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[David Gibson]

On Mon, Sep 19, 2011 at 09:45:34AM -0700, Anton Staaf wrote:
> On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
> >> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> >> > > With this patch the following property assignment:
> >> > >
> >> > >     property = <0x12345678 'a' '\r' 100>;
> >> > >
> >> > > is equivalent to:
> >> > >
> >> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> >> > >
> >> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >
> >> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> >>
> >> So, I *think* we want to wait until the question of size
> >> is resolved some more, right?  Or, take this in any event
> >> as "without a type indicator they are all 32-bit values"?
> >
> > No this patch is fine to take without changing the cell size
> > semantics.  It's just that it becomes a lot more useful when we do get
> > those.
> 
> Yup, I'm working on a size patch by the way.  Any comments on my
> previous post about it would be helpful.  But in the mean time I'm
> going ahead with a solution where the current cell size is stored in
> the "struct data" type references are not allowed in cell lists of
> size other than 32 bits.

Ah, sorry, I meant to give comments on that earlier but got
sidetracked.

Storing the cell size in struct data doesn't really work - a single
property could be assembled from several cell lists of different
sizes.  By the time the reference substitution happens, they will have
been all merged into a single struct data.

I think prohibiting cell references anywhere but 32-bit cell lists is
the right approach, but we need to work out a way to do the check
during the parse phase.

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Anton Staaf]

On Mon, Sep 19, 2011 at 5:59 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Sep 19, 2011 at 09:45:34AM -0700, Anton Staaf wrote:
>> On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
>> >> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
>> >> > > With this patch the following property assignment:
>> >> > >
>> >> > >     property = <0x12345678 'a' '\r' 100>;
>> >> > >
>> >> > > is equivalent to:
>> >> > >
>> >> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
>> >> > >
>> >> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >> >
>> >> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>> >>
>> >> So, I *think* we want to wait until the question of size
>> >> is resolved some more, right?  Or, take this in any event
>> >> as "without a type indicator they are all 32-bit values"?
>> >
>> > No this patch is fine to take without changing the cell size
>> > semantics.  It's just that it becomes a lot more useful when we do get
>> > those.
>>
>> Yup, I'm working on a size patch by the way.  Any comments on my
>> previous post about it would be helpful.  But in the mean time I'm
>> going ahead with a solution where the current cell size is stored in
>> the "struct data" type references are not allowed in cell lists of
>> size other than 32 bits.
>
> Ah, sorry, I meant to give comments on that earlier but got
> sidetracked.
>
> Storing the cell size in struct data doesn't really work - a single
> property could be assembled from several cell lists of different
> sizes.  By the time the reference substitution happens, they will have
> been all merged into a single struct data.
>
> I think prohibiting cell references anywhere but 32-bit cell lists is
> the right approach, but we need to work out a way to do the check
> during the parse phase.

Yes absolutely.  My intent with storing the current cell size in the
data struct was to use that to do the parse time rejection of
references in all but 32-bit cell lists.  That is the current cell
size would not be used past parsing, for exactly the reason you
mention.  My thought was that every creation of a cell list would set
the current cell size to 32 or the value defined by /size/ and the
closing '>' would set it back to 0.  The empty_data initializer would
set the current cell size to 0, even though it should never be used
outside of the <...> context.  Then when a reference or literal are to
be appended to the current data struct we can check for the value 32
if it's a reference, and otherwise use the current cell size to
validate (ensure it fits in the current cell size) and pad (add
leading zero's) the literal.  Padding won't actually be required if
the literal parsing routine always returns a 64-bit value.

Thanks,
    Anton

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[David Gibson]

On Mon, Sep 19, 2011 at 07:54:09PM -0700, Anton Staaf wrote:
> On Mon, Sep 19, 2011 at 5:59 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Sep 19, 2011 at 09:45:34AM -0700, Anton Staaf wrote:
> >> On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
> >> >> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> >> >> > > With this patch the following property assignment:
> >> >> > >
> >> >> > >     property = <0x12345678 'a' '\r' 100>;
> >> >> > >
> >> >> > > is equivalent to:
> >> >> > >
> >> >> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> >> >> > >
> >> >> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >> >
> >> >> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> >> >>
> >> >> So, I *think* we want to wait until the question of size
> >> >> is resolved some more, right?  Or, take this in any event
> >> >> as "without a type indicator they are all 32-bit values"?
> >> >
> >> > No this patch is fine to take without changing the cell size
> >> > semantics.  It's just that it becomes a lot more useful when we do get
> >> > those.
> >>
> >> Yup, I'm working on a size patch by the way.  Any comments on my
> >> previous post about it would be helpful.  But in the mean time I'm
> >> going ahead with a solution where the current cell size is stored in
> >> the "struct data" type references are not allowed in cell lists of
> >> size other than 32 bits.
> >
> > Ah, sorry, I meant to give comments on that earlier but got
> > sidetracked.
> >
> > Storing the cell size in struct data doesn't really work - a single
> > property could be assembled from several cell lists of different
> > sizes.  By the time the reference substitution happens, they will have
> > been all merged into a single struct data.
> >
> > I think prohibiting cell references anywhere but 32-bit cell lists is
> > the right approach, but we need to work out a way to do the check
> > during the parse phase.
> 
> Yes absolutely.  My intent with storing the current cell size in the
> data struct was to use that to do the parse time rejection of
> references in all but 32-bit cell lists.  That is the current cell
> size would not be used past parsing, for exactly the reason you
> mention.  My thought was that every creation of a cell list would set
> the current cell size to 32 or the value defined by /size/ and the
> closing '>' would set it back to 0.  The empty_data initializer would
> set the current cell size to 0, even though it should never be used
> outside of the <...> context.  Then when a reference or literal are to
> be appended to the current data struct we can check for the value 32
> if it's a reference, and otherwise use the current cell size to
> validate (ensure it fits in the current cell size) and pad (add
> leading zero's) the literal.  Padding won't actually be required if
> the literal parsing routine always returns a 64-bit value.

Oh, I see.  I guess that would work, but it's a really nasty misuse of
a long-term data structure to store some short-term information.  No,
I'm pretty sure we can rearrange the grammer to handle this more
cleanly.

Maybe something like this:

cellarrayprefix = DT_SIZE DT_LITERAL '<'
                | cellarrayprefix cellval
		| cellarrayprefix DT_REF
		| cellarrayprefix DT_LABEL

The semantic type of cellarray prefix would need to contain both a
struct data and a int for the cell size, which is a bit fiddly, but
should be doable.  The DT_REF case can check the cell size part of $1
and error if it's no 32-bit.

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Anton Staaf]

On Mon, Sep 19, 2011 at 8:34 PM, David Gibson
<david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> On Mon, Sep 19, 2011 at 07:54:09PM -0700, Anton Staaf wrote:
>> On Mon, Sep 19, 2011 at 5:59 PM, David Gibson
>> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> > On Mon, Sep 19, 2011 at 09:45:34AM -0700, Anton Staaf wrote:
>> >> On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
>> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>> >> > On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
>> >> >> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
>> >> >> > > With this patch the following property assignment:
>> >> >> > >
>> >> >> > >     property = <0x12345678 'a' '\r' 100>;
>> >> >> > >
>> >> >> > > is equivalent to:
>> >> >> > >
>> >> >> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
>> >> >> > >
>> >> >> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >> >> >
>> >> >> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>> >> >>
>> >> >> So, I *think* we want to wait until the question of size
>> >> >> is resolved some more, right?  Or, take this in any event
>> >> >> as "without a type indicator they are all 32-bit values"?
>> >> >
>> >> > No this patch is fine to take without changing the cell size
>> >> > semantics.  It's just that it becomes a lot more useful when we do get
>> >> > those.
>> >>
>> >> Yup, I'm working on a size patch by the way.  Any comments on my
>> >> previous post about it would be helpful.  But in the mean time I'm
>> >> going ahead with a solution where the current cell size is stored in
>> >> the "struct data" type references are not allowed in cell lists of
>> >> size other than 32 bits.
>> >
>> > Ah, sorry, I meant to give comments on that earlier but got
>> > sidetracked.
>> >
>> > Storing the cell size in struct data doesn't really work - a single
>> > property could be assembled from several cell lists of different
>> > sizes.  By the time the reference substitution happens, they will have
>> > been all merged into a single struct data.
>> >
>> > I think prohibiting cell references anywhere but 32-bit cell lists is
>> > the right approach, but we need to work out a way to do the check
>> > during the parse phase.
>>
>> Yes absolutely.  My intent with storing the current cell size in the
>> data struct was to use that to do the parse time rejection of
>> references in all but 32-bit cell lists.  That is the current cell
>> size would not be used past parsing, for exactly the reason you
>> mention.  My thought was that every creation of a cell list would set
>> the current cell size to 32 or the value defined by /size/ and the
>> closing '>' would set it back to 0.  The empty_data initializer would
>> set the current cell size to 0, even though it should never be used
>> outside of the <...> context.  Then when a reference or literal are to
>> be appended to the current data struct we can check for the value 32
>> if it's a reference, and otherwise use the current cell size to
>> validate (ensure it fits in the current cell size) and pad (add
>> leading zero's) the literal.  Padding won't actually be required if
>> the literal parsing routine always returns a 64-bit value.
>
> Oh, I see.  I guess that would work, but it's a really nasty misuse of
> a long-term data structure to store some short-term information.  No,
> I'm pretty sure we can rearrange the grammer to handle this more
> cleanly.
>
> Maybe something like this:
>
> cellarrayprefix = DT_SIZE DT_LITERAL '<'
>                | cellarrayprefix cellval
>                | cellarrayprefix DT_REF
>                | cellarrayprefix DT_LABEL
>
> The semantic type of cellarray prefix would need to contain both a
> struct data and a int for the cell size, which is a bit fiddly, but
> should be doable.  The DT_REF case can check the cell size part of $1
> and error if it's no 32-bit.

I like this more than my solution.  I've written it up along with some
tests.  I'm now working on the dts format doc.

Should I submit this assuming that the character literal support in
cell lists will be accepted shortly?  Or should I remove the use of
character literals from my test case?  It would be simpler for me to
leave the patches based on top of the character literal in cell list
patch that has been acked but not submitted.  I won't base them on top
of the character literals in bytestring patch since that is not going
in without more discussion.  Also, would you prefer that I squash the
test cases into the commit with the variable sized cell support and
documentation changes?

Thanks,
    Anton

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[David Gibson]

On Wed, Sep 21, 2011 at 12:38:17PM -0700, Anton Staaf wrote:
> On Mon, Sep 19, 2011 at 8:34 PM, David Gibson
> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Mon, Sep 19, 2011 at 07:54:09PM -0700, Anton Staaf wrote:
> >> On Mon, Sep 19, 2011 at 5:59 PM, David Gibson
> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> > On Mon, Sep 19, 2011 at 09:45:34AM -0700, Anton Staaf wrote:
> >> >> On Sun, Sep 18, 2011 at 7:00 PM, David Gibson
> >> >> <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >> >> > On Sat, Sep 17, 2011 at 11:49:21AM -0500, Jon Loeliger wrote:
> >> >> >> > On Fri, Sep 09, 2011 at 12:16:30PM -0700, Anton Staaf wrote:
> >> >> >> > > With this patch the following property assignment:
> >> >> >> > >
> >> >> >> > >     property = <0x12345678 'a' '\r' 100>;
> >> >> >> > >
> >> >> >> > > is equivalent to:
> >> >> >> > >
> >> >> >> > >     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> >> >> >> > >
> >> >> >> > > Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> >> >> >
> >> >> >> > Acked-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> >> >> >>
> >> >> >> So, I *think* we want to wait until the question of size
> >> >> >> is resolved some more, right?  Or, take this in any event
> >> >> >> as "without a type indicator they are all 32-bit values"?
> >> >> >
> >> >> > No this patch is fine to take without changing the cell size
> >> >> > semantics.  It's just that it becomes a lot more useful when we do get
> >> >> > those.
> >> >>
> >> >> Yup, I'm working on a size patch by the way.  Any comments on my
> >> >> previous post about it would be helpful.  But in the mean time I'm
> >> >> going ahead with a solution where the current cell size is stored in
> >> >> the "struct data" type references are not allowed in cell lists of
> >> >> size other than 32 bits.
> >> >
> >> > Ah, sorry, I meant to give comments on that earlier but got
> >> > sidetracked.
> >> >
> >> > Storing the cell size in struct data doesn't really work - a single
> >> > property could be assembled from several cell lists of different
> >> > sizes.  By the time the reference substitution happens, they will have
> >> > been all merged into a single struct data.
> >> >
> >> > I think prohibiting cell references anywhere but 32-bit cell lists is
> >> > the right approach, but we need to work out a way to do the check
> >> > during the parse phase.
> >>
> >> Yes absolutely.  My intent with storing the current cell size in the
> >> data struct was to use that to do the parse time rejection of
> >> references in all but 32-bit cell lists.  That is the current cell
> >> size would not be used past parsing, for exactly the reason you
> >> mention.  My thought was that every creation of a cell list would set
> >> the current cell size to 32 or the value defined by /size/ and the
> >> closing '>' would set it back to 0.  The empty_data initializer would
> >> set the current cell size to 0, even though it should never be used
> >> outside of the <...> context.  Then when a reference or literal are to
> >> be appended to the current data struct we can check for the value 32
> >> if it's a reference, and otherwise use the current cell size to
> >> validate (ensure it fits in the current cell size) and pad (add
> >> leading zero's) the literal.  Padding won't actually be required if
> >> the literal parsing routine always returns a 64-bit value.
> >
> > Oh, I see.  I guess that would work, but it's a really nasty misuse of
> > a long-term data structure to store some short-term information.  No,
> > I'm pretty sure we can rearrange the grammer to handle this more
> > cleanly.
> >
> > Maybe something like this:
> >
> > cellarrayprefix = DT_SIZE DT_LITERAL '<'
> >                | cellarrayprefix cellval
> >                | cellarrayprefix DT_REF
> >                | cellarrayprefix DT_LABEL
> >
> > The semantic type of cellarray prefix would need to contain both a
> > struct data and a int for the cell size, which is a bit fiddly, but
> > should be doable.  The DT_REF case can check the cell size part of $1
> > and error if it's no 32-bit.
> 
> I like this more than my solution.  I've written it up along with some
> tests.  I'm now working on the dts format doc.
> 
> Should I submit this assuming that the character literal support in
> cell lists will be accepted shortly?

I'm in favour of applying the char literals in cell lists patch
immediately.  Jon?

>  Or should I remove the use of
> character literals from my test case?  It would be simpler for me to
> leave the patches based on top of the character literal in cell list
> patch that has been acked but not submitted.  I won't base them on top
> of the character literals in bytestring patch since that is not going
> in without more discussion.  Also, would you prefer that I squash the
> test cases into the commit with the variable sized cell support and
> documentation changes?

Yes, I generally prefer not to have tests patches segregated from the
things they test.

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

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Jon Loeliger]

> 
> I'm in favour of applying the char literals in cell lists patch
> immediately.  Jon?

Done.

jdl

Re: [PATCH v4 2/3] dtc: Support character literals in cell lists

[Jon Loeliger]

> With this patch the following property assignment:
> 
>     property = <0x12345678 'a' '\r' 100>;
> 
> is equivalent to:
> 
>     property = <0x12345678 0x00000061 0x0000000D 0x00000064>
> 
> Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
> Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Apllied.

Thanks,
jdl

[PATCH v4 3/3] dtc: Support character literals in bytestrings

[Anton Staaf]

With this patch the following property assignment:

    property = ['a' 2b '\r'];

is equivalent to:

    property = [61 2b 0d];

Signed-off-by: Anton Staaf <robotboy-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Jon Loeliger <jdl-CYoMK+44s/E@public.gmane.org>
Cc: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 Documentation/dts-format.txt |    5 +++--
 dtc-parser.y                 |    4 ++++
 tests/char_literal.c         |    8 ++++++++
 tests/char_literal.dts       |    1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/dts-format.txt b/Documentation/dts-format.txt
index eae8b76..555bd89 100644
--- a/Documentation/dts-format.txt
+++ b/Documentation/dts-format.txt
@@ -48,11 +48,12 @@ NUL-terminated strings, as bytestrings or a combination of these.
 	e.g. compatible = "simple-bus";
 
 * A bytestring is enclosed in square brackets [] with each byte
-  represented by two hexadecimal digits.  Spaces between each byte are
-  optional.
+  represented by two hexadecimal digits or a character literal.
+  Spaces between each byte or character literal are optional.
 
 	e.g. local-mac-address = [00 00 12 34 56 78]; or equivalently
 	     local-mac-address = [000012345678];
+	e.g. keymap = ['a' 'b' 'c' 'd'];
 
 * Values may have several comma-separated components, which are
   concatenated together.
diff --git a/dtc-parser.y b/dtc-parser.y
index 554f11a..bc05a24 100644
--- a/dtc-parser.y
+++ b/dtc-parser.y
@@ -286,6 +286,10 @@ bytestring:
 		{
 			$$ = data_add_marker($1, LABEL, $2);
 		}
+	| bytestring DT_CHAR_LITERAL
+		{
+			$$ = data_append_byte($1, eval_char_literal($2));
+		}
 	;
 
 subnodes:
diff --git a/tests/char_literal.c b/tests/char_literal.c
index 150f2a0..c6e2405 100644
--- a/tests/char_literal.c
+++ b/tests/char_literal.c
@@ -33,6 +33,11 @@ int main(int argc, char *argv[])
 {
 	void *fdt;
 	uint32_t expected_cells[5];
+	uint8_t expected_bytes[5] = {TEST_CHAR1,
+				     TEST_CHAR2,
+				     TEST_CHAR3,
+				     TEST_CHAR4,
+				     TEST_CHAR5};
 
 	expected_cells[0] = cpu_to_fdt32((unsigned char)TEST_CHAR1);
 	expected_cells[1] = cpu_to_fdt32((unsigned char)TEST_CHAR2);
@@ -46,5 +51,8 @@ int main(int argc, char *argv[])
 	check_getprop(fdt, 0, "char-literal-cells",
 		      sizeof(expected_cells), expected_cells);
 
+	check_getprop(fdt, 0, "char-literal-bytes",
+		      sizeof(expected_bytes), expected_bytes);
+
 	PASS();
 }
diff --git a/tests/char_literal.dts b/tests/char_literal.dts
index 22e17ed..9baba5c 100644
--- a/tests/char_literal.dts
+++ b/tests/char_literal.dts
@@ -2,4 +2,5 @@
 
 / {
 	char-literal-cells = <'\r' 'b' '\0' '\'' '\xff'>;
+	char-literal-bytes = ['\r' 'b' '\0' '\'' '\xff'];
 };
-- 
1.7.3.1