Kbuild.include: addtree: Remove quotes before matching path
diff mbox

Message ID 20170318215823.GS4152@decadent.org.uk
State New
Headers show

Commit Message

Ben Hutchings March 18, 2017, 9:58 p.m. UTC
systemtap currently fails to build modules when the kernel source and
object trees are separate.

systemtap adds something like -I"/usr/share/systemtap/runtime" to
EXTRA_CFLAGS, and addtree should not adjust this as it's specifying an
absolute directory.  But since make has no understanding of shell
quoting, it does anyway.

For a long time this didn't matter, because addtree would still emit
the original -I option after the adjusted one.  However, commit
db547ef19064 ("Kbuild: don't add obj tree in additional includes")
changed it to remove the original -I option.

Remove quotes (both double and single) before matching against the
excluded patterns.

References: https://bugs.debian.org/856474
Reported-and-tested-by: Jack Henschel <jackdev@mailbox.org>
Reported-and-tested-by: Ritesh Raj Sarraf <rrs@debian.org>
Fixes: db547ef19064 ("Kbuild: don't add obj tree in additional includes")
Cc: stable@vger.kernel.org # 4.8+
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---

Comments

Masahiro Yamada April 3, 2017, 7:42 a.m. UTC | #1
+To Arnd
+To Michal

2017-03-19 6:58 GMT+09:00 Ben Hutchings <ben@decadent.org.uk>:
> systemtap currently fails to build modules when the kernel source and
> object trees are separate.
>
> systemtap adds something like -I"/usr/share/systemtap/runtime" to
> EXTRA_CFLAGS, and addtree should not adjust this as it's specifying an
> absolute directory.  But since make has no understanding of shell
> quoting, it does anyway.
>
> For a long time this didn't matter, because addtree would still emit
> the original -I option after the adjusted one.  However, commit
> db547ef19064 ("Kbuild: don't add obj tree in additional includes")
> changed it to remove the original -I option.
>
> Remove quotes (both double and single) before matching against the
> excluded patterns.
>
> References: https://bugs.debian.org/856474
> Reported-and-tested-by: Jack Henschel <jackdev@mailbox.org>
> Reported-and-tested-by: Ritesh Raj Sarraf <rrs@debian.org>
> Fixes: db547ef19064 ("Kbuild: don't add obj tree in additional includes")
> Cc: stable@vger.kernel.org # 4.8+
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> --- a/scripts/Kbuild.include
> +++ b/scripts/Kbuild.include
> @@ -207,7 +207,7 @@ hdr-inst := -f $(srctree)/scripts/Makefi
>  # Prefix -I with $(srctree) if it is not an absolute path.
>  # skip if -I has no parameter
>  addtree = $(if $(patsubst -I%,%,$(1)), \
> -$(if $(filter-out -I/% -I./% -I../%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
> +$(if $(filter-out -I/% -I./% -I../%,$(subst $(quote),,$(subst $(squote),,$(1)))),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
>
>  # Find all -I options and call addtree
>  flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))



I have been thinking about this patch.

We touched addtree every time a new problem popped up.

It would be easy to pick up this patch,
but it looks like a crazy idea
to play pattern-matching in the addtree even more.



[1] -Ifoo/bar/baz
[2] -I"foo/bar/baz"
[3] -I foo/bar/baz    (whitespace(s) after -I)

All of these are valid notation,
and should be handled in the same way.

However, addtree expects only [1],
(then this patch is going to add [2] in the soup).


Each Makefile knows it wants to see
additional headers in the source tree, or objtree.

I am guessing the right approach in a long run is,
we require -I to specify $(srctree) or $(objtree) explicitly.

ccflags-y := -I$(srctree)/foo/bar/baz

or

ccflags-y := -I$(objtree)/foo/bar/baz


(For the latter, we can omit $(objtree)/ as it is ./)


Then, delete $(call flags,_c_flags) after the conversion.

Any comments?
Michal Marek April 3, 2017, 1:25 p.m. UTC | #2
On 2017-04-03 09:42, Masahiro Yamada wrote:
> Each Makefile knows it wants to see
> additional headers in the source tree, or objtree.
> 
> I am guessing the right approach in a long run is,
> we require -I to specify $(srctree) or $(objtree) explicitly.
> 
> ccflags-y := -I$(srctree)/foo/bar/baz
> 
> or
> 
> ccflags-y := -I$(objtree)/foo/bar/baz
> 
> 
> (For the latter, we can omit $(objtree)/ as it is ./)
> 
> 
> Then, delete $(call flags,_c_flags) after the conversion.

Agreed. The addtree function is more of a hack to make things just work
with O=, but AFAIK there is no clean way to implement VPATH for -I
arguments. So it's sensible to get rid of the hack. It looks like it's
going to be lot of work though:

$ git grep -e '-I' -- '*Makefile*' | wc -l
732
$ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l
166

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
Sam Ravnborg April 3, 2017, 8:20 p.m. UTC | #3
On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote:
> On 2017-04-03 09:42, Masahiro Yamada wrote:
> > Each Makefile knows it wants to see
> > additional headers in the source tree, or objtree.
> > 
> > I am guessing the right approach in a long run is,
> > we require -I to specify $(srctree) or $(objtree) explicitly.
> > 
> > ccflags-y := -I$(srctree)/foo/bar/baz
> > 
> > or
> > 
> > ccflags-y := -I$(objtree)/foo/bar/baz
> > 
> > 
> > (For the latter, we can omit $(objtree)/ as it is ./)
> > 
> > 
> > Then, delete $(call flags,_c_flags) after the conversion.
> 
> Agreed. The addtree function is more of a hack to make things just work
> with O=, but AFAIK there is no clean way to implement VPATH for -I
> arguments. So it's sensible to get rid of the hack. It looks like it's
> going to be lot of work though:
> 
> $ git grep -e '-I' -- '*Makefile*' | wc -l
> 732
> $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l
> 166

There was a goal long time ago that moving the kernel source should
not trigger a rebuild.
Any hardcoded path would violate this (like $(srctree), $(objtree))

I dunno if this is really something to aim for today.

I have personally from time to time renamed the directory where
I have kernel soruce (which is seen like moving the kernel source),
and would not be happy if this always triggered a full rebuild.
But this is frankly a corner case.

	Sam
--
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
Masahiro Yamada April 4, 2017, 3:56 a.m. UTC | #4
Hi Sam,


2017-04-04 5:20 GMT+09:00 Sam Ravnborg <sam@ravnborg.org>:
> On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote:
>> On 2017-04-03 09:42, Masahiro Yamada wrote:
>> > Each Makefile knows it wants to see
>> > additional headers in the source tree, or objtree.
>> >
>> > I am guessing the right approach in a long run is,
>> > we require -I to specify $(srctree) or $(objtree) explicitly.
>> >
>> > ccflags-y := -I$(srctree)/foo/bar/baz
>> >
>> > or
>> >
>> > ccflags-y := -I$(objtree)/foo/bar/baz
>> >
>> >
>> > (For the latter, we can omit $(objtree)/ as it is ./)
>> >
>> >
>> > Then, delete $(call flags,_c_flags) after the conversion.
>>
>> Agreed. The addtree function is more of a hack to make things just work
>> with O=, but AFAIK there is no clean way to implement VPATH for -I
>> arguments. So it's sensible to get rid of the hack. It looks like it's
>> going to be lot of work though:
>>
>> $ git grep -e '-I' -- '*Makefile*' | wc -l
>> 732
>> $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l
>> 166
>
> There was a goal long time ago that moving the kernel source should
> not trigger a rebuild.
> Any hardcoded path would violate this (like $(srctree), $(objtree))

Even if we avoid hard-coding $(srctree) in each Makefile,
"addtree" will modify the -I paths and the modified paths
will be recorded in .*.cmd files.


If we really want to achieve the goal,
my old patch seemed to have a point:
https://patchwork.kernel.org/patch/4511991/
This was rejected due to side effects, anyway.
Michal Marek April 4, 2017, 7:02 a.m. UTC | #5
Dne 3.4.2017 v 22:20 Sam Ravnborg napsal(a):
> On Mon, Apr 03, 2017 at 03:25:10PM +0200, Michal Marek wrote:
>> On 2017-04-03 09:42, Masahiro Yamada wrote:
>>> Each Makefile knows it wants to see
>>> additional headers in the source tree, or objtree.
>>>
>>> I am guessing the right approach in a long run is,
>>> we require -I to specify $(srctree) or $(objtree) explicitly.
>>>
>>> ccflags-y := -I$(srctree)/foo/bar/baz
>>>
>>> or
>>>
>>> ccflags-y := -I$(objtree)/foo/bar/baz
>>>
>>>
>>> (For the latter, we can omit $(objtree)/ as it is ./)
>>>
>>>
>>> Then, delete $(call flags,_c_flags) after the conversion.
>>
>> Agreed. The addtree function is more of a hack to make things just work
>> with O=, but AFAIK there is no clean way to implement VPATH for -I
>> arguments. So it's sensible to get rid of the hack. It looks like it's
>> going to be lot of work though:
>>
>> $ git grep -e '-I' -- '*Makefile*' | wc -l
>> 732
>> $ git grep -e '-I *\$(\(src\|obj\)tree)' -- '*Makefile*' | wc -l
>> 166
> 
> There was a goal long time ago that moving the kernel source should
> not trigger a rebuild.
> Any hardcoded path would violate this (like $(srctree), $(objtree))
> 
> I dunno if this is really something to aim for today.
> 
> I have personally from time to time renamed the directory where
> I have kernel soruce (which is seen like moving the kernel source),
> and would not be happy if this always triggered a full rebuild.
> But this is frankly a corner case.

This should be working nowadays if you build without O= or if the
buildtree points to a subdirectory of the source tree. The srctree
variable is set to . or .. in such case. Although when I tried it now,
it still does not look perfect:

$ cd /dev/shm/mmarek/linux-2.6
$ make O=build -s defconfig
$ make O=build -s -j64
Setup is 15500 bytes (padded to 15872 bytes).
System is 6530 kB
CRC 4bc8f6e2
Kernel: arch/x86/boot/bzImage is ready  (#1)
$ grep -r $PWD build/
Binary file build/arch/x86/boot/setup.elf matches
Binary file build/arch/x86/boot/header.o matches
Binary file build/arch/x86/boot/video.o matches
Binary file build/arch/x86/boot/cpu.o matches
Binary file build/arch/x86/boot/printf.o matches
Binary file build/arch/x86/boot/string.o matches
Binary file build/arch/x86/boot/video-mode.o matches
Binary file build/arch/x86/boot/video-vga.o matches
Binary file build/arch/x86/boot/early_serial_console.o matches
Binary file build/arch/x86/boot/video-vesa.o matches
Binary file build/arch/x86/boot/cpucheck.o matches
Binary file build/arch/x86/boot/video-bios.o matches
Binary file build/arch/x86/boot/tty.o matches
Binary file build/arch/x86/boot/memory.o matches
Binary file build/arch/x86/boot/pm.o matches
Binary file build/arch/x86/boot/cmdline.o matches
Binary file build/arch/x86/boot/main.o matches
Binary file build/arch/x86/boot/a20.o matches
Binary file build/arch/x86/boot/regs.o matches
Binary file build/arch/x86/boot/version.o matches
Binary file build/arch/x86/boot/edd.o matches
Binary file build/arch/x86/boot/cpuflags.o matches
Binary file build/arch/x86/boot/pmjump.o matches
Binary file build/arch/x86/boot/copy.o matches
Binary file build/arch/x86/boot/bioscall.o matches
Binary file build/arch/x86/realmode/rm/video-bios.o matches
Binary file build/arch/x86/realmode/rm/video-mode.o matches
Binary file build/arch/x86/realmode/rm/video-vga.o matches
Binary file build/arch/x86/realmode/rm/video-vesa.o matches
Binary file build/arch/x86/realmode/rm/wakemain.o matches
Binary file build/arch/x86/realmode/rm/regs.o matches
Binary file build/arch/x86/realmode/rm/trampoline_64.o matches
Binary file build/arch/x86/realmode/rm/wakeup_asm.o matches
Binary file build/arch/x86/realmode/rm/copy.o matches
Binary file build/arch/x86/realmode/rm/bioscall.o matches
Binary file build/arch/x86/realmode/rm/reboot.o matches

All the 32bit object files contain the full path, but none of the .cmd
files do?! And we do trigger rebuild of a few files after moving the
tree, but we do so even in the original location, so it looks like
somebody got a custom rule wrong again.

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

Patch
diff mbox

--- a/scripts/Kbuild.include
+++ b/scripts/Kbuild.include
@@ -207,7 +207,7 @@  hdr-inst := -f $(srctree)/scripts/Makefi
 # Prefix -I with $(srctree) if it is not an absolute path.
 # skip if -I has no parameter
 addtree = $(if $(patsubst -I%,%,$(1)), \
-$(if $(filter-out -I/% -I./% -I../%,$(1)),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
+$(if $(filter-out -I/% -I./% -I../%,$(subst $(quote),,$(subst $(squote),,$(1)))),$(patsubst -I%,-I$(srctree)/%,$(1)),$(1)))
 
 # Find all -I options and call addtree
 flags = $(foreach o,$($(1)),$(if $(filter -I%,$(o)),$(call addtree,$(o)),$(o)))