linux-kernel - Re: [GIT PULL 00/13] Annotation improvements (G+ edition)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+55aFxY0MWR2vTRwf7a6+wm0gXet-X+i11=V_OapneciTJJMg@mail.gmail.com>
Date:	Thu, 19 Apr 2012 17:31:04 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Arnaldo Carvalho de Melo <acme@...radead.org>
Cc:	Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
	David Ahern <dsahern@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Mike Galbraith <efault@....de>,
	Namhyung Kim <namhyung@...il.com>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Stephane Eranian <eranian@...gle.com>, arnaldo.melo@...il.com,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [GIT PULL 00/13] Annotation improvements (G+ edition)

On Thu, Apr 19, 2012 at 1:33 PM, Arnaldo Carvalho de Melo
<acme@...radead.org> wrote:
>
> Annotate improvements
>
> Now the default annotate browser uses a much more compact format, implementing
> suggestions made made by several people, notably Linus.
>
> Here is part of the new __list_del_entry annotation:

Ok, so I did a quick test-run, and things are definitely more readable.

However, from a "where is the loop" angle, the one thing that is
actually fairly hard to visually see is the branches.

Sure, you can find them, but they are not nearly as easy to pick out
visually as the branch targets are, and I do wonder if it wouldn't
make sense to try to make them stand out a bit more.

What is visually important is
 (a) seeing that it's a branch
 (b) seeing the direction of the branch (to pick up on loops more easily).

I suspect that doing a whole arrow is just going to be a horrible
criss-crossing mess (although with some graphical UI it might not be
as bad as with the textual one), but I think even just doing it in
entirely local scope of the particular line that contains the branch
would be perfectly fine.

This, for example, is a snippet of a pretty simple function
(avc_lookup()) with a few branches and a few branch targets:

    2.08 │      test   %rdx,%rdx
    0.00 │      jne    38
    0.00 │      jmp    53
    5.13 │ 30:  mov    (%rdx),%rdx
    0.74 │      test   %rdx,%rdx
    0.00 │      je     53
    0.06 │ 38:  lea    -0x20(%rdx),%rax
   59.64 │      cmp    -0x20(%rdx),%edi
    0.40 │      jne    30
   17.36 │      cmp    -0x18(%rdx),%cx
    0.00 │      jne    30
    1.91 │      cmp    0x4(%rax),%esi
    0.00 │      jne    30
    0.22 │      test   %rax,%rax
    0.20 │      je     53
    1.68 │      leaveq
    1.90 │      retq
    0.00 │ 53:  incl   %gs:0xe214
    0.00 │      xor    %eax,%eax

and the branch targets are fairly easy to pick up, but seeing the
branches - and their directions - themselves takes much more parsing.
Ok, it's actually pretty simple above, because they have a faily
obvious pattern, but in bigger functions they really are harder to
pick out.

Now, what if the branch instruction was just followed with a simple
"arrow up/down" thing, and to make the distinction really simple to
see visually, make the indentation slightly different for the
back/forwards case, so that you don't have to really look at the
(crappy) arrow head to find possible loops (backwards jumps).

The above snippet would then become:

    2.08        test   %rdx,%rdx
    0.00        jne    38                  ----v
    0.00        jmp    53                  ----v
    5.13   30:  mov    (%rdx),%rdx
    0.74        test   %rdx,%rdx
    0.00        je     53                  ----v
    0.06   38:  lea    -0x20(%rdx),%rax
   59.64        cmp    -0x20(%rdx),%edi
    0.40        jne    30                      ----^
   17.36        cmp    -0x18(%rdx),%cx
    0.00        jne    30                      ----^
    1.91        cmp    0x4(%rax),%esi
    0.00        jne    30                      ----^
    0.22        test   %rax,%rax
    0.20        je     53                  ----v
    1.68        leaveq
    1.90        retq
    0.00   53:  incl   %gs:0xe214
    0.00        xor    %eax,%eax

where I think it's fairly easy to pick up the branches. Sure, once you
really need to connect them, you do need to compare the offsets (easy
in the above case, less obvious with big complex functions), but just
making it easy to *see* the backwards branches makes it easier to see
the loop, methinks.

I dunno. Just a suggestion. I definitely like the new format better
already, regardless of any arrows.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/