Re: [PATCH 3/3 v2] annotations: add the annotation functionality

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

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

On Mon, Jan 15, 2018 at 07:24:10PM +0100, Julia Lawall wrote:
> This commit provides two new command-line options:
> 
> --annotate (abbreviated -T)
> --annotate-full (abbreviated -F)

What's the rationale for having two different versions of the
annotations?

> 
> --annotate provides one or more filenames and line numbers indicating
> the origin of a given line.  The filename is expressed relative the the
> filename provided on the command line.  Nothing is printed for overlays,
> etc.
> 
> --annotate-full provides one or more tuples of: filename, starting line,
> starting column, ending line ending column.  The full path is given for
> the file name.  Overlays, etc are annotated with <no-file>:<no-line>.
> 
> There are numerous changes in srcpos to provide the relative filenames
> (variables initial_path, initial_pathlen and initial_cpp, new functions
> set_initial_path and shorten_to_initial_path, and changes in
> srcfile_push and srcpos_set_line).  The change in srcpos_set_line takes
> care of the case where cpp is used as a preprocessor.  In that case the
> initial file name is not the one provided on the command line but the
> one found at the beginnning of the cpp output.
> 
> The new functions srcpos_string_comment, srcpos_string_first, and
> srcpos_string_last print the annotations.  srcpos_string_comment is
> recursive to print a list of source file positions.
> 
> Note that the column numbers in the --annotate-full case may be of
> limited use when cpp is first used, because the columns are those in the
> output generated by cpp, which are not the same as the ones in the
> source code.  It may be that cpp simply replaces tabs by spaces.
> 
> Various changes are sprinkled throughout treesource.c to print the
> annotations.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
> Signed-off-by: Frank Rowand <frank.rowand-7U/KSKJipcs@public.gmane.org>
> ---
> 
> v2: Squash all of the annotation support into a single patch, enable
> printing a list of positions.
> 
>  dtc.c        |  17 ++++++++-
>  dtc.h        |   2 ++
>  srcpos.c     | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  srcpos.h     |   2 ++
>  treesource.c |  47 +++++++++++++++++++++----
>  5 files changed, 169 insertions(+), 10 deletions(-)
> 
> diff --git a/dtc.c b/dtc.c
> index c36994e..0153b1a 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -35,6 +35,8 @@ int phandle_format = PHANDLE_EPAPR;	/* Use linux,phandle or phandle properties *
>  int generate_symbols;	/* enable symbols & fixup support */
>  int generate_fixups;		/* suppress generation of fixups on symbol support */
>  int auto_label_aliases;		/* auto generate labels -> aliases */
> +bool annotate;		 /* =false, annotate .dts with input source location */
> +bool annotate_full; /* =false, annotate .dts with full input source location */
>  
>  static int is_power_of_2(int x)
>  {
> @@ -60,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
>  
>  /* Usage related data. */
>  static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@Ahv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@ATFhv";
>  static struct option const usage_long_opts[] = {
>  	{"quiet",            no_argument, NULL, 'q'},
>  	{"in-format",         a_argument, NULL, 'I'},
> @@ -81,6 +83,8 @@ static struct option const usage_long_opts[] = {
>  	{"error",             a_argument, NULL, 'E'},
>  	{"symbols",	     no_argument, NULL, '@'},
>  	{"auto-alias",       no_argument, NULL, 'A'},
> +	{"annotate",         no_argument, NULL, 'T'},
> +	{"annotate-full",    no_argument, NULL, 'F'},
>  	{"help",             no_argument, NULL, 'h'},
>  	{"version",          no_argument, NULL, 'v'},
>  	{NULL,               no_argument, NULL, 0x0},
> @@ -114,6 +118,8 @@ static const char * const usage_opts_help[] = {
>  	"\n\tEnable/disable errors (prefix with \"no-\")",
>  	"\n\tEnable generation of symbols",
>  	"\n\tEnable auto-alias of labels",
> +	"\n\tAnnotate output .dts with input source file and line",
> +	"\n\tAnnotate output .dts with input source file (full path) line and column",
>  	"\n\tPrint this help and exit",
>  	"\n\tPrint version and exit",
>  	NULL,
> @@ -259,6 +265,11 @@ int main(int argc, char *argv[])
>  		case 'A':
>  			auto_label_aliases = 1;
>  			break;
> +		case 'F':
> +			annotate_full = true;

Uncommented fallthrough.  Please don't do that.

> +		case 'T':
> +			annotate = true;
> +			break;
>  
>  		case 'h':
>  			usage(NULL);
> @@ -297,6 +308,10 @@ int main(int argc, char *argv[])
>  				outform = "dts";
>  		}
>  	}
> +
> +	if (annotate && (!streq(inform, "dts") || !streq(outform, "dts")))
> +		die("--annotate and --annotate-full require -I dts -O dts\n");
>  	if (streq(inform, "dts"))
>  		dti = dt_from_source(arg);
>  	else if (streq(inform, "fs"))
> diff --git a/dtc.h b/dtc.h
> index 3a85058..0b8141b 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -58,6 +58,8 @@ extern int phandle_format;	/* Use linux,phandle or phandle properties */
>  extern int generate_symbols;	/* generate symbols for nodes with labels */
>  extern int generate_fixups;	/* generate fixups */
>  extern int auto_label_aliases;	/* auto generate labels -> aliases */
> +extern bool annotate;		/* annotate .dts with input source location */
> +extern bool annotate_full;   /* annotate .dts with detailed source location */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> diff --git a/srcpos.c b/srcpos.c
> index 2ed794b..04e78ba 100644
> --- a/srcpos.c
> +++ b/srcpos.c
> @@ -33,6 +33,9 @@ struct search_path {
>  /* This is the list of directories that we search for source files */
>  static struct search_path *search_path_head, **search_path_tail;
>  
> +/* Detect infinite include recursion. */
> +#define MAX_SRCFILE_DEPTH     (100)
> +static int srcfile_depth; /* = 0 */
>  
>  static char *get_dirname(const char *path)
>  {
> @@ -51,11 +54,51 @@ static char *get_dirname(const char *path)
>  
>  FILE *depfile; /* = NULL */
>  struct srcfile_state *current_srcfile; /* = NULL */
> +static char *initial_path; /* = NULL */
> +static int initial_pathlen; /* = 0 */
> +static bool initial_cpp = true;
>  
> -/* Detect infinite include recursion. */
> -#define MAX_SRCFILE_DEPTH     (100)
> -static int srcfile_depth; /* = 0 */
> +static void set_initial_path(char *fname)
> +{
> +	int i, len = strlen(fname);
> +
> +	xasprintf(&initial_path, "%s", fname);
> +	initial_pathlen = 0;
> +	for (i = 0; i != len; i++)
> +		if (initial_path[i] == '/')
> +			initial_pathlen++;
> +}
>  
> +static char *shorten_to_initial_path(char *fname)
> +{
> +	char *p1, *p2, *prevslash1 = NULL;
> +	int slashes = 0;
> +
> +	for (p1 = fname, p2 = initial_path; *p1 && *p2; p1++, p2++) {
> +		if (*p1 != *p2)
> +			break;
> +		if (*p1 == '/') {
> +			prevslash1 = p1;
> +			slashes++;
> +		}
> +	}
> +	p1 = prevslash1 + 1;
> +	if (prevslash1) {
> +		int diff = initial_pathlen - slashes, i, j;
> +		int restlen = strlen(fname) - (p1 - fname);
> +		char *res;
> +
> +		res = xmalloc((3 * diff) + restlen + 1);
> +		for (i = 0, j = 0; i != diff; i++) {
> +			res[j++] = '.';
> +			res[j++] = '.';
> +			res[j++] = '/';
> +		}
> +		strcpy(res + j, p1);
> +		return res;
> +	}
> +	return fname;
> +}
>  
>  /**
>   * Try to open a file in a given directory.
> @@ -157,6 +200,9 @@ void srcfile_push(const char *fname)
>  	srcfile->colno = 0;
>  
>  	current_srcfile = srcfile;
> +
> +	if (srcfile_depth == 1)
> +		set_initial_path(srcfile->name);
>  }
>  
>  bool srcfile_pop(void)
> @@ -351,6 +397,60 @@ srcpos_string(struct srcpos *pos)
>  	return pos_str;
>  }
>  
> +static char *
> +srcpos_string_comment(struct srcpos *pos, bool first_line, bool full)
> +{
> +	char *pos_str, *fname, *first, *rest;
> +
> +	if (!pos) {
> +		if (full) {
> +			xasprintf(&pos_str, "<no-file>:<no-line>");
> +			return pos_str;
> +		} else {
> +			return NULL;
> +		}
> +	}
> +
> +	if (!pos->file)
> +		fname = "<no-file>";
> +	else if (!pos->file->name)
> +		fname = "<no-filename>";
> +	else if (full)
> +		fname = pos->file->name;
> +	else
> +		fname = shorten_to_initial_path(pos->file->name);
> +
> +	if (full)
> +		xasprintf(&first, "%s:%d:%d-%d:%d", fname,
> +			  pos->first_line, pos->first_column,
> +			  pos->last_line, pos->last_column);
> +	else
> +		xasprintf(&first, "%s:%d", fname,
> +			  first_line ? pos->first_line : pos->last_line);
> +
> +	if (pos->next != NULL) {
> +		rest = srcpos_string_comment(pos->next, first_line, full);
> +		xasprintf(&pos_str, "%s, %s", first, rest);
> +		free(first);
> +		free(rest);
> +	} else {
> +		pos_str = first;
> +	}
> +
> +	return pos_str;
> +}
> +
> +char *srcpos_string_first(struct srcpos *pos, bool full)
> +{
> +	return srcpos_string_comment(pos, true, full);
> +}
> +
> +char *srcpos_string_last(struct srcpos *pos, bool full)
> +{
> +	return srcpos_string_comment(pos, false, full);
> +}
> +
> +
>  void srcpos_verror(struct srcpos *pos, const char *prefix,
>  		   const char *fmt, va_list va)
>  {
> @@ -379,4 +479,9 @@ void srcpos_set_line(char *f, int l)
>  {
>  	current_srcfile->name = f;
>  	current_srcfile->lineno = l;
> +
> +	if (initial_cpp) {
> +		initial_cpp = false;
> +		set_initial_path(f);
> +	}
>  }
> diff --git a/srcpos.h b/srcpos.h
> index c22d6ba..a538c4e 100644
> --- a/srcpos.h
> +++ b/srcpos.h
> @@ -111,6 +111,8 @@ extern struct srcpos *srcpos_combine(struct srcpos *left_srcpos,
>  extern struct srcpos *srcpos_extend(struct srcpos *new_srcpos,
>  				    struct srcpos *old_srcpos);
>  extern char *srcpos_string(struct srcpos *pos);
> +extern char *srcpos_string_first(struct srcpos *pos, bool full);
> +extern char *srcpos_string_last(struct srcpos *pos, bool full);
>  
>  extern void PRINTF(3, 0) srcpos_verror(struct srcpos *pos, const char *prefix,
>  					const char *fmt, va_list va);
> diff --git a/treesource.c b/treesource.c
> index 2461a3d..f454ba4 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -199,10 +199,20 @@ static void write_propval(FILE *f, struct property *prop)
>  	struct marker *m = prop->val.markers;
>  	int nnotstring = 0, nnul = 0;
>  	int nnotstringlbl = 0, nnotcelllbl = 0;
> +	char *srcstr;
>  	int i;
>  
>  	if (len == 0) {
> -		fprintf(f, ";\n");
> +		fprintf(f, ";");
> +		if (annotate) {
> +			srcstr = srcpos_string_first(prop->srcpos,
> +						     annotate_full);
> +			if (srcstr) {
> +				fprintf(f, " /* %s */", srcstr);
> +				free(srcstr);
> +			}
> +		}
> +		fprintf(f, "\n");
>  		return;
>  	}
>  
> @@ -230,7 +240,15 @@ static void write_propval(FILE *f, struct property *prop)
>  		write_propval_bytes(f, prop->val);
>  	}
>  
> -	fprintf(f, ";\n");
> +	fprintf(f, ";");
> +	if (annotate) {
> +		srcstr = srcpos_string_first(prop->srcpos, annotate_full);
> +		if (srcstr) {
> +			fprintf(f, " /* %s */", srcstr);
> +			free(srcstr);
> +		}
> +	}
> +	fprintf(f, "\n");
>  }
>  
>  static void write_tree_source_node(FILE *f, struct node *tree, int level)
> @@ -238,14 +256,24 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  	struct property *prop;
>  	struct node *child;
>  	struct label *l;
> +	char *srcstr;
>  
>  	write_prefix(f, level);
>  	for_each_label(tree->labels, l)
>  		fprintf(f, "%s: ", l->label);
>  	if (tree->name && (*tree->name))
> -		fprintf(f, "%s {\n", tree->name);
> +		fprintf(f, "%s {", tree->name);
>  	else
> -		fprintf(f, "/ {\n");
> +		fprintf(f, "/ {");
> +
> +	if (annotate) {
> +		srcstr = srcpos_string_first(tree->srcpos, annotate_full);
> +		if (srcstr) {
> +			fprintf(f, " /* %s */", srcstr);
> +			free(srcstr);
> +		}
> +	}
> +	fprintf(f, "\n");
>  
>  	for_each_property(tree, prop) {
>  		write_prefix(f, level+1);
> @@ -259,10 +287,17 @@ static void write_tree_source_node(FILE *f, struct node *tree, int level)
>  		write_tree_source_node(f, child, level+1);
>  	}
>  	write_prefix(f, level);
> -	fprintf(f, "};\n");
> +	fprintf(f, "};");
> +	if (annotate) {
> +		srcstr = srcpos_string_last(tree->srcpos, annotate_full);
> +		if (srcstr) {
> +			fprintf(f, " /* %s */", srcstr);
> +			free(srcstr);
> +		}
> +	}
> +	fprintf(f, "\n");
>  }
>  
> -
>  void dt_to_source(FILE *f, struct dt_info *dti)
>  {
>  	struct reserve_info *re;

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

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