Re: [PATCH 02/31] kconfig: Add support for conditional values

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3624 bytes --]

On Thu, Jan 13, 2022 at 04:01:45PM +0100, Rasmus Villemoes wrote:
> On 13/01/2022 13.52, Tom Rini wrote:
> > On Thu, Jan 13, 2022 at 08:56:02AM +0100, Rasmus Villemoes wrote:
> >> On 12/01/2022 22.56, Tom Rini wrote:
> >>> I also think I've seen cases where doing:
> >>> if (CONFIG_EVALUATES_TO_ZERO) {
> >>>   ...
> >>> }
> >>>
> >>> takes more space in the binary than an #ifdef does.
> >>
> >> Please provide a specific example. If CONFIG_EVALUATES_TO_ZERO is any
> >> integer-constant-expression evaluating at compile-time to 0, gcc throws
> >> away the whole block very early during parsing. If it doesn't, that's a
> >> compiler bug, so let's please not make decisions based on
> >> not-even-anecdotal data.
> > 
> > OK.  I believe it was commit 7856cd5a6dd6 ("Convert CONFIG_SYS_PCI_64BIT
> > to Kconfig") a few platforms changed size 
> 
> Can you remember which ones? I'd like to see if I can reproduce.
> 
> That said, that commit made the Kconfig symbol 'default y if PPC'. Are
> you really sure all ppc-boards that set CONFIG_PCI also used to set
> SYS_PCI_64BIT?
> 
> And another thing I notice is that a lot of the #define removals remove
> 
> #define CONFIG_SYS_PCI_64BIT
> 
> and not
> 
> #define CONFIG_SYS_PCI_64BIT 1
> 
> Now that doesn't matter for the places that test the definedness of
> CONFIG_SYS_PCI_64BIT, because kconfig either doesn't define it or define
> it with value 1. But it does matter for (the single) IS_ENABLED() use,
> because IS_ENABLED(bla) evaluates to 1 if and only if bla expands to 1.
> Or rather, if and only if __ARG_PLACEHOLDER_ concatenated with the
> expansion of bla in turn expands to "0, " - which only happens if we hit
> the __ARG_PLACEHOLDER_1 macro.
> 
> So when bla is defined with an empty expansion, for the purpose of
> IS_ENABLED, it might as well not be defined or expand to 0 or to
> gobbledygook.
> 
> And when one knows what to look for, it's easy to demonstrate:
> 
> $ export ARCH=ppc
> $ export CROSS_COMPILE=powerpc-linux-gnu-
> $ git checkout 7856cd5a6dd6~1
> $ make T1042D4RDB_defconfig
> $ make drivers/pci/pci-uclass.i
> $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i
> 
>   if (!(0) &&
>       type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) {
>    ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); });
>    continue;
>   }
> 
> $ git checkout 7856cd5a6dd6
> $ make T1042D4RDB_defconfig
> $ make drivers/pci/pci-uclass.i
> $ grep -C3 'beyond the 32-bit boundary' drivers/pci/pci-uclass.i
> 
>   if (!(1) &&
>       type == 0x00000000 && ((u32)(((pci_addr) >> 16) >> 16))) {
>    ({ if (0) printf(" - beyond the 32-bit boundary, ignoring\n"); });
>    continue;
>   }
> 
> Whether that change makes the generated code smaller or larger I can't
> say, but it's certainly not a nop semantically. [Of course, the change
> is for the better, as the generated code now matches the intention;
> previously 64 bit pci addresses would be ignored for the boards that had
> an empty definition of CONFIG_SYS_PCI_64BIT.]
> 
> But it has nothing whatsoever to do with whether gcc is capable of
> throwing away a whole "if (0)" block. But I will believe that other
> Kconfig conversions have been bit by the same issue, making it _seem_
> like IS_ENABLED() is somehow at fault and #ifdefs are "better".

Yes, that's what happened there, thanks for digging in and explaining.
What I really meant by "better" was "same size when converting" which is
important when migrating a ton of machines I can only very partially
test.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]