Message ID | 20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-74339-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:693c:2685:b0:108:e6aa:91d0 with SMTP id mn5csp915388dyc; Wed, 21 Feb 2024 01:01:03 -0800 (PST) X-Forwarded-Encrypted: i=2; AJvYcCX5YJto2l51X13ex/WigmhQhzjAzosxUYVgYIEbP19lRRp0F7fofPiqsH37PpqDDPDxafqHo0vq7p1b3zf/wQ68Gl2F6Q== X-Google-Smtp-Source: AGHT+IEKnNvGKdOkZ5hcf0QYUmO9OFtzp6pDOD/6jfKv5qCjqvnxGOmvehb7zoFZkw0L5LO7NtNe X-Received: by 2002:a17:906:ac5:b0:a3e:ac2e:65b7 with SMTP id z5-20020a1709060ac500b00a3eac2e65b7mr5265538ejf.57.1708506063523; Wed, 21 Feb 2024 01:01:03 -0800 (PST) Received: from am.mirrors.kernel.org (am.mirrors.kernel.org. [147.75.80.249]) by mx.google.com with ESMTPS id w25-20020a170906481900b00a3e9055c0d7si2751690ejq.947.2024.02.21.01.01.03 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Feb 2024 01:01:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-74339-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@valentinobst.de header.s=s1-ionos header.b=gEGdeNEd; arc=fail (body hash mismatch); spf=pass (google.com: domain of linux-kernel+bounces-74339-ouuuleilei=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) smtp.mailfrom="linux-kernel+bounces-74339-ouuuleilei=gmail.com@vger.kernel.org" Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by am.mirrors.kernel.org (Postfix) with ESMTPS id F2CA81F23CDE for <ouuuleilei@gmail.com>; Wed, 21 Feb 2024 09:01:02 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 54D923D3A0; Wed, 21 Feb 2024 09:00:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=valentinobst.de header.i=kernel@valentinobst.de header.b="gEGdeNEd" Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.17.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 487BF3CF72 for <linux-kernel@vger.kernel.org>; Wed, 21 Feb 2024 09:00:39 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=212.227.17.10 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708506042; cv=none; b=cC95Zji5n+jEnlWg6eHH2V8WDr5Yuwmz2FTfi3Ij+aTsEgDq1EGFrZbk50cP+5wHW8JGeecpxgDz7/M4df40hFGyg6a03Di0JlWsWY0/nNbbY3xkmvCN1jlc0cV3GKmsvItiU2XF68Zh6twOW7jsQ66dfdaA/pmJN0kRnZGi3mk= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1708506042; c=relaxed/simple; bh=lOk/Sj21EXxCRmBe+YSmEWfqFcljKsQ0DRuxtfgqqT0=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:To:Cc; b=s92lYTg1Pp11zczyhGDHzAFYayfA+VRZa8maWIUPtVI7e40N2n0vBHt/hIWg14SzPxBPnINEKw9LcFCPVtQydlrO+D+ssu+PCl6Iyn0QiXQvaLWikuRrYCwuMgVgW8aaKQ53q+BVtfxQNwyN+ZXcOgkiVWGbrIBnzLCtj+uP+ng= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=valentinobst.de; spf=pass smtp.mailfrom=valentinobst.de; dkim=pass (2048-bit key) header.d=valentinobst.de header.i=kernel@valentinobst.de header.b=gEGdeNEd; arc=none smtp.client-ip=212.227.17.10 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=valentinobst.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=valentinobst.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=valentinobst.de; s=s1-ionos; t=1708506030; x=1709110830; i=kernel@valentinobst.de; bh=lOk/Sj21EXxCRmBe+YSmEWfqFcljKsQ0DRuxtfgqqT0=; h=X-UI-Sender-Class:From:Date:Subject:To:Cc; b=gEGdeNEdnMF8XKOrjBbVFcFz8tB7/dKO+hCk8n4MeBxU+pBTLNmEY4K75BA2tttk gtbQMFJuPNoUzlnbUTXqEeYnbQ51B+K56zgrZQa3CaUeSPuCz3hxWmfUPsJ+EpjlW zXK3MFjmybz74et4o6Snf3WkIuiZHBk3devfZ4lsT5Z7xmPBxOKnIusdPXX/iZYbX pSRzc/Saa6jkAzYezvbJAPsIBFovozXdhSxjO2491Qmro0MFHD7p9DjJGp14StJ7I wM8jorlnyecylKWKVmZP7n6pMeQnYo1RKgEwIomKiqsgcpcMA/2AeLln3Fy+eA7Xm yGPi+0dzx8j0CB9m/g== X-UI-Sender-Class: 55c96926-9e95-11ee-ae09-1f7a4046a0f6 Received: from [192.168.2.229] ([79.233.63.110]) by mrelayeu.kundenserver.de (mreue108 [213.165.67.113]) with ESMTPSA (Nemesis) id 1N6KQZ-1qs7DZ2vsH-016j74; Wed, 21 Feb 2024 09:54:04 +0100 From: Valentin Obst <kernel@valentinobst.de> Date: Wed, 21 Feb 2024 09:53:37 +0100 Subject: [PATCH] x86/tools: fix line number reported for malformed lines Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de> X-B4-Tracking: v=1; b=H4sIABC61WUC/x2MwQqDMBAFf0X27EKyFlP6K+JBzYtdKKskIIL47 4YeZ2DmooKsKPRpLso4tOhmFXzb0PKdbAVrrEzi5OVEPJ/vntWKccSyRWT+qYGTnhxmnwQ+dJI S1X7PqPr/Hsb7fgBrqPPNawAAAA== To: Ingo Molnar <mingo@kernel.org>, Ingo Molnar <mingo@redhat.com> Cc: 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, Valentin Obst <kernel@valentinobst.de> X-Mailer: b4 0.13.0 X-Developer-Signature: v=1; a=ed25519-sha256; t=1708505639; l=4033; i=kernel@valentinobst.de; s=20240131; h=from:subject:message-id; bh=6jeuUZIWEEM6MczBBiqjTLcxOXq2Iex7vvCz0nnWibU=; b=vXw4UH3+h4Qo0u+jiIR3NzJr4RQ9SMlFuITEcLk/vPHJsi6whbcvNbyijjDj9XN0DjBIYg/FV lTfVcXooyCEDOAQGIQIh1dkqZwedw7Vykc53VahcxLmO3k1TzO3oxEi X-Developer-Key: i=kernel@valentinobst.de; a=ed25519; pk=3s7U8y0mqkaiurgHSQQTYWOo2tw5HgzCg5vnJVfw37Y= X-Provags-ID: V03:K1:8CQzPJdQOevycA67JGlkGzRtIoqyGOvtDOELvrhAlxXlRmeumAw cyMmaQAENoJQGOAzF5Pv8KuiJJO4FjsxaaMSo0Ix69jlCjZ1JMVGJSy8M5zvJe0BY1gZIgK VjGeMniG37PWoh8uMaDSBy/tZ8/S6Ib0uLef55V5Y2CUFFVfCg9Excxkvk4CpbUEZqhXNWR nv9Lh8Lb9a1HBI9YRKFwg== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:BlQJO2SEwwM=;giTIobp/tT9baZQjNYx+KP3oR6G BKjF31uEjg5Uv1fyIwznEnBu+8XXRq3NDOvZS3btIqZvof2KY/FX68Dtzqt5t2ymgdUmJA2G1 jV2nkbdHbeXSrTYnTBbMP1Qx1JCGyg7Fyh+6+L7epc8quLv+nwsayV3R+ij0JY54K6awOthMb kbzMHUkvuU4jtAy/Mw/o1JufpYcTNA4GY6ywLounjl11FwgYgAEwvYRdiZiw0+UdmCBJVRFKP IXpfULIavtVKTU7gMOCLlc2K2ujb1oFBw117auc7/1ln8G7XsWS/VWmjBOews1CtLKa8CxxtV IqkRf5hd5X/9NyKTc625iObWD2sc6J6Zt2Qe+6pOmEqah0gBqschqEugXd2siCCWmZ+zMzgBu viD5nX2vtqZkck6x4XGf4BjoLGIFZjrQEjknVcmTIwdVaQQrDsurQs9nlTWHxDloNFpXI2obg uzxJrB+EEzJeWWvXMbxY0Wzo6yMnLv8B4tz0alU407RJRgXsTkJS2cCgrRx7xnGZtBqJdDrpc SjkdtBcjGGQU6Isy3qxoSgD+rYbpORl5TGYODEYrGoUJlJQhd8uJovgJxRqfqNI0EmSseapg9 QCyKUcQoaPcMKokSccIyNDSoTvBeoTpHq7HQj6BFAlRo+Txo4ZUkfB8K2qiAcohTEpLWcS+vk 3Nd45+BXeDb922Uph/r9jwAzjwxvVEqSNgVs4+YfV9Ew4lcNqcv6/0DcJpDer6SYifJLPBtN3 G2sPwW7+rc3yYmvqv2aeca7nXyyanr3D2Fl9E27ihoIURWCuts3JrI= X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1791498454366368791 X-GMAIL-MSGID: 1791498454366368791 |
Series |
x86/tools: fix line number reported for malformed lines
|
|
Commit Message
Valentin Obst
Feb. 21, 2024, 8:53 a.m. UTC
While debugging the `X86_DECODER_SELFTEST` failure first reported in [1],
we noticed that the line numbers reported by the `insn_decoder_test` tool
do not correspond to the line in the output of `objdump_reformat.awk` that
was causing the failure:
# TEST posttest
llvm-objdump -d -j .text ./vmlinux | \
awk -f ./arch/x86/tools/objdump_reformat.awk | \
arch/x86/tools/insn_decoder_test -y -v
arch/x86/tools/insn_decoder_test: error: malformed line 1657116:
68db0
$ llvm-objdump -d -j .text ./vmlinux | \
awk -f ./arch/x86/tools/objdump_reformat.awk > objdump_reformat.txt
$ head -n `echo 1657116+2 | bc` objdump_reformat.txt | tail -n 5
ffffffff815430b1 41 8b 47 1c movl
ffffffff815430b5 89 c1 movl
ffffffff815430b7 81 c9 00 40 00 00 orl
ffffffff815430bd 41 89 4e 18 movl
ffffffff815430c1 a8 40 testb
These lines are perfectly fine. The reason is that the line count reported
by the tool only includes instruction lines, i.e., it does not count symbol
lines.
Add a new variable to count the combined (insn+symbol) line count and
report this in the error message. With this patch, the line reported by the
tool is the line causing the failure (long line wrapped at 75 chars):
# TEST posttest
llvm-objdump -d -j .text ./vmlinux | \
awk -f ./arch/x86/tools/objdump_reformat.awk | \
arch/x86/tools/insn_decoder_test -y -v
arch/x86/tools/insn_decoder_test: error: malformed line 1699686:
68db0
$ head -n ` echo 1699686+2 | bc` objdump_reformat.txt | tail -n 5
ffffffff81568dac c3 retq
<_RNvXsP_NtCs7qddEHlz8fK_4core3fmtRINtNtNtNtB7_4iter8adapters5chain5Chain
INtNtBA_7flatten7FlattenINtNtB7_6option8IntoIterNtNtB7_4char11EscapeDebug
EEINtB1a_7FlatMapNtNtNtB7_3str4iter5CharsB1T_NtB2D_23CharEscapeDebugConti
nueEENtB5_5Debug3fmtB7_>:ffffffff81568db0
ffffffff81568dad 0f 1f 00 nopl
ffffffff81568db0 f3 0f 1e fa endbr64
ffffffff81568db4 41 56 pushq
[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.]
[1]: https://lore.kernel.org/lkml/Y9ES4UKl%2F+DtvAVS@gmail.com/T/
[2]: https://lore.kernel.org/rust-for-linux/20231119180145.157455-1-sergiocollado@gmail.com/
Signed-off-by: Valentin Obst <kernel@valentinobst.de>
---
arch/x86/tools/insn_decoder_test.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
---
base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d
change-id: 20240221-x86-insn-decoder-line-fix-7b1f2e1732ff
Best regards,
--
Valentin Obst <kernel@valentinobst.de>
Comments
On Wed, Feb 21, 2024 at 10:00 AM Valentin Obst <kernel@valentinobstde> 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> 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/topic/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-sergio.collado@gmail.com/ [2] Cheers, Miguel
> 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
On Wed, Feb 21, 2024 at 10:51 PM Valentin Obst <kernel@valentinobstde> wrote: > > Thanks! > > Cross checked this as well, can confirm your assessment. Thanks for > bringing this up. My pleasure! > 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.) In that case, up to you -- whatever you consider fair for this particular patch. > Didn't add it because the discussion does not mention this particular > issue, but it might indeed be good for some context. Makes sense -- I saw the [1] reference and I thought it could be a nice complement to it, but it is true that it may be not that useful, so please feel free to leave it out. > 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? Typically, tags are picked up by maintainers when they apply the patch (if it is the last version, otherwise you would already pick them up in the next version you send). However, in this case, since we have the Cc stable@ and you also have the most context to decide on the tags (e.g. for the Reported-by etc.), I would send a v2. Thanks! Cheers, Miguel
diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c index 472540aeabc2..727017a3c3c7 100644 --- a/arch/x86/tools/insn_decoder_test.c +++ b/arch/x86/tools/insn_decoder_test.c @@ -114,6 +114,7 @@ int main(int argc, char **argv) unsigned char insn_buff[16]; struct insn insn; int insns = 0; + int lines = 0; int warnings = 0; parse_args(argc, argv); @@ -123,6 +124,8 @@ int main(int argc, char **argv) int nb = 0, ret; unsigned int b; + lines++; + if (line[0] == '<') { /* Symbol line */ strcpy(sym, line); @@ -134,12 +137,12 @@ int main(int argc, char **argv) strcpy(copy, line); tab1 = strchr(copy, '\t'); if (!tab1) - malformed_line(line, insns); + malformed_line(line, lines); s = tab1 + 1; s += strspn(s, " "); tab2 = strchr(s, '\t'); if (!tab2) - malformed_line(line, insns); + malformed_line(line, lines); *tab2 = '\0'; /* Characters beyond tab2 aren't examined */ while (s < tab2) { if (sscanf(s, "%x", &b) == 1) {