Received-SPF: pass (google.com: domain of linux-kernel+bounces-75579-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: Valentin Obst <kernel@valentinobst.de>
To: miguel.ojeda.sandonis@gmail.com
Cc: a.hindborg@samsung.com,
	david@readahead.eu,
	gregkh@linuxfoundation.org,
	hpa@zytor.com,
	john.m.baublitz@gmail.com,
	kernel@valentinobst.de,
	linux-kernel@vger.kernel.org,
	mhiramat@kernel.org,
	mingo@kernel.org,
	mingo@redhat.com,
	ojeda@kernel.org,
	peterz@infradead.org,
	sergio.collado@gmail.com,
	stable@vger.kernel.org,
	tglx@linutronix.de,
	x86@kernel.org
Subject: Re: [PATCH] x86/tools: fix line number reported for malformed lines
Date: Wed, 21 Feb 2024 22:49:32 +0100
Message-ID: <20240221214939.4715-1-kernel@valentinobst.de>
In-Reply-To: <CANiq72=gYWZ24EEqRL71Vh+YjjK9Bu0QfxGEBzee16QAf4Q6XA@mail.gmail.com>
References: <CANiq72=gYWZ24EEqRL71Vh+YjjK9Bu0QfxGEBzee16QAf4Q6XA@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:mOOoXRIRWVQ=;kxOewaJtG6hZXrCnHemyZf/SZrx
 QQ/Q+2L73TzGrKO2bu0S6XlU3HcPKWw7Rzm/0CstjBGrSiMIvOMpA3mjerIkMfPA85ENlvqiO
 oG+N6zVs4Jiyzwapz3JUW21+DhI9MhP7kaRmKcAXbRw0Kc+22XNubcetaXPk5ptrIkwHmoNYk
 vuK8EBaL3+OKf4Be+i5yvcYkC5x3Y0VdK9OQUWypSvBaONhscT+X3BhQ8fdYcoq7bj9SgIjlw
 bhHMO35bgNV9JUfexPKDPLgt37o7xkw51NkTKOIwrwSP9QIjWohMwFGVbI26UEOolSena7OZy
 ewPGNvePYYb2DPWNgUvtzdXo4XuIsc9KZZ9AMqltppIzct0YC/sEtimbdQ6Lg0C1x5VewNJq7
 vdbvwrgFkNcoDyFYBbzWIalK/TTAj2PJ9uFM8D4BEKKTZ6YC6WDo8fqM+QNH0Q1IjxVlT3xDV
 V8rKbfhoQiIvpiEq1Vyedm6vckWTllAw4DGAUWCGf7dyXsQcXFYpOHf4y3sBJTEwEgknL3YvG
 RKFlsE7SxFTln3jlDSlSwBnTPk0LOOAR4XeXj9x8duNdzKh2C9M6j9BQ2/KtAdXYdsbF2C/ls
 r+Rc9mbEdx7HdC62kFlQvB1NtLLpz7/CnpWzNoi8PSbig3tlBM5Mreji4uQcfVM6s0EUEqptT
 qHT90jIX3GJ20xOAUDM7E7nxf1Wn9/FCa4RLCpCXDlC/UfQe2/HLvIe9BDoSd+31HqgBJIIYO
 t74x7AwE/GuYbpMlFNajsNmySR/RH3oHaqsCSPu2k2BlYfiv719eUw=

> On Wed, Feb 21, 2024 at 10:00=E2=80=AFAM Valentin Obst <kernel@valentino=
bst.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 sec=
ond
> > line is reported. Still the spatial closeness between the reported lin=
e 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 fai=
led")

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-Hel=
p/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