Re: [RFC PATCH 4/7] kconfig: support new special property shell=

[Ulf Magnusson]

On Mon, Feb 12, 2018 at 12:44 PM, Ulf Magnusson <ulfalizer@xxxxxxxxx> wrote:
> On Mon, Feb 12, 2018 at 11:44 AM, Masahiro Yamada
> <yamada.masahiro@xxxxxxxxxxxxx> wrote:
>> 2018-02-11 19:34 GMT+09:00 Ulf Magnusson <ulfalizer@xxxxxxxxx>:
>>> Looks to me like there's a few unrelated issues here:
>>>
>>>
>>> 1. The stack protector support test scripts
>>>
>>> Worthwhile IMO if they (*in practice*) prevent hard-to-debug build errors or a
>>> subtly broken kernel from being built.
>>>
>>> A few questions:
>>>
>>>     - How do things fail with a broken stack protector implementation?
>>>
>>>     - How common are those broken compilers?
>>>
>>>     - Do you really need to pass $(KBUILD_CPPFLAGS) when testing for breakage,
>>>       or would a simpler static test work in practice?
>>>
>>>       I don't know how messy it would be to get $(KBUILD_CPPFLAGS) into
>>>       Kconfig, but should make sure it's actually needed in any case.
>>>
>>>       The scripts are already split up as
>>>
>>>           scripts/gcc-$(SRCARCH)_$(BITS)-has-stack-protector.sh
>>>
>>>       by the way, though only gcc-x86_32-has-stack-protector.sh and
>>>       gcc-x86_64-has-stack-protector.sh exist.
>>>
>>>     - How old do you need to go with GCC for -fno-stack-protector to give an
>>>       error (i.e., for not even the option to be recognized)? Is it still
>>>       warranted to test for it?
>>>
>>> Adding some CCs who worked on the stack protector test scripts.
>>>
>>> And yeah, I was assuming that needing support scripts would be rare, and that
>>> you'd usually just check whether gcc accepts the flag.
>>>
>>> When you Google "gcc broken stack protector", the top hits about are about the
>>> scripts/gcc-x86_64-has-stack-protector.sh script in the kernel throwing a false
>>> positive by the way (fixed in 82031ea29e45 ("scripts/has-stack-protector: add
>>> -fno-PIE")).
>>>
>>>
>>> 2. Whether to hide the Kconfig stack protector alternatives or always show them
>>>
>>> Or equivalently, whether to automatically fall back on other stack protector
>>> alternatives (including no stack protector) if the one specified in the .config
>>> isn't available.
>>>
>>> I'll let you battle this one out. In any case, as a user, I'd want a
>>> super-clear message telling me what to change if the build breaks because of
>>> missing stack protector support.
>>>
>>>
>>> 3. Whether to implement CC_STACKPROTECTOR_AUTO in Kconfig or the Makefiles
>>>
>>> I'd just go with whatever is simplest here. I don't find the Kconfig version
>>> too bad, but I'm already very familiar with Kconfig, so it's harder for me to
>>> tell how it looks to other people.
>>>
>>> I'd add some comments to explain the idea in the final version.
>>>
>>> @Kees:
>>> I was assuming that the Makefiles would error out with a message if none of the
>>> CC_STACKPROTECTOR_* variables are set, in addition to the Kconfig warning.
>>>
>>> You could offload part of that check to Kconfig with something like
>>>
>>>         config CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>>                 def_bool CC_STACKPROTECTOR_STRONG || \
>>>                          CC_STACKPROTECTOR_REGULAR || \
>>>                          CC_STACKPROTECTOR_NONE
>>>
>>> CHOSEN_STACKPROTECTOR_AVAILABLE could then be checked in the Makefile.
>>> It has the advantage of making the constraint clear in the Kconfig file
>>> at least.
>>>
>>> You could add some kind of assert feature to Kconfig too, but IMO it's not
>>> warranted purely for one-offs like this at least.
>>>
>>> That's details though. I'd want to explain it with a comment in any case if we
>>> go with something like this, since it's slightly kludgy and subtle
>>> (CC_STACKPROTECTOR_{STRONG,REGULAR,NONE} form a kind of choice, only you can't
>>> express it like that directly, since it's derived from other symbols).
>>>
>>>
>>> Here's an overview of the current Kconfig layout by the way, assuming
>>> the old no-fallback behavior and CC_STACKPROTECTOR_AUTO being
>>> implemented in Kconfig:
>>>
>>>         # Feature tests
>>>         CC_HAS_STACKPROTECTOR_STRONG
>>>         CC_HAS_STACKPROTECTOR_REGULAR
>>>         CC_HAS_STACKPROTECTOR_NONE
>>>
>>>         # User request
>>>         WANT_CC_STACKPROTECTOR_AUTO
>>>         WANT_CC_STACKPROTECTOR_STRONG
>>>         WANT_CC_STACKPROTECTOR_REGULAR
>>>         WANT_CC_STACKPROTECTOR_NONE
>>>
>>>         # The actual "output" to the Makefiles
>>>         CC_STACKPROTECTOR_STRONG
>>>         CC_STACKPROTECTOR_REGULAR
>>>         CC_STACKPROTECTOR_NONE
>>>
>>>         # Some possible output "nicities"
>>>         CHOSEN_CC_STACKPROTECTOR_AVAILABLE
>>>         CC_OPT_STACKPROTECTOR
>>>
>>> Does anyone have objections to the naming or other things? I saw some
>>> references to "Santa's wish list" in messages of commits that dealt with other
>>> variables named WANT_*, though I didn't look into those cases. ;)
>>>
>>
>>
>>
>> I think Linus's comment was dismissed here.
>>
>>
>> Linus said:
>>
>>> But yes, I also reacted to your earlier " It can't silently rewrite it
>>> to _REGULAR because the compiler support for _STRONG regressed."
>>> Because it damn well can. If the compiler doesn't support
>>> -fstack-protector-strong, we can just fall back on -fstack-protector.
>>> Silently. No extra crazy complex logic for that either.
>>
>>
>> If I understood his comment correctly,
>> we do not need either WANT_* or _AUTO.
>>
>>
>>
>>
>> Kees' comment:
>>
>>> In the stack-protector case, this becomes quite important, since the
>>> goal is to record the user's selection regardless of compiler
>>> capability. For example, if someone selects _REGULAR, it shouldn't
>>> "upgrade" to _STRONG. (Similarly for _NONE.)
>>
>> No.  Kconfig will not do this silently.
>
> (Playing devil's advocate...)
>
> If the user selected _STRONG and it becomes unavailable later, it
> seems to silently fall back on other options, even for oldnoconfig
> (which just checks if there are any new symbols in the choice).
>
> It would be possible to also check if the old user selection still
> applies btw. I do that in Kconfiglib. It's arguable whether that
> matches the intent of oldnoconfig.

*oldconfig

These things are so closely named. :P

Cheers,
Ulf