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.
* Al Viro <[email protected]> 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
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?
2009/3/19 Ingo Molnar <[email protected]>:
>
> * Al Viro <[email protected]> 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
Hi Vegard,
On Thu, 19 Mar 2009 21:20:51 +0100 Vegard Nossum <[email protected]> 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 [email protected]
http://www.canb.auug.org.au/~sfr/
* Vegard Nossum <[email protected]> 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
On Fri, Mar 20, 2009 at 07:08:53PM +0100, Ingo Molnar wrote:
>
> * Vegard Nossum <[email protected]> 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
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 <[email protected]> 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...
[removed duplicate Al Viro from Cc]
2009/3/20 Al Viro <[email protected]>:
> 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 <[email protected]> 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
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.
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
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