test-suite: new preprocessor test case

Commit Message

Hannes Eder March 19, 2009, 5:56 p.m. UTC

Sparse currently fails on this test.

Signed-off-by: Hannes Eder <[e-mail]>
---

I discovered this while hunting down the sparse segfault when checking kernel/cred.c.

When preprocessing the file with gcc, gcc remains silent, but sparse
complains:

include/linux/skbuff.h:381:9: error: expected preprocessor identifier
include/linux/skbuff.h:381:9: error: expected preprocessor identifier
include/linux/skbuff.h:381:9: error: garbage at end: )
include/linux/skbuff.h:381:9: error: expected preprocessor identifier
include/linux/skbuff.h:381:9: error: expected preprocessor identifier
include/linux/skbuff.h:381:9: error: garbage at end: )

this is covered by the test case in a more abstract form.  Sparse
further complains about this, which is not yet covered in a test case:

kernel/cred.c:281:9: error: unmatched #endif in stream
kernel/cred.c:281:9: error: unmatched #endif in stream
kernel/cred.c:281:9: error: unmatched #endif in stream

 validation/preprocessor/preprocessor22.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)
 create mode 100644 validation/preprocessor/preprocessor22.c


--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Al Viro March 19, 2009, 6:26 p.m. UTC | #1

On Thu, Mar 19, 2009 at 06:56:11PM +0100, Hannes Eder wrote:
> Sparse currently fails on this test.

It doesn't.  6.10.3p11: "If there are sequences of preprocessing tokens
within the list of arguments that would otherwise act as preprocessing
directives, the behavior is undefined."

You are asking for identical nasal demons from two implementations, when
it's not even promised that the same kind will fly on two invocations of
the same implementation...

Seriously, this is undefined behaviour *and* it's extermely hard to come
up with self-consistent semantics for it.  Standard doesn't even try and
implementations are doing whatever's more convenient at the moment.  Try
to think of it and you'll come up with really ugly corner cases very fast.

What we probably ought to do is a warning when such stuff happens.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hannes Eder March 19, 2009, 6:51 p.m. UTC | #2

On Thu, Mar 19, 2009 at 7:26 PM, Al Viro <[e-mail]> wrote:
> On Thu, Mar 19, 2009 at 06:56:11PM +0100, Hannes Eder wrote:
>> Sparse currently fails on this test.
>
> It doesn't.  6.10.3p11: "If there are sequences of preprocessing tokens
> within the list of arguments that would otherwise act as preprocessing
> directives, the behavior is undefined."
>
> You are asking for identical nasal demons from two implementations, when
> it's not even promised that the same kind will fly on two invocations of
> the same implementation...
>
> Seriously, this is undefined behaviour *and* it's extermely hard to come
> up with self-consistent semantics for it.  Standard doesn't even try and
> implementations are doing whatever's more convenient at the moment.  Try
> to think of it and you'll come up with really ugly corner cases very fast.
>
> What we probably ought to do is a warning when such stuff happens.

Ok, I see. It's not a bug, but I wouldn't consider it as a feature either :)

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

where gcc happily compiles it, because it preprocesses it differently

e.g.

$ gcc -E -P validation/preprocessor/preprocessor22.c
struct { int b; } a;;

comparing to

$ sparse -E
struct {
} a;;

This difference makes sparse at the moment less applicable to the
linux(-next) tree.

What can be done about that?

-Hannes
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Al Viro March 19, 2009, 7:07 p.m. UTC | #3

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Derek M Jones March 19, 2009, 7:24 p.m. UTC | #4

Al,

> It doesn't.  6.10.3p11: "If there are sequences of preprocessing tokens
> within the list of arguments that would otherwise act as preprocessing
> directives, the behavior is undefined."
> 
> You are asking for identical nasal demons from two implementations, when
> it's not even promised that the same kind will fly on two invocations of
> the same implementation...

So what you are saying is that this is a bug in the header.
This is the approach I would favor.

> Seriously, this is undefined behaviour *and* it's extermely hard to come
> up with self-consistent semantics for it.  Standard doesn't even try and
> implementations are doing whatever's more convenient at the moment.  Try
> to think of it and you'll come up with really ugly corner cases very fast.

Unless gcc comes up with consistent behavior every time I would not
expect this usage to hang around very long in the header.

> What we probably ought to do is a warning when such stuff happens.

You mean a more easy to understand message.

Ingo Molnar March 19, 2009, 7:27 p.m. UTC | #5

* Al Viro <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Al Viro March 19, 2009, 7:39 p.m. UTC | #6

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?
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vegard Nossum March 19, 2009, 8:20 p.m. UTC | #7

2009/3/19 Ingo Molnar <[e-mail]>:
>
> * Al Viro <[e-mail]> 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

Stephen Rothwell March 19, 2009, 10:07 p.m. UTC | #8

Hi Vegard,

On Thu, 19 Mar 2009 21:20:51 +0100 Vegard Nossum <[e-mail]> 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.

Ingo Molnar March 20, 2009, 6:08 p.m. UTC | #9

* Vegard Nossum <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Al Viro March 20, 2009, 7:04 p.m. UTC | #10

On Fri, Mar 20, 2009 at 07:08:53PM +0100, Ingo Molnar wrote:
> 
> * Vegard Nossum <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Al Viro March 20, 2009, 7:14 p.m. UTC | #11

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 <[e-mail]> 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...
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Vegard Nossum March 20, 2009, 11:16 p.m. UTC | #12

[removed duplicate Al Viro from Cc]

2009/3/20 Al Viro <[e-mail]>:
> 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 <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Al Viro March 20, 2009, 11:44 p.m. UTC | #13

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Johannes Berg March 21, 2009, 8:34 a.m. UTC | #14

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

H. Peter Anvin March 27, 2009, 3:13 a.m. UTC | #15

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
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/validation/preprocessor/preprocessor22.c b/validation/preprocessor/preprocessor22.c
new file mode 100644
index 0000000..838479f
--- /dev/null
+++ b/validation/preprocessor/preprocessor22.c
@@ -0,0 +1,21 @@ 
+#define CONFIG_FOO 1
+
+#define define_struct(name, fields...) struct fields name;
+
+define_struct(a, {
+#ifdef CONFIG_FOO
+  int b;
+#endif
+});
+/*
+ * check-name: Preprocessor #22
+ * check-description: Sparse gets this wrong, should be fixed
+ * check-command: sparse -E $file
+ * check-known-to-fail
+ *
+ * check-output-start
+
+struct {
+int b; } a;;
+ * check-output-end
+ */