Re: [RFC PATCH v6 2/3] dtc: dts source location annotation

[Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 10/6/2015 10:32 PM, David Gibson wrote:
> On Tue, Oct 06, 2015 at 12:45:34AM -0700, Frank Rowand wrote:
>> On 10/5/2015 9:56 PM, David Gibson wrote:
>>> On Fri, Oct 02, 2015 at 09:52:48PM -0700, Frank Rowand wrote:
>>>> From: Frank Rowand <frank.rowand-/MT0OVThwyLZJqsBc5GL+g@public.gmane.org>
>>>>
>>>> Proof of concept patch.
>>>>
>>>> Annotates input source file and line number of nodes and properties
>>>> as comments in output .dts file when --annotate flag is supplied.
>>
>> < snip >
>>
>>
>>>> Index: b/srcpos.c
>>>> ===================================================================
>>>> --- a/srcpos.c
>>>> +++ b/srcpos.c
>>>> @@ -246,6 +246,41 @@ srcpos_copy(struct srcpos *pos)
>>>>  	return pos_new;
>>>>  }
>>>>  
>>>> +struct srcpos *
>>>> +srcpos_copy_all(struct srcpos *pos)
>>>> +{
>>>> +	struct srcpos *pos_new;
>>>> +	struct srcfile_state *srcfile_state;
>>>> +
>>>> +	if (!pos)
>>>> +		return NULL;
>>>> +
>>>> +	pos_new = srcpos_copy(pos);
>>>> +
>>>> +	if (pos_new) {
>>>> +		/* allocate without free */
>>>> +		srcfile_state = xmalloc(sizeof(struct srcfile_state));
>>>> +		memcpy(srcfile_state, pos->file, sizeof(struct srcfile_state));
>>>> +
>>>> +		pos_new->file = srcfile_state;
>>>> +	}
>>>> +
>>>> +	return pos_new;
>>>> +}
>>>
>>> You still don't need this function.  srcfile_states have unlimited
>>> lifetime.
>>
>> My observation about this is buried in a late reply to v5, so
>> copying here:
>>
>>    If I don't allocate a new copy of pos->file, then the file names are
>>    incorrect.  I'm not sure why.  I can dig into this deeper to try to
>>    understand what is going on if you want me to.
>>
>> It sounds like I do need to debug this.
> 
> That's weird.  Yeah, we need to debug this.

OK, I will dig into this.


> Btw, I was thinking about the filenames - in particular the way the
> current draft will give you very unwield full paths.  I was thinking
> about a different approach which should shorten those without
> requiring different invocation or hand hacking:
>    * For the "base" dts file (the one passed on the command line),
>      always use just the basename (i.e. final piece of the file path)
>    * For all other files, print the relative path from the directory
>      of the base file
> 
> /include/ directives already open files relative to the directory of
> the file including the /include/ so we have some of the mechanisms for
> this already.
> 
> I think this will require a moderate amount of extra work, so I
> wouldn't suggest it for the first cut, but does it sound like a good
> approach in principle to you?

Yes, I think that is a great way to approach it.  I'll take a look at
how much work this is, and if small enough I will add another patch
to the end of the series to do this.

-Frank