Re: [PATCH] x86/tools: fix line number reported for malformed lines

[Valentin Obst]

> On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst <kernel@xxxxxxxxxxxxxxx> wrote:
> >
> > While debugging the `X86_DECODER_SELFTEST` failure first reported in [1],
> > [In this case the line causing the failure is interpreted as two lines by
> > the tool (due to its length, but this is fixed by [1, 2]), and the second
> > line is reported. Still the spatial closeness between the reported line and
> > the line causing the failure would have made debugging a lot easier.]
>
> Thanks Valentin, John et al. for digging into this (and the related
> issue) -- very much appreciated.
>
> It looks good to me:
>
> Reviewed-by: Miguel Ojeda <ojeda@xxxxxxxxxx>
> Tested-by: Miguel Ojeda <ojeda@xxxxxxxxxx>

Thanks!

>
> This should probably have a Fixes tag -- from a quick look, the
> original test did not seem to have the problem because `insns` was
> equivalent to the number of lines since there was no `if ... {
> continue; }` for the symbol case. At some point that branch was added,
> so that was not true anymore, thus that one should probably be the
> Fixes tag, though please double-check:
>
>     Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")

Cross checked this as well, can confirm your assessment. Thanks for
bringing this up.

>
> It is a minor issue for sure, so perhaps not worth backporting, but
> nevertheless the hash is in a very old kernel, and thus the issue
> applies to all stable kernels. So it does not hurt flagging it to the
> stable team:
>
>     Cc: stable@xxxxxxxxxxxxxxx
>
> In addition, John reported the original issue, but this one was found
> due to that one, and I am not exactly sure who did what here. Please
> consider whether one of the following (or similar) may be fair:
>
>     Reported-by: John Baublitz <john.m.baublitz@xxxxxxxxx>
>     Debugged-by: John Baublitz <john.m.baublitz@xxxxxxxxx>

Absolutely, without him reporting the test failure and narrowing down the
config I'd have never looked at this file. Adding him for **both** is fair.
(This particular fix was not discussed on Zulip though, its just something
I noticed along the way.)

>
> An extra Link to the discussion in Zulip could be nice too:
>
>     Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_decoder_test.20failure/near/421075039

Didn't add it because the discussion does not mention this particular
issue, but it might indeed be good for some context.

Will this need a v2, or are all of the 'Fixes', 'Reported-By',
'Debugged-By', 'Tested-By', 'Reviewed-By' and 'Link' tags something that
maintainers may add when merging?

    - Best Valentin

>
> Finally, a nit: links are typically written like the following -- you
> can still use bracket references at the end:
>
>     Link: https://lore.kernel.org/lkml/Y9ES4UKl%2F+DtvAVS@xxxxxxxxx/T/ [1]
>     Link: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collado@xxxxxxxxx/
> [2]
>
> Cheers,
> Miguel