| Message ID | 20140726163551.19857.17737.stgit@zurg (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2014-07-26 18:35, Konstantin Khlebnikov wrote: > This already has been fixed in commit c353acba28fb3fa1fd05fd > ("kbuild: make: fix if_changed when command contains backslashes") > but escaping still isn't perfect and triggers false-positive rebuilds. > > For x86 problem happens every time, because rules in arch/x86/realmode/rm/ > and arch/x86/boot/ contains commands like sed -n -e 's/foo\(.*\)/\1/p'. > Backslash in \1 isn't escaped and turns into ascii symbol with code 1. > Macro if_changed detects command change and rebuilds target again and again. > > Backslash escaping conflicts with other passes because it's used for escaping > other symbols. To avoid that current macro handles only double backslashes. > Obviously this doesn't work for \1 like above. > > This patch reorders passes. It doubles all backslashes before escaping # and ' > > Visible effect in rebuilding x86/defconfig without changes, before patch: > > blind@zurg:~/src/linux$ make V=2 > CHK include/config/kernel.release > CHK include/generated/uapi/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh - due to target missing > CHK include/generated/compile.h > PASYMS arch/x86/realmode/rm/pasyms.h - due to command line change With which make and shell version are you seeing this? While the patch looks correct, I can't reproduce the error here: $ make V=2 CHK include/config/kernel.release CHK include/generated/uapi/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh - due to target missing CHK include/generated/compile.h Kernel: arch/x86/boot/bzImage is ready (#1) Building modules, stage 2. MODPOST 12 modules - due to target is PHONY $ cat arch/x86/realmode/rm/.pasyms.h.cmd cmd_arch/x86/realmode/rm/pasyms.h := nm arch/x86/realmode/rm/header.o arch/x86/realmode/rm/trampoline_64.o arch/x86/realmode/rm/stack.o arch/x86/realmode/rm/reboot.o arch/x86/realmode/rm/wakeup_asm.o arch/x86/realmode/rm/wakemain.o arch/x86/realmode/rm/video-mode.o arch/x86/realmode/rm/copy.o arch/x86/realmode/rm/bioscall.o arch/x86/realmode/rm/regs.o arch/x86/realmode/rm/video-vga.o arch/x86/realmode/rm/video-vesa.o arch/x86/realmode/rm/video-bios.o | sed -n -r -e 's/^([0-9a-fA-F]+) [ABCDGRSTVW] (.+)$$/pa_\2 = \2;/p' | sort | uniq > arch/x86/realmode/rm/pasyms.h This is with both 3.16 and my kbuild branch, which is based on 3.16-rc1. Also, in the description, you say that backslash escaping conflicts with escaping # and ', but this has nothing to do with the \2 backreference in the sed command. Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 6, 2014 at 3:45 PM, Michal Marek <mmarek@suse.cz> wrote: > On 2014-07-26 18:35, Konstantin Khlebnikov wrote: >> This already has been fixed in commit c353acba28fb3fa1fd05fd >> ("kbuild: make: fix if_changed when command contains backslashes") >> but escaping still isn't perfect and triggers false-positive rebuilds. >> >> For x86 problem happens every time, because rules in arch/x86/realmode/rm/ >> and arch/x86/boot/ contains commands like sed -n -e 's/foo\(.*\)/\1/p'. >> Backslash in \1 isn't escaped and turns into ascii symbol with code 1. >> Macro if_changed detects command change and rebuilds target again and again. >> >> Backslash escaping conflicts with other passes because it's used for escaping >> other symbols. To avoid that current macro handles only double backslashes. >> Obviously this doesn't work for \1 like above. >> >> This patch reorders passes. It doubles all backslashes before escaping # and ' >> >> Visible effect in rebuilding x86/defconfig without changes, before patch: >> >> blind@zurg:~/src/linux$ make V=2 >> CHK include/config/kernel.release >> CHK include/generated/uapi/linux/version.h >> CHK include/generated/utsrelease.h >> CALL scripts/checksyscalls.sh - due to target missing >> CHK include/generated/compile.h >> PASYMS arch/x86/realmode/rm/pasyms.h - due to command line change > > With which make and shell version are you seeing this? While the patch > looks correct, I can't reproduce the error here: /bin/sh points to dash (debian default setup). I cannot reproduce this using bash. That explains why this bug is still here. > > $ make V=2 > CHK include/config/kernel.release > CHK include/generated/uapi/linux/version.h > CHK include/generated/utsrelease.h > CALL scripts/checksyscalls.sh - due to target missing > CHK include/generated/compile.h > Kernel: arch/x86/boot/bzImage is ready (#1) > Building modules, stage 2. > MODPOST 12 modules - due to target is PHONY > $ cat arch/x86/realmode/rm/.pasyms.h.cmd > cmd_arch/x86/realmode/rm/pasyms.h := nm arch/x86/realmode/rm/header.o > arch/x86/realmode/rm/trampoline_64.o arch/x86/realmode/rm/stack.o > arch/x86/realmode/rm/reboot.o arch/x86/realmode/rm/wakeup_asm.o > arch/x86/realmode/rm/wakemain.o arch/x86/realmode/rm/video-mode.o > arch/x86/realmode/rm/copy.o arch/x86/realmode/rm/bioscall.o > arch/x86/realmode/rm/regs.o arch/x86/realmode/rm/video-vga.o > arch/x86/realmode/rm/video-vesa.o arch/x86/realmode/rm/video-bios.o | > sed -n -r -e 's/^([0-9a-fA-F]+) [ABCDGRSTVW] (.+)$$/pa_\2 = \2;/p' | > sort | uniq > arch/x86/realmode/rm/pasyms.h > > This is with both 3.16 and my kbuild branch, which is based on 3.16-rc1. > Also, in the description, you say that backslash escaping conflicts with > escaping # and ', but this has nothing to do with the \2 backreference > in the sed command. I'm not quite understand what the difference between escaping standalone backslash ( ' \ ' -> \' \\ \' ) that backreference ( ' \2 ' -> \' \\2 \' ) We have to double backslashes, but this must be done before escaping # and ', otherwise doubling undoes their escaping. > > Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-08-06 14:19, Konstantin Khlebnikov wrote: > On Wed, Aug 6, 2014 at 3:45 PM, Michal Marek <mmarek@suse.cz> wrote: >> On 2014-07-26 18:35, Konstantin Khlebnikov wrote: >>> This already has been fixed in commit c353acba28fb3fa1fd05fd >>> ("kbuild: make: fix if_changed when command contains backslashes") >>> but escaping still isn't perfect and triggers false-positive rebuilds. >>> >>> For x86 problem happens every time, because rules in arch/x86/realmode/rm/ >>> and arch/x86/boot/ contains commands like sed -n -e 's/foo\(.*\)/\1/p'. >>> Backslash in \1 isn't escaped and turns into ascii symbol with code 1. >>> Macro if_changed detects command change and rebuilds target again and again. >>> >>> Backslash escaping conflicts with other passes because it's used for escaping >>> other symbols. To avoid that current macro handles only double backslashes. >>> Obviously this doesn't work for \1 like above. >>> >>> This patch reorders passes. It doubles all backslashes before escaping # and ' >>> >>> Visible effect in rebuilding x86/defconfig without changes, before patch: >>> >>> blind@zurg:~/src/linux$ make V=2 >>> CHK include/config/kernel.release >>> CHK include/generated/uapi/linux/version.h >>> CHK include/generated/utsrelease.h >>> CALL scripts/checksyscalls.sh - due to target missing >>> CHK include/generated/compile.h >>> PASYMS arch/x86/realmode/rm/pasyms.h - due to command line change >> >> With which make and shell version are you seeing this? While the patch >> looks correct, I can't reproduce the error here: > > /bin/sh points to dash (debian default setup). > > I cannot reproduce this using bash. That explains why this bug is still here. So the difference between the shells is that their 'echo' builtin treats \<number> differently: $ ./dash -c "echo '\2'" $ ./dash -c "echo '\2'" | xxd 0000000: 020a .. $ /bin/bash -c "echo '\2'" \2 For some reason we fail to escape the \ and dash treats \2 as \02. POSIX says that this is implementation-defined, so none of the shells is wrong. I need to look into this a bit further. I'll most likely end up applying your patch, but I'd like to understand the fix in detail first. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index 122f95c..edd0b0d 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -219,7 +219,7 @@ endif # >$< substitution to preserve $ when reloading .cmd file # note: when using inline perl scripts [perl -e '...$$t=1;...'] # in $(cmd_xxx) double $$ your perl vars -make-cmd = $(subst \\,\\\\,$(subst \#,\\\#,$(subst $$,$$$$,$(call escsq,$(cmd_$(1)))))) +make-cmd = $(call escsq,$(subst \#,\\\#,$(subst \,\\,$(subst $$,$$$$,$(cmd_$(1)))))) # Find any prerequisites that is newer than target or that does not exist. # PHONY targets skipped in both cases.
This already has been fixed in commit c353acba28fb3fa1fd05fd ("kbuild: make: fix if_changed when command contains backslashes") but escaping still isn't perfect and triggers false-positive rebuilds. For x86 problem happens every time, because rules in arch/x86/realmode/rm/ and arch/x86/boot/ contains commands like sed -n -e 's/foo\(.*\)/\1/p'. Backslash in \1 isn't escaped and turns into ascii symbol with code 1. Macro if_changed detects command change and rebuilds target again and again. Backslash escaping conflicts with other passes because it's used for escaping other symbols. To avoid that current macro handles only double backslashes. Obviously this doesn't work for \1 like above. This patch reorders passes. It doubles all backslashes before escaping # and ' Visible effect in rebuilding x86/defconfig without changes, before patch: blind@zurg:~/src/linux$ make V=2 CHK include/config/kernel.release CHK include/generated/uapi/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh - due to target missing CHK include/generated/compile.h PASYMS arch/x86/realmode/rm/pasyms.h - due to command line change LDS arch/x86/realmode/rm/realmode.lds - due to: arch/x86/realmode/rm/pasyms.h LD arch/x86/realmode/rm/realmode.elf - due to: arch/x86/realmode/rm/realmode.lds RELOCS arch/x86/realmode/rm/realmode.relocs - due to: arch/x86/realmode/rm/realmode.elf OBJCOPY arch/x86/realmode/rm/realmode.bin - due to: arch/x86/realmode/rm/realmode.elf arch/x86/realmode/rm/realmode.relocs AS arch/x86/realmode/rmpiggy.o - due to: arch/x86/realmode/rm/realmode.bin LD arch/x86/realmode/built-in.o - due to: arch/x86/realmode/rmpiggy.o LD arch/x86/built-in.o - due to: arch/x86/realmode/built-in.o LINK vmlinux - due to: arch/x86/built-in.o LD vmlinux.o MODPOST vmlinux.o - due to vmlinux.o not in $(targets) GEN .version CHK include/generated/compile.h UPD include/generated/compile.h CC init/version.o - due to: include/generated/compile.h LD init/built-in.o - due to: init/version.o KSYM .tmp_kallsyms1.o KSYM .tmp_kallsyms2.o LD vmlinux SORTEX vmlinux SYSMAP System.map VOFFSET arch/x86/boot/voffset.h - due to: vmlinux OBJCOPY arch/x86/boot/compressed/vmlinux.bin - due to: vmlinux GZIP arch/x86/boot/compressed/vmlinux.bin.gz - due to: arch/x86/boot/compressed/vmlinux.bin MKPIGGY arch/x86/boot/compressed/piggy.S - due to: arch/x86/boot/compressed/vmlinux.bin.gz AS arch/x86/boot/compressed/piggy.o - due to: arch/x86/boot/compressed/piggy.S LD arch/x86/boot/compressed/vmlinux - due to: arch/x86/boot/compressed/piggy.o ZOFFSET arch/x86/boot/zoffset.h - due to: arch/x86/boot/compressed/vmlinux AS arch/x86/boot/header.o - due to: arch/x86/boot/voffset.h arch/x86/boot/zoffset.h CC arch/x86/boot/version.o - due to: include/generated/compile.h LD arch/x86/boot/setup.elf - due to: arch/x86/boot/header.o arch/x86/boot/version.o OBJCOPY arch/x86/boot/setup.bin - due to: arch/x86/boot/setup.elf OBJCOPY arch/x86/boot/vmlinux.bin - due to: arch/x86/boot/compressed/vmlinux BUILD arch/x86/boot/bzImage - due to: arch/x86/boot/setup.bin arch/x86/boot/vmlinux.bin Setup is 15676 bytes (padded to 15872 bytes). System is 5569 kB CRC c06bbc0c Kernel: arch/x86/boot/bzImage is ready (#4) Building modules, stage 2. MODPOST 12 modules - due to target is PHONY After: blind@zurg:~/src/linux$ make V=2 CHK include/config/kernel.release CHK include/generated/uapi/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh - due to target missing CHK include/generated/compile.h Kernel: arch/x86/boot/bzImage is ready (#5) Building modules, stage 2. MODPOST 12 modules - due to target is PHONY Signed-off-by: Konstantin Khlebnikov <koct9i@gmail.com> --- scripts/Kbuild.include | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html