Skip to contentSkip to conversation

WARN_ON_SMP(): Add comment to explain ({0;})

tip:core/urgent H. Peter Anvin 5 participants 8 messages Mar 28, 2011 – Mar 29, 2011 2 messages not in the archive

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

H. Peter Anvin

author
#1 lore raw
Quoted 9 lines · tip-bot for Steven Rostedt

WARN_ON_SMP(): Add comment to explain ({0;})

The define to use ({0;}) for the !CONFIG_SMP case of WARN_ON_SMP() can be confusing. As the WARN_ON_SMP() needs to be a nop when CONFIG_SMP is not set, including all its parameters must not be evaluated, and that it must work as both a stand alone statement and inside an if condition, we define it to a funky ({0;}).

Quoted 7 lines

A simple "0" will not work as it causes gcc to give the warning that the statement has no effect.

As this strange definition has raised a few eyebrows from some major kernel developers, it is wise to document why we create such a work of art.

What the heck is wrong with the idiomatic and non-gcc-extension-using:

	((void)0)

?

-hpa

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
#2 lore raw
Quoted 18 lines · H. Peter Anvin
Quoted 9 lines · tip-bot for Steven Rostedt

WARN_ON_SMP(): Add comment to explain ({0;})

The define to use ({0;}) for the !CONFIG_SMP case of WARN_ON_SMP() can be confusing. As the WARN_ON_SMP() needs to be a nop when CONFIG_SMP is not set, including all its parameters must not be evaluated, and that it must work as both a stand alone statement and inside an if condition, we define it to a funky ({0;}).

Quoted 7 lines

A simple "0" will not work as it causes gcc to give the warning that the statement has no effect.

As this strange definition has raised a few eyebrows from some major kernel developers, it is wise to document why we create such a work of art.

Quoted 5 lines

What the heck is wrong with the idiomatic and non-gcc-extension-using:

       ((void)0)

?

AFAIK you cannot use it within an if-statement.

Quoted 12 lines

       -hpa

--
H. Peter Anvin, Intel Open Source Technology Center I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 2 lines
Thanks,
//richard
author
#3 lore raw
Quoted 9 lines · richard -rw- weinberger
Quoted 6 lines

What the heck is wrong with the idiomatic and non-gcc-extension-using:

       ((void)0)

?

AFAIK you cannot use it within an if-statement.

OK, fair enough.

-hpa

Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.
#4 lore raw
Quoted 4 lines · H. Peter Anvin
Quoted 3 lines · richard -rw- weinberger
Quoted 3 lines

What the heck is wrong with the idiomatic and non-gcc-extension-using:

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
       ((void)0)

?

AFAIK you cannot use it within an if-statement.

OK, fair enough.

If people hate the ({0;}) so much, we could replace it with:

static inline int _WARN_ON_SMP(void) { return 0; } #define WARN_ON_SMP(x) _WARN_ON_SMP()

That would pretty much do the same thing.

o Keeps the parameters from being evaluated, as they may not be defined for SMP

o Can be used as a standalone statement without gcc complaining

o Can be used within an if condition.

Geeze, I never expected such a fuss over a simple change ;)

-- Steve

#5 lore raw
Quoted 1 line · Steven Rostedt

Geeze, I never expected such a fuss over a simple change ;)

And it's still broken ;-)

[spoiler space]

WARN_ON_SMP(x) should expand to x, not 0.

#6 lore raw
Quoted 6 lines · Alexey Dobriyan
Quoted 1 line · Steven Rostedt

Geeze, I never expected such a fuss over a simple change ;)

And it's still broken ;-)

WARN_ON_SMP(x) should expand to x, not 0.

Dunno, we really do not want to *guarantee* side-effects in WARN_ON() constructs.

Thanks,

Ingo

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw
Quoted 2 lines · H. Peter Anvin

I think the question is what are the expected semantics if WARN_ON_SMP is used in an if... especially for UP.

Well, actually this whole patchset was created because we added the semantic if (WARN_ON_SMP(x)).

This patch: https://lkml.org/lkml/2011/3/11/463

Added the following code:

+static void __unqueue_futex(struct futex_q *q)
+{
+ struct futex_hash_bucket *hb;
+
+ if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
+ || plist_node_empty(&q->list)))
+ return;
+
+ hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);
+ plist_del(&q->list, &hb->chain);
+}

Which on !CONFIG_SMP broke, because that "|| !spin_is_locked()" always return true and we trigger the WARN_ON() as well as returning early, breaking the code, and triggering bugs, as we never do the "plist_del()".

To fix this, I added: https://lkml.org/lkml/2011/3/17/312

Which does this change:

@@ -785,8 +785,8 @@ static void __unqueue_futex(struct futex_q *q)
{
struct futex_hash_bucket *hb;

- if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
- || plist_node_empty(&q->list)))
+ if (WARN_ON_SMP(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
+ || WARN_ON(plist_node_empty(&q->list)))
return;

hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);

But as WARN_ON_SMP() currently had no if () context, I needed to first make the change of: https://lkml.org/lkml/2011/3/17/313

#ifdef CONFIG_SMP

 # define WARN_ON_SMP(x)			WARN_ON(x)
 #else
-# define WARN_ON_SMP(x)			do { } while (0)
+# define WARN_ON_SMP(x)			({0;})
 #endif

But if this is such a problem, we can revert all this and do:

@@ -785,8 +785,11 @@ static void __unqueue_futex(struct futex_q *q)
{
struct futex_hash_bucket *hb;

- if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr)
- || plist_node_empty(&q->list)))
+#ifdef CONFIG_SMP
+ if (WARN_ON(!q->lock_ptr || !spin_is_locked(q->lock_ptr))
+ return;
+#endif
+ if (WARN_ON(plist_node_empty(&q->list)))
return;

hb = container_of(q->lock_ptr, struct futex_hash_bucket, lock);

And leave the WARN_ON_SMP() alone.

-- Steve

#9 lore raw
In reply to Alexey Dobriyan
Quoted 6 lines · Alexey Dobriyan
Quoted 1 line · Steven Rostedt

Geeze, I never expected such a fuss over a simple change ;)

And it's still broken ;-)

[spoiler space]

I didn't understand this "space" and never went down to look more. Should have said "see below".

Quoted 1 line

WARN_ON_SMP(x) should expand to x, not 0.

No it should expand to zero, please READ THE COMMENT!

OK, it was not part of this patch, but it was in the patch that this patch was added for.

+/*
+ * WARN_ON_SMP() is for cases that the warning is either
+ * meaningless for !SMP or may even cause failures.
+ * This is usually used for cases that we have
+ * WARN_ON(!spin_is_locked(&lock)) checks, as spin_is_locked()
+ * returns 0 for uniprocessor settings.
+ * It can also be used with values that are only defined
+ * on SMP:
+ *
+ * struct foo {
+ * [...]
+ * #ifdef CONFIG_SMP
+ * int bar;
+ * #endif
+ * };
+ *
+ * void func(struct foo *zoot)
+ * {
+ * WARN_ON_SMP(!zoot->bar);
+ *
+ * For CONFIG_SMP, WARN_ON_SMP() should act the same as WARN_ON(),
+ * and should be a nop and return false for uniprocessor.
+ *
+ * if (WARN_ON_SMP(x)) returns true only when CONFIG_SMP is set
+ * and x is true.
+ */

-- Steve