[PATCH 0/2] *** Fix reformat_objdump.awk ***

[PATCH 0/2] *** Fix reformat_objdump.awk ***

[Samuel Zeter]

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

[PATCH 1/2] arch/x86/tools/objdump_reformat.awk: Ensure regex matches fwait

[Samuel Zeter]

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 <samuelzeter@gmail.com>
---
 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

[PATCH 2/2] arch/x86/tools/objdump_reformat.awk: Allow for spaces

[Samuel Zeter]

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 <samuelzeter@gmail.com>
---
 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

Re: [PATCH 0/2] *** Fix reformat_objdump.awk ***

[Nathan Chancellor]

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 <nathan@kernel.org>

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/20220106023606.283953-1-samuelzeter@gmail.com/

[1]: https://git.kernel.org/nathan/c/2f137c324b21f1c21b8830d8896cb9957009f969

Cheers,
Nathan

Re: [PATCH 0/2] *** Fix reformat_objdump.awk ***

[Samuel Zeter]

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.

Re: [PATCH 0/2] *** Fix reformat_objdump.awk ***

[Nathan Chancellor]

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

[PATCH v2 0/2] *** Fix reformat_objdump.awk ***

[Sam Zeter]

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

[PATCH v2 1/2] arch/x86/tools/objdump_reformat.awk: Ensure regex matches fwait

[Sam Zeter]

From: Samuel Zeter <samuelzeter@gmail.com>

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

The regex match now accepts spaces or tabs, along with the "fwait"
instruction.

Signed-off-by: Samuel Zeter <samuelzeter@gmail.com>
---
 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 f418c91b7..276e572a6 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.35.1

[PATCH v2 2/2] arch/x86/tools/objdump_reformat.awk: Allow for spaces

[Sam Zeter]

From: Samuel Zeter <samuelzeter@gmail.com>

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.

The instruction line being tokenized now handles a space and colon,
or tab delimiter.

Signed-off-by: Samuel Zeter <samuelzeter@gmail.com>
---
 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 276e572a6..a4120d907 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.35.1

Re: [PATCH v2 0/2] *** Fix reformat_objdump.awk ***

[Nathan Chancellor]

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 <mhiramat@kernel.org>
Tested-by: Nathan Chancellor <nathan@kernel.org>

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/20220106023606.283953-1-samuelzeter@gmail.com/

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

Re: [PATCH 0/2] *** Fix reformat_objdump.awk ***

[Masami Hiramatsu]

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 <mhiramat@kernel.org>

for this series.

And if you resend it, please add "x86@kernel.org" to Cc.

Thank you!

On Thu, 6 Jan 2022 12:02:25 -0700
Nathan Chancellor <nathan@kernel.org> 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 <nathan@kernel.org>
> 
> 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/20220106023606.283953-1-samuelzeter@gmail.com/
> 
> [1]: https://git.kernel.org/nathan/c/2f137c324b21f1c21b8830d8896cb9957009f969
> 
> Cheers,
> Nathan


-- 
Masami Hiramatsu <mhiramat@kernel.org>