2019-05-28 15:48:30

by Pascal Van Leeuwen

[permalink] [raw]
Subject: Conding style question regarding configuration

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
http://www.insidesecure.com


2019-05-28 15:52:37

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: Conding style question regarding configuration

On Tue, 28 May 2019 at 17:47, Pascal Van Leeuwen
<[email protected]> 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.

2019-05-28 16:01:53

by Sandy Harris

[permalink] [raw]
Subject: Re: Conding style question regarding configuration

Pascal Van Leeuwen <[email protected]> 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

2019-05-28 19:28:11

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: Conding style question regarding configuration

> > 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
http://www.insidesecure.com



2019-05-29 16:07:56

by Christophe Leroy

[permalink] [raw]
Subject: Re: Conding style question regarding configuration

Pascal Van Leeuwen <[email protected]> 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
> http://www.insidesecure.com


2019-05-30 10:18:34

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: Conding style question regarding configuration

> >> 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
http://www.insidesecure.com

2019-05-30 10:25:47

by Ard Biesheuvel

[permalink] [raw]
Subject: Re: Conding style question regarding configuration

On Thu, 30 May 2019 at 12:16, Pascal Van Leeuwen
<[email protected]> 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.

2019-06-03 07:26:14

by Christophe Leroy

[permalink] [raw]
Subject: Re: Conding style question regarding configuration



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
> http://www.insidesecure.com
>

2019-06-03 07:26:14

by Pascal Van Leeuwen

[permalink] [raw]
Subject: RE: Conding style question regarding configuration

> > 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
http://www.insidesecure.com