LinuxLists.cc - [PATCH 0/8] drop if around WARN

2012-11-03 20:30:33

Subject: [PATCH 0/8] drop if around WARN_ON

These patches convert a conditional with a simple test expression and a
then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
will test the condition.

// <smpl>
@@
expression e;
@@

(
if(<+...e(...)...+>) WARN_ON(1);
|
- if (e) WARN_ON(1);
+ WARN_ON(e);
)// </smpl>

2012-11-03 20:30:39

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 3/8] arch/x86/kernel/cpu/mtrr/cleanup.c: drop if around WARN_ON

2012-11-03 20:30:53

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 2/8] drivers/scsi/bfa/bfa_svc.c: drop if around WARN_ON

2012-11-03 20:30:55

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 1/8] fs/btrfs: drop if around WARN_ON

From: Julia Lawall <[email protected]>

Just use WARN_ON rather than an if containing only WARN_ON(1).

A simplified version of the semantic patch that makes this transformation
is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression e;
@@
- if (e) WARN_ON(1);
+ WARN_ON(e);
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
fs/btrfs/backref.c | 3 +--
fs/btrfs/ctree.c | 9 +++------
fs/btrfs/inode.c | 3 +--
3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 208d8aa..a321952 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -890,8 +890,7 @@ again:
while (!list_empty(&prefs)) {
ref = list_first_entry(&prefs, struct __prelim_ref, list);
list_del(&ref->list);
- if (ref->count < 0)
- WARN_ON(1);
+ WARN_ON(ref->count < 0);
if (ref->count && ref->root_id && ref->parent == 0) {
/* no parent == root of tree */
ret = ulist_add(roots, ref->root_id, 0, GFP_NOFS);
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cdfb4c4..8ecb995 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1469,10 +1469,8 @@ int btrfs_realloc_node(struct btrfs_trans_handle *trans,
if (cache_only && parent_level != 1)
return 0;

- if (trans->transaction != root->fs_info->running_transaction)
- WARN_ON(1);
- if (trans->transid != root->fs_info->generation)
- WARN_ON(1);
+ WARN_ON(trans->transaction != root->fs_info->running_transaction);
+ WARN_ON(trans->transid != root->fs_info->generation);

parent_nritems = btrfs_header_nritems(parent);
blocksize = btrfs_level_size(root, parent_level - 1);
@@ -3403,8 +3401,7 @@ static noinline int __push_leaf_right(struct btrfs_trans_handle *trans,
if (push_items == 0)
goto out_unlock;

- if (!empty && push_items == left_nritems)
- WARN_ON(1);
+ WARN_ON(!empty && push_items == left_nritems);

/* push left to right */
right_nritems = btrfs_header_nritems(right);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 95542a1..0d169ab 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1657,8 +1657,7 @@ static noinline int add_pending_csums(struct btrfs_trans_handle *trans,
int btrfs_set_extent_delalloc(struct inode *inode, u64 start, u64 end,
struct extent_state **cached_state)
{
- if ((end & (PAGE_CACHE_SIZE - 1)) == 0)
- WARN_ON(1);
+ WARN_ON((end & (PAGE_CACHE_SIZE - 1)) == 0);
return set_extent_delalloc(&BTRFS_I(inode)->io_tree, start, end,
cached_state, GFP_NOFS);
}

2012-11-03 20:30:52

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 8/8] drivers/net/wireless/ath/ath6kl/hif.c: drop if around WARN_ON

2012-11-03 20:30:49

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 7/8] drivers/scsi/scsi_transport_fc.c: drop if around WARN_ON

2012-11-03 20:30:48

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 6/8] arch/arm/mach-omap2/dpll3xxx.c: drop if around WARN_ON

2012-11-03 20:30:47

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 5/8] drivers/scsi/qla2xxx/qla_nx.c: drop if around WARN_ON

2012-11-03 20:30:44

by Julia Lawall

[permalink] [raw]

Subject: [PATCH 4/8] fs/ext3/inode.c: drop if around WARN_ON

2012-11-04 14:59:40

by Sasha Levin

[permalink] [raw]

Subject: Re: [PATCH 0/8] drop if around WARN_ON

Hi Julia,

On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <[email protected]> wrote:
> These patches convert a conditional with a simple test expression and a
> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
> will test the condition.
>
> // <smpl>
> @@
> expression e;
> @@
>
> (
> if(<+...e(...)...+>) WARN_ON(1);
> |
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> )// </smpl>

So this deals with WARN_ON(), are you considering doing the same for
the rest of it's friends?

Thanks,
Sasha

2012-11-04 15:57:40

by Julia Lawall

[permalink] [raw]

Subject: Re: [PATCH 0/8] drop if around WARN_ON

On Sun, 4 Nov 2012, Sasha Levin wrote:

> Hi Julia,
>
> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <[email protected]> wrote:
>> These patches convert a conditional with a simple test expression and a
>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>> will test the condition.
>>
>> // <smpl>
>> @@
>> expression e;
>> @@
>>
>> (
>> if(<+...e(...)...+>) WARN_ON(1);
>> |
>> - if (e) WARN_ON(1);
>> + WARN_ON(e);
>> )// </smpl>
>
> So this deals with WARN_ON(), are you considering doing the same for
> the rest of it's friends?

I tried WARN_ON_ONCE, but the pattern never occurred. Are there others
that are worth trying?

thanks,
julia

2012-11-04 16:01:16

by Sasha Levin

[permalink] [raw]

Subject: Re: [PATCH 0/8] drop if around WARN_ON

On Sun, Nov 4, 2012 at 10:57 AM, Julia Lawall <[email protected]> wrote:
> On Sun, 4 Nov 2012, Sasha Levin wrote:
>
>> Hi Julia,
>>
>> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <[email protected]> wrote:
>>>
>>> These patches convert a conditional with a simple test expression and a
>>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>>> will test the condition.
>>>
>>> // <smpl>
>>> @@
>>> expression e;
>>> @@
>>>
>>> (
>>> if(<+...e(...)...+>) WARN_ON(1);
>>> |
>>> - if (e) WARN_ON(1);
>>> + WARN_ON(e);
>>> )// </smpl>
>>
>>
>> So this deals with WARN_ON(), are you considering doing the same for
>> the rest of it's friends?
>
>
> I tried WARN_ON_ONCE, but the pattern never occurred. Are there others that
> are worth trying?

Definitely!

Here's the semantic patch I've got:

@@
expression e;
@@

(
- if (e) WARN_ON(1);
+ WARN_ON(e);
|
- if (e) WARN_ON_ONCE(1);
+ WARN_ON_ONCE(e);
|
- if (e) WARN_ON_SMP(1);
+ WARN_ON_SMP(e);
|
- if (e) BUG();
+ BUG_ON(e);
)

This gave me a really huge patch output.

I can send it out if you think the patch above looks good.

Thanks,
Sasha

2012-11-04 16:17:07

by Julia Lawall

[permalink] [raw]

Subject: Re: [PATCH 0/8] drop if around WARN_ON

On Sun, 4 Nov 2012, Sasha Levin wrote:

> On Sun, Nov 4, 2012 at 10:57 AM, Julia Lawall <[email protected]> wrote:
>> On Sun, 4 Nov 2012, Sasha Levin wrote:
>>
>>> Hi Julia,
>>>
>>> On Sat, Nov 3, 2012 at 4:30 PM, Julia Lawall <[email protected]> wrote:
>>>>
>>>> These patches convert a conditional with a simple test expression and a
>>>> then branch that only calls WARN_ON(1) to just a call to WARN_ON, which
>>>> will test the condition.
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression e;
>>>> @@
>>>>
>>>> (
>>>> if(<+...e(...)...+>) WARN_ON(1);
>>>> |
>>>> - if (e) WARN_ON(1);
>>>> + WARN_ON(e);
>>>> )// </smpl>
>>>
>>>
>>> So this deals with WARN_ON(), are you considering doing the same for
>>> the rest of it's friends?
>>
>>
>> I tried WARN_ON_ONCE, but the pattern never occurred. Are there others that
>> are worth trying?
>
> Definitely!
>
> Here's the semantic patch I've got:
>
> @@
> expression e;
> @@
>
> (
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> |
> - if (e) WARN_ON_ONCE(1);
> + WARN_ON_ONCE(e);
> |
> - if (e) WARN_ON_SMP(1);
> + WARN_ON_SMP(e);
> |
> - if (e) BUG();
> + BUG_ON(e);
> )
>
> This gave me a really huge patch output.
>
> I can send it out if you think the patch above looks good.

I didn't change any cases where the if test contains a function call. The
current definitions of WARN_ON seem to always evaluate the condition
expression, but I was worried that that might not always be the case. And
calling a function (the ones I remember were some kinds of print
functions) seems like something one might not want buried in the argument
of a debugging macro.

WARN_ON_SMP is just WARN_ON if CONFIG_SMP is true, but it is just 0
otherwise. So in that case it seems important to check that one is not
throwing away something important.

I remember working on the BUG_ON case several years ago, and other people
worked on it too, but I guess some are still there... The current
definitions of BUG_ON seem to keep the condition, but there are quite a
few specialized definitions, so someone at some point might make a version
that does not have that property.

julia

2012-11-05 02:48:04

by Sasha Levin

[permalink] [raw]

Subject: Re: [PATCH 0/8] drop if around WARN_ON

On Sun, Nov 4, 2012 at 11:16 AM, Julia Lawall <[email protected]> wrote:
> I didn't change any cases where the if test contains a function call. The
> current definitions of WARN_ON seem to always evaluate the condition
> expression, but I was worried that that might not always be the case. And
> calling a function (the ones I remember were some kinds of print functions)
> seems like something one might not want buried in the argument of a
> debugging macro.

Makes sense.

> WARN_ON_SMP is just WARN_ON if CONFIG_SMP is true, but it is just 0
> otherwise. So in that case it seems important to check that one is not
> throwing away something important.

Yup, we just need to make sure that the expression being evaluated doesn't
have side-effects.

> I remember working on the BUG_ON case several years ago, and other people
> worked on it too, but I guess some are still there... The current
> definitions of BUG_ON seem to keep the condition, but there are quite a few
> specialized definitions, so someone at some point might make a version that
> does not have that property.

It makes sense to keep an eye for such things when converting code. I
also don't think we'll get to see a version of BUG_ON which doesn't
evaluate the expression since the kernel already has more than enough
BUG_ONs that assume the expression will be evaluated:

BUG_ON(HYPERVISOR_callback_op(CALLBACKOP_register, &event));
BUG_ON(gpiochip_add(&gemini_gpio_chip));
BUG_ON(clocksource_register_hz(&sirfsoc_clocksource, CLOCK_TICK_RATE));
BUG_ON(gpio_request(ZOOM2_HEADSET_MUX_GPIO, "hs_mux") < 0);

And so on, so we're probably safe converting to BUG_ON even if the
condition is a function, as long as it doesn't create a long and
complicated BUG_ON() ofcourse.

Thanks,
Sasha

2012-11-05 15:39:35

by David Sterba

[permalink] [raw]

Subject: Re: [PATCH 1/8] fs/btrfs: drop if around WARN_ON

On Sat, Nov 03, 2012 at 09:30:18PM +0100, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Just use WARN_ON rather than an if containing only WARN_ON(1).
>
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
Reviewed-by: David Sterba <[email protected]>

2012-11-06 23:04:43

by Jan Kara

[permalink] [raw]

Subject: Re: [PATCH 4/8] fs/ext3/inode.c: drop if around WARN_ON

On Sat 03-11-12 21:30:21, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Just use WARN_ON rather than an if containing only WARN_ON(1).
>
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
Thanks. Added into my tree.

Honza

> ---
> fs/ext3/inode.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/ext3/inode.c b/fs/ext3/inode.c
> index 7e87e37..b176d42 100644
> --- a/fs/ext3/inode.c
> +++ b/fs/ext3/inode.c
> @@ -1071,8 +1071,7 @@ struct buffer_head *ext3_getblk(handle_t *handle, struct inode *inode,
> * mapped. 0 in case of a HOLE.
> */
> if (err > 0) {
> - if (err > 1)
> - WARN_ON(1);
> + WARN_ON(err > 1);
> err = 0;
> }
> *errp = err;
>
--
Jan Kara <[email protected]>
SUSE Labs, CR

2012-11-16 11:40:03

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH 8/8] drivers/net/wireless/ath/ath6kl/hif.c: drop if around WARN_ON

On 11/03/2012 10:30 PM, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Just use WARN_ON rather than an if containing only WARN_ON(1).
>
> A simplified version of the semantic patch that makes this transformation
> is as follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression e;
> @@
> - if (e) WARN_ON(1);
> + WARN_ON(e);
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>

Thanks, applied to ath6kl.git.

Kalle