2019-02-19 09:34:59

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 1/5] scripts/gdb: delay generation of gdb constants.py

scripts/gdb/linux/constants.py is never used in the kernel build
process. There is no good reason to create it so early.

Get it out of the 'prepare' stage.

Signed-off-by: Masahiro Yamada <[email protected]>
---

Kbuild | 10 ----------
Makefile | 11 +++++++++++
2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/Kbuild b/Kbuild
index 65db5be..4cebcc7 100644
--- a/Kbuild
+++ b/Kbuild
@@ -6,7 +6,6 @@
# 2) Generate timeconst.h
# 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
# 4) Check for missing system calls
-# 5) Generate constants.py (may need bounds.h)

#####
# 1) Generate bounds.h
@@ -58,14 +57,5 @@ quiet_cmd_syscalls = CALL $<
missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
$(call cmd,syscalls)

-#####
-# 5) Generate constants for Python GDB integration
-#
-
-extra-$(CONFIG_GDB_SCRIPTS) += build_constants_py
-
-build_constants_py: $(timeconst-file) $(bounds-file)
- @$(MAKE) $(build)=scripts/gdb/linux $@
-
# Keep these three files during make clean
no-clean-files := $(bounds-file) $(offsets-file) $(timeconst-file)
diff --git a/Makefile b/Makefile
index 88db36b..26dbcb7 100644
--- a/Makefile
+++ b/Makefile
@@ -1513,6 +1513,17 @@ PHONY += $(DOC_TARGETS)
$(DOC_TARGETS): scripts_basic FORCE
$(Q)$(MAKE) $(build)=Documentation $@

+# Misc
+# ---------------------------------------------------------------------------
+
+PHONY += scripts_gdb
+scripts_gdb: prepare
+ $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
+
+ifdef CONFIG_GDB_SCRIPTS
+all: scripts_gdb
+endif
+
else # KBUILD_EXTMOD

###
--
2.7.4



2019-02-19 09:34:13

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 4/5] kbuild: create symlink to vmlinux-gdb.py in scripts_gdb target

It is weird to create gdb stuff as a side-effect of vmlinux.

Move it to a more relevant place.

Signed-off-by: Masahiro Yamada <[email protected]>
---

Makefile | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index a5762c6..0459260 100644
--- a/Makefile
+++ b/Makefile
@@ -1015,9 +1015,6 @@ cmd_link-vmlinux = \
$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)

vmlinux: scripts/link-vmlinux.sh autoksyms_recursive $(vmlinux-deps) FORCE
-ifdef CONFIG_GDB_SCRIPTS
- $(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)
-endif
+$(call if_changed,link-vmlinux)

targets := vmlinux
@@ -1519,6 +1516,7 @@ $(DOC_TARGETS): scripts_basic FORCE
PHONY += scripts_gdb
scripts_gdb: prepare
$(Q)$(MAKE) $(build)=scripts/gdb
+ $(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)

ifdef CONFIG_GDB_SCRIPTS
all: scripts_gdb
--
2.7.4


2019-02-19 09:35:39

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 5/5] scripts/gdb: refactor rules for symlink creation

gdb-scripts is not a real object, but (ab)used like a phony target.

Rewrite the code in a more Kbuild-ish way. Add symlinks to extra-y
and use if_changed.

Signed-off-by: Masahiro Yamada <[email protected]>
---

scripts/gdb/linux/Makefile | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
index 7545806..3df395a 100644
--- a/scripts/gdb/linux/Makefile
+++ b/scripts/gdb/linux/Makefile
@@ -1,13 +1,17 @@
# SPDX-License-Identifier: GPL-2.0
-always := gdb-scripts

-SRCTREE := $(abspath $(srctree))
-
-$(obj)/gdb-scripts:
ifneq ($(KBUILD_SRC),)
- $(Q)ln -fsn $(SRCTREE)/$(obj)/*.py $(objtree)/$(obj)
+
+symlinks := $(patsubst $(srctree)/$(src)/%,%,$(wildcard $(srctree)/$(src)/*.py))
+
+quiet_cmd_symlink = SYMLINK $@
+ cmd_symlink = ln -fsn $(patsubst $(obj)/%,$(abspath $(srctree))/$(src)/%,$@) $@
+
+extra-y += $(symlinks)
+$(addprefix $(obj)/, $(symlinks)): FORCE
+ $(call if_changed,symlink)
+
endif
- @:

quiet_cmd_gen_constants_py = GEN $@
cmd_gen_constants_py = \
@@ -18,4 +22,4 @@ extra-y += constants.py
$(obj)/constants.py: $(src)/constants.py.in FORCE
$(call if_changed_dep,gen_constants_py)

-clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
+clean-files := *.pyc *.pyo
--
2.7.4


2019-02-19 09:35:42

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 2/5] kbuild: remove unimportant comments from ./Kbuild

Every time we add/remove a target, we need to touch the header part,
including renumbering. This is not so important information.

Numbering targets is rather misleading because they are not necessarily
generated in this order. For example, 1) and 2) can be executed
simultaneously when the -j option is given.

Signed-off-by: Masahiro Yamada <[email protected]>
---

Kbuild | 15 ++++-----------
1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/Kbuild b/Kbuild
index 4cebcc7..a07bbd6 100644
--- a/Kbuild
+++ b/Kbuild
@@ -1,14 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
#
# Kbuild for top-level directory of the kernel
-# This file takes care of the following:
-# 1) Generate bounds.h
-# 2) Generate timeconst.h
-# 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
-# 4) Check for missing system calls

#####
-# 1) Generate bounds.h
+# Generate bounds.h

bounds-file := include/generated/bounds.h

@@ -19,7 +14,7 @@ $(bounds-file): kernel/bounds.s FORCE
$(call filechk,offsets,__LINUX_BOUNDS_H__)

#####
-# 2) Generate timeconst.h
+# Generate timeconst.h

timeconst-file := include/generated/timeconst.h

@@ -31,8 +26,7 @@ $(timeconst-file): kernel/time/timeconst.bc FORCE
$(call filechk,gentimeconst)

#####
-# 3) Generate asm-offsets.h
-#
+# Generate asm-offsets.h

offsets-file := include/generated/asm-offsets.h

@@ -45,8 +39,7 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
$(call filechk,offsets,__ASM_OFFSETS_H__)

#####
-# 4) Check for missing system calls
-#
+# Check for missing system calls

always += missing-syscalls
targets += missing-syscalls
--
2.7.4


2019-02-19 09:36:29

by Masahiro Yamada

[permalink] [raw]
Subject: [PATCH 3/5] scripts/gdb: do not descend into scripts/gdb from scripts

Currently, Kbuild descends from scripts/Makefile to scripts/gdb/Makefile
just for creating symbolic links, but it does not need to do it so early.

Merge the two descending paths to simplify the code.

Signed-off-by: Masahiro Yamada <[email protected]>
---

Makefile | 2 +-
scripts/Makefile | 3 +--
scripts/gdb/linux/Makefile | 9 +++------
3 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/Makefile b/Makefile
index 26dbcb7..a5762c6 100644
--- a/Makefile
+++ b/Makefile
@@ -1518,7 +1518,7 @@ $(DOC_TARGETS): scripts_basic FORCE

PHONY += scripts_gdb
scripts_gdb: prepare
- $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
+ $(Q)$(MAKE) $(build)=scripts/gdb

ifdef CONFIG_GDB_SCRIPTS
all: scripts_gdb
diff --git a/scripts/Makefile b/scripts/Makefile
index feb1f71..9d442ee 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -39,7 +39,6 @@ build_unifdef: $(obj)/unifdef
subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
subdir-$(CONFIG_MODVERSIONS) += genksyms
subdir-$(CONFIG_SECURITY_SELINUX) += selinux
-subdir-$(CONFIG_GDB_SCRIPTS) += gdb

# Let clean descend into subdirs
-subdir- += basic dtc kconfig mod package
+subdir- += basic dtc gdb kconfig mod package
diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
index aba23be..7545806 100644
--- a/scripts/gdb/linux/Makefile
+++ b/scripts/gdb/linux/Makefile
@@ -14,11 +14,8 @@ quiet_cmd_gen_constants_py = GEN $@
$(CPP) -E -x c -P $(c_flags) $< > $@ ;\
sed -i '1,/<!-- end-c-headers -->/d;' $@

-targets += constants.py
-$(obj)/constants.py: $(SRCTREE)/$(obj)/constants.py.in FORCE
+extra-y += constants.py
+$(obj)/constants.py: $(src)/constants.py.in FORCE
$(call if_changed_dep,gen_constants_py)

-build_constants_py: $(obj)/constants.py
- @:
-
-clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py) $(obj)/constants.py
+clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
--
2.7.4


2019-02-27 11:45:47

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 1/5] scripts/gdb: delay generation of gdb constants.py

Hi Yamada-san

Thank you for the patch,

On 19/02/2019 09:33, Masahiro Yamada wrote:
> scripts/gdb/linux/constants.py is never used in the kernel build
> process. There is no good reason to create it so early.
>

I agree, I originally put it here to ensure it was only built after
bounds.h (following the asm-offsets.h example I expect). It has no
requirement to be built in this stage itself.

Depending on the prepare stage instead sounds a lot more reasonable.

Thanks

Reviewed-by: Kieran Bingham <[email protected]>

> Get it out of the 'prepare' stage.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> Kbuild | 10 ----------
> Makefile | 11 +++++++++++
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Kbuild b/Kbuild
> index 65db5be..4cebcc7 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -6,7 +6,6 @@
> # 2) Generate timeconst.h
> # 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
> # 4) Check for missing system calls
> -# 5) Generate constants.py (may need bounds.h)
>
> #####
> # 1) Generate bounds.h
> @@ -58,14 +57,5 @@ quiet_cmd_syscalls = CALL $<
> missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
> $(call cmd,syscalls)
>
> -#####
> -# 5) Generate constants for Python GDB integration
> -#
> -
> -extra-$(CONFIG_GDB_SCRIPTS) += build_constants_py
> -
> -build_constants_py: $(timeconst-file) $(bounds-file)
> - @$(MAKE) $(build)=scripts/gdb/linux $@
> -
> # Keep these three files during make clean
> no-clean-files := $(bounds-file) $(offsets-file) $(timeconst-file)
> diff --git a/Makefile b/Makefile
> index 88db36b..26dbcb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1513,6 +1513,17 @@ PHONY += $(DOC_TARGETS)
> $(DOC_TARGETS): scripts_basic FORCE
> $(Q)$(MAKE) $(build)=Documentation $@
>
> +# Misc
> +# ---------------------------------------------------------------------------
> +
> +PHONY += scripts_gdb
> +scripts_gdb: prepare
> + $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
> +
> +ifdef CONFIG_GDB_SCRIPTS
> +all: scripts_gdb
> +endif
> +
> else # KBUILD_EXTMOD
>
> ###
>


--
Regards
--
Kieran

2019-02-27 11:45:53

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 2/5] kbuild: remove unimportant comments from ./Kbuild

Hi Yamada-san,

On 19/02/2019 09:33, Masahiro Yamada wrote:
> Every time we add/remove a target, we need to touch the header part,
> including renumbering. This is not so important information.
>
> Numbering targets is rather misleading because they are not necessarily
> generated in this order. For example, 1) and 2) can be executed
> simultaneously when the -j option is given.
>
> Signed-off-by: Masahiro Yamada <[email protected]>

Sounds reasonable to me.

Reviewed-by: Kieran Bingham <[email protected]>

> ---
>
> Kbuild | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/Kbuild b/Kbuild
> index 4cebcc7..a07bbd6 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -1,14 +1,9 @@
> # SPDX-License-Identifier: GPL-2.0
> #
> # Kbuild for top-level directory of the kernel
> -# This file takes care of the following:
> -# 1) Generate bounds.h
> -# 2) Generate timeconst.h
> -# 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
> -# 4) Check for missing system calls
>
> #####
> -# 1) Generate bounds.h
> +# Generate bounds.h
>
> bounds-file := include/generated/bounds.h
>
> @@ -19,7 +14,7 @@ $(bounds-file): kernel/bounds.s FORCE
> $(call filechk,offsets,__LINUX_BOUNDS_H__)
>
> #####
> -# 2) Generate timeconst.h
> +# Generate timeconst.h
>
> timeconst-file := include/generated/timeconst.h
>
> @@ -31,8 +26,7 @@ $(timeconst-file): kernel/time/timeconst.bc FORCE
> $(call filechk,gentimeconst)
>
> #####
> -# 3) Generate asm-offsets.h
> -#
> +# Generate asm-offsets.h
>
> offsets-file := include/generated/asm-offsets.h
>
> @@ -45,8 +39,7 @@ $(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
> $(call filechk,offsets,__ASM_OFFSETS_H__)
>
> #####
> -# 4) Check for missing system calls
> -#
> +# Check for missing system calls
>
> always += missing-syscalls
> targets += missing-syscalls
>


--
--
Kieran

2019-02-27 12:21:26

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 3/5] scripts/gdb: do not descend into scripts/gdb from scripts

Hi Yamada-san,

On 19/02/2019 09:33, Masahiro Yamada wrote:
> Currently, Kbuild descends from scripts/Makefile to scripts/gdb/Makefile
> just for creating symbolic links, but it does not need to do it so early.
>
> Merge the two descending paths to simplify the code.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> Makefile | 2 +-
> scripts/Makefile | 3 +--
> scripts/gdb/linux/Makefile | 9 +++------
> 3 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26dbcb7..a5762c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1518,7 +1518,7 @@ $(DOC_TARGETS): scripts_basic FORCE
>
> PHONY += scripts_gdb
> scripts_gdb: prepare
> - $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
> + $(Q)$(MAKE) $(build)=scripts/gdb
>
> ifdef CONFIG_GDB_SCRIPTS
> all: scripts_gdb
> diff --git a/scripts/Makefile b/scripts/Makefile
> index feb1f71..9d442ee 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -39,7 +39,6 @@ build_unifdef: $(obj)/unifdef
> subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
> subdir-$(CONFIG_MODVERSIONS) += genksyms
> subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> -subdir-$(CONFIG_GDB_SCRIPTS) += gdb
>
> # Let clean descend into subdirs
> -subdir- += basic dtc kconfig mod package
> +subdir- += basic dtc gdb kconfig mod package
> diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
> index aba23be..7545806 100644
> --- a/scripts/gdb/linux/Makefile
> +++ b/scripts/gdb/linux/Makefile
> @@ -14,11 +14,8 @@ quiet_cmd_gen_constants_py = GEN $@
> $(CPP) -E -x c -P $(c_flags) $< > $@ ;\
> sed -i '1,/<!-- end-c-headers -->/d;' $@
>
> -targets += constants.py
> -$(obj)/constants.py: $(SRCTREE)/$(obj)/constants.py.in FORCE
> +extra-y += constants.py
> +$(obj)/constants.py: $(src)/constants.py.in FORCE
> $(call if_changed_dep,gen_constants_py)
>
> -build_constants_py: $(obj)/constants.py
> - @:
> -
> -clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py) $(obj)/constants.py
> +clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)


This looks like the generated file will no longer be cleaned on
distclean, if the build is 'in-tree'... Is that right? Or OK if so?

Perhaps I'm mis-understanding the "$(if $(KBUILD_SRC),*.py)" statement
intent?

As long as you're content with the result, and it's understood:

Reviewed-by: Kieran Bingham <[email protected]>


--
Regards
--
Kieran

2019-02-27 12:22:09

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 4/5] kbuild: create symlink to vmlinux-gdb.py in scripts_gdb target

Hi Yamada-san,

On 19/02/2019 09:33, Masahiro Yamada wrote:
> It is weird to create gdb stuff as a side-effect of vmlinux.
>
> Move it to a more relevant place.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> Makefile | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index a5762c6..0459260 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1015,9 +1015,6 @@ cmd_link-vmlinux = \
> $(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) $@, true)
>
> vmlinux: scripts/link-vmlinux.sh autoksyms_recursive $(vmlinux-deps) FORCE
> -ifdef CONFIG_GDB_SCRIPTS
> - $(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)
> -endif

My initial thought was, doesn't this have a dependency on the vmlinux
... but of course it doesn't. It's not symlinking to the output binary -
it's creating a helper link for GDB to know how to tie in the
scripts-gdb package. It doesn't require vmlinux to exist at all.

So yes, I agree this is fine.


> +$(call if_changed,link-vmlinux)
>
> targets := vmlinux
> @@ -1519,6 +1516,7 @@ $(DOC_TARGETS): scripts_basic FORCE
> PHONY += scripts_gdb
> scripts_gdb: prepare
> $(Q)$(MAKE) $(build)=scripts/gdb
> + $(Q)ln -fsn $(abspath $(srctree)/scripts/gdb/vmlinux-gdb.py)

and especially as we now have this convenient location for it.
This looks much better.

Reviewed-by: Kieran Bingham <[email protected]>

>
> ifdef CONFIG_GDB_SCRIPTS
> all: scripts_gdb
>


--
Regards
--
Kieran

2019-02-27 12:23:47

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 5/5] scripts/gdb: refactor rules for symlink creation

Hi Yamada-san,

On 19/02/2019 09:33, Masahiro Yamada wrote:
> gdb-scripts is not a real object, but (ab)used like a phony target.
>
> Rewrite the code in a more Kbuild-ish way. Add symlinks to extra-y
> and use if_changed.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---
>
> scripts/gdb/linux/Makefile | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
> index 7545806..3df395a 100644
> --- a/scripts/gdb/linux/Makefile
> +++ b/scripts/gdb/linux/Makefile
> @@ -1,13 +1,17 @@
> # SPDX-License-Identifier: GPL-2.0
> -always := gdb-scripts
>
> -SRCTREE := $(abspath $(srctree))
> -
> -$(obj)/gdb-scripts:
> ifneq ($(KBUILD_SRC),)
> - $(Q)ln -fsn $(SRCTREE)/$(obj)/*.py $(objtree)/$(obj)
> +
> +symlinks := $(patsubst $(srctree)/$(src)/%,%,$(wildcard $(srctree)/$(src)/*.py))
> +
> +quiet_cmd_symlink = SYMLINK $@
> + cmd_symlink = ln -fsn $(patsubst $(obj)/%,$(abspath $(srctree))/$(src)/%,$@) $@
> +
> +extra-y += $(symlinks)
> +$(addprefix $(obj)/, $(symlinks)): FORCE
> + $(call if_changed,symlink)
> +
> endif
> - @:
>
> quiet_cmd_gen_constants_py = GEN $@
> cmd_gen_constants_py = \
> @@ -18,4 +22,4 @@ extra-y += constants.py
> $(obj)/constants.py: $(src)/constants.py.in FORCE
> $(call if_changed_dep,gen_constants_py)
>
> -clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
> +clean-files := *.pyc *.pyo

Perhaps this answers my earlier question.
I guess the extra-y hook is somehow handling the clean up of these files?

Aha - yes, I've just found it in

Documentation/kbuild/makefiles.txt:
> === 5 Kbuild clean infrastructure
> ...
> Kbuild knows targets listed in $(hostprogs-y), $(hostprogs-m), $(always), $(extra-y) and $(targets). They are all deleted during "make clean".

Perfect, so this is much better.

Reviewed-by: Kieran Bingham <[email protected]>

--
Regards
--
Kieran

2019-02-27 12:42:42

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 5/5] scripts/gdb: refactor rules for symlink creation

Hi Kieran,


On Wed, Feb 27, 2019 at 8:56 PM Kieran Bingham
<[email protected]> wrote:
>
> Hi Yamada-san,
>
> On 19/02/2019 09:33, Masahiro Yamada wrote:
> > gdb-scripts is not a real object, but (ab)used like a phony target.
> >
> > Rewrite the code in a more Kbuild-ish way. Add symlinks to extra-y
> > and use if_changed.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > scripts/gdb/linux/Makefile | 18 +++++++++++-------
> > 1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
> > index 7545806..3df395a 100644
> > --- a/scripts/gdb/linux/Makefile
> > +++ b/scripts/gdb/linux/Makefile
> > @@ -1,13 +1,17 @@
> > # SPDX-License-Identifier: GPL-2.0
> > -always := gdb-scripts
> >
> > -SRCTREE := $(abspath $(srctree))
> > -
> > -$(obj)/gdb-scripts:
> > ifneq ($(KBUILD_SRC),)
> > - $(Q)ln -fsn $(SRCTREE)/$(obj)/*.py $(objtree)/$(obj)
> > +
> > +symlinks := $(patsubst $(srctree)/$(src)/%,%,$(wildcard $(srctree)/$(src)/*.py))
> > +
> > +quiet_cmd_symlink = SYMLINK $@
> > + cmd_symlink = ln -fsn $(patsubst $(obj)/%,$(abspath $(srctree))/$(src)/%,$@) $@
> > +
> > +extra-y += $(symlinks)
> > +$(addprefix $(obj)/, $(symlinks)): FORCE
> > + $(call if_changed,symlink)
> > +
> > endif
> > - @:
> >
> > quiet_cmd_gen_constants_py = GEN $@
> > cmd_gen_constants_py = \
> > @@ -18,4 +22,4 @@ extra-y += constants.py
> > $(obj)/constants.py: $(src)/constants.py.in FORCE
> > $(call if_changed_dep,gen_constants_py)
> >
> > -clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
> > +clean-files := *.pyc *.pyo
>
> Perhaps this answers my earlier question.
> I guess the extra-y hook is somehow handling the clean up of these files?
>
> Aha - yes, I've just found it in
>
> Documentation/kbuild/makefiles.txt:
> > === 5 Kbuild clean infrastructure
> > ...
> > Kbuild knows targets listed in $(hostprogs-y), $(hostprogs-m), $(always), $(extra-y) and $(targets). They are all deleted during "make clean".
>
> Perfect, so this is much better.

Exactly.

If you are interested in the real code,
see scripts/Makefile.clean


__clean-files := $(extra-y) $(extra-m) $(extra-) \
$(always) $(targets) $(clean-files) \
$(hostprogs-y) $(hostprogs-m) $(hostprogs-) \
$(hostlibs-y) $(hostlibs-m) $(hostlibs-) \
$(hostcxxlibs-y) $(hostcxxlibs-m)





> Reviewed-by: Kieran Bingham <[email protected]>
>
> --
> Regards
> --
> Kieran



--
Best Regards
Masahiro Yamada

2019-02-27 12:43:46

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 3/5] scripts/gdb: do not descend into scripts/gdb from scripts

Hi Kieran,

On Wed, Feb 27, 2019 at 8:46 PM Kieran Bingham
<[email protected]> wrote:
>
> Hi Yamada-san,
>
> On 19/02/2019 09:33, Masahiro Yamada wrote:
> > Currently, Kbuild descends from scripts/Makefile to scripts/gdb/Makefile
> > just for creating symbolic links, but it does not need to do it so early.
> >
> > Merge the two descending paths to simplify the code.
> >
> > Signed-off-by: Masahiro Yamada <[email protected]>
> > ---
> >
> > Makefile | 2 +-
> > scripts/Makefile | 3 +--
> > scripts/gdb/linux/Makefile | 9 +++------
> > 3 files changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 26dbcb7..a5762c6 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1518,7 +1518,7 @@ $(DOC_TARGETS): scripts_basic FORCE
> >
> > PHONY += scripts_gdb
> > scripts_gdb: prepare
> > - $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
> > + $(Q)$(MAKE) $(build)=scripts/gdb
> >
> > ifdef CONFIG_GDB_SCRIPTS
> > all: scripts_gdb
> > diff --git a/scripts/Makefile b/scripts/Makefile
> > index feb1f71..9d442ee 100644
> > --- a/scripts/Makefile
> > +++ b/scripts/Makefile
> > @@ -39,7 +39,6 @@ build_unifdef: $(obj)/unifdef
> > subdir-$(CONFIG_GCC_PLUGINS) += gcc-plugins
> > subdir-$(CONFIG_MODVERSIONS) += genksyms
> > subdir-$(CONFIG_SECURITY_SELINUX) += selinux
> > -subdir-$(CONFIG_GDB_SCRIPTS) += gdb
> >
> > # Let clean descend into subdirs
> > -subdir- += basic dtc kconfig mod package
> > +subdir- += basic dtc gdb kconfig mod package
> > diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
> > index aba23be..7545806 100644
> > --- a/scripts/gdb/linux/Makefile
> > +++ b/scripts/gdb/linux/Makefile
> > @@ -14,11 +14,8 @@ quiet_cmd_gen_constants_py = GEN $@
> > $(CPP) -E -x c -P $(c_flags) $< > $@ ;\
> > sed -i '1,/<!-- end-c-headers -->/d;' $@
> >
> > -targets += constants.py
> > -$(obj)/constants.py: $(SRCTREE)/$(obj)/constants.py.in FORCE
> > +extra-y += constants.py
> > +$(obj)/constants.py: $(src)/constants.py.in FORCE
> > $(call if_changed_dep,gen_constants_py)
> >
> > -build_constants_py: $(obj)/constants.py
> > - @:
> > -
> > -clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py) $(obj)/constants.py
> > +clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
>
>
> This looks like the generated file will no longer be cleaned on
> distclean, if the build is 'in-tree'... Is that right? Or OK if so?


As you noticed in 5/5, files listed in $(extra-y) are cleaned up.

So, I removed the now redundant $(obj)/constants.py from 'clean-files'.



> Perhaps I'm mis-understanding the "$(if $(KBUILD_SRC),*.py)" statement
> intent?


$(if $(KBUILD_SRC),*.py) will clean *.py only for out-of-tree build.

'*.py' files in the build directory are symbolic links.

'*.py' files in the source directory are check-in source files.



> As long as you're content with the result, and it's understood:
>
> Reviewed-by: Kieran Bingham <[email protected]>



--
Best Regards

Masahiro Yamada

2019-02-27 12:44:42

by Masahiro Yamada

[permalink] [raw]
Subject: Re: [PATCH 1/5] scripts/gdb: delay generation of gdb constants.py

On Tue, Feb 19, 2019 at 6:34 PM Masahiro Yamada
<[email protected]> wrote:
>
> scripts/gdb/linux/constants.py is never used in the kernel build
> process. There is no good reason to create it so early.
>
> Get it out of the 'prepare' stage.
>
> Signed-off-by: Masahiro Yamada <[email protected]>
> ---

Added Kieran's Reviewed-by, and applied to linux-kbuild.


>
> Kbuild | 10 ----------
> Makefile | 11 +++++++++++
> 2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/Kbuild b/Kbuild
> index 65db5be..4cebcc7 100644
> --- a/Kbuild
> +++ b/Kbuild
> @@ -6,7 +6,6 @@
> # 2) Generate timeconst.h
> # 3) Generate asm-offsets.h (may need bounds.h and timeconst.h)
> # 4) Check for missing system calls
> -# 5) Generate constants.py (may need bounds.h)
>
> #####
> # 1) Generate bounds.h
> @@ -58,14 +57,5 @@ quiet_cmd_syscalls = CALL $<
> missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
> $(call cmd,syscalls)
>
> -#####
> -# 5) Generate constants for Python GDB integration
> -#
> -
> -extra-$(CONFIG_GDB_SCRIPTS) += build_constants_py
> -
> -build_constants_py: $(timeconst-file) $(bounds-file)
> - @$(MAKE) $(build)=scripts/gdb/linux $@
> -
> # Keep these three files during make clean
> no-clean-files := $(bounds-file) $(offsets-file) $(timeconst-file)
> diff --git a/Makefile b/Makefile
> index 88db36b..26dbcb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1513,6 +1513,17 @@ PHONY += $(DOC_TARGETS)
> $(DOC_TARGETS): scripts_basic FORCE
> $(Q)$(MAKE) $(build)=Documentation $@
>
> +# Misc
> +# ---------------------------------------------------------------------------
> +
> +PHONY += scripts_gdb
> +scripts_gdb: prepare
> + $(Q)$(MAKE) $(build)=scripts/gdb/linux build_constants_py
> +
> +ifdef CONFIG_GDB_SCRIPTS
> +all: scripts_gdb
> +endif
> +
> else # KBUILD_EXTMOD
>
> ###
> --
> 2.7.4
>


--
Best Regards
Masahiro Yamada

2019-02-27 13:15:29

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH 5/5] scripts/gdb: refactor rules for symlink creation

Hi Yamada-san,

On 27/02/2019 12:32, Masahiro Yamada wrote:
> Hi Kieran,
>
>
> On Wed, Feb 27, 2019 at 8:56 PM Kieran Bingham
> <[email protected]> wrote:
>>
>> Hi Yamada-san,
>>
>> On 19/02/2019 09:33, Masahiro Yamada wrote:
>>> gdb-scripts is not a real object, but (ab)used like a phony target.
>>>
>>> Rewrite the code in a more Kbuild-ish way. Add symlinks to extra-y
>>> and use if_changed.
>>>
>>> Signed-off-by: Masahiro Yamada <[email protected]>
>>> ---
>>>
>>> scripts/gdb/linux/Makefile | 18 +++++++++++-------
>>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/scripts/gdb/linux/Makefile b/scripts/gdb/linux/Makefile
>>> index 7545806..3df395a 100644
>>> --- a/scripts/gdb/linux/Makefile
>>> +++ b/scripts/gdb/linux/Makefile
>>> @@ -1,13 +1,17 @@
>>> # SPDX-License-Identifier: GPL-2.0
>>> -always := gdb-scripts
>>>
>>> -SRCTREE := $(abspath $(srctree))
>>> -
>>> -$(obj)/gdb-scripts:
>>> ifneq ($(KBUILD_SRC),)
>>> - $(Q)ln -fsn $(SRCTREE)/$(obj)/*.py $(objtree)/$(obj)
>>> +
>>> +symlinks := $(patsubst $(srctree)/$(src)/%,%,$(wildcard $(srctree)/$(src)/*.py))
>>> +
>>> +quiet_cmd_symlink = SYMLINK $@
>>> + cmd_symlink = ln -fsn $(patsubst $(obj)/%,$(abspath $(srctree))/$(src)/%,$@) $@
>>> +
>>> +extra-y += $(symlinks)
>>> +$(addprefix $(obj)/, $(symlinks)): FORCE
>>> + $(call if_changed,symlink)
>>> +
>>> endif
>>> - @:
>>>
>>> quiet_cmd_gen_constants_py = GEN $@
>>> cmd_gen_constants_py = \
>>> @@ -18,4 +22,4 @@ extra-y += constants.py
>>> $(obj)/constants.py: $(src)/constants.py.in FORCE
>>> $(call if_changed_dep,gen_constants_py)
>>>
>>> -clean-files := *.pyc *.pyo $(if $(KBUILD_SRC),*.py)
>>> +clean-files := *.pyc *.pyo
>>
>> Perhaps this answers my earlier question.
>> I guess the extra-y hook is somehow handling the clean up of these files?
>>
>> Aha - yes, I've just found it in
>>
>> Documentation/kbuild/makefiles.txt:
>>> === 5 Kbuild clean infrastructure
>>> ...
>>> Kbuild knows targets listed in $(hostprogs-y), $(hostprogs-m), $(always), $(extra-y) and $(targets). They are all deleted during "make clean".
>>
>> Perfect, so this is much better.
>
> Exactly.
>
> If you are interested in the real code,
> see scripts/Makefile.clean
>
>
> __clean-files := $(extra-y) $(extra-m) $(extra-) \
> $(always) $(targets) $(clean-files) \
> $(hostprogs-y) $(hostprogs-m) $(hostprogs-) \
> $(hostlibs-y) $(hostlibs-m) $(hostlibs-) \
> $(hostcxxlibs-y) $(hostcxxlibs-m)
>

Thank you - that makes it very clear.

Thanks again for you work :)

--
Kieran


>
>> Reviewed-by: Kieran Bingham <[email protected]>
>>
>> --
>> Regards
>> --
>> Kieran
>
>
>

--
Regards
--
Kieran