Commit 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
included symbol lines in the post-processed objdump output consumed by
the insn decoder test. This broke the `instuction lines == total lines`
property that `insn_decoder_test.c` relied upon to print the offending
line's number in error messages. This has the consequence that the line
number reported on a test failure is unreated to, and much smaller than,
the line that actually caused the problem.
Add a new variable that counts the combined (insn+symbol) line count and
report this in the error message.
Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
Cc: [email protected]
Reviewed-by: Miguel Ojeda <[email protected]>
Tested-by: Miguel Ojeda <[email protected]>
Reported-by: John Baublitz <[email protected]>
Debugged-by: John Baublitz <[email protected]>
Signed-off-by: Valentin Obst <[email protected]>
---
See v2's commit message and [1] for context why this bug made debugging a
test failure harder than necessary.
[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_decoder_test.20failure/near/421075039
Changes in v3:
- Add Cc stable tag in sign-off area.
- Make commit message less verbose.
- Link to v2: https://lore.kernel.org/r/20240223-x86-insn-decoder-line-fix-v2-1-cde49c69f402@valentinobst.de
Changes in v2:
- Added tags 'Reviewed-by', 'Tested-by', 'Reported-by', 'Debugged-by',
'Link', and 'Fixes'.
- Explain why this patch fixes the commit mentioned in the 'Fixes' tag.
- CCed the stable list and sent to all x86 maintainers.
- Link to v1: https://lore.kernel.org/r/20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de
---
arch/x86/tools/insn_decoder_test.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
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) {
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240221-x86-insn-decoder-line-fix-7b1f2e1732ff
Best regards,
--
Valentin Obst <[email protected]>
On Fri, 29 Mar 2024 12:31:58 +0100
Valentin Obst <[email protected]> wrote:
> Commit 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
> included symbol lines in the post-processed objdump output consumed by
> the insn decoder test. This broke the `instuction lines == total lines`
> property that `insn_decoder_test.c` relied upon to print the offending
> line's number in error messages. This has the consequence that the line
> number reported on a test failure is unreated to, and much smaller than,
> the line that actually caused the problem.
>
> Add a new variable that counts the combined (insn+symbol) line count and
> report this in the error message.
This looks good to me. Thanks!
Acked-by: Masami Hiramatsu (Google) <[email protected]>
>
> Fixes: 35039eb6b199 ("x86: Show symbol name if insn decoder test failed")
> Cc: [email protected]
> Reviewed-by: Miguel Ojeda <[email protected]>
> Tested-by: Miguel Ojeda <[email protected]>
> Reported-by: John Baublitz <[email protected]>
> Debugged-by: John Baublitz <[email protected]>
> Signed-off-by: Valentin Obst <[email protected]>
> ---
> See v2's commit message and [1] for context why this bug made debugging a
> test failure harder than necessary.
>
> [1]: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/insn_decoder_test.20failure/near/421075039
>
> Changes in v3:
> - Add Cc stable tag in sign-off area.
> - Make commit message less verbose.
> - Link to v2: https://lore.kernel.org/r/20240223-x86-insn-decoder-line-fix-v2-1-cde49c69f402@valentinobst.de
>
> Changes in v2:
> - Added tags 'Reviewed-by', 'Tested-by', 'Reported-by', 'Debugged-by',
> 'Link', and 'Fixes'.
> - Explain why this patch fixes the commit mentioned in the 'Fixes' tag.
> - CCed the stable list and sent to all x86 maintainers.
> - Link to v1: https://lore.kernel.org/r/20240221-x86-insn-decoder-line-fix-v1-1-47cd5a1718c6@valentinobst.de
> ---
> arch/x86/tools/insn_decoder_test.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> 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) {
>
> ---
> base-commit: 4cece764965020c22cff7665b18a012006359095
> change-id: 20240221-x86-insn-decoder-line-fix-7b1f2e1732ff
>
> Best regards,
> --
> Valentin Obst <[email protected]>
>
--
Masami Hiramatsu (Google) <[email protected]>