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

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

On 01/16/18 17:13, David Gibson wrote:
> 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?

I'll put an example to try to explain, at the end of this email.


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

< snip >


For a real world Linux devicetree source file, I often use
arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
In these examples, I am using the version in Linux v4.15-rc7.

My basic premise is that --annotate-full provides a lot of useful
information, but that information is usually not needed.  But the
extra information makes the output very dense and much harder for
a human eye to scan or read.

In the Linux sources (an argument _against_ my case :-)), the
full path of the source files normally provides no value.  Part
of this the way our source tree is structured.

I will use the Linux script scripts/dtc/dtx_diff to generate my
examples of annotated files.  This script takes care of all of
the include paths, invoking cpp, and other details of the linux
use of dtc. When provided a single argument, dtx_diff simply does
the compile with '-O dts'.  I have modified the normal dtx_diff
to pass on --annotate and --annotate-full to dtc. 

First, I create two annotated files, one with --annotate, the other
with --annotate-full:

$ scripts/dtc/dtx_diff --annotate arch/arm/boot/dts/qcom-apq8074-dragonboard.dts >qcom-apq8074-dragonboard--annotate.dts

$ scripts/dtc/dtx_diff --annotate-full arch/arm/boot/dts/qcom-apq8074-dragonboard.dts >qcom-apq8074-dragonboard--annotate-full.dts


Then just look at the first few lines of each of the two examples:

$ head qcom-apq8074-dragonboard--annotate.dts
/dts-v1/;

/ { /* qcom-apq8074-dragonboard.dts:6, qcom-msm8974.dtsi:11, skeleton.dtsi:12 */
	#address-cells = <0x1>; /* skeleton.dtsi:13 */
	#size-cells = <0x1>; /* skeleton.dtsi:14 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* qcom-apq8074-dragonboard.dts:8 */
	interrupt-parent = <0x1>; /* qcom-msm8974.dtsi:14 */
	model = "Qualcomm APQ8074 Dragonboard"; /* qcom-apq8074-dragonboard.dts:7 */

	adsp-pil { /* qcom-msm8974.dtsi:243 */

$ head qcom-apq8074-dragonboard--annotate-full.dts
/dts-v1/;

/ { /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:6:0-346:2, arch/arm/boot/dts/qcom-msm8974.dtsi:11:0-1148:2, arch/arm/boot/dts/skeleton.dtsi:12:0-18:2 */
	#address-cells = <0x1>; /* arch/arm/boot/dts/skeleton.dtsi:13:1-13:22 */
	#size-cells = <0x1>; /* arch/arm/boot/dts/skeleton.dtsi:14:1-14:19 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:8:1-8:57 */
	interrupt-parent = <0x1>; /* arch/arm/boot/dts/qcom-msm8974.dtsi:14:1-14:28 */
	model = "Qualcomm APQ8074 Dragonboard"; /* arch/arm/boot/dts/qcom-apq8074-dragonboard.dts:7:1-7:40 */

	adsp-pil { /* arch/arm/boot/dts/qcom-msm8974.dtsi:243:1-262:3 */


The --annotate-full version is harder for _me_ to scan.

The --annotate-full version results in a much longer line (and this effect gets even
worse when there are nodes nested several levels).

One can make the argument that the long path is the fault of the way that
Linux is structured, and it should be my problem to resolve that.  And to
a certain extent I can.  I could package a simple sed command in a script.
The simplistic hard coded version of the sed command (since I know the base
path of the devicetree source file that I am compiling) is:

$ sed -e 's|arch/arm/boot/dts/||g' -e 's|:[0-9]*-[0-9]*:[0-9]*||g' -e 's| /\* <no-file>:<no-line> \*/||' qcom-apq8074-dragonboard--annotate-full.dts > qcom-apq8074-dragonboard--annotate-full--then-sed.dts

The second "-e" substitution is not needed to strip the path off, but I was
going for the full simplification.  Looking at the first few lines of that
transformation, I see that it is basically the same as using --annotate.
Looking deeper into the file, which I don't show here, some of the row
numbers are different, due to my simplistic sed regular expression.

$ head qcom-apq8074-dragonboard--annotate-full--then-sed.dts
/dts-v1/;

/ { /* qcom-apq8074-dragonboard.dts:6, qcom-msm8974.dtsi:11, skeleton.dtsi:12 */
	#address-cells = <0x1>; /* skeleton.dtsi:13 */
	#size-cells = <0x1>; /* skeleton.dtsi:14 */
	compatible = "qcom,apq8074-dragonboard", "qcom,apq8074"; /* qcom-apq8074-dragonboard.dts:8 */
	interrupt-parent = <0x1>; /* qcom-msm8974.dtsi:14 */
	model = "Qualcomm APQ8074 Dragonboard"; /* qcom-apq8074-dragonboard.dts:7 */

	adsp-pil { /* qcom-msm8974.dtsi:243 */

So I could achieve the equivalent of --annotate by post-processing, possible
with the result of slightly different line numbers if I'm sloppy.  But if
dtc can do the simplification, I don't need to add the additional external processing.

The question of whether to include just the starting line number, or the
starting line/column and ending line/column was the trade off between
more information vs less dense information being easier to scan and
read.

Having both --annotate and --annotate-full lets the user make the trade
off as needed in any particular case.

-Frank