On Fri, Oct 26, 2018 at 05:44:34PM +0200, Julia Lawall wrote:
> Provide the new command-line option:
>
> --annotate (abbreviated -T)
>
> --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.
>
> -T can be repeated giving more verbose annotations. These consist of
> 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>.
>
> The verbose annotations may be too verbose for normal use.
>
> 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.
>
> Various changes are sprinkled throughout treesource.c to print the
> annotations.
>
> Signed-off-by: Julia Lawall <Julia.Lawall@xxxxxxx>
> ---
>
> v6: SoB added, v5 information added
>
> v5: The main change is that there is no more --annotate-full argument.
> Instead that is obtained with -T -T.
This looks good, but there's one nit I'd still like fixed.
[snip]
> diff --git a/srcpos.c b/srcpos.c
> index cba1c0f..05f73e0 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;
I don't think this logic will be very robust if people do things like
use two slashes in a row in path (non-canonical, but usually accepted
the same as one slash in practice). But it shouldn't do anything
worse than mess up the annotations, so it can be fixed later.
> + 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;
The real problem is that this could return either an allocated
string..
> + }
> + return fname;
..or a borrowed string. Which means that..
> +}
>
> /**
> * Try to open a file in a given directory.
> @@ -157,6 +200,9 @@ void srcfile_push(const char *fname)
> srcfile->colno = 1;
>
> current_srcfile = srcfile;
> +
> + if (srcfile_depth == 1)
> + set_initial_path(srcfile->name);
> }
>
> bool srcfile_pop(void)
> @@ -288,6 +334,59 @@ srcpos_string(struct srcpos *pos)
> return pos_str;
> }
>
> +static char *
> +srcpos_string_comment(struct srcpos *pos, bool first_line, int level)
> +{
> + char *pos_str, *fname, *first, *rest;
> +
> + if (!pos) {
> + if (level > 1) {
> + 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 (level > 1)
> + fname = pos->file->name;
> + else
> + fname = shorten_to_initial_path(pos->file->name);
.. this path might leak.
I know I said I'm not especially concerned about leaks in dtc, but
this could potentially be called a lot of times, leaking on every one,
so I would like to fix this before merging.
--
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:
signature.asc
Description: PGP signature
