[0/5] dtc: srcpos, input handling cleanups

[0/5] dtc: srcpos, input handling cleanups

[David Gibson]

While working on the expression / macro / function support stuff, my
irritation with some uglies I've been meaning to clean up for ages
reached critical point.

So, here's a patch series cleaning up the input file handling, and the
srcpos / parser location tracking stuff.  This is a fusion of some
stuff I've had on the back-burner for a while with the srcpos related
pieces from jdl's interpreted language series.

I'm pretty happy with patches 1-4, but 5 probably needs a bit more
polish, and I'd like to add another patch adding column number
tracking and a cleaner way of doing line tracking.  But it's late and
I'd like to get what I have out for people to look at.

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

[1/5] dtc: Implement and use an xstrdup() function

[David Gibson]

Many places in dtc use strdup(), but none of them actually check the
return value to see if the implied allocation succeeded.  This is a
potential bug, which we fix in the patch below by replacing strdup()
with an xstrdup() which in analogy to xmalloc() will quit with a fatal
error if the allocation fails.

xstrdup() is defined in srcpos.c, because that's available to both dtc
itself and the conversion program which also uses it.  While we're at
it, we add standard double-include protection to srcpos.h which was
missing it.

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

---
 convert-dtsv0-lexer.l |    2 +-
 dtc-lexer.l           |   15 +++++++--------
 dtc-parser.y          |    1 -
 dtc.c                 |    1 -
 dtc.h                 |    2 ++
 flattree.c            |    7 +++----
 fstree.c              |    4 ++--
 srcpos.c              |   13 +++++++++++--
 srcpos.h              |    6 ++++++
 treesource.c          |    1 -
 10 files changed, 32 insertions(+), 20 deletions(-)

Index: dtc/convert-dtsv0-lexer.l
===================================================================
--- dtc.orig/convert-dtsv0-lexer.l	2008-10-03 00:03:36.000000000 +1000
+++ dtc/convert-dtsv0-lexer.l	2008-10-03 00:03:38.000000000 +1000
@@ -185,7 +185,7 @@ const struct {
 
 <PROPNODENAME>{PROPNODECHAR}+ {
 			ECHO;
-			last_name = strdup(yytext);
+			last_name = xstrdup(yytext);
 			BEGIN(INITIAL);
 		}
 
Index: dtc/dtc-lexer.l
===================================================================
--- dtc.orig/dtc-lexer.l	2008-10-03 00:03:36.000000000 +1000
+++ dtc/dtc-lexer.l	2008-10-03 00:03:38.000000000 +1000
@@ -35,7 +35,6 @@ LINECOMMENT	"//".*\n
 
 %{
 #include "dtc.h"
-#include "srcpos.h"
 #include "dtc-parser.tab.h"
 
 
@@ -105,7 +104,7 @@ static int pop_input_file(void);
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
 			DPRINT("Label: %s\n", yytext);
-			yylval.labelref = strdup(yytext);
+			yylval.labelref = xstrdup(yytext);
 			yylval.labelref[yyleng-1] = '\0';
 			return DT_LABEL;
 		}
@@ -128,7 +127,7 @@ static int pop_input_file(void);
 <INITIAL>[0-9a-fA-F]+	{
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
-			yylval.literal = strdup(yytext);
+			yylval.literal = xstrdup(yytext);
 			DPRINT("Literal: '%s'\n", yylval.literal);
 			return DT_LEGACYLITERAL;
 		}
@@ -136,7 +135,7 @@ static int pop_input_file(void);
 <V1>[0-9]+|0[xX][0-9a-fA-F]+      {
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
-			yylval.literal = strdup(yytext);
+			yylval.literal = xstrdup(yytext);
 			DPRINT("Literal: '%s'\n", yylval.literal);
 			return DT_LITERAL;
 		}
@@ -145,7 +144,7 @@ static int pop_input_file(void);
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
 			DPRINT("Ref: %s\n", yytext+1);
-			yylval.labelref = strdup(yytext+1);
+			yylval.labelref = xstrdup(yytext+1);
 			return DT_REF;
 		}
 
@@ -154,7 +153,7 @@ static int pop_input_file(void);
 			yylloc.first_line = yylineno;
 			yytext[yyleng-1] = '\0';
 			DPRINT("Ref: %s\n", yytext+2);
-			yylval.labelref = strdup(yytext+2);
+			yylval.labelref = xstrdup(yytext+2);
 			return DT_REF;
 		}
 
@@ -162,7 +161,7 @@ static int pop_input_file(void);
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
 			DPRINT("Ref: %s\n", yytext+1);
-			yylval.labelref = strdup(yytext+1);
+			yylval.labelref = xstrdup(yytext+1);
 			return DT_REF;
 		}
 
@@ -186,7 +185,7 @@ static int pop_input_file(void);
 			yylloc.file = srcpos_file;
 			yylloc.first_line = yylineno;
 			DPRINT("PropNodeName: %s\n", yytext);
-			yylval.propnodename = strdup(yytext);
+			yylval.propnodename = xstrdup(yytext);
 			BEGIN_DEFAULT();
 			return DT_PROPNODENAME;
 		}
Index: dtc/dtc-parser.y
===================================================================
--- dtc.orig/dtc-parser.y	2008-10-03 00:03:36.000000000 +1000
+++ dtc/dtc-parser.y	2008-10-03 00:03:38.000000000 +1000
@@ -24,7 +24,6 @@
 #include <stdio.h>
 
 #include "dtc.h"
-#include "srcpos.h"
 
 extern int yylex(void);
 
Index: dtc/dtc.c
===================================================================
--- dtc.orig/dtc.c	2008-10-03 00:03:36.000000000 +1000
+++ dtc/dtc.c	2008-10-03 00:03:38.000000000 +1000
@@ -19,7 +19,6 @@
  */
 
 #include "dtc.h"
-#include "srcpos.h"
 
 #include "version_gen.h"
 
Index: dtc/dtc.h
===================================================================
--- dtc.orig/dtc.h	2008-10-03 00:03:36.000000000 +1000
+++ dtc/dtc.h	2008-10-03 00:03:38.000000000 +1000
@@ -34,6 +34,8 @@
 #include <libfdt_env.h>
 #include <fdt.h>
 
+#include "srcpos.h"
+
 #define DEFAULT_FDT_VERSION	17
 /*
  * Command line options
Index: dtc/flattree.c
===================================================================
--- dtc.orig/flattree.c	2008-10-03 00:03:36.000000000 +1000
+++ dtc/flattree.c	2008-10-03 00:03:38.000000000 +1000
@@ -19,7 +19,6 @@
  */
 
 #include "dtc.h"
-#include "srcpos.h"
 
 #define FTF_FULLPATH	0x1
 #define FTF_VARALIGN	0x2
@@ -601,7 +600,7 @@ static char *flat_read_string(struct inb
 		len++;
 	} while ((*p++) != '\0');
 
-	str = strdup(inb->ptr);
+	str = xstrdup(inb->ptr);
 
 	inb->ptr += len;
 
@@ -643,7 +642,7 @@ static char *flat_read_stringtable(struc
 		p++;
 	}
 
-	return strdup(inb->base + offset);
+	return xstrdup(inb->base + offset);
 }
 
 static struct property *flat_read_property(struct inbuf *dtbuf,
@@ -710,7 +709,7 @@ static char *nodename_from_path(const ch
 	if (!streq(ppath, "/"))
 		plen++;
 
-	return strdup(cpath + plen);
+	return xstrdup(cpath + plen);
 }
 
 static struct node *unflatten_tree(struct inbuf *dtbuf,
Index: dtc/fstree.c
===================================================================
--- dtc.orig/fstree.c	2008-10-03 00:03:36.000000000 +1000
+++ dtc/fstree.c	2008-10-03 00:03:38.000000000 +1000
@@ -58,7 +58,7 @@ static struct node *read_fstree(const ch
 					"WARNING: Cannot open %s: %s\n",
 					tmpnam, strerror(errno));
 			} else {
-				prop = build_property(strdup(de->d_name),
+				prop = build_property(xstrdup(de->d_name),
 						      data_copy_file(pfile,
 								     st.st_size),
 						      NULL);
@@ -69,7 +69,7 @@ static struct node *read_fstree(const ch
 			struct node *newchild;
 
 			newchild = read_fstree(tmpnam);
-			newchild = name_node(newchild, strdup(de->d_name),
+			newchild = name_node(newchild, xstrdup(de->d_name),
 					     NULL);
 			add_child(tree, newchild);
 		}
Index: dtc/srcpos.c
===================================================================
--- dtc.orig/srcpos.c	2008-10-03 00:03:36.000000000 +1000
+++ dtc/srcpos.c	2008-10-03 00:03:38.000000000 +1000
@@ -20,6 +20,15 @@
 #include "dtc.h"
 #include "srcpos.h"
 
+char *xstrdup(const char *s)
+{
+	int len = strlen(s);
+	char *dup = xmalloc(len + 1);
+
+	memcpy(dup, s, len+1);
+	return dup;
+}
+
 /*
  * Like yylineno, this is the current open file pos.
  */
@@ -39,7 +48,7 @@ static int dtc_open_one(struct dtc_file 
 		strcat(fullname, "/");
 		strcat(fullname, fname);
 	} else {
-		fullname = strdup(fname);
+		fullname = xstrdup(fname);
 	}
 
 	file->file = fopen(fullname, "r");
@@ -85,7 +94,7 @@ struct dtc_file *dtc_open_file(const cha
 		if (!file->file)
 			goto fail;
 
-		file->name = strdup(fname);
+		file->name = xstrdup(fname);
 		return file;
 	}
 
Index: dtc/srcpos.h
===================================================================
--- dtc.orig/srcpos.h	2008-10-03 00:03:36.000000000 +1000
+++ dtc/srcpos.h	2008-10-03 00:03:38.000000000 +1000
@@ -1,3 +1,5 @@
+#ifndef _SRCPOS_H
+#define _SRCPOS_H
 /*
  * Copyright 2007 Jon Loeliger, Freescale Semiconductor, Inc.
  *
@@ -24,6 +26,8 @@
 
 #include <stdio.h>
 
+char *xstrdup(const char *s);
+
 struct dtc_file {
 	char *dir;
 	const char *name;
@@ -83,3 +87,5 @@ struct search_path {
 extern struct dtc_file *dtc_open_file(const char *fname,
                                       const struct search_path *search);
 extern void dtc_close_file(struct dtc_file *file);
+
+#endif /* _SRCPOS_H */
Index: dtc/treesource.c
===================================================================
--- dtc.orig/treesource.c	2008-10-03 00:03:36.000000000 +1000
+++ dtc/treesource.c	2008-10-03 00:03:38.000000000 +1000
@@ -19,7 +19,6 @@
  */
 
 #include "dtc.h"
-#include "srcpos.h"
 
 extern FILE *yyin;
 extern int yyparse(void);


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

[2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[David Gibson]

Current, every lexer rule starts with some boiler plate to update the
yylloc value for use by the parser.  One of the rules, even mistakenly
has a redundant allocation to one of the members.

This patch uses the flex YY_USER_ACTION macro hook, which is executed
before every rule to avoid this duplication.

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

---
 dtc-lexer.l |   36 +++++-------------------------------
 1 file changed, 5 insertions(+), 31 deletions(-)

Index: dtc/dtc-lexer.l
===================================================================
--- dtc.orig/dtc-lexer.l	2008-10-03 00:03:38.000000000 +1000
+++ dtc/dtc-lexer.l	2008-10-03 00:03:48.000000000 +1000
@@ -37,6 +37,11 @@ LINECOMMENT	"//".*\n
 #include "dtc.h"
 #include "dtc-parser.tab.h"
 
+#define	YY_USER_ACTION \
+	{ \
+		yylloc.file = srcpos_file; \
+		yylloc.first_line = yylineno; \
+	}
 
 /*#define LEXDEBUG	1*/
 
@@ -74,18 +79,13 @@ static int pop_input_file(void);
 		}
 
 <*>{STRING}	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("String: %s\n", yytext);
 			yylval.data = data_copy_escape_string(yytext+1,
 					yyleng-2);
-			yylloc.first_line = yylineno;
 			return DT_STRING;
 		}
 
 <*>"/dts-v1/"	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Keyword: /dts-v1/\n");
 			dts_version = 1;
 			BEGIN_DEFAULT();
@@ -93,16 +93,12 @@ static int pop_input_file(void);
 		}
 
 <*>"/memreserve/"	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Keyword: /memreserve/\n");
 			BEGIN_DEFAULT();
 			return DT_MEMRESERVE;
 		}
 
 <*>{LABEL}:	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Label: %s\n", yytext);
 			yylval.labelref = xstrdup(yytext);
 			yylval.labelref[yyleng-1] = '\0';
@@ -110,8 +106,6 @@ static int pop_input_file(void);
 		}
 
 <INITIAL>[bodh]# {
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			if (*yytext == 'b')
 				yylval.cbase = 2;
 			else if (*yytext == 'o')
@@ -125,32 +119,24 @@ static int pop_input_file(void);
 		}
 
 <INITIAL>[0-9a-fA-F]+	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			yylval.literal = xstrdup(yytext);
 			DPRINT("Literal: '%s'\n", yylval.literal);
 			return DT_LEGACYLITERAL;
 		}
 
 <V1>[0-9]+|0[xX][0-9a-fA-F]+      {
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			yylval.literal = xstrdup(yytext);
 			DPRINT("Literal: '%s'\n", yylval.literal);
 			return DT_LITERAL;
 		}
 
 \&{LABEL}	{	/* label reference */
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Ref: %s\n", yytext+1);
 			yylval.labelref = xstrdup(yytext+1);
 			return DT_REF;
 		}
 
 "&{/"{PATHCHAR}+\}	{	/* new-style path reference */
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			yytext[yyleng-1] = '\0';
 			DPRINT("Ref: %s\n", yytext+2);
 			yylval.labelref = xstrdup(yytext+2);
@@ -158,32 +144,24 @@ static int pop_input_file(void);
 		}
 
 <INITIAL>"&/"{PATHCHAR}+ {	/* old-style path reference */
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Ref: %s\n", yytext+1);
 			yylval.labelref = xstrdup(yytext+1);
 			return DT_REF;
 		}
 
 <BYTESTRING>[0-9a-fA-F]{2} {
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			yylval.byte = strtol(yytext, NULL, 16);
 			DPRINT("Byte: %02x\n", (int)yylval.byte);
 			return DT_BYTE;
 		}
 
 <BYTESTRING>"]"	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("/BYTESTRING\n");
 			BEGIN_DEFAULT();
 			return ']';
 		}
 
 <PROPNODENAME>{PROPNODECHAR}+ {
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("PropNodeName: %s\n", yytext);
 			yylval.propnodename = xstrdup(yytext);
 			BEGIN_DEFAULT();
@@ -191,8 +169,6 @@ static int pop_input_file(void);
 		}
 
 "/incbin/"	{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Binary Include\n");
 			return DT_INCBIN;
 		}
@@ -202,8 +178,6 @@ static int pop_input_file(void);
 <*>{LINECOMMENT}+ /* eat C++-style comments */
 
 <*>.		{
-			yylloc.file = srcpos_file;
-			yylloc.first_line = yylineno;
 			DPRINT("Char: %c (\\x%02x)\n", yytext[0],
 				(unsigned)yytext[0]);
 			if (yytext[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

[3/5] dtc: Cleanup yyerrorf() function

[David Gibson]

Currently, we put the source file name into the yylloc variable, but
never use the stored value.  Instead the yyerrorf() function directly
accesses srcpos_file to get the input file name.

That works in practice, but is likely not to always be correct if we
ever re-enable the glr-parser option.  Even now, its correctness
relies on the exact point in time bison executes the semantic rules
w.r.t. to the lexing rules, which is probably correct but not
obviously correct, which is far from ideal.

So, this patch replaces yyerrorf() with a srcpos_error() function
which pulls the filename information out of the yylloc variable, which
bison is explicitly supposed to get right for us.

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

---
 dtc-parser.y |   32 ++++++++++----------------------
 srcpos.c     |   13 +++++++++++++
 srcpos.h     |    6 ++----
 3 files changed, 25 insertions(+), 26 deletions(-)

Index: dtc/dtc-parser.y
===================================================================
--- dtc.orig/dtc-parser.y	2008-10-03 00:03:38.000000000 +1000
+++ dtc/dtc-parser.y	2008-10-03 00:03:57.000000000 +1000
@@ -30,6 +30,7 @@ extern int yylex(void);
 extern struct boot_info *the_boot_info;
 extern int treesource_error;
 
+static void yyerror(char const *s);
 static unsigned long long eval_literal(const char *s, int base, int bits);
 %}
 
@@ -206,10 +207,13 @@ propdata:
 			struct data d = empty_data;
 
 			if ($6 != 0)
-				if (fseek(file->file, $6, SEEK_SET) != 0)
-					yyerrorf("Couldn't seek to offset %llu in \"%s\": %s",
-						 (unsigned long long)$6,
-						 $4.val, strerror(errno));
+				if (fseek(file->file, $6, SEEK_SET) != 0) {
+					srcpos_error(&yylloc, "Couldn't seek to"
+						     " offset %llu in \"%s\": %s",
+						     (unsigned long long)$6,
+						     $4.val, strerror(errno));
+					treesource_error = 1;
+				}
 
 			d = data_copy_file(file->file, $8);
 
@@ -338,26 +342,10 @@ label:
 
 %%
 
-void yyerrorf(char const *s, ...)
+static void yyerror(char const *s)
 {
-	const char *fname = srcpos_file ? srcpos_file->name : "<no-file>";
-	va_list va;
-	va_start(va, s);
-
-	if (strcmp(fname, "-") == 0)
-		fname = "stdin";
-
-	fprintf(stderr, "%s:%d ", fname, yylloc.first_line);
-	vfprintf(stderr, s, va);
-	fprintf(stderr, "\n");
-
+	srcpos_error(&yylloc, "%s", s);
 	treesource_error = 1;
-	va_end(va);
-}
-
-void yyerror (char const *s)
-{
-	yyerrorf("%s", s);
 }
 
 static unsigned long long eval_literal(const char *s, int base, int bits)
Index: dtc/srcpos.c
===================================================================
--- dtc.orig/srcpos.c	2008-10-03 00:03:38.000000000 +1000
+++ dtc/srcpos.c	2008-10-03 00:03:57.000000000 +1000
@@ -29,6 +29,19 @@ char *xstrdup(const char *s)
 	return dup;
 }
 
+void srcpos_error(YYLTYPE *loc, char const *fmt, ...)
+{
+	va_list va;
+	va_start(va, fmt);
+
+	fprintf(stderr, "%s:%d ", loc->file->name, loc->first_line);
+
+	vfprintf(stderr, fmt, va);
+	fprintf(stderr, "\n");
+
+	va_end(va);
+}
+
 /*
  * Like yylineno, this is the current open file pos.
  */
Index: dtc/srcpos.h
===================================================================
--- dtc.orig/srcpos.h	2008-10-03 00:03:38.000000000 +1000
+++ dtc/srcpos.h	2008-10-03 00:03:57.000000000 +1000
@@ -72,10 +72,8 @@ typedef struct YYLTYPE {
 	}								\
     while (YYID (0))
 
-
-
-extern void yyerror(char const *);
-extern void yyerrorf(char const *, ...) __attribute__((format(printf, 1, 2)));
+void srcpos_error(YYLTYPE *loc, char const *fmt, ...)
+	__attribute__((format(printf, 2, 3)));
 
 extern struct dtc_file *srcpos_file;
 


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

[4/5] dtc: Cleanup yylloc type and handling

[David Gibson]

This patch makes several cleanups to the handling of location
variables as they're carried through the parser.  Specifically:

	- (for now) remove column numbers from YYLTYPE, since we were
never correctly filling them in from the lexer in any case.

	- split the 'file' field in YYLTYPE into first and last file,
since for big parser groupings the first and last token could in
theory be in different files.

	- use plain strings to store the files in YYLTYPE, instead of
dtc_file pointer.  There's no need for the other info from dtc_file,
and the strings will have nicer lifetime properties in future patches.

	- reorganize YYLTYPE into first and last nested structure
fields, each containing a file and line number, which will be more
convenient for us later on.

	- no longer use the undocumented magic #defines
YYLTYPE_IS_DECLARED and YYLTYPE_IS_TRIVIAL.  Instead we define our own
structure for the locations and make YYLTYPE a macro expanding to it,
as described in the flex info pages.

	- reformat the YYLLOC_DEFAULT macro into the indentation style
we use everywhere else (as well as updating it to match the other
changes)

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

---
 dtc-lexer.l |    4 +--
 srcpos.c    |   11 +++++++--
 srcpos.h    |   70 ++++++++++++++++++++++--------------------------------------
 3 files changed, 37 insertions(+), 48 deletions(-)

Index: dtc/srcpos.h
===================================================================
--- dtc.orig/srcpos.h	2008-10-03 00:03:57.000000000 +1000
+++ dtc/srcpos.h	2008-10-03 00:04:07.000000000 +1000
@@ -18,60 +18,42 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307
  *                                                                   USA
  */
-
-/*
- * Augment the standard YYLTYPE with a filenum index into an
- * array of all opened filenames.
- */
-
 #include <stdio.h>
 
 char *xstrdup(const char *s);
 
+struct srcpos {
+	int line;
+	const char *file;
+};
+
+struct srcloc {
+	struct srcpos first, last;
+};
+
+#define YYLTYPE struct srcloc
+
+#define YYLLOC_DEFAULT(Cur, Rhs, N)					\
+	do {								\
+		if (N) {						\
+			(Cur).first.file = YYRHSLOC(Rhs, 1).first.file;	\
+			(Cur).first.line = YYRHSLOC(Rhs, 1).first.line; \
+			(Cur).last.file = YYRHSLOC(Rhs, N).last.file;	\
+			(Cur).last.line = YYRHSLOC(Rhs, N).last.line;	\
+		} else {						\
+			(Cur).first.line = (Cur).last.line 		\
+				= YYRHSLOC(Rhs, 0).last.line;		\
+			(Cur).first.file = (Cur).last.file		\
+				= YYRHSLOC(Rhs, 0).last.file;		\
+		}							\
+	} while (0)
+
 struct dtc_file {
 	char *dir;
 	const char *name;
 	FILE *file;
 };
 
-#if ! defined(YYLTYPE) && ! defined(YYLTYPE_IS_DECLARED)
-typedef struct YYLTYPE {
-    int first_line;
-    int first_column;
-    int last_line;
-    int last_column;
-    struct dtc_file *file;
-} YYLTYPE;
-
-#define YYLTYPE_IS_DECLARED	1
-#define YYLTYPE_IS_TRIVIAL	1
-#endif
-
-/* Cater to old parser templates. */
-#ifndef YYID
-#define YYID(n)	(n)
-#endif
-
-#define YYLLOC_DEFAULT(Current, Rhs, N)					\
-    do									\
-      if (YYID (N))							\
-	{								\
-	  (Current).first_line   = YYRHSLOC (Rhs, 1).first_line;	\
-	  (Current).first_column = YYRHSLOC (Rhs, 1).first_column;	\
-	  (Current).last_line    = YYRHSLOC (Rhs, N).last_line;		\
-	  (Current).last_column  = YYRHSLOC (Rhs, N).last_column;	\
-	  (Current).file         = YYRHSLOC (Rhs, N).file;		\
-	}								\
-      else								\
-	{								\
-	  (Current).first_line   = (Current).last_line   =		\
-	    YYRHSLOC (Rhs, 0).last_line;				\
-	  (Current).first_column = (Current).last_column =		\
-	    YYRHSLOC (Rhs, 0).last_column;				\
-	  (Current).file         = YYRHSLOC (Rhs, 0).file;		\
-	}								\
-    while (YYID (0))
-
 void srcpos_error(YYLTYPE *loc, char const *fmt, ...)
 	__attribute__((format(printf, 2, 3)));
 
Index: dtc/dtc-lexer.l
===================================================================
--- dtc.orig/dtc-lexer.l	2008-10-03 00:03:48.000000000 +1000
+++ dtc/dtc-lexer.l	2008-10-03 00:04:07.000000000 +1000
@@ -39,8 +39,8 @@ LINECOMMENT	"//".*\n
 
 #define	YY_USER_ACTION \
 	{ \
-		yylloc.file = srcpos_file; \
-		yylloc.first_line = yylineno; \
+		yylloc.first.file = yylloc.last.file = srcpos_file->name; \
+		yylloc.first.line = yylloc.last.line = yylineno; \
 	}
 
 /*#define LEXDEBUG	1*/
Index: dtc/srcpos.c
===================================================================
--- dtc.orig/srcpos.c	2008-10-03 00:03:57.000000000 +1000
+++ dtc/srcpos.c	2008-10-03 00:04:07.000000000 +1000
@@ -29,12 +29,19 @@ char *xstrdup(const char *s)
 	return dup;
 }
 
-void srcpos_error(YYLTYPE *loc, char const *fmt, ...)
+void srcpos_error(struct srcloc *loc, char const *fmt, ...)
 {
 	va_list va;
 	va_start(va, fmt);
 
-	fprintf(stderr, "%s:%d ", loc->file->name, loc->first_line);
+	if (!streq(loc->first.file, loc->last.file))
+		fprintf(stderr, "%s:%d-%s:%d ", loc->first.file, loc->first.line,
+			loc->last.file, loc->last.line);
+	else if (loc->first.line != loc->last.line)
+		fprintf(stderr, "%s:%d-%d ", loc->first.file, loc->first.line,
+			loc->last.line);
+	else
+		fprintf(stderr, "%s:%d ", loc->first.file, loc->first.line);
 
 	vfprintf(stderr, fmt, va);
 	fprintf(stderr, "\n");


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

[5/5] dtc: Clean up source file management

[David Gibson]

This patch cleans up our handling of input files, particularly dts
source files, but also (to an extent) other input files such as those
used by /incbin/ and those used in -I dtb and -I fs modes.

We eliminate the current clunky mechanism which combines search paths
(which we don't actually use at present) with the open relative to
current source file behaviour, which we do.

Instead there's a single srcfile_open() entry point for callers which
opens a new input file relative to the current source file (which the
srcpos code tracks internally).  It doesn't currently do search paths,
but we can add that later without messing with the callers, by drawing
the search path from a global (which makes sense anyway, rather than
shuffling it around the rest of the processing code).

That suffices for non-dts input files.  For the actual dts files,
srcfile_push() and srcfile_pop() wrappers open the file while also
keeping track of it as the current source file for future opens.

push_input_file() and pop_input_file() from the lexer become much
simpler, just using the srcfile_{push,pop}() functions plus flex's
yy{push,pop}_buffer_state() builtins.

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

---
 convert-dtsv0-lexer.l |    6 +
 dtc-lexer.l           |   91 +++--------------------------
 dtc-parser.y          |   16 ++---
 dtc.c                 |   24 -------
 dtc.h                 |    4 -
 flattree.c            |   24 +++----
 srcpos.c              |  155 +++++++++++++++++++++++++++-----------------------
 srcpos.h              |   26 +++-----
 treesource.c          |    4 -
 9 files changed, 133 insertions(+), 217 deletions(-)

Index: dtc/srcpos.c
===================================================================
--- dtc.orig/srcpos.c	2008-10-03 00:04:07.000000000 +1000
+++ dtc/srcpos.c	2008-10-03 00:04:16.000000000 +1000
@@ -29,6 +29,45 @@ char *xstrdup(const char *s)
 	return dup;
 }
 
+char *join_path(const char *path, const char *name)
+{
+	int lenp = strlen(path);
+	int lenn = strlen(name);
+	int len;
+	int needslash = 1;
+	char *str;
+
+	len = lenp + lenn + 2;
+	if ((lenp > 0) && (path[lenp-1] == '/')) {
+		needslash = 0;
+		len--;
+	}
+
+	str = xmalloc(len);
+	memcpy(str, path, lenp);
+	if (needslash) {
+		str[lenp] = '/';
+		lenp++;
+	}
+	memcpy(str+lenp, name, lenn+1);
+	return str;
+}
+
+static char *dirname(const char *path)
+{
+	const char *slash = strrchr(path, '/');
+
+	if (slash) {
+		int len = slash - path;
+		char *dir = xmalloc(len + 1);
+
+		memcpy(dir, path, len);
+		dir[len] = '\0';
+		return dir;
+	} else
+		return NULL;
+}
+
 void srcpos_error(struct srcloc *loc, char const *fmt, ...)
 {
 	va_list va;
@@ -49,97 +88,75 @@ void srcpos_error(struct srcloc *loc, ch
 	va_end(va);
 }
 
+struct srcfile_state *current_srcfile; /* = NULL */
 /*
- * Like yylineno, this is the current open file pos.
+ * Detect infinite include recursion.
  */
+#define MAX_SRCFILE_DEPTH	(100)
+static int srcfile_depth; /* = 0 */
 
-struct dtc_file *srcpos_file;
-
-static int dtc_open_one(struct dtc_file *file,
-                        const char *search,
-                        const char *fname)
+FILE *srcfile_open(const char *fname, char **fullnamep)
 {
+	FILE *f;
 	char *fullname;
 
-	if (search) {
-		fullname = xmalloc(strlen(search) + strlen(fname) + 2);
-
-		strcpy(fullname, search);
-		strcat(fullname, "/");
-		strcat(fullname, fname);
+	if (streq(fname, "-")) {
+		f = stdin;
+		fullname = xstrdup("<stdin>");
 	} else {
-		fullname = xstrdup(fname);
+		if (!current_srcfile || !current_srcfile->dir
+		    || (fname[0] == '/'))
+			fullname = xstrdup(fname);
+		else
+			fullname = join_path(current_srcfile->dir, fname);
+
+		f = fopen(fullname, "r");
+		if (!f)
+			die("Couldn't open \"%s\": %s\n", fname,
+			    strerror(errno));
 	}
 
-	file->file = fopen(fullname, "r");
-	if (!file->file) {
+	if (fullnamep)
+		*fullnamep = fullname;
+	else
 		free(fullname);
-		return 0;
-	}
 
-	file->name = fullname;
-	return 1;
+	return f;
 }
 
-
-struct dtc_file *dtc_open_file(const char *fname,
-                               const struct search_path *search)
+void srcfile_push(const char *fname)
 {
-	static const struct search_path default_search = { NULL, NULL, NULL };
+	struct srcfile_state *srcfile;
 
-	struct dtc_file *file;
-	const char *slash;
+	if (srcfile_depth++ >= MAX_SRCFILE_DEPTH)
+		die("Includes nested too deeply");
 
-	file = xmalloc(sizeof(struct dtc_file));
+	srcfile = xmalloc(sizeof(*srcfile));
 
-	slash = strrchr(fname, '/');
-	if (slash) {
-		char *dir = xmalloc(slash - fname + 1);
-
-		memcpy(dir, fname, slash - fname);
-		dir[slash - fname] = 0;
-		file->dir = dir;
-	} else {
-		file->dir = NULL;
-	}
-
-	if (streq(fname, "-")) {
-		file->name = "stdin";
-		file->file = stdin;
-		return file;
-	}
-
-	if (fname[0] == '/') {
-		file->file = fopen(fname, "r");
-		if (!file->file)
-			goto fail;
+	srcfile->f = srcfile_open(fname, &srcfile->name);
+	srcfile->dir = dirname(srcfile->name);
+	srcfile->prev = current_srcfile;
+	current_srcfile = srcfile;
 
-		file->name = xstrdup(fname);
-		return file;
-	}
+	/* FIXME: We allow srcfile->name to leak, because there could
+	 * still be aliases to it from yylloc variables */
+}
 
-	if (!search)
-		search = &default_search;
+int srcfile_pop(void)
+{
+	struct srcfile_state *srcfile = current_srcfile;
 
-	while (search) {
-		if (dtc_open_one(file, search->dir, fname))
-			return file;
+	assert(srcfile);
 
-		if (errno != ENOENT)
-			goto fail;
+	current_srcfile = srcfile->prev;
 
-		search = search->next;
-	}
+	if (fclose(srcfile->f))
+		die("Error closing \"%s\": %s\n", srcfile->name, strerror(errno));
 
-fail:
-	die("Couldn't open \"%s\": %s\n", fname, strerror(errno));
-}
-
-void dtc_close_file(struct dtc_file *file)
-{
-	if (fclose(file->file))
-		die("Error closing \"%s\": %s\n", file->name, strerror(errno));
+	if (srcfile->dir)
+		free(srcfile->dir);
+	//free(srcfile->name); // FIXME
+	free(srcfile);
 
-	free(file->dir);
-	free(file);
+	return current_srcfile ? 1 : 0;
 }
Index: dtc/dtc.c
===================================================================
--- dtc.orig/dtc.c	2008-10-03 00:03:38.000000000 +1000
+++ dtc/dtc.c	2008-10-03 00:04:16.000000000 +1000
@@ -30,30 +30,6 @@ int reservenum;		/* Number of memory res
 int minsize;		/* Minimum blob size */
 int padsize;		/* Additional padding to blob */
 
-char *join_path(const char *path, const char *name)
-{
-	int lenp = strlen(path);
-	int lenn = strlen(name);
-	int len;
-	int needslash = 1;
-	char *str;
-
-	len = lenp + lenn + 2;
-	if ((lenp > 0) && (path[lenp-1] == '/')) {
-		needslash = 0;
-		len--;
-	}
-
-	str = xmalloc(len);
-	memcpy(str, path, lenp);
-	if (needslash) {
-		str[lenp] = '/';
-		lenp++;
-	}
-	memcpy(str+lenp, name, lenn+1);
-	return str;
-}
-
 static void fill_fullpaths(struct node *tree, const char *prefix)
 {
 	struct node *child;
Index: dtc/convert-dtsv0-lexer.l
===================================================================
--- dtc.orig/convert-dtsv0-lexer.l	2008-10-03 00:03:38.000000000 +1000
+++ dtc/convert-dtsv0-lexer.l	2008-10-03 00:04:16.000000000 +1000
@@ -229,8 +229,10 @@ static void convert_file(const char *fna
 	memcpy(newname, fname, len);
 	memcpy(newname + len, suffix, sizeof(suffix));
 
-	srcpos_file = dtc_open_file(fname, NULL);
-	yyin = srcpos_file->file;
+	yyin = fopen(fname, "r");
+	if (!yyin)
+		die("Couldn't open input file %s: %s\n",
+		    fname, strerror(errno));
 
 	yyout = fopen(newname, "w");
 	if (!yyout)
Index: dtc/flattree.c
===================================================================
--- dtc.orig/flattree.c	2008-10-03 00:03:38.000000000 +1000
+++ dtc/flattree.c	2008-10-03 00:04:16.000000000 +1000
@@ -775,7 +775,7 @@ static struct node *unflatten_tree(struc
 
 struct boot_info *dt_from_blob(const char *fname)
 {
-	struct dtc_file *dtcf;
+	FILE *f;
 	uint32_t magic, totalsize, version, size_dt, boot_cpuid_phys;
 	uint32_t off_dt, off_str, off_mem_rsvmap;
 	int rc;
@@ -790,14 +790,14 @@ struct boot_info *dt_from_blob(const cha
 	uint32_t val;
 	int flags = 0;
 
-	dtcf = dtc_open_file(fname, NULL);
+	f = srcfile_open(fname, NULL);
 
-	rc = fread(&magic, sizeof(magic), 1, dtcf->file);
-	if (ferror(dtcf->file))
+	rc = fread(&magic, sizeof(magic), 1, f);
+	if (ferror(f))
 		die("Error reading DT blob magic number: %s\n",
 		    strerror(errno));
 	if (rc < 1) {
-		if (feof(dtcf->file))
+		if (feof(f))
 			die("EOF reading DT blob magic number\n");
 		else
 			die("Mysterious short read reading magic number\n");
@@ -807,11 +807,11 @@ struct boot_info *dt_from_blob(const cha
 	if (magic != FDT_MAGIC)
 		die("Blob has incorrect magic number\n");
 
-	rc = fread(&totalsize, sizeof(totalsize), 1, dtcf->file);
-	if (ferror(dtcf->file))
+	rc = fread(&totalsize, sizeof(totalsize), 1, f);
+	if (ferror(f))
 		die("Error reading DT blob size: %s\n", strerror(errno));
 	if (rc < 1) {
-		if (feof(dtcf->file))
+		if (feof(f))
 			die("EOF reading DT blob size\n");
 		else
 			die("Mysterious short read reading blob size\n");
@@ -831,12 +831,12 @@ struct boot_info *dt_from_blob(const cha
 	p = blob + sizeof(magic)  + sizeof(totalsize);
 
 	while (sizeleft) {
-		if (feof(dtcf->file))
+		if (feof(f))
 			die("EOF before reading %d bytes of DT blob\n",
 			    totalsize);
 
-		rc = fread(p, 1, sizeleft, dtcf->file);
-		if (ferror(dtcf->file))
+		rc = fread(p, 1, sizeleft, f);
+		if (ferror(f))
 			die("Error reading DT blob: %s\n",
 			    strerror(errno));
 
@@ -899,7 +899,7 @@ struct boot_info *dt_from_blob(const cha
 
 	free(blob);
 
-	dtc_close_file(dtcf);
+	fclose(f);
 
 	return build_boot_info(reservelist, tree, boot_cpuid_phys);
 }
Index: dtc/srcpos.h
===================================================================
--- dtc.orig/srcpos.h	2008-10-03 00:04:07.000000000 +1000
+++ dtc/srcpos.h	2008-10-03 00:04:16.000000000 +1000
@@ -21,6 +21,7 @@
 #include <stdio.h>
 
 char *xstrdup(const char *s);
+char *join_path(const char *path, const char *name);
 
 struct srcpos {
 	int line;
@@ -48,24 +49,21 @@ struct srcloc {
 		}							\
 	} while (0)
 
-struct dtc_file {
-	char *dir;
-	const char *name;
-	FILE *file;
-};
-
 void srcpos_error(YYLTYPE *loc, char const *fmt, ...)
 	__attribute__((format(printf, 2, 3)));
 
-extern struct dtc_file *srcpos_file;
-
-struct search_path {
-	const char *dir; /* NULL for current directory */
-	struct search_path *prev, *next;
+struct srcfile_state {
+	FILE *f;
+	char *name;
+	char *dir;
+	int lineno;
+	struct srcfile_state *prev;
 };
 
-extern struct dtc_file *dtc_open_file(const char *fname,
-                                      const struct search_path *search);
-extern void dtc_close_file(struct dtc_file *file);
+extern struct srcfile_state *current_srcfile; /* = NULL */
+
+FILE *srcfile_open(const char *fname, char **fullnamep);
+void srcfile_push(const char *fname);
+int srcfile_pop(void);
 
 #endif /* _SRCPOS_H */
Index: dtc/treesource.c
===================================================================
--- dtc.orig/treesource.c	2008-10-03 00:03:38.000000000 +1000
+++ dtc/treesource.c	2008-10-03 00:04:16.000000000 +1000
@@ -31,8 +31,8 @@ struct boot_info *dt_from_source(const c
 	the_boot_info = NULL;
 	treesource_error = 0;
 
-	srcpos_file = dtc_open_file(fname, NULL);
-	yyin = srcpos_file->file;
+	srcfile_push(fname);
+	yyin = current_srcfile->f;
 
 	if (yyparse() != 0)
 		die("Unable to parse input tree\n");
Index: dtc/dtc-lexer.l
===================================================================
--- dtc.orig/dtc-lexer.l	2008-10-03 00:04:07.000000000 +1000
+++ dtc/dtc-lexer.l	2008-10-03 00:04:16.000000000 +1000
@@ -39,7 +39,7 @@ LINECOMMENT	"//".*\n
 
 #define	YY_USER_ACTION \
 	{ \
-		yylloc.first.file = yylloc.last.file = srcpos_file->name; \
+		yylloc.first.file = yylloc.last.file = current_srcfile->name; \
 		yylloc.first.line = yylloc.last.line = yylineno; \
 	}
 
@@ -194,100 +194,29 @@ static int pop_input_file(void);
 
 %%
 
-
-/*
- * Stack of nested include file contexts.
- */
-
-struct incl_file {
-	struct dtc_file *file;
-	YY_BUFFER_STATE yy_prev_buf;
-	int yy_prev_lineno;
-	struct incl_file *prev;
-};
-
-static struct incl_file *incl_file_stack;
-
-
-/*
- * Detect infinite include recursion.
- */
-#define MAX_INCLUDE_DEPTH	(100)
-
-static int incl_depth = 0;
-
-
 static void push_input_file(const char *filename)
 {
-	struct incl_file *incl_file;
-	struct dtc_file *newfile;
-	struct search_path search, *searchptr = NULL;
-
 	assert(filename);
 
-	if (incl_depth++ >= MAX_INCLUDE_DEPTH)
-		die("Includes nested too deeply");
+	current_srcfile->lineno = yylineno;
 
-	if (srcpos_file) {
-		search.dir = srcpos_file->dir;
-		search.next = NULL;
-		search.prev = NULL;
-		searchptr = &search;
-	}
+	srcfile_push(filename);
 
-	newfile = dtc_open_file(filename, searchptr);
-
-	incl_file = xmalloc(sizeof(struct incl_file));
-
-	/*
-	 * Save current context.
-	 */
-	incl_file->yy_prev_buf = YY_CURRENT_BUFFER;
-	incl_file->yy_prev_lineno = yylineno;
-	incl_file->file = srcpos_file;
-	incl_file->prev = incl_file_stack;
-
-	incl_file_stack = incl_file;
-
-	/*
-	 * Establish new context.
-	 */
-	srcpos_file = newfile;
+	yyin = current_srcfile->f;
 	yylineno = 1;
-	yyin = newfile->file;
-	yy_switch_to_buffer(yy_create_buffer(yyin, YY_BUF_SIZE));
+
+	yypush_buffer_state(yy_create_buffer(yyin, YY_BUF_SIZE));
 }
 
 
 static int pop_input_file(void)
 {
-	struct incl_file *incl_file;
-
-	if (incl_file_stack == 0)
+	if (srcfile_pop() == 0)
 		return 0;
 
-	dtc_close_file(srcpos_file);
-
-	/*
-	 * Pop.
-	 */
-	--incl_depth;
-	incl_file = incl_file_stack;
-	incl_file_stack = incl_file->prev;
-
-	/*
-	 * Recover old context.
-	 */
-	yy_delete_buffer(YY_CURRENT_BUFFER);
-	yy_switch_to_buffer(incl_file->yy_prev_buf);
-	yylineno = incl_file->yy_prev_lineno;
-	srcpos_file = incl_file->file;
-	yyin = incl_file->file ? incl_file->file->file : NULL;
-
-	/*
-	 * Free old state.
-	 */
-	free(incl_file);
+	yypop_buffer_state();
+	yylineno = current_srcfile->lineno;
+	yyin = current_srcfile->f;
 
 	return 1;
 }
Index: dtc/dtc-parser.y
===================================================================
--- dtc.orig/dtc-parser.y	2008-10-03 00:03:57.000000000 +1000
+++ dtc/dtc-parser.y	2008-10-03 00:04:16.000000000 +1000
@@ -202,12 +202,11 @@ propdata:
 		}
 	| propdataprefix DT_INCBIN '(' DT_STRING ',' addr ',' addr ')'
 		{
-			struct search_path path = { srcpos_file->dir, NULL, NULL };
-			struct dtc_file *file = dtc_open_file($4.val, &path);
+			FILE *f = srcfile_open($4.val, NULL);
 			struct data d = empty_data;
 
 			if ($6 != 0)
-				if (fseek(file->file, $6, SEEK_SET) != 0) {
+				if (fseek(f, $6, SEEK_SET) != 0) {
 					srcpos_error(&yylloc, "Couldn't seek to"
 						     " offset %llu in \"%s\": %s",
 						     (unsigned long long)$6,
@@ -215,21 +214,20 @@ propdata:
 					treesource_error = 1;
 				}
 
-			d = data_copy_file(file->file, $8);
+			d = data_copy_file(f, $8);
 
 			$$ = data_merge($1, d);
-			dtc_close_file(file);
+			fclose(f);
 		}
 	| propdataprefix DT_INCBIN '(' DT_STRING ')'
 		{
-			struct search_path path = { srcpos_file->dir, NULL, NULL };
-			struct dtc_file *file = dtc_open_file($4.val, &path);
+			FILE *f = srcfile_open($4.val, NULL);
 			struct data d = empty_data;
 
-			d = data_copy_file(file->file, -1);
+			d = data_copy_file(f, -1);
 
 			$$ = data_merge($1, d);
-			dtc_close_file(file);
+			fclose(f);
 		}
 	| propdata DT_LABEL
 		{
Index: dtc/dtc.h
===================================================================
--- dtc.orig/dtc.h	2008-10-03 00:03:38.000000000 +1000
+++ dtc/dtc.h	2008-10-03 00:04:16.000000000 +1000
@@ -241,8 +241,4 @@ struct boot_info *dt_from_source(const c
 
 struct boot_info *dt_from_fs(const char *dirname);
 
-/* misc */
-
-char *join_path(const char *path, const char *name);
-
 #endif /* _DTC_H */

-- 
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: [4/5] dtc: Cleanup yylloc type and handling

[Jon Loeliger]

> This patch makes several cleanups to the handling of location
> variables as they're carried through the parser.  Specifically:
> 
> 	- (for now) remove column numbers from YYLTYPE, since we were
> never correctly filling them in from the lexer in any case.

I fully implemented both line and column handling, as far as
I can tell, correctly.  Bug fixes welcome as needed, of course.

jdl

Re: [4/5] dtc: Cleanup yylloc type and handling

[David Gibson]

On Fri, Oct 03, 2008 at 02:24:26PM -0500, Jon Loeliger wrote:
> > This patch makes several cleanups to the handling of location
> > variables as they're carried through the parser.  Specifically:
> > 
> > 	- (for now) remove column numbers from YYLTYPE, since we were
> > never correctly filling them in from the lexer in any case.
> 
> I fully implemented both line and column handling, as far as
> I can tell, correctly.  Bug fixes welcome as needed, of course.

In the midst of totally unrelated stuff.  Separate patches for
logically separate changes, please.

I now have a patch on top of this series (well, a slightly updated
verison of this series) which puts column number handling back.  This
was just the neatest way 

-- 
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: [3/5] dtc: Cleanup yyerrorf() function

[Jon Loeliger]

> Currently, we put the source file name into the yylloc variable, but
> never use the stored value.  Instead the yyerrorf() function directly
> accesses srcpos_file to get the input file name.
> 
> That works in practice, but is likely not to always be correct if we
> ever re-enable the glr-parser option.  Even now, its correctness
> relies on the exact point in time bison executes the semantic rules
> w.r.t. to the lexing rules, which is probably correct but not
> obviously correct, which is far from ideal.
> 
> So, this patch replaces yyerrorf() with a srcpos_error() function
> which pulls the filename information out of the yylloc variable, which
> bison is explicitly supposed to get right for us.
> 
> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

This isn't how I'd like to see this work at all.

The original intent goes like this:

- As a file is referenced, it is put on a list (or array) of files.
  This list is essentially write-only so that any file ever
  referenced accumulates into this list.

- The source positions maintain a pointer (or index) into that
  table of file names.

- The table of files is *always* available, even long after
  parsing has finished.

- There is an entirely different stack of directories that
  tracks where file references are resolved.

Thus, a routine like srcpos_error() should take a srcpos
for its basis information.  Yes, that is the same type
as the YYLTYPE during parsing, but my point is that the
srcpos type is conceptually longer lasting than just parsing
and will be available by later semantic processing passes too.

Thanks,
jdl

Re: [3/5] dtc: Cleanup yyerrorf() function

[David Gibson]

On Fri, Oct 03, 2008 at 02:22:41PM -0500, Jon Loeliger wrote:
> > Currently, we put the source file name into the yylloc variable, but
> > never use the stored value.  Instead the yyerrorf() function directly
> > accesses srcpos_file to get the input file name.
> > 
> > That works in practice, but is likely not to always be correct if we
> > ever re-enable the glr-parser option.  Even now, its correctness
> > relies on the exact point in time bison executes the semantic rules
> > w.r.t. to the lexing rules, which is probably correct but not
> > obviously correct, which is far from ideal.
> > 
> > So, this patch replaces yyerrorf() with a srcpos_error() function
> > which pulls the filename information out of the yylloc variable, which
> > bison is explicitly supposed to get right for us.
> > 
> > Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> This isn't how I'd like to see this work at all.
> 
> The original intent goes like this:
> 
> - As a file is referenced, it is put on a list (or array) of files.
>   This list is essentially write-only so that any file ever
>   referenced accumulates into this list.
> 
> - The source positions maintain a pointer (or index) into that
>   table of file names.

Ok, this is not a bad idea.  But there's no sign of this "original
intent" either in what we had before, or in what's there after your
new language series.  dtc_file structures are just allocated bare,
free()ed on dtc_file_close() and pointers to them are handed around
unsafely.

> - The table of files is *always* available, even long after
>   parsing has finished.

Again, not a bad idea - it certainly fixes the lifetime issue.  But
orthogonal certainly from this patch, and really from 4/5 too.

> - There is an entirely different stack of directories that
>   tracks where file references are resolved.

Again, orthogonal.  The stack could easily reference a table of files
rather than containing the file information directly.

> Thus, a routine like srcpos_error() should take a srcpos
> for its basis information.  Yes, that is the same type
> as the YYLTYPE during parsing, but my point is that the
> srcpos type is conceptually longer lasting than just parsing
> and will be available by later semantic processing passes too.

Huh!?  This makes no sense to me.  AFAICT the discussion above is
talking about the lifetime and allocation of dtc_file structures, or
something of similar intent.  Now you're talking about the srcpos
structures.  I don't see how either this patch of mine, or the next
makes the srcpos/YYLTYPE structure *not* potentially longer lasting.

-- 
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: [2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[Jon Loeliger]

On Fri, 2008-10-03 at 00:05 +1000, David Gibson wrote:
> Current, every lexer rule starts with some boiler plate to update the
> yylloc value for use by the parser.  One of the rules, even mistakenly
> has a redundant allocation to one of the members.
> 
> This patch uses the flex YY_USER_ACTION macro hook, which is executed
> before every rule to avoid this duplication.
> 
> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> ---

This, and the xstrdup() change are both excellent suggestions.

jdl

Re: [2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[David Gibson]

On Thu, Oct 02, 2008 at 11:25:27AM -0500, Jon Loeliger wrote:
> On Fri, 2008-10-03 at 00:05 +1000, David Gibson wrote:
> > Current, every lexer rule starts with some boiler plate to update the
> > yylloc value for use by the parser.  One of the rules, even mistakenly
> > has a redundant allocation to one of the members.
> > 
> > This patch uses the flex YY_USER_ACTION macro hook, which is executed
> > before every rule to avoid this duplication.
> > 
> > Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> > 
> > ---
> 
> This, and the xstrdup() change are both excellent suggestions.

Ok, so lets apply them, clearing a bit more muck out of the way to
focus on the new language stuff.

But does this imply you don't like the rest of the series?

-- 
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: [2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[Jon Loeliger]

David Gibson wrote:
> On Thu, Oct 02, 2008 at 11:25:27AM -0500, Jon Loeliger wrote:
>> On Fri, 2008-10-03 at 00:05 +1000, David Gibson wrote:
>>> Current, every lexer rule starts with some boiler plate to update the
>>> yylloc value for use by the parser.  One of the rules, even mistakenly
>>> has a redundant allocation to one of the members.
>>>
>>> This patch uses the flex YY_USER_ACTION macro hook, which is executed
>>> before every rule to avoid this duplication.
>>>
>>> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>>
>>> ---
>> This, and the xstrdup() change are both excellent suggestions.
> 
> Ok, so lets apply them, clearing a bit more muck out of the way to
> focus on the new language stuff.
> 
> But does this imply you don't like the rest of the series?
> 

Dave,

You need to interpret less and quit trying to out-guess people.
I for one just don't like it.

Here's what I'm doing right now.

First, I buy the YY_USER_ACTION patch entirely.  I've refactored
it to apply to my branch to verify that it works with my code as well.

So I am going to apply it to the *current* master branch and
rebase my stuff on to it to bring it forward through my patches.
Then, as you say, we'll clear some muck.

Second, I buy the desire for strdup() -> xstrdup().  However, I
am not going to put xstrdup() into srcpos.c.  No way, not no how.
So I am going to introduce utiil.[ch] or so.  And to get it into
the convert tool, I am first going to refactor the Makfile.

Finally, I've not entirely read through and understood some of
the parts of the rest of your series yet.  There are definitely
parts that I'd like to pick up, and yet there are parts that I
am not sure about yet.  I need to re-read and understand it more.

In the meantime, I'm going to make further progress on cleaning
up "the muck".

Thanks,
jdl

Re: [2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[David Gibson]

On Fri, Oct 03, 2008 at 09:17:53AM -0500, Jon Loeliger wrote:
> David Gibson wrote:
>> On Thu, Oct 02, 2008 at 11:25:27AM -0500, Jon Loeliger wrote:
>>> On Fri, 2008-10-03 at 00:05 +1000, David Gibson wrote:
>>>> Current, every lexer rule starts with some boiler plate to update the
>>>> yylloc value for use by the parser.  One of the rules, even mistakenly
>>>> has a redundant allocation to one of the members.
>>>>
>>>> This patch uses the flex YY_USER_ACTION macro hook, which is executed
>>>> before every rule to avoid this duplication.
>>>>
>>>> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
>>>>
>>>> ---
>>> This, and the xstrdup() change are both excellent suggestions.
>>
>> Ok, so lets apply them, clearing a bit more muck out of the way to
>> focus on the new language stuff.
>>
>> But does this imply you don't like the rest of the series?
>
> Dave,
>
> You need to interpret less and quit trying to out-guess people.
> I for one just don't like it.

Ok, my apologies.  I'm aware that I've been impatient and rude.

But, this language thing has become urgent with very little notice;
you're not the only one with other things to work on too.  I think
both in implementation and especially in language design your current
patch series had not had sufficient thought and polish yet, and seeing
people gung-ho about merging ugliness into a project I have such a
stake in makes me quite upset.

-- 
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: [2/5] dtc: Use flex's YY_USER_ACTION feature to avoid code duplication

[Jon Loeliger]

> Current, every lexer rule starts with some boiler plate to update the
> yylloc value for use by the parser.  One of the rules, even mistakenly
> has a redundant allocation to one of the members.
> 
> This patch uses the flex YY_USER_ACTION macro hook, which is executed
> before every rule to avoid this duplication.
> 
> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Applied.

Thanks,
jdl

Re: [1/5] dtc: Implement and use an xstrdup() function

[Jon Loeliger]

> Many places in dtc use strdup(), but none of them actually check the
> return value to see if the implied allocation succeeded.  This is a
> potential bug, which we fix in the patch below by replacing strdup()
> with an xstrdup() which in analogy to xmalloc() will quit with a fatal
> error if the allocation fails.
> 
> xstrdup() is defined in srcpos.c, because that's available to both dtc
> itself and the conversion program which also uses it.  While we're at
> it, we add standard double-include protection to srcpos.h which was
> missing it.
> 
> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>


This was applied with a minor change, placing the new function
in the file util.h and util.c instead.  New patch posted.

Thanks,
jdl

Re: [1/5] dtc: Implement and use an xstrdup() function

[David Gibson]

On Fri, Oct 03, 2008 at 12:17:27PM -0500, Jon Loeliger wrote:
> > Many places in dtc use strdup(), but none of them actually check the
> > return value to see if the implied allocation succeeded.  This is a
> > potential bug, which we fix in the patch below by replacing strdup()
> > with an xstrdup() which in analogy to xmalloc() will quit with a fatal
> > error if the allocation fails.
> > 
> > xstrdup() is defined in srcpos.c, because that's available to both dtc
> > itself and the conversion program which also uses it.  While we're at
> > it, we add standard double-include protection to srcpos.h which was
> > missing it.
> > 
> > Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
> 
> 
> This was applied with a minor change, placing the new function
> in the file util.h and util.c instead.  New patch posted.

Yeah, reasonable change, I probably should have done that.  Actually I
probably should have created a util.c when I first made convert-dtsv0,
to contain the die() and xmalloc() functions.

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