On Mon, Mar 04, 2019 at 11:43:13PM +0100, Robin Meijboom wrote:
> On 28-02-19 23:29, Josh Poimboeuf wrote:
> > On Thu, Feb 28, 2019 at 09:12:19PM +0100, Robin Meijboom wrote:
> >> Hi Masahiro,
> >>
> >> On 28-02-19 14:36, Masahiro Yamada wrote:
> >>> On Fri, Feb 15, 2019 at 11:15 AM Robin Meijboom <robin@xxxxxxxxxxxxx> wrote:
> >>>>
> >>>> Make clean currently does not remove the generated files for objtool:
> >>>> tools/objtool/objtool, tools/objtool/fixdep, and
> >>>> tools/objtool/arch/x86/lib/inat-tables.c.
> >>>>
> >>>> Clean these files up as part of make clean.
> >>>>
> >>>> Fixes: b9ab5ebb14ec ("objtool: Add CONFIG_STACK_VALIDATION option") and
> >>>> bug report 199485 (https://bugzilla.kernel.org/show_bug.cgi?id=199485).
> >>>>
> >>>> Signed-off-by: Robin Meijboom <robin@xxxxxxxxxxxxx>
> >>>> ---
> >>>> In the discussions I didn't find a reason for keeping the files, so I
> >>>> assume it is an oversight. Otherwise I would have expected them to be
> >>>> removed at least by make distconfig (which they are not).
> >>>>
> >>>> Tested by compiling, cleaning, compiling again, and booting on x86_64.
> >>>>
> >>>> diff --git a/Makefile b/Makefile
> >>>> index 141653226f3c..81a8149a805f 100644
> >>>> --- a/Makefile
> >>>> +++ b/Makefile
> >>>> @@ -1328,6 +1328,8 @@ endif # CONFIG_MODULES
> >>>>
> >>>> # Directories & files removed with 'make clean'
> >>>> CLEAN_DIRS += $(MODVERDIR) include/ksym
> >>>> +CLEAN_FILES += tools/objtool/objtool tools/objtool/fixdep \
> >>>> + tools/objtool/arch/$(ARCH)/lib/inat-tables.c
> >>>>
> >>>> # Directories & files removed with 'make mrproper'
> >>>> MRPROPER_DIRS += include/config usr/include include/generated \
> >>>>
> >>>
> >>>
> >>>
>
> Hi Josh,
>
> Thanks for your feedback.
>
> >>> I see the same artifacts are cleaned up by tools/objtool/Makefile:
> >>>
> >>> https://github.com/torvalds/linux/blob/v5.0-rc7/tools/objtool/Makefile#L59
> >>>
> >>>
> >>> So, this patch proves the 'clean' target
> >>> in tools/objtool/Makefile is useless.
> >>>
> >>
> >> True. I believe this is because kbuild does not descend into that directory
> >> as it does with the other directories. It works if you issue 'make clean'
> >> from tools or tools/objtool.
> >
> > Right. Objtool is intended to be a standalone tool, though it's
> > currently a bit kernel-specific. So a clean target within tools/objtool
> > makes sense I think.
> >
>
> Clear. Currently objtool is also cleaned by the 'clean' target in
> tools/Makefile. Would the above suggest that should be removed, or is it
> fine as long as it is not in the top-level Makefile?
The top-level makefile uses the tools/Makefile to build objtool via the
tools/objtool target, which then descends to the objtool Makefile. So
IMO, for consistency, the objtool_clean target should remain in
tools/Makefile as well, so the top-level Makefile can use that target to
clean objtool.
> >>> BTW, 'make clean' must keep all the generated files
> >>> that are needed to compile external modules.
> >>>
> >>> I guess cleaning objtool is wrong.
> >>
> >> I was considering the same. However, I did not find this in the discussions,
> >> and as you pointed out the authors have included these items under the
> >> 'make clean' target in tools/objtool/Makefile (which does not work for the
> >> above reason).
> >>
> >> Additionally, if cleaning objtool is wrong, I would expect it to be at
> >> least included in 'make distclean' or 'make mrproper' target (which it is
> >> not).
> >>
> >> Josh, feel free to comment on the above.
> >
> > Objtool is needed for module builds, so it should *not* be removed with
> > 'make clean' from the top-level directory.
> >
> > I guess it makes sense to remove it for mrproper (and by extension,
> > distclean). Maybe mrproper could use tools/objtool's clean target
> > somehow.
> >
>
> Since we have to use an exception anyway, my initial thought was to
> use the 'standard exception' and include the artifacts in
> MRPROPER_FILES. However, your idea is of course to _not_ have to
> update the top-level Makefile every time you make a change to objtool
> artifacts. I'm thinking about an elegant solution.
How about this? Seems to work for me.
diff --git a/Makefile b/Makefile
index ac5ac28a24e9..7e6696c9b862 100644
--- a/Makefile
+++ b/Makefile
@@ -1364,7 +1364,7 @@ PHONY += $(mrproper-dirs) mrproper
$(mrproper-dirs):
$(Q)$(MAKE) $(clean)=$(patsubst _mrproper_%,%,$@)
-mrproper: clean $(mrproper-dirs)
+mrproper: clean $(mrproper-dirs) tools/objtool_clean
$(call cmd,rmdirs)
$(call cmd,rmfiles)
