Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Al Viro]

On Thu, Mar 19, 2009 at 07:51:22PM +0100, Hannes Eder wrote:
> When currently running sparse agains the current linux-next tree, a
> lot of checks produce error messages like this:
> 
> include/linux/skbuff.h:381:9: error: expected preprocessor identifier

Cute.  If anything, this kmemcheck_define_bitfield stuff needs to be moved
inside the ifdefs.

Folks, this is not a valid C, period.  And no, there's no promise that
gcc won't change its behaviour on such constructs whenever they feel
like that.

Preprocessor directives do not belong in argument lists.  Not #ifdef,
not #define, not #include; this is undefined behaviour.

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Ingo Molnar]

* Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Thu, Mar 19, 2009 at 07:51:22PM +0100, Hannes Eder wrote:
> > When currently running sparse agains the current linux-next tree, a
> > lot of checks produce error messages like this:
> > 
> > include/linux/skbuff.h:381:9: error: expected preprocessor identifier
> 
> Cute.  If anything, this kmemcheck_define_bitfield stuff needs to be moved
> inside the ifdefs.
> 
> Folks, this is not a valid C, period.  And no, there's no promise 
> that gcc won't change its behaviour on such constructs whenever 
> they feel like that.
> 
> Preprocessor directives do not belong in argument lists.  Not 
> #ifdef, not #define, not #include; this is undefined behaviour.

Agreed.

Vegard, it's this bit:

        kmemcheck_define_bitfield(flags2, {
#ifdef CONFIG_IPV6_NDISC_NODETYPE
                __u8                    ndisc_nodetype:2;
#endif
#if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
                __u8                    do_not_encrypt:1;
                __u8                    requeue:1;
#endif
        });

	Ingo

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Al Viro]

On Thu, Mar 19, 2009 at 08:27:58PM +0100, Ingo Molnar wrote:
> Vegard, it's this bit:
> 
>         kmemcheck_define_bitfield(flags2, {
> #ifdef CONFIG_IPV6_NDISC_NODETYPE
>                 __u8                    ndisc_nodetype:2;
> #endif
> #if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
>                 __u8                    do_not_encrypt:1;
>                 __u8                    requeue:1;
> #endif
>         });

BTW, there's a related turd: kernel/cred.c
        if (
#ifdef CONFIG_KEYS
                !p->cred->thread_keyring &&
#endif
                clone_flags & CLONE_THREAD
            ) {
is not only ucking fugly, it's not a valid C if you have PROFILE_ALL_BRANCHES
set.  Why?  Because then we get if() #defined ;-/

BTW^2, speaking of that ifdef...  What happens to
static void put_cred_rcu(struct rcu_head *rcu)
{
        struct cred *cred = container_of(rcu, struct cred, rcu);

        if (atomic_read(&cred->usage) != 0)
                panic("CRED: put_cred_rcu() sees %p with usage %d\n",
                      cred, atomic_read(&cred->usage));

        security_cred_free(cred);
        key_put(cred->thread_keyring);
        key_put(cred->request_key_auth);
        release_tgcred(cred);
        put_group_info(cred->group_info);
        free_uid(cred->user);
        kmem_cache_free(cred_jar, cred);
}
if CONFIG_KEYS is not set?

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Vegard Nossum]

2009/3/19 Ingo Molnar <mingo@elte.hu>:
>
> * Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
>> On Thu, Mar 19, 2009 at 07:51:22PM +0100, Hannes Eder wrote:
>> > When currently running sparse agains the current linux-next tree, a
>> > lot of checks produce error messages like this:
>> >
>> > include/linux/skbuff.h:381:9: error: expected preprocessor identifier
>>
>> Cute.  If anything, this kmemcheck_define_bitfield stuff needs to be moved
>> inside the ifdefs.
>>
>> Folks, this is not a valid C, period.  And no, there's no promise
>> that gcc won't change its behaviour on such constructs whenever
>> they feel like that.
>>
>> Preprocessor directives do not belong in argument lists.  Not
>> #ifdef, not #define, not #include; this is undefined behaviour.
>
> Agreed.
>
> Vegard, it's this bit:
>
>        kmemcheck_define_bitfield(flags2, {
> #ifdef CONFIG_IPV6_NDISC_NODETYPE
>                __u8                    ndisc_nodetype:2;
> #endif
> #if defined(CONFIG_MAC80211) || defined(CONFIG_MAC80211_MODULE)
>                __u8                    do_not_encrypt:1;
>                __u8                    requeue:1;
> #endif
>        });
>
>        Ingo
>

Hm.

Is this really not valid C?

It worked with GCC, so I assumed it was. My mistake.

Okay, that puts us in a bit of a tight spot, with regards to kmemcheck, I mean.

Maybe I should just take up GCC development instead, and implement a
-fkmemcheck or something.

(To get rid of the bitfield false positives, I mean.)

I guess this means that kmemcheck branch should be withdrawn from
linux-next, at least temporarily, as I have no immediate
workarounds/alternatives. Stephen, can you drop it?


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Stephen Rothwell]

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

Hi Vegard,

On Thu, 19 Mar 2009 21:20:51 +0100 Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
> I guess this means that kmemcheck branch should be withdrawn from
> linux-next, at least temporarily, as I have no immediate
> workarounds/alternatives. Stephen, can you drop it?

OK, I will remove it starting today.  Let me know when you want it back
in.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Ingo Molnar]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> I guess this means that kmemcheck branch should be withdrawn from 
> linux-next, at least temporarily, as I have no immediate 
> workarounds/alternatives. Stephen, can you drop it?

Al Viro, well done :-(

	Ingo

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Al Viro]

On Fri, Mar 20, 2009 at 07:08:53PM +0100, Ingo Molnar wrote:
> 
> * Vegard Nossum <vegard.nossum@gmail.com> wrote:
> 
> > I guess this means that kmemcheck branch should be withdrawn from 
> > linux-next, at least temporarily, as I have no immediate 
> > workarounds/alternatives. Stephen, can you drop it?
> 
> Al Viro, well done :-(
> 
> 	Ingo

What the fuck?

Al

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Al Viro]

On Fri, Mar 20, 2009 at 07:04:09PM +0000, Al Viro wrote:
> On Fri, Mar 20, 2009 at 07:08:53PM +0100, Ingo Molnar wrote:
> > 
> > * Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > 
> > > I guess this means that kmemcheck branch should be withdrawn from 
> > > linux-next, at least temporarily, as I have no immediate 
> > > workarounds/alternatives. Stephen, can you drop it?
> > 
> > Al Viro, well done :-(
> > 
> > 	Ingo
> 
> What the fuck?

While we are at it, there *is* an obvious workaround:
#ifdef ...
	#define L1 <list>
#else
	#define L1
#endif
#ifdef ...
	#define L2 <list>
#else
	#define L2
#endif
	your_macro(...
	L1
	L2
	...)
#undef L1
#undef L2

Ingo, care to explain what the hell had your reply above been about?
Especially since we both apparently agree that code in question did
need fixing, what with your immediate ACK upthread...

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Vegard Nossum]

[removed duplicate Al Viro from Cc]

2009/3/20 Al Viro <viro@zeniv.linux.org.uk>:
> On Fri, Mar 20, 2009 at 07:04:09PM +0000, Al Viro wrote:
>> On Fri, Mar 20, 2009 at 07:08:53PM +0100, Ingo Molnar wrote:
>> >
>> > * Vegard Nossum <vegard.nossum@gmail.com> wrote:
>> >
>> > > I guess this means that kmemcheck branch should be withdrawn from
>> > > linux-next, at least temporarily, as I have no immediate
>> > > workarounds/alternatives. Stephen, can you drop it?
>> >
>> > Al Viro, well done :-(

[snip]

> Ingo, care to explain what the hell had your reply above been about?
> Especially since we both apparently agree that code in question did
> need fixing, what with your immediate ACK upthread...
>

Hi,

I think it is simply the frustration of discovering this rather
serious flaw just when the dust has settled, and with no capacity to
really fix it in a satisfactory way. But we should be thankful for the
heads up and try again to remember the value of linux-next and those
who test it!

(The solution you sketched is still quite an uglification of the
original code, something we tried to minimize using the construct you
saw.)

So, Ingo: There's no way this could have been merged in mainline with
such a defect, and it would be a lot worse if it wasn't discovered at
this point. We'll just have to be creative (again!) and I'm sure
Stephen can revive the tree when it's been fixed.


Vegard

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Al Viro]

On Sat, Mar 21, 2009 at 12:16:17AM +0100, Vegard Nossum wrote:

> (The solution you sketched is still quite an uglification of the
> original code, something we tried to minimize using the construct you
> saw.)

Frankly, I'd suggest expanding that sucker and being done with that.
However, more interesting question is whether you really need the
named field to be a struct.  If not, something like
bitfields_start(name)
....
bitfields_end
would work just fine, without all that fun.

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[Johannes Berg]

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

On Fri, 2009-03-20 at 19:14 +0000, Al Viro wrote:

> Ingo, care to explain what the hell had your reply above been about?

Yes, please do. You're showing a little confusing behaviour here,
telling me in one thread to show more respect for other people, and ...

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Nasal demons in preprocessor use (Re: [PATCH] test-suite: new preprocessor test case)

[H. Peter Anvin]

Johannes Berg wrote:
> On Fri, 2009-03-20 at 19:14 +0000, Al Viro wrote:
> 
>> Ingo, care to explain what the hell had your reply above been about?
> 
> Yes, please do. You're showing a little confusing behaviour here,
> telling me in one thread to show more respect for other people, and ...
> 
> johannes

I'm not Ingo, but I took his statement at exactly face value...

Well done for catching a serious problem, and :-( for the serious 
problem being discovered at this late stage.

	-hpa