Received-SPF: pass (google.com: domain of linux-kernel+bounces-74773-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de>
In-Reply-To: <20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de>
From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date: Wed, 21 Feb 2024 14:19:33 +0100
Message-ID: <CANiq72=gYWZ24EEqRL71Vh+YjjK9Bu0QfxGEBzee16QAf4Q6XA@mail.gmail.com>
Subject: Re: [PATCH] x86/tools: fix line number reported for malformed lines
To: Valentin Obst <kernel@valentinobst.de>
Cc: Ingo Molnar <mingo@kernel.org>, Ingo Molnar <mingo@redhat.com>, 
	Andreas Hindborg <a.hindborg@samsung.com>, John Baublitz <john.m.baublitz@gmail.com>, 
	David Rheinsberg <david@readahead.eu>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	"H. Peter Anvin" <hpa@zytor.com>, Masami Hiramatsu <mhiramat@kernel.org>, Miguel Ojeda <ojeda@kernel.org>, 
	Peter Zijlstra <peterz@infradead.org>, 
	=?UTF-8?Q?Sergio_Gonz=C3=A1lez_Collado?= <sergio.collado@gmail.com>, 
	Thomas Gleixner <tglx@linutronix.de>, linux-kernel@vger.kernel.org, x86@kernel.org, 
	stable@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 21, 2024 at 10:00=E2=80=AFAM 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 a=
nd
> 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>

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=
")

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>

An extra Link to the discussion in Zulip could be nice too:

    Link: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/t=
opic/insn_decoder_test.20failure/near/421075039

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-se=
rgio.collado@gmail.com/
[2]

Cheers,
Miguel