Re: [PATCH 1/2] kconfig: Print full defined and depends for multiply-defined symbols

[Stefan Hengelein]

2015-04-11 22:23 GMT+02:00 Paul Bolle <pebolle@xxxxxxxxxx>:
> On Sat, 2015-04-11 at 21:58 +0200, Stefan Hengelein wrote:
>> 2015-04-11 20:56 GMT+02:00 Paul Bolle <pebolle@xxxxxxxxxx>:
>> > On Sat, 2015-04-11 at 18:36 +0200, Stefan Hengelein wrote:
>> What i meant to say, you won't get a prompt (or for mconf, won't see
>> it in the menu) if THUMB2_KERNEL is disabled, FRAME_POINTER will
>> simply be enabled when the default condition in the definition without
>> the prompt is satisfied.
>>
>> Therefore it might be misleading to add it to the conditions.
>
> That's a NAK to this patch, isn't it?

That's not for me to decide. Maybe I missed something!
But I wouldn't merge it in the current state.

>
>> >> I personally would prefer to
>> >> additionally find the second definition that doesn't have a prompt and
>> >> other dependencies instead of adding them to the first entry, but
>> >> that's just my personal preference.
>> >
>> > I notice myself getting rather grumpy. (That usually translates to:
>> > "Drop it, and revisit in a few days".) Let me explain.
>> >
>> > This is the arm64 entry:
>> >     config FRAME_POINTER
>> >             bool
>> >             default y
>> >
>> > This is the hexagon entry
>> >     config FRAME_POINTER
>> >             def_bool y
>> >
>> > This is the m32r entry:
>> >     config FRAME_POINTER
>> >             bool "Compile the kernel with frame pointers"
>> >             help
>> >               If you say Y here [...]
>> >
>> > And this is the sparc entry:
>> >     config FRAME_POINTER
>> >             bool
>> >             depends on MCOUNT
>> >             default y
>> >
>> > You'd expect these entries to yield really simple results when doing a
>> > search in menuconfig. But the results show unparseable crap[1]. (And I'm
>> > afraid Gregory's patch would make that even worse. Gregory: please prove
>> > me wrong.)
>>
>> would you please define unparseable crap?
>
> This is what I see for m32r:
>     Depends on: DEBUG_KERNEL [=y] && (CRIS || M68K || FRV || UML || AVR32 || SUPERH || BLACKFIN || MN10300 || METAG) || [...]
>     Selected by: FAULT_INJECTION_STACKTRACE_FILTER [=n] && FAULT_INJECTION_DEBUG_FS [=n] && STACKTRACE_SUPPORT && !X86_6[...]
>
> No one is going to understand what that means. (Did I say I was grumpy?)
> Sure, it might be actually correct for most architectures. But it
> resembles in no way what one expects to see after reading just the m32r
> entry.
>
>>  the only odd thing i notice
>> when i call menuconfig on hexagon is a really long "Selected by: "
>> list
>
> Yes. That list makes no sense whatsoever. (Did I say I was grumpy?)

Well, FAULT_INJECTION_STACKTRACE_FILTER selects FRAME_POINTER if
FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && [...]

i'm assuming the [=X] statements say what the current value is.
To see the other architecture symbols is just a matter of how that
symbol was modelled, i guess it wouldn't be easy to filter that out in
a generic way.

>
>> > So to the grumpy me it looks like either:
>> > - menuconfig handles these redefinitions incorrectly in its UI;
>> > - these redefinitions are actually complicated (as in: somehow they
>> > concatenate the dependencies, etc.) and we should probably disallow
>> > them. Because otherwise looking at a Kconfig entry tells you very little
>> > about what is actually going on for the architecture you're interested
>> > in.
>> >
>> > What is the grumpy me missing here?
>>
>> Redefinitions are more of an "overwrite" than a "add conditions to the entry".
>
> That's again a NAK to this patch, isn't it?
>
>> It's perfectly reasonable for architecture A to say: if these
>> conditions hold, i want to enable option B, not matter what the
>> Kconfigfile in lib/ says (like arm64 does with FRAME_POINTER, it is
>> always on, (depending on if there are other dependencies around it)).
>>
>> Redefinitions are a little more complicated...
>> If you have two options with the same symbol and both have a prompt,
>> you will see it two times in conf. Meaning, Kconfig doesn't merge both
>> declarations but they are separate two different instructions,
>> affecting the same symbol.
>
> You lost me there.

Create a Kconfig file with
config A
    bool "A"

config A
    bool "B"

call scripts/kconfig/conf on it and you'll see what i meant.

>
>> With menuconfig it's the same, it will show both definitions in the
>> menu, they might however be in another submenu, depending on the
>> dependencies both definitions have.
>>
>> The kconfig rules state only one definition should have a prompt, but
>> as you can see, m32r does violate this "rule" and it doesn't break
>> kconfig ;)
>>
>> That's why i said i'd prefer to have both declarations printed than
>> adding the conditions from the second definition to the printed entry
>> of the first....
>> If the order is different, you might only see the definition without
>> the prompt (what happens for hexagon) and miss the second possibility
>> to enable the feature.
>
> I'm beyond confused now. (But happy to have dragged you into this
> discussion. I think we're making progress.)
>
> I'd really prefer things to be simpler: how is anyone reading the
> Kconfig entries I quoted going to realize all that?
>

No one has to, most of the things i explained to you come from a few
years of experience with Kconfig. FRAME_POINTER is a complicated
example, it is selected although it has dependencies or a prompt AND
it is redefined in many architectures.
AFAIUI, the "depends on" or "selected by" output should give hints
what you have to enable to get a prompt for that option or simply
enable it.

Wouldn't mentioning a symbol two times (because there are two
declarations) also confuse users if they search for FRAME_POINTER? But
at least it would give hints were both declarations are defined
without mixing them up.


Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/