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

[Michal Marek <mmarek@suse.cz>]

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