These are two small patches which originally dealt with
the problem found at:
https://github.com/ClangBuiltLinux/linux/issues/1364
The original steps to reproduce were:
$ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 defconfig
$ scripts/config -e X86_DECODER_SELFTEST
$ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 olddefconfig bzImage
Which resulted in the error:
arch/x86/tools/insn_decoder_test: warning: objdump says 0 bytes, but
insn_get_length() says 2
Upon inspection it turned out llvm-objdump was formatting its
output differently, which caused objdump_reformat.awk to incorrectly
output its values.
After fixing that bug, a second one was seen where the instruction
"wait" was incorrectly matched with "fwait", which again caused
insn_decoder_test to fail.
Samuel Zeter (2):
arch/x86/tools/objdump_reformat.awk: Ensure regex matches fwait
arch/x86/tools/objdump_reformat.awk: Allow for spaces
arch/x86/tools/objdump_reformat.awk | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.32.0
If there is "wait" mnemonic in the line being parsed, it is incorrectly
handled by the script, and an extra line of "fwait" in
objdump_reformat's output is inserted. As insn_decoder_test relies
upon the formatted output, the test fails.
This is reproducible when disassembling with llvm-objdump:
Pre-processed lines:
ffffffff81033e72: 9b wait
ffffffff81033e73: 48 c7 c7 89 50 42 82 movq
After objdump_reformat.awk:
ffffffff81033e72: 9b fwait
ffffffff81033e72: wait
ffffffff81033e73: 48 c7 c7 89 50 42 82 movq
This patch fixes the issue by requiring spaces, or tabs, along with the
"fwait" instruction in the regex match.
Signed-off-by: Samuel Zeter <[email protected]>
---
arch/x86/tools/objdump_reformat.awk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/tools/objdump_reformat.awk b/arch/x86/tools/objdump_reformat.awk
index f418c91b71f0..276e572a6f60 100644
--- a/arch/x86/tools/objdump_reformat.awk
+++ b/arch/x86/tools/objdump_reformat.awk
@@ -12,7 +12,7 @@ BEGIN {
prev_hex = ""
prev_mnemonic = ""
bad_expr = "(\\(bad\\)|^rex|^.byte|^rep(z|nz)$|^lock$|^es$|^cs$|^ss$|^ds$|^fs$|^gs$|^data(16|32)$|^addr(16|32|64))"
- fwait_expr = "^9b "
+ fwait_expr = "^9b[ \t]*fwait"
fwait_str="9b\tfwait"
}
--
2.32.0
objdump and llvm-objdump have differing output formats. Specifically,
objump will format its output as: address:<tab>hex, whereas
llvm-objdump displays its output as address:<space>hex.
objdump_reformat.awk incorrectly handles this discrepancy due to
the unexpected space and as a result insn_decoder_test fails, as
its input is garbled.
This patch ensures that the instruction line being tokenized can
support either a space and colon, or tab delimiter.
Signed-off-by: Samuel Zeter <[email protected]>
---
arch/x86/tools/objdump_reformat.awk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/tools/objdump_reformat.awk b/arch/x86/tools/objdump_reformat.awk
index 276e572a6f60..a4120d907277 100644
--- a/arch/x86/tools/objdump_reformat.awk
+++ b/arch/x86/tools/objdump_reformat.awk
@@ -22,7 +22,7 @@ BEGIN {
}
/^ *[0-9a-f]+:/ {
- if (split($0, field, "\t") < 3) {
+ if (split($0, field, /: |\t/) < 3) {
# This is a continuation of the same insn.
prev_hex = prev_hex field[2]
} else {
--
2.32.0
On Thu, Jan 06, 2022 at 01:36:03PM +1100, Samuel Zeter wrote:
> These are two small patches which originally dealt with
> the problem found at:
>
> https://github.com/ClangBuiltLinux/linux/issues/1364
>
> The original steps to reproduce were:
> $ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 defconfig
> $ scripts/config -e X86_DECODER_SELFTEST
> $ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 olddefconfig bzImage
>
> Which resulted in the error:
> arch/x86/tools/insn_decoder_test: warning: objdump says 0 bytes, but
> insn_get_length() says 2
>
> Upon inspection it turned out llvm-objdump was formatting its
> output differently, which caused objdump_reformat.awk to incorrectly
> output its values.
>
> After fixing that bug, a second one was seen where the instruction
> "wait" was incorrectly matched with "fwait", which again caused
> insn_decoder_test to fail.
Thanks a lot for sending these fixes!
I can confirm with this series and the removal of chkobjdump.awk [1] on
top of v5.16-rc8, the insn_decoder_test now passes with LLVM 11 through
14.
Tested-by: Nathan Chancellor <[email protected]>
For the future, I recommend putting the maintainers in the "To" field,
rather than "Cc", to ensure they actually see it. Additionally, I see
some small nits in the commit message that the tip maintainers might
comment on, see
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
for some more info.
Masami Hiramatsu originally wrote this file and has a few fixes to it
since, adding him now for review. The original thread is available at:
https://lore.kernel.org/r/[email protected]/
[1]: https://git.kernel.org/nathan/c/2f137c324b21f1c21b8830d8896cb9957009f969
Cheers,
Nathan
Thanks for the feedback, Nathan.
> For the future, I recommend putting the maintainers in the "To" field,
> rather than "Cc", to ensure they actually see it. Additionally, I see
> some small nits in the commit message that the tip maintainers might
> comment on, see
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
Thanks for the link, I missed that one. What were the nits apparent to you
in the commit message?
Sam.
On Fri, Jan 07, 2022 at 10:23:07AM +1100, Samuel Zeter wrote:
> Thanks for the feedback, Nathan.
>
> > For the future, I recommend putting the maintainers in the "To" field,
> > rather than "Cc", to ensure they actually see it. Additionally, I see
> > some small nits in the commit message that the tip maintainers might
> > comment on, see
> >
> > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
> >
> Thanks for the link, I missed that one. What were the nits apparent to you
> in the commit message?
I primarily just saw a couple instances of "This patch", which is
frowned upon in the main submitting patches document:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#submittingpatches
I thought I saw something else but upon further inspection, I didn't.
It is minor enough that I would wait for further review comments from
others to submit a v2.
Cheers,
Nathan
Hi Samuel and Nathan,
Thanks for working on this issue. I didn't noticed that the llvm has
this difference. Anyway both patches look good to me.
Acked-by: Masami Hiramatsu <[email protected]>
for this series.
And if you resend it, please add "[email protected]" to Cc.
Thank you!
On Thu, 6 Jan 2022 12:02:25 -0700
Nathan Chancellor <[email protected]> wrote:
> On Thu, Jan 06, 2022 at 01:36:03PM +1100, Samuel Zeter wrote:
> > These are two small patches which originally dealt with
> > the problem found at:
> >
> > https://github.com/ClangBuiltLinux/linux/issues/1364
> >
> > The original steps to reproduce were:
> > $ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 defconfig
> > $ scripts/config -e X86_DECODER_SELFTEST
> > $ make -skj"$(nproc)" LLVM=1 LLVM_IAS=1 olddefconfig bzImage
> >
> > Which resulted in the error:
> > arch/x86/tools/insn_decoder_test: warning: objdump says 0 bytes, but
> > insn_get_length() says 2
> >
> > Upon inspection it turned out llvm-objdump was formatting its
> > output differently, which caused objdump_reformat.awk to incorrectly
> > output its values.
> >
> > After fixing that bug, a second one was seen where the instruction
> > "wait" was incorrectly matched with "fwait", which again caused
> > insn_decoder_test to fail.
>
> Thanks a lot for sending these fixes!
>
> I can confirm with this series and the removal of chkobjdump.awk [1] on
> top of v5.16-rc8, the insn_decoder_test now passes with LLVM 11 through
> 14.
>
> Tested-by: Nathan Chancellor <[email protected]>
>
> For the future, I recommend putting the maintainers in the "To" field,
> rather than "Cc", to ensure they actually see it. Additionally, I see
> some small nits in the commit message that the tip maintainers might
> comment on, see
>
> https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog
>
> for some more info.
>
> Masami Hiramatsu originally wrote this file and has a few fixes to it
> since, adding him now for review. The original thread is available at:
>
> https://lore.kernel.org/r/[email protected]/
>
> [1]: https://git.kernel.org/nathan/c/2f137c324b21f1c21b8830d8896cb9957009f969
>
> Cheers,
> Nathan
--
Masami Hiramatsu <[email protected]>
v1 -> v2
- coding style commit message amendments.
Samuel Zeter (2):
arch/x86/tools/objdump_reformat.awk: Ensure regex matches fwait
arch/x86/tools/objdump_reformat.awk: Allow for spaces
arch/x86/tools/objdump_reformat.awk | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--
2.35.1
Hi Sam,
On Fri, Mar 04, 2022 at 02:16:09PM +1100, Sam Zeter wrote:
> v1 -> v2
> - coding style commit message amendments.
>
> Samuel Zeter (2):
> arch/x86/tools/objdump_reformat.awk: Ensure regex matches fwait
> arch/x86/tools/objdump_reformat.awk: Allow for spaces
>
> arch/x86/tools/objdump_reformat.awk | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thank you for sending v2!
Since you only updated the commit messages (i.e., no functional
changes), you should carry forward my tested-by and Masami's acked-by on
both patches:
Acked-by: Masami Hiramatsu <[email protected]>
Tested-by: Nathan Chancellor <[email protected]>
Additionally, for any future series/revisions, I would recommend leaving
the rest of the cover letter's message intact, so that people who missed
v1 are not in the dark on the purpose of the series (link provided for
convenience):
https://lore.kernel.org/r/[email protected]/
Regardless, those are just small process suggestions. I think this
should be ready to go, if the x86 maintainers have no objections. Great
work!
Cheers,
Nathan