lkml.org 
[lkml]   [2024]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] x86/tools: fix line number reported for malformed lines
Date
> On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst <kernel@valentinobst.de> 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@kernel.org>
> Tested-by: Miguel Ojeda <ojeda@kernel.org>

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@vger.kernel.org
>
> 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@gmail.com>
> Debugged-by: John Baublitz <john.m.baublitz@gmail.com>

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@gmail.com/T/ [1]
> Link: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergio.collado@gmail.com/
> [2]
>
> Cheers,
> Miguel

\
 
 \ /
  Last update: 2024-05-27 15:15    [W:0.055 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site