Conding style question regarding configuration

Conding style question regarding configuration

[Pascal Van Leeuwen]

Hi,

Quick question regarding how to configure out code depending on a CONFIG_xxx 
switch. As far as I understood so far, the proper way to do this is not by
doing an #ifdef but by using a regular if with IS_ENABLED like so:

if (IS_ENABLED(CONFIG_PCI)) {
} 

Such that the compiler can still check the code even if the switch is 
disabled. Now that all works fine and dandy for statements within a
function, but how do you configure out, say, global variable definitions
referencing types that are tied to this configuration switch? Or should
I just leave them in, depending on the compiler to optimize them away?

Obviously the code depends on those variables again, so if it's not
done consistently the compiler will complain somehow if the switch is not 
defined ...

Also, with if (IS_ENABLED()) I cannot remove my function prototypes,
just the function body. Is that really how it's supposed to be done?

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com

Re: Conding style question regarding configuration

[Ard Biesheuvel]

On Tue, 28 May 2019 at 17:47, Pascal Van Leeuwen
<pvanleeuwen@insidesecure.com> wrote:
>
> Hi,
>
> Quick question regarding how to configure out code depending on a CONFIG_xxx
> switch. As far as I understood so far, the proper way to do this is not by
> doing an #ifdef but by using a regular if with IS_ENABLED like so:
>
> if (IS_ENABLED(CONFIG_PCI)) {
> }
>
> Such that the compiler can still check the code even if the switch is
> disabled. Now that all works fine and dandy for statements within a
> function, but how do you configure out, say, global variable definitions
> referencing types that are tied to this configuration switch? Or should
> I just leave them in, depending on the compiler to optimize them away?
>
> Obviously the code depends on those variables again, so if it's not
> done consistently the compiler will complain somehow if the switch is not
> defined ...
>
> Also, with if (IS_ENABLED()) I cannot remove my function prototypes,
> just the function body. Is that really how it's supposed to be done?
>

Yes. Code and data with static linkage will just be optimized away by
the compiler if the CONFIG_xx option is not enabled, so all you need
to guard are the actual statements, function calls etc.

Re: Conding style question regarding configuration

[Sandy Harris]

Pascal Van Leeuwen <pvanleeuwen@insidesecure.com> wrote:

> ... the proper way to do this is not by
> doing an #ifdef but by using a regular if with IS_ENABLED like so:
>
> if (IS_ENABLED(CONFIG_PCI)) {}

See also:
http://doc.cat-v.org/henry_spencer/ifdef_considered_harmful

RE: Conding style question regarding configuration

[Pascal Van Leeuwen]

> > Quick question regarding how to configure out code depending on a
> CONFIG_xxx
> > switch. As far as I understood so far, the proper way to do this is
> not by
> > doing an #ifdef but by using a regular if with IS_ENABLED like so:
> >
> > if (IS_ENABLED(CONFIG_PCI)) {
> > }
> >
> > Such that the compiler can still check the code even if the switch is
> > disabled. Now that all works fine and dandy for statements within a
> > function, but how do you configure out, say, global variable
> definitions
> > referencing types that are tied to this configuration switch? Or
> should
> > I just leave them in, depending on the compiler to optimize them away?
> >
> > Obviously the code depends on those variables again, so if it's not
> > done consistently the compiler will complain somehow if the switch is
> not
> > defined ...
> >
> > Also, with if (IS_ENABLED()) I cannot remove my function prototypes,
> > just the function body. Is that really how it's supposed to be done?
> >
> 
> Yes. Code and data with static linkage will just be optimized away by
> the compiler if the CONFIG_xx option is not enabled, so all you need
> to guard are the actual statements, function calls etc.
>
Ok, makes sense. Then I'll just config out the relevant function bodies
and assume the compiler will do the rest ...

Thanks,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com

Re: Conding style question regarding configuration

[Christophe Leroy]

Pascal Van Leeuwen <pvanleeuwen@insidesecure.com> a écrit :

>> > Quick question regarding how to configure out code depending on a
>> CONFIG_xxx
>> > switch. As far as I understood so far, the proper way to do this is
>> not by
>> > doing an #ifdef but by using a regular if with IS_ENABLED like so:
>> >
>> > if (IS_ENABLED(CONFIG_PCI)) {
>> > }
>> >
>> > Such that the compiler can still check the code even if the switch is
>> > disabled. Now that all works fine and dandy for statements within a
>> > function, but how do you configure out, say, global variable
>> definitions
>> > referencing types that are tied to this configuration switch? Or
>> should
>> > I just leave them in, depending on the compiler to optimize them away?
>> >
>> > Obviously the code depends on those variables again, so if it's not
>> > done consistently the compiler will complain somehow if the switch is
>> not
>> > defined ...
>> >
>> > Also, with if (IS_ENABLED()) I cannot remove my function prototypes,
>> > just the function body. Is that really how it's supposed to be done?
>> >
>>
>> Yes. Code and data with static linkage will just be optimized away by
>> the compiler if the CONFIG_xx option is not enabled, so all you need
>> to guard are the actual statements, function calls etc.
>>
> Ok, makes sense. Then I'll just config out the relevant function bodies
> and assume the compiler will do the rest ...
>

No need to config out function bodies when they are static.
If not, it's better to group then in a C file and associate that file  
to the config symbol through Makefile

Christophe

> Thanks,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
> www.insidesecure.com

RE: Conding style question regarding configuration

[Pascal Van Leeuwen]

> >> Yes. Code and data with static linkage will just be optimized away by
> >> the compiler if the CONFIG_xx option is not enabled, so all you need
> >> to guard are the actual statements, function calls etc.
> >>
> > Ok, makes sense. Then I'll just config out the relevant function bodies
> > and assume the compiler will do the rest ...
> >
> 
> No need to config out function bodies when they are static.
>
Well, I got a complaint from someone that my driver updates for adding PCIE
support wouldn't  compile properly on a platform without a PCI(E) subsystem.
So I figure I do have to config out the references to PCI specific function
calls to fix that.

Or are you just referring to bodies of static subfunctions that are no
longer being called? Would the compiler skip those entirely?

> If not, it's better to group then in a C file and associate that file
> to the config symbol through Makefile
> 
> Christophe

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com

Re: Conding style question regarding configuration

[Ard Biesheuvel]

On Thu, 30 May 2019 at 12:16, Pascal Van Leeuwen
<pvanleeuwen@insidesecure.com> wrote:
>
> > >> Yes. Code and data with static linkage will just be optimized away by
> > >> the compiler if the CONFIG_xx option is not enabled, so all you need
> > >> to guard are the actual statements, function calls etc.
> > >>
> > > Ok, makes sense. Then I'll just config out the relevant function bodies
> > > and assume the compiler will do the rest ...
> > >
> >
> > No need to config out function bodies when they are static.
> >
> Well, I got a complaint from someone that my driver updates for adding PCIE
> support wouldn't  compile properly on a platform without a PCI(E) subsystem.
> So I figure I do have to config out the references to PCI specific function
> calls to fix that.
>
> Or are you just referring to bodies of static subfunctions that are no
> longer being called? Would the compiler skip those entirely?
>

The idea is that, by doing something like

static int bar;

static void foo(void)
{
    bar = 1;
}

if (IS_ENABLED(CONFIG_FOO))
    foo();

the function foo() or the variable bar don't have to be decorated with
#ifdefs or anything. The compiler will not complain that they are
unused if CONFIG_FOO is not enabled, and the contents of foo() are
always visible to the compiler, and so any programming errors will be
caught regardless of whether CONFIG_FOO is set.

Re: Conding style question regarding configuration

[Christophe Leroy]

Le 30/05/2019 à 12:16, Pascal Van Leeuwen a écrit :
>>>> Yes. Code and data with static linkage will just be optimized away by
>>>> the compiler if the CONFIG_xx option is not enabled, so all you need
>>>> to guard are the actual statements, function calls etc.
>>>>
>>> Ok, makes sense. Then I'll just config out the relevant function bodies
>>> and assume the compiler will do the rest ...
>>>
>>
>> No need to config out function bodies when they are static.
>>
> Well, I got a complaint from someone that my driver updates for adding PCIE
> support wouldn't  compile properly on a platform without a PCI(E) subsystem.
> So I figure I do have to config out the references to PCI specific function
> calls to fix that.

Do you have a link to your driver updates ? We could help you find the 
best solution.

Christophe

> 
> Or are you just referring to bodies of static subfunctions that are no
> longer being called? Would the compiler skip those entirely?
> 
>> If not, it's better to group then in a C file and associate that file
>> to the config symbol through Makefile
>>
>> Christophe
> 
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
> www.insidesecure.com
> 

RE: Conding style question regarding configuration

[Pascal Van Leeuwen]

> > Well, I got a complaint from someone that my driver updates for adding
> PCIE
> > support wouldn't  compile properly on a platform without a PCI(E)
> subsystem.
> > So I figure I do have to config out the references to PCI specific
> function
> > calls to fix that.
> 
> Do you have a link to your driver updates ? We could help you find the
> best solution.
> 
> Christophe
> 

Everyone is free to have a look at my Git tree. Help is appreciated:
https://github.com/pvanleeuwen/linux.git, branch "is_driver_armada_fix"

But please keep in mind that this is not an "official" update yet, I'm 
working with the official driver maintainer to get some of my changes 
merged in with his changes.

I do have a follow-up question though:
What if the function is referenced from a pci_driver struct?
I can't use IS_ENABLED around the struct definition, so in that case
the functions will still be referenced and I probably do have to use
IS_ENABLED to strip their bodies? Correct?

Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Inside Secure
www.insidesecure.com