[PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We do need to look at the asm output of c files for various reasons. In
order to do so, we do make <path-to-filename>.s.

However, it can happen that the Kconfig option which enables the
creation of readable asm CONFIG_READABLE_ASM is disabled. What is more,
we want this option enabled because it indirectly enables the issue of
line numbers in the asm done by gcc's switch -g.

So issue a warning that the asm output might not be optimally readable
if that option is disabled.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Makefile               | 2 +-
 scripts/Makefile.build | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index e43244263306..d9b505360c2a 100644
--- a/Makefile
+++ b/Makefile
@@ -1513,7 +1513,7 @@ else
 endif
 
 %.s: %.c prepare scripts FORCE
-	$(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)
+	$(Q)$(MAKE) $(build)=$(build-dir) asm_target=$@ $(target-dir)$(notdir $@)
 %.i: %.c prepare scripts FORCE
 	$(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)
 %.o: %.c prepare scripts FORCE
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bf3e6778cd71..8aaf1c04fe18 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -147,7 +147,12 @@ $(multi-objs-y:.o=.s)   : modname = $(modname-multi)
 $(multi-objs-y:.o=.lst) : modname = $(modname-multi)
 
 quiet_cmd_cc_s_c = CC $(quiet_modtag)  $@
-cmd_cc_s_c       = $(CC) $(c_flags) $(DISABLE_LTO) -fverbose-asm -S -o $@ $<
+cmd_cc_s_c       = $(CC) $(c_flags) $(DISABLE_LTO) -fverbose-asm -S -o $@ $< \
+		   $(if $(findstring $(asm_target),$@),\
+			$(if $(__cmd_cc_s_c_once),,\
+				$(if $(CONFIG_READABLE_ASM),,\
+					$(warning Enable CONFIG_READABLE_ASM for more helpful asm); \
+					$(eval __cmd_cc_s_c_once=1)))) \
 
 $(obj)/%.s: $(src)/%.c FORCE
 	$(call if_changed_dep,cc_s_c)
-- 
2.0.0

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Borislav Petkov]

On Sun, Oct 05, 2014 at 05:32:34PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> We do need to look at the asm output of c files for various reasons. In
> order to do so, we do make <path-to-filename>.s.
> 
> However, it can happen that the Kconfig option which enables the
> creation of readable asm CONFIG_READABLE_ASM is disabled. What is more,
> we want this option enabled because it indirectly enables the issue of
> line numbers in the asm done by gcc's switch -g.

Not really: so this is enabled by CONFIG_DEBUG_INFO and there are two
options: we make CONFIG_READABLE_ASM depend on it which makes people
answer a couple more questions or we simply add it to the command
building the .s file.

I say we do the second thing:

--
From: Borislav Petkov <bp@suse.de>
Subject: [PATCH -v1.1] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

We do need to look at the asm output of c files for various reasons. In
order to do so, we do make <path-to-filename>.s.

However, it can happen that the Kconfig option which enables the
creation of readable asm CONFIG_READABLE_ASM is disabled.

So issue a warning that the asm output might not be optimally readable
if that option is disabled.

Additionally, add the -g switch to the .s build command so that gcc
issues line numbers too.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 Makefile               | 2 +-
 scripts/Makefile.build | 7 ++++++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index e43244263306..d9b505360c2a 100644
--- a/Makefile
+++ b/Makefile
@@ -1513,7 +1513,7 @@ else
 endif
 
 %.s: %.c prepare scripts FORCE
-	$(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)
+	$(Q)$(MAKE) $(build)=$(build-dir) asm_target=$@ $(target-dir)$(notdir $@)
 %.i: %.c prepare scripts FORCE
 	$(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)
 %.o: %.c prepare scripts FORCE
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index bf3e6778cd71..d55651d39638 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -147,7 +147,12 @@ $(multi-objs-y:.o=.s)   : modname = $(modname-multi)
 $(multi-objs-y:.o=.lst) : modname = $(modname-multi)
 
 quiet_cmd_cc_s_c = CC $(quiet_modtag)  $@
-cmd_cc_s_c       = $(CC) $(c_flags) $(DISABLE_LTO) -fverbose-asm -S -o $@ $<
+cmd_cc_s_c       = $(CC) $(c_flags) $(DISABLE_LTO) -fverbose-asm -g -S -o $@ $< \
+		   $(if $(findstring $(asm_target),$@),\
+			$(if $(__cmd_cc_s_c_once),,\
+				$(if $(CONFIG_READABLE_ASM),,\
+					$(warning Enable CONFIG_READABLE_ASM for more helpful asm); \
+					$(eval __cmd_cc_s_c_once=1)))) \
 
 $(obj)/%.s: $(src)/%.c FORCE
 	$(call if_changed_dep,cc_s_c)
-- 
2.0.0


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Michal Marek]

On 2014-10-05 17:58, Borislav Petkov wrote:
> On Sun, Oct 05, 2014 at 05:32:34PM +0200, Borislav Petkov wrote:
>> From: Borislav Petkov <bp@suse.de>
>>
>> We do need to look at the asm output of c files for various reasons. In
>> order to do so, we do make <path-to-filename>.s.
>>
>> However, it can happen that the Kconfig option which enables the
>> creation of readable asm CONFIG_READABLE_ASM is disabled. What is more,
>> we want this option enabled because it indirectly enables the issue of
>> line numbers in the asm done by gcc's switch -g.
> 
> Not really: so this is enabled by CONFIG_DEBUG_INFO and there are two
> options: we make CONFIG_READABLE_ASM depend on it which makes people
> answer a couple more questions or we simply add it to the command
> building the .s file.
> 
> I say we do the second thing:
> 
> --
> From: Borislav Petkov <bp@suse.de>
> Subject: [PATCH -v1.1] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets
> 
> We do need to look at the asm output of c files for various reasons. In
> order to do so, we do make <path-to-filename>.s.
> 
> However, it can happen that the Kconfig option which enables the
> creation of readable asm CONFIG_READABLE_ASM is disabled.
> 
> So issue a warning that the asm output might not be optimally readable
> if that option is disabled.
> 
> Additionally, add the -g switch to the .s build command so that gcc
> issues line numbers too.

This violates the principle of least surprise:

make $file.s
as -o $file.o $file.s

should be equivalent to

make $file.o

Why not simply check both READABLE_ASM and DEBUG_INFO?  Also, it's more
straightforward to print the warning in the top-level Makefile rule than
to add a conditional to the generic rule, like this:

diff --git a/Makefile b/Makefile
index 106f300..2b4f62f 100644
--- a/Makefile
+++ b/Makefile
@@ -1505,6 +1505,9 @@ else
 endif
 
 %.s: %.c prepare scripts FORCE
+ifeq ($(CONFIG_READABLE_ASM)$(CONFIG_DEBUG_INFO),)
+       $(warning Enable CONFIG_READABLE_ASM and CONFIG_DEBUG_INFO more helpful asm)
+endif
        $(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)
 %.i: %.c prepare scripts FORCE
        $(Q)$(MAKE) $(build)=$(build-dir) $(target-dir)$(notdir $@)

I wonder if the whole check shouldn't be wrapped in an ifdef
CONFIG_DEBUG_KERNEL.

Michal

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Borislav Petkov]

On Tue, Oct 07, 2014 at 02:13:56PM +0200, Michal Marek wrote:
> This violates the principle of least surprise:
> 
> make $file.s
> as -o $file.o $file.s
> 
> should be equivalent to
> 
> make $file.o

I know but we need to enable -g for .s targets so that we get the .loc
annotation (i.e., line numbers) in asm which is very helpful.

But the least surprise principle is a valid point. Maybe we should warn
about it too when building .s targets...?

Or, maybe I should try to find out whether there's another gcc option
which adds ".loc" annotations alone...

> Why not simply check both READABLE_ASM and DEBUG_INFO?  Also, it's more
> straightforward to print the warning in the top-level Makefile rule than
> to add a conditional to the generic rule, like this:

The problem here is that we're building a couple of .s targets
regardless of what the make command contains, like bounds.s and such.

And we want to issue the warning only when we have an .s file as
an explicit target on the command line. That's why I'm doing that
asm_target assignment dance and something similar to WARN_ON_ONCE...

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Michal Marek]

On 2014-10-07 14:27, Borislav Petkov wrote:
> On Tue, Oct 07, 2014 at 02:13:56PM +0200, Michal Marek wrote:
>> This violates the principle of least surprise:
>>
>> make $file.s
>> as -o $file.o $file.s
>>
>> should be equivalent to
>>
>> make $file.o
> 
> I know but we need to enable -g for .s targets so that we get the .loc
> annotation (i.e., line numbers) in asm which is very helpful.
> 
> But the least surprise principle is a valid point. Maybe we should warn
> about it too when building .s targets...?
> 
> Or, maybe I should try to find out whether there's another gcc option
> which adds ".loc" annotations alone...

Such option would be best of course. BTW, do you know about make
$file.lst to produce an 'annotated disassembly'?


>> Why not simply check both READABLE_ASM and DEBUG_INFO?  Also, it's more
>> straightforward to print the warning in the top-level Makefile rule than
>> to add a conditional to the generic rule, like this:
> 
> The problem here is that we're building a couple of .s targets
> regardless of what the make command contains, like bounds.s and such.

The toplevel Makefile rule (where your patch adds the asm_target=$@
variable) is only used for manual invocation. bounds.s and the like are
handled by Makefile.build directly.

Michal

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Borislav Petkov]

On Tue, Oct 07, 2014 at 03:45:43PM +0200, Michal Marek wrote:
> Such option would be best of course. BTW, do you know about make
> $file.lst to produce an 'annotated disassembly'?

Yep. Although I sometimes find it harder to parse than looking at
.loc-annotated asm and the c-source.

> The toplevel Makefile rule (where your patch adds the asm_target=$@
> variable) is only used for manual invocation. bounds.s and the like are
> handled by Makefile.build directly.

Ah ok, I'll keep that in mind next time. I was seeing the warning being
issued multiple times, thus the once-tracking.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH] Kbuild: Check for CONFIG_READABLE_ASM when building .s targets

[Borislav Petkov]

Hey Michal,

On Tue, Oct 07, 2014 at 02:13:56PM +0200, Michal Marek wrote:
> This violates the principle of least surprise:
> 
> make $file.s
> as -o $file.o $file.s
> 
> should be equivalent to
> 
> make $file.o

on a second thought, I think we don't care about this. Because we build
the objects in kbuild with make and not with gas directly. IOW, we never
use the intermittent product file.s when building file.o but we start
again from file.c.

And besides, even if we did use gas, we would still need to pass
KBUILD_AFLAGS to it.

Hmmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--