Skip to contentSkip to conversation

BUG -> BUG_ON conversions

PATCH In review Adrian Bunk 8 participants 20 messages Aug 28, 2004 – Aug 29, 2004

Subsystems (17)
Memory management MM core Scheduler exec and binfmt +13 more subsystems
lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

cover BUG -> BUG_ON conversions

Hi Andrew,

in the following mails are the first [1] three patches that convert

if(foo)

	BUG():

to

  BUG_ON(foo);

This makes the code slightly better readable and it might result in slightly better code with recent gcc versions due to the "unlikely" in the definition of BUG_ON (it might not be a measurable difference, but it comes for free).

Obviosly, in constructs like

  if (foo) {
	printk(KERN_ERR "some error");
	BUG();

}

or

switch (foo) {
case A:

	...
	break;
  case B:
	...
	break;
  default:
	BUG();

}

BUG() can't be replaced by BUG_ON(), and it's therefore unchanged.

cu
Adrian

[1] I plan to send more such patches.

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
author
#1 lore raw

PATCH 1/3 ipc/ BUG -> BUG_ON conversions

The patch below does BUG -> BUG_ON conversions in ipc/ .

diffstat output:

 ipc/msg.c  |    3 +--
 ipc/sem.c  |    6 ++----
 ipc/shm.c  |   12 ++++--------
 ipc/util.c |    6 ++----
 4 files changed, 9 insertions(+), 18 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>
ipc/msg.c renamed from ipc/msg.c.old +1 -2
@@ -215,8 +215,7 @@
 		ret = -EEXIST;
 	} else {
 		msq = msg_lock(id);
-		if(msq==NULL)
-			BUG();
+		BUG_ON(msq == NULL);
 		if (ipcperms(&msq->q_perm, msgflg))
 			ret = -EACCES;
 		else {
ipc/sem.c renamed from ipc/sem.c.old +2 -4
@@ -222,8 +222,7 @@
 		err = -EEXIST;
 	} else {
 		sma = sem_lock(id);
-		if(sma==NULL)
-			BUG();
+		BUG_ON(sma == NULL);
 		if (nsems > sma->sem_nsems)
 			err = -EINVAL;
 		else if (ipcperms(&sma->sem_perm, semflg))

@@ -1160,8 +1159,7 @@
 
 	sma = sem_lock(semid);
 	if(sma==NULL) {
-		if(queue.prev != NULL)
-			BUG();
+		BUG_ON(queue.prev != NULL);
 		error = -EIDRM;
 		goto out_free;
 	}
ipc/shm.c renamed from ipc/shm.c.old +4 -8
@@ -86,8 +86,7 @@
 static inline void shm_inc (int id) {
 	struct shmid_kernel *shp;
 
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = current->tgid;
 	shp->shm_nattch++;

@@ -137,8 +136,7 @@
 
 	down (&shm_ids.sem);
 	/* remove from the list of attaches of the shm segment */
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));
 	shp->shm_lprid = current->tgid;
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;

@@ -261,8 +259,7 @@
 		err = -EEXIST;
 	} else {
 		shp = shm_lock(id);
-		if(shp==NULL)
-			BUG();
+		BUG_ON(shp == NULL);
 		if (shp->shm_segsz < size)
 			err = -EINVAL;
 		else if (ipcperms(&shp->shm_perm, shmflg))

@@ -744,8 +741,7 @@
 	up_write(&current->mm->mmap_sem);
 
 	down (&shm_ids.sem);
-	if(!(shp = shm_lock(shmid)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(shmid)));
 	shp->shm_nattch--;
 	if(shp->shm_nattch == 0 &&
 	   shp->shm_flags & SHM_DEST)
ipc/util.c renamed from ipc/util.c.old +2 -4
@@ -216,8 +216,7 @@
 {
 	struct kern_ipc_perm* p;
 	int lid = id % SEQ_MULTIPLIER;
-	if(lid >= ids->size)
-		BUG();
+	BUG_ON(lid >= ids->size);
 
 	/* 
 	 * do not need a rcu_dereference()() here to force ordering

@@ -225,8 +224,7 @@
 	 */	
 	p = ids->entries[lid].p;
 	ids->entries[lid].p = NULL;
-	if(p==NULL)
-		BUG();
+	BUG_ON(p == NULL);
 	ids->in_use--;
 
 	if (lid == ids->max_id) {
#2 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

ipc/shm.c line 89

Quoted 12 lines · Adrian Bunk

The patch below does BUG -> BUG_ON conversions in ipc/ .
--- linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c.old 2004-08-28
15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c 2004-08-28
16:02:56.000000000 +0200

ipc/shm.c renamed from ipc/shm.c.old +1 -2
@@ -86,8 +86,7 @@
 static inline void shm_inc (int id) {
 	struct shmid_kernel *shp;
 
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));

This won't work:

With debugging mode:
if (unlikely(!(shp = shm_lock(id)))) BUG();

Without debugging mode:
do { } while(0)

Anything you put in BUG_ON() must *NOT* have side effects.

ipc/shm.c line 139

Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.old +1 -2
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = current->tgid;
 	shp->shm_nattch++;

@@ -137,8 +136,7 @@
 
 	down (&shm_ids.sem);
 	/* remove from the list of attaches of the shm segment */
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));

Same here

ipc/shm.c line 744

Quoted 10 lines
ipc/shm.c renamed from ipc/shm.c.old +1 -2
 	shp->shm_lprid = current->tgid;
 	shp->shm_dtim = get_seconds();
 	shp->shm_nattch--;

@@ -744,8 +741,7 @@
 	up_write(&current->mm->mmap_sem);
 
 	down (&shm_ids.sem);
-	if(!(shp = shm_lock(shmid)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(shmid)));

And here

ipc/shm.c line 747

Quoted 3 lines
ipc/shm.c renamed from ipc/shm.c.old +0 -0
 	shp->shm_nattch--;
 	if(shp->shm_nattch == 0 &&
 	   shp->shm_flags & SHM_DEST)

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$ L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+ PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r !y?(-) ------END GEEK CODE BLOCK------

author
#3 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

Quoted 16 lines · Kyle Moffett

ipc/shm.c line 89

Quoted 12 lines · Adrian Bunk

The patch below does BUG -> BUG_ON conversions in ipc/ .
--- linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c.old 2004-08-28
15:55:28.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/ipc/shm.c 2004-08-28
16:02:56.000000000 +0200

ipc/shm.c renamed from ipc/shm.c.old +1 -2
@@ -86,8 +86,7 @@
 static inline void shm_inc (int id) {
 	struct shmid_kernel *shp;
 
-	if(!(shp = shm_lock(id)))
-		BUG();
+	BUG_ON(!(shp = shm_lock(id)));

This won't work:

Quoted 5 lines

With debugging mode:
if (unlikely(!(shp = shm_lock(id)))) BUG();

Without debugging mode:
do { } while(0)

Where in 2.6.9-rc1-mm1 is this "Without debugging mode" defined?

Quoted 2 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#4 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

Quoting Adrian Bunk <[email protected]>:

Quoted 6 lines
Quoted 2 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

#if is_debugging_on

# define BUG_ON(x) do { if (unlikely(x)) BUG(); } while (0)
#else
# define BUG_ON(x) do { if (x) { /* nothing */ } } while (0)
#endif
Quoted 3 lines

cu
Adrian

Signature · 1 line
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]
#5 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

In reply to Adrian Bunk
Quoted 7 lines · Adrian Bunk
Quoted 3 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

Nevertheless, I think I'd prefer that we not move code which has side-effects into BUG_ONs. For some reason it seems neater that way.

Plus one would like to be able to do

	BUG_ON(strlen(str) > 22);

and have strlen() not be evaluated if BUG_ON is disabled.

A minor distinction, but one which it would be nice to preserve.

author
#6 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

Quoted 12 lines · Andrew Morton
Quoted 7 lines · Adrian Bunk
Quoted 3 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

Nevertheless, I think I'd prefer that we not move code which has side-effects into BUG_ONs. For some reason it seems neater that way.

Quoted 7 lines

Plus one would like to be able to do

	BUG_ON(strlen(str) > 22);

and have strlen() not be evaluated if BUG_ON is disabled.

A minor distinction, but one which it would be nice to preserve.

OTOH, only very few people use the disabled BUG_ON, and a statement with a side effect might stay there unnoticed for some time.

If it's a
#define BUG_ON(x)
and in one place something with a side effect slipped into the BUG_ON, you have a classical example for a heisenbug...

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#7 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

In reply to Andrew Morton
Quoted 12 lines · Andrew Morton
Quoted 7 lines · Adrian Bunk
Quoted 3 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

Nevertheless, I think I'd prefer that we not move code which has side-effects into BUG_ONs. For some reason it seems neater that way.

Quoted 7 lines

Plus one would like to be able to do

	BUG_ON(strlen(str) > 22);

and have strlen() not be evaluated if BUG_ON is disabled.

A minor distinction, but one which it would be nice to preserve.

Precisely, I fully agree (even though BUG_ON() will never be defined away, if you should not do the check kill it completely).

Signature · 1 line
Jens Axboe
#8 lore raw

Re: [2.6 patch][1/3] ipc/ BUG -> BUG_ON conversions

In reply to Andrew Morton
Quoted 9 lines · Andrew Morton
Quoted 7 lines · Adrian Bunk
Quoted 3 lines

Anything you put in BUG_ON() must *NOT* have side effects.
...

I'd have said exactly the same some time ago, but I was convinced by Arjan that if done correctly, a BUG_ON() with side effects is possible with no extra cost even if you want to make BUG configurably do nothing.

Quoted 8 lines

Nevertheless, I think I'd prefer that we not move code which has side-effects into BUG_ONs. For some reason it seems neater that way.

Plus one would like to be able to do

	BUG_ON(strlen(str) > 22);

and have strlen() not be evaluated if BUG_ON is disabled.

Well, strlen doesn't actually have a side effect, so it could be marked pure and then be optimized away.

I'll push configurable BUG_ON support from -tiny to you shortly so this will stop being theoretical.

Signature · 1 line
Mathematics is the supreme nostalgia of our time.
author
#9 lore raw

PATCH 2/3 kernel/ BUG -> BUG_ON conversions

In reply to Adrian Bunk

The patch below does BUG -> BUG_ON conversions in kernel/ .

diffstat output:

 kernel/cpu.c      |    8 +++-----
 kernel/exit.c     |   13 +++++--------
 kernel/fork.c     |    3 +--
 kernel/module.c   |    9 +++------
 kernel/power/pm.c |    3 +--
 kernel/printk.c   |    6 ++----
 kernel/ptrace.c   |    6 ++----
 kernel/signal.c   |   27 +++++++++------------------
 kernel/softirq.c  |    6 ++----
 kernel/timer.c    |    3 +--
 10 files changed, 29 insertions(+), 55 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>
kernel/cpu.c renamed from kernel/cpu.c.old +3 -5
@@ -156,9 +156,8 @@
 	kthread_bind(p, smp_processor_id());
 
 	/* CPU is completely dead: tell everyone.  Too late to complain. */
-	if (notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
-	    == NOTIFY_BAD)
-		BUG();
+	BUG_ON(notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
+	       == NOTIFY_BAD);
 
 	check_for_tasks(cpu);
 

@@ -203,8 +202,7 @@
 	ret = __cpu_up(cpu);
 	if (ret != 0)
 		goto out_notify;
-	if (!cpu_online(cpu))
-		BUG();
+	BUG_ON(!cpu_online(cpu));
 
 	/* Now call notifier in preparation. */
 	notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);
kernel/exit.c renamed from kernel/exit.c.old +5 -8
@@ -503,7 +503,7 @@
 		down_read(&mm->mmap_sem);
 	}
 	atomic_inc(&mm->mm_count);
-	if (mm != tsk->active_mm) BUG();
+	BUG_ON(mm != tsk->active_mm);
 	/* more a memory barrier than a real lock */
 	task_lock(tsk);
 	tsk->mm = NULL;

@@ -878,11 +877,9 @@
 	const struct list_head *tmp, *head = &link->pidptr->task_list;
 
 #ifdef CONFIG_SMP
-	if (!p->sighand)
-		BUG();
-	if (!spin_is_locked(&p->sighand->siglock) &&
-				!rwlock_is_locked(&tasklist_lock))
-		BUG();
+	BUG_ON(!p->sighand);
+	BUG_ON(!spin_is_locked(&p->sighand->siglock) &&
+				!rwlock_is_locked(&tasklist_lock));
 #endif
 	tmp = link->pid_chain.next;
 	if (tmp == head)

@@ -1350,8 +1347,7 @@
 		if (options & __WNOTHREAD)
 			break;
 		tsk = next_thread(tsk);
-		if (tsk->signal != current->signal)
-			BUG();
+		BUG_ON(tsk->signal != current->signal);
 	} while (tsk != current);
 
 	read_unlock(&tasklist_lock);
kernel/fork.c renamed from kernel/fork.c.old +1 -2
@@ -809,8 +809,7 @@
 	struct files_struct *files  = current->files;
 	int rc;
 
-	if(!files)
-		BUG();
+	BUG_ON(!files);
 
 	/* This can race but the race causes us to copy when we don't
 	   need to and drop the copy */
kernel/module.c renamed from kernel/module.c.old +3 -6
@@ -655,8 +655,7 @@
 	const unsigned long *crc;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!__find_symbol(symbol, &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol(symbol, &owner, &crc, 1));
 	module_put(owner);
 	spin_unlock_irqrestore(&modlist_lock, flags);
 }

@@ -667,8 +666,7 @@
 	unsigned long flags;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!kernel_text_address((unsigned long)addr))
-		BUG();
+	BUG_ON(!kernel_text_address((unsigned long)addr));
 
 	module_put(module_text_address((unsigned long)addr));
 	spin_unlock_irqrestore(&modlist_lock, flags);

@@ -905,8 +903,7 @@
 	const unsigned long *crc;
 	struct module *owner;
 
-	if (!__find_symbol("struct_module", &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol("struct_module", &owner, &crc, 1));
 	return check_version(sechdrs, versindex, "struct_module", mod,
 			     crc);
 }
kernel/power/pm.c renamed from kernel/power/pm.c.old +1 -2
@@ -156,8 +156,7 @@
 	int status = 0;
 	unsigned long prev_state, next_state;
 
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 
 	switch (rqst) {
 	case PM_SUSPEND:
kernel/printk.c renamed from kernel/printk.c.old +2 -4
@@ -418,8 +418,7 @@
 	unsigned long cur_index, start_print;
 	static int msg_level = -1;
 
-	if (((long)(start - end)) > 0)
-		BUG();
+	BUG_ON(((long)(start - end)) > 0);
 
 	cur_index = start;
 	start_print = start;

@@ -596,8 +595,7 @@
  */
 void acquire_console_sem(void)
 {
-	if (in_interrupt())
-		BUG();
+	BUG_ON(in_interrupt());
 	down(&console_sem);
 	console_locked = 1;
 	console_may_schedule = 1;
kernel/ptrace.c renamed from kernel/ptrace.c.old +2 -4
@@ -28,8 +28,7 @@
  */
 void __ptrace_link(task_t *child, task_t *new_parent)
 {
-	if (!list_empty(&child->ptrace_list))
-		BUG();
+	BUG_ON(!list_empty(&child->ptrace_list));
 	if (child->parent == new_parent)
 		return;
 	list_add(&child->ptrace_list, &child->parent->ptrace_children);

@@ -46,8 +45,7 @@
  */
 void __ptrace_unlink(task_t *child)
 {
-	if (!child->ptrace)
-		BUG();
+	BUG_ON(!child->ptrace);
 	child->ptrace = 0;
 	if (list_empty(&child->ptrace_list))
 		return;
kernel/signal.c renamed from kernel/signal.c.old +9 -18
@@ -346,10 +346,8 @@
 	struct signal_struct * sig = tsk->signal;
 	struct sighand_struct * sighand = tsk->sighand;
 
-	if (!sig)
-		BUG();
-	if (!atomic_read(&sig->count))
-		BUG();
+	BUG_ON(!sig);
+	BUG_ON(!atomic_read(&sig->count));
 	spin_lock(&sighand->siglock);
 	if (atomic_dec_and_test(&sig->count)) {
 		if (tsk == sig->curr_target)

@@ -816,11 +814,9 @@
 {
 	int ret = 0;
 
-	if (!irqs_disabled())
-		BUG();
+	BUG_ON(!irqs_disabled());
 #ifdef CONFIG_SMP
-	if (!spin_is_locked(&t->sighand->siglock))
-		BUG();
+	BUG_ON(!spin_is_locked(&t->sighand->siglock));
 #endif
 
 	if (((unsigned long)info > 2) && (info->si_code == SI_TIMER))

@@ -1008,8 +1004,7 @@
 	int ret = 0;
 
 #ifdef CONFIG_SMP
-	if (!spin_is_locked(&p->sighand->siglock))
-		BUG();
+	BUG_ON(!spin_is_locked(&p->sighand->siglock));
 #endif
 	handle_stop_signal(sig, p);
 

@@ -1378,8 +1373,7 @@
 		 * If an SI_TIMER entry is already queue just increment
 		 * the overrun count.
 		 */
-		if (q->info.si_code != SI_TIMER)
-			BUG();
+		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
 		goto out;
 	} 

@@ -1425,8 +1419,7 @@
 		 * the overrun count.  Other uses should not try to
 		 * send the signal multiple times.
 		 */
-		if (q->info.si_code != SI_TIMER)
-			BUG();
+		BUG_ON(q->info.si_code != SI_TIMER);
 		q->info.si_overrun++;
 		goto out;
 	} 

@@ -1474,8 +1467,7 @@
 	do {
 		wake_up_interruptible_sync(&tsk->wait_chldexit);
 		tsk = next_thread(tsk);
-		if (tsk->signal != parent->signal)
-			BUG();
+		BUG_ON(tsk->signal != parent->signal);
 	} while (tsk != parent);
 }
 

@@ -1490,8 +1482,7 @@
 	int why, status;
 	struct sighand_struct *psig;
 
-	if (sig == -1)
-		BUG();
+	BUG_ON(sig == -1);
 
 	BUG_ON(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
kernel/softirq.c renamed from kernel/softirq.c.old +2 -4
@@ -240,8 +240,7 @@
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
+				BUG_ON(!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state));
 				t->func(t->data);
 				tasklet_unlock(t);
 				continue;

@@ -273,8 +272,7 @@
 
 		if (tasklet_trylock(t)) {
 			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
-					BUG();
+				BUG_ON(!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state));
 				t->func(t->data);
 				tasklet_unlock(t);
 				continue;
kernel/timer.c renamed from kernel/timer.c.old +1 -2
@@ -1374,8 +1374,7 @@
 		spin_lock(&new_base->lock);
 	}
 
-	if (old_base->running_timer)
-		BUG();
+	BUG_ON(old_base->running_timer);
 	for (i = 0; i < TVR_SIZE; i++)
 		if (!migrate_timer_list(new_base, old_base->tv1.vec + i))
 			goto unlock_again;
#10 lore raw

Re: [2.6 patch][2/3] kernel/ BUG -> BUG_ON conversions

Quoted 7 lines · Adrian Bunk

The patch below does BUG -> BUG_ON conversions in kernel/ .

--- linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c.old 2004-08-28
16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/cpu.c 2004-08-28
16:06:24.000000000 +0200

kernel/cpu.c line 160

Quoted 8 lines
kernel/cpu.c renamed from kernel/cpu.c.old +2 -3
@@ -156,9 +156,8 @@
 	kthread_bind(p, smp_processor_id());
 
 	/* CPU is completely dead: tell everyone.  Too late to complain. */
-	if (notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
-	    == NOTIFY_BAD)
-		BUG();
+	BUG_ON(notifier_call_chain(&cpu_chain, CPU_DEAD, (void *)(long)cpu)
+	       == NOTIFY_BAD);

Can't do this, without DEBUG the notifier_call_chain _won't_ be executed

kernel/cpu.c line 205

Quoted 9 lines
kernel/cpu.c renamed from kernel/cpu.c.old +1 -2
 	check_for_tasks(cpu);
 

@@ -203,8 +202,7 @@
 	ret = __cpu_up(cpu);
 	if (ret != 0)
 		goto out_notify;
-	if (!cpu_online(cpu))
-		BUG();
+	BUG_ON(!cpu_online(cpu));

Does cpu_online() have any side effects?

Quoted 18 lines
kernel/cpu.c renamed from kernel/cpu.c.old +0 -1
 	/* Now call notifier in preparation. */
 	notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c.old	2004-08-28

16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/exit.c 2004-08-28
16:08:24.000000000 +0200

kernel/exit.c renamed from kernel/exit.c.old +1 -1
@@ -503,7 +503,7 @@
 		down_read(&mm->mmap_sem);
 	}
 	atomic_inc(&mm->mm_count);
-	if (mm != tsk->active_mm) BUG();
+	BUG_ON(mm != tsk->active_mm);
 	/* more a memory barrier than a real lock */
 	task_lock(tsk);
 	tsk->mm = NULL;

@@ -878,11 +877,9 @@
 	const struct list_head *tmp, *head = &link->pidptr->task_list;

kernel/exit.c line 882

Quoted 10 lines
kernel/exit.c renamed from kernel/exit.c.old +3 -5
 
 #ifdef CONFIG_SMP
-	if (!p->sighand)
-		BUG();
-	if (!spin_is_locked(&p->sighand->siglock) &&
-				!rwlock_is_locked(&tasklist_lock))
-		BUG();
+	BUG_ON(!p->sighand);
+	BUG_ON(!spin_is_locked(&p->sighand->siglock) &&
+				!rwlock_is_locked(&tasklist_lock));

These _should_ be OK, but I'm not sure if spin_is_locked and rwlock_is_locked have any memory barrier guarantees.

Quoted 32 lines
kernel/exit.c renamed from kernel/exit.c.old +1 -3
 #endif
 	tmp = link->pid_chain.next;
 	if (tmp == head)

@@ -1350,8 +1347,7 @@
 		if (options & __WNOTHREAD)
 			break;
 		tsk = next_thread(tsk);
-		if (tsk->signal != current->signal)
-			BUG();
+		BUG_ON(tsk->signal != current->signal);
 	} while (tsk != current);
 
 	read_unlock(&tasklist_lock);
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c.old	2004-08-28

16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/fork.c 2004-08-28
16:08:45.000000000 +0200

kernel/fork.c renamed from kernel/fork.c.old +1 -3
@@ -809,8 +809,7 @@
 	struct files_struct *files  = current->files;
 	int rc;
 
-	if(!files)
-		BUG();
+	BUG_ON(!files);
 
 	/* This can race but the race causes us to copy when we don't
 	   need to and drop the copy */
--- linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c.old	2004-08-28

16:05:17.000000000 +0200
+++ linux-2.6.9-rc1-mm1-full-3.4/kernel/module.c 2004-08-28
16:13:44.000000000 +0200

kernel/module.c line 658

Quoted 6 lines
kernel/module.c renamed from kernel/module.c.old +1 -2
@@ -655,8 +655,7 @@
 	const unsigned long *crc;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!__find_symbol(symbol, &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol(symbol, &owner, &crc, 1));

Does __find_symbol have side effects?

kernel/module.c line 669

Quoted 10 lines
kernel/module.c renamed from kernel/module.c.old +1 -2
 	module_put(owner);
 	spin_unlock_irqrestore(&modlist_lock, flags);
 }

@@ -667,8 +666,7 @@
 	unsigned long flags;
 
 	spin_lock_irqsave(&modlist_lock, flags);
-	if (!kernel_text_address((unsigned long)addr))
-		BUG();
+	BUG_ON(!kernel_text_address((unsigned long)addr));

Side effects?

kernel/module.c line 909

Quoted 12 lines
kernel/module.c renamed from kernel/module.c.old +1 -2
 	module_put(module_text_address((unsigned long)addr));
 	spin_unlock_irqrestore(&modlist_lock, flags);

@@ -905,8 +903,7 @@
 	const unsigned long *crc;
 	struct module *owner;
 
-	if (!__find_symbol("struct_module", &owner, &crc, 1))
-		BUG();
+	BUG_ON(!__find_symbol("struct_module", &owner, &crc, 1));
 	return check_version(sechdrs, versindex, "struct_module", mod,
 			     crc);
 }

[Bunch of patch chomped]

Please verify that you don't pull important function calls into BUG_ON() statements.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$ L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+ PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r !y?(-) ------END GEEK CODE BLOCK------

author
#11 lore raw

PATCH 3/3 mm/ BUG -> BUG_ON conversions

In reply to Adrian Bunk

The patch below does BUG -> BUG_ON conversions in mm/ .

diffstat output:

 mm/bootmem.c    |    6 ++----
 mm/filemap.c    |    6 ++----
 mm/highmem.c    |   15 +++++----------
 mm/memory.c     |   12 ++++--------
 mm/mempool.c    |    5 +++--
 mm/mmap.c       |   12 ++++--------
 mm/mprotect.c   |    3 +--
 mm/msync.c      |    3 +--
 mm/page_alloc.c |    3 +--
 mm/pdflush.c    |    3 +--
 mm/shmem.c      |    3 +--
 mm/slab.c       |   30 ++++++++++--------------------
 mm/swap.c       |   12 ++++--------
 mm/swap_state.c |    6 ++----
 mm/swapfile.c   |    6 ++----
 mm/vmalloc.c    |    3 +--
 mm/vmscan.c     |   18 ++++++------------
 17 files changed, 50 insertions(+), 96 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>
mm/bootmem.c renamed from mm/bootmem.c.old +2 -4
@@ -125,8 +125,7 @@
 	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
 
 	for (i = sidx; i < eidx; i++) {
-		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
 	}
 }
 

@@ -246,8 +245,7 @@
 	 * Reserve the area now:
 	 */
 	for (i = start; i < start+areasize; i++)
-		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));
 	memset(ret, 0, size);
 	return ret;
 }
mm/filemap.c renamed from mm/filemap.c.old +2 -4
@@ -440,8 +440,7 @@
 void fastcall unlock_page(struct page *page)
 {
 	smp_mb__before_clear_bit();
-	if (!TestClearPageLocked(page))
-		BUG();
+	BUG_ON(!TestClearPageLocked(page));
 	smp_mb__after_clear_bit(); 
 	wake_up_page(page);
 }

@@ -455,8 +454,7 @@
 void end_page_writeback(struct page *page)
 {
 	if (!TestClearPageReclaim(page) || rotate_reclaimable_page(page)) {
-		if (!test_clear_page_writeback(page))
-			BUG();
+		BUG_ON(!test_clear_page_writeback(page));
 		smp_mb__after_clear_bit();
 	}
 	wake_up_page(page);
mm/highmem.c renamed from mm/highmem.c.old +5 -10
@@ -79,8 +79,7 @@
 		pkmap_count[i] = 0;
 
 		/* sanity check */
-		if (pte_none(pkmap_page_table[i]))
-			BUG();
+		BUG_ON(pte_none(pkmap_page_table[i]));
 
 		/*
 		 * Don't need an atomic fetch-and-clear op here;

@@ -161,8 +160,7 @@
 	if (!vaddr)
 		vaddr = map_new_virtual(page);
 	pkmap_count[PKMAP_NR(vaddr)]++;
-	if (pkmap_count[PKMAP_NR(vaddr)] < 2)
-		BUG();
+	BUG_ON(pkmap_count[PKMAP_NR(vaddr)] < 2);
 	spin_unlock(&kmap_lock);
 	return (void*) vaddr;
 }

@@ -177,8 +175,7 @@
 
 	spin_lock(&kmap_lock);
 	vaddr = (unsigned long)page_address(page);
-	if (!vaddr)
-		BUG();
+	BUG_ON(!vaddr);
 	nr = PKMAP_NR(vaddr);
 
 	/*

@@ -223,8 +220,7 @@
 		return 0;
 
 	page_pool = mempool_create(POOL_SIZE, page_pool_alloc, page_pool_free, NULL);
-	if (!page_pool)
-		BUG();
+	BUG_ON(!page_pool);
 	printk("highmem bounce pool size: %d pages\n", POOL_SIZE);
 
 	return 0;

@@ -266,8 +262,7 @@
 		return 0;
 
 	isa_page_pool = mempool_create(ISA_POOL_SIZE, page_pool_alloc, page_pool_free, (void *) __GFP_DMA);
-	if (!isa_page_pool)
-		BUG();
+	BUG_ON(!isa_page_pool);
 
 	printk("isa bounce pool size: %d pages\n", ISA_POOL_SIZE);
 	return 0;
mm/memory.c renamed from mm/memory.c.old +4 -8
@@ -902,8 +902,7 @@
 
 	dir = pgd_offset(mm, address);
 	flush_cache_range(vma, beg, end);
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 
 	spin_lock(&mm->page_table_lock);
 	do {

@@ -986,8 +985,7 @@
 	phys_addr -= from;
 	dir = pgd_offset(mm, from);
 	flush_cache_range(vma, beg, end);
-	if (from >= end)
-		BUG();
+	BUG_ON(from >= end);
 
 	spin_lock(&mm->page_table_lock);
 	do {

@@ -1766,10 +1764,8 @@
 
 	vma = find_vma(current->mm, addr);
 	write = (vma->vm_flags & VM_WRITE) != 0;
-	if (addr >= end)
-		BUG();
-	if (end > vma->vm_end)
-		BUG();
+	BUG_ON(addr >= end);
+	BUG_ON(end > vma->vm_end);
 	len = (end+PAGE_SIZE-1)/PAGE_SIZE-addr/PAGE_SIZE;
 	ret = get_user_pages(current, current->mm, addr,
 			len, write, 0, NULL, NULL);
mm/mempool.c renamed from mm/mempool.c.old +3 -2
@@ -170,8 +170,9 @@
  */
 void mempool_destroy(mempool_t *pool)
 {
-	if (pool->curr_nr != pool->min_nr)
-		BUG();		/* There were outstanding elements */
+	/* There were outstanding elements */
+	BUG_ON(pool->curr_nr != pool->min_nr);
+
 	free_pool(pool);
 }
 EXPORT_SYMBOL(mempool_destroy);
mm/mmap.c renamed from mm/mmap.c.old +4 -8
@@ -197,8 +197,7 @@
 	i = browse_rb(&mm->mm_rb);
 	if (i != mm->map_count)
 		printk("map_count %d rb %d\n", mm->map_count, i), bug = 1;
-	if (bug)
-		BUG();
+	BUG_ON(bug);
 }
 #else
 #define validate_mm(mm) do { } while (0)

@@ -333,8 +332,7 @@
 	struct rb_node ** rb_link, * rb_parent;
 
 	__vma = find_vma_prepare(mm, vma->vm_start,&prev, &rb_link, &rb_parent);
-	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+	BUG_ON(__vma && __vma->vm_start < vma->vm_end);
 	__vma_link(mm, vma, prev, rb_link, rb_parent);
 	mm->map_count++;
 }

@@ -704,8 +702,7 @@
 	 * (e.g. stash info in next's anon_vma_node when assigning
 	 * an anon_vma, or when trying vma_merge).  Another time.
 	 */
-	if (find_vma_prev(vma->vm_mm, vma->vm_start, &near) != vma)
-		BUG();
+	BUG_ON(find_vma_prev(vma->vm_mm, vma->vm_start, &near) != vma);
 	if (!near)
 		goto none;
 

@@ -1886,8 +1883,7 @@
 		vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
 	}
 	__vma = find_vma_prepare(mm,vma->vm_start,&prev,&rb_link,&rb_parent);
-	if (__vma && __vma->vm_start < vma->vm_end)
-		BUG();
+	BUG_ON(__vma && __vma->vm_start < vma->vm_end);
 	vma_link(mm, vma, prev, rb_link, rb_parent);
 }
mm/mprotect.c renamed from mm/mprotect.c.old +1 -2
@@ -95,8 +95,7 @@
 
 	dir = pgd_offset(current->mm, start);
 	flush_cache_range(vma, beg, end);
-	if (start >= end)
-		BUG();
+	BUG_ON(start >= end);
 	spin_lock(&current->mm->page_table_lock);
 	do {
 		change_pmd_range(dir, start, end - start, newprot);
mm/msync.c renamed from mm/msync.c.old +1 -2
@@ -113,8 +113,7 @@
 	if (is_vm_hugetlb_page(vma))
 		goto out;
 
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 	do {
 		error |= filemap_sync_pmd_range(dir, address, end, vma, flags);
 		address = (address + PGDIR_SIZE) & PGDIR_MASK;
mm/page_alloc.c renamed from mm/page_alloc.c.old +1 -2
@@ -188,8 +188,7 @@
 		destroy_compound_page(page, order);
 	mask = (~0UL) << order;
 	page_idx = page - base;
-	if (page_idx & ~mask)
-		BUG();
+	BUG_ON(page_idx & ~mask);
 	index = page_idx >> (1 + order);
 
 	zone->free_pages += 1 << order;
mm/pdflush.c renamed from mm/pdflush.c.old +1 -2
@@ -191,8 +191,7 @@
 	unsigned long flags;
 	int ret = 0;
 
-	if (fn == NULL)
-		BUG();		/* Hard to diagnose if it's deferred */
+	BUG_ON(fn == NULL);	/* Hard to diagnose if it's deferred */
 
 	spin_lock_irqsave(&pdflush_lock, flags);
 	if (list_empty(&pdflush_list)) {
mm/shmem.c renamed from mm/shmem.c.old +1 -2
@@ -1696,8 +1696,7 @@
 		struct page *page;
 
 		page = find_get_page(dentry->d_inode->i_mapping, 0);
-		if (!page)
-			BUG();
+		BUG_ON(!page);
 		kunmap(page);
 		mark_page_accessed(page);
 		page_cache_release(page);
mm/slab.c renamed from mm/slab.c.old +10 -20
@@ -743,8 +743,7 @@
 
 	cache_estimate(0, cache_cache.objsize, cache_line_size(), 0,
 				&left_over, &cache_cache.num);
-	if (!cache_cache.num)
-		BUG();
+	BUG_ON(!cache_cache.num);
 
 	cache_cache.colour = left_over/cache_cache.colour_off;
 	cache_cache.colour_next = 0;

@@ -887,8 +886,7 @@
 	const unsigned long nr_freed = i;
 
 	while (i--) {
-		if (!TestClearPageSlab(page))
-			BUG();
+		BUG_ON(!TestClearPageSlab(page));
 		page++;
 	}
 	sub_page_state(nr_slab, nr_freed);

@@ -1199,8 +1197,7 @@
 	 * Always checks flags, a caller might be expecting debug
 	 * support which isn't available.
 	 */
-	if (flags & ~CREATE_MASK)
-		BUG();
+	BUG_ON(flags & ~CREATE_MASK);
 
 	if (align) {
 		/* combinations of forced alignment and advanced debugging is

@@ -1476,8 +1473,7 @@
 	func(arg);
 	local_irq_enable();
 
-	if (smp_call_function(func, arg, 1, 1))
-		BUG();
+	BUG_ON(smp_call_function(func, arg, 1, 1));
 
 	preempt_enable();
 }

@@ -1529,8 +1525,7 @@
 
 		slabp = list_entry(cachep->lists.slabs_free.prev, struct slab, list);
 #if DEBUG
-		if (slabp->inuse)
-			BUG();
+		BUG_ON(slabp->inuse);
 #endif
 		list_del(&slabp->list);
 

@@ -1554,8 +1549,7 @@
  */
 int kmem_cache_shrink(kmem_cache_t *cachep)
 {
-	if (!cachep || in_interrupt())
-		BUG();
+	BUG_ON(!cachep || in_interrupt());
 
 	return __cache_shrink(cachep);
 }

@@ -1583,8 +1577,7 @@
 {
 	int i;
 
-	if (!cachep || in_interrupt())
-		BUG();
+	BUG_ON(!cachep || in_interrupt());
 
 	/* Don't let CPUs to come and go */
 	lock_cpu_hotplug();

@@ -1703,11 +1696,9 @@
 static void kmem_flagcheck(kmem_cache_t *cachep, int flags)
 {
 	if (flags & SLAB_DMA) {
-		if (!(cachep->gfpflags & GFP_DMA))
-			BUG();
+		BUG_ON(!(cachep->gfpflags & GFP_DMA));
 	} else {
-		if (cachep->gfpflags & GFP_DMA)
-			BUG();
+		BUG_ON(cachep->gfpflags & GFP_DMA);
 	}
 }
 

@@ -1741,8 +1732,7 @@
 	/* Be lazy and only check for valid flags here,
  	 * keeping it out of the critical path in kmem_cache_alloc().
 	 */
-	if (flags & ~(SLAB_DMA|SLAB_LEVEL_MASK|SLAB_NO_GROW))
-		BUG();
+	BUG_ON(flags & ~(SLAB_DMA|SLAB_LEVEL_MASK|SLAB_NO_GROW));
 	if (flags & SLAB_NO_GROW)
 		return 0;
mm/swap.c renamed from mm/swap.c.old +4 -8
@@ -90,8 +90,7 @@
 		list_add_tail(&page->lru, &zone->inactive_list);
 		inc_page_state(pgrotated);
 	}
-	if (!test_clear_page_writeback(page))
-		BUG();
+	BUG_ON(!test_clear_page_writeback(page));
 	spin_unlock_irqrestore(&zone->lru_lock, flags);
 	return 0;
 }

@@ -302,8 +301,7 @@
 			zone = pagezone;
 			spin_lock_irq(&zone->lru_lock);
 		}
-		if (TestSetPageLRU(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
 		add_page_to_inactive_list(zone, page);
 	}
 	if (zone)

@@ -329,10 +327,8 @@
 			zone = pagezone;
 			spin_lock_irq(&zone->lru_lock);
 		}
-		if (TestSetPageLRU(page))
-			BUG();
-		if (TestSetPageActive(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
+		BUG_ON(TestSetPageActive(page));
 		add_page_to_active_list(zone, page);
 	}
 	if (zone)
mm/swap_state.c renamed from mm/swap_state.c.old +2 -4
@@ -144,8 +144,7 @@
 	int pf_flags;
 	int err;
 
-	if (!PageLocked(page))
-		BUG();
+	BUG_ON(!PageLocked(page));
 
 	for (;;) {
 		entry = get_swap_page();

@@ -229,8 +228,7 @@
 	if (!err) {
 		remove_from_page_cache(page);
 		page_cache_release(page);	/* pagecache ref */
-		if (!swap_duplicate(entry))
-			BUG();
+		BUG_ON(!swap_duplicate(entry));
 		SetPageDirty(page);
 		INC_CACHE_INFO(add_total);
 	} else if (err == -EEXIST)
mm/swapfile.c renamed from mm/swapfile.c.old +2 -4
@@ -312,8 +312,7 @@
 {
 	int retval = 0;
 
-	if (!PageLocked(page))
-		BUG();
+	BUG_ON(!PageLocked(page));
 	switch (page_count(page)) {
 	case 3:
 		if (!PagePrivate(page))

@@ -506,8 +505,7 @@
 	end = address + size;
 	if (end > PGDIR_SIZE)
 		end = PGDIR_SIZE;
-	if (address >= end)
-		BUG();
+	BUG_ON(address >= end);
 	do {
 		foundaddr = unuse_pmd(vma, pmd, address, end - address,
 						offset, entry, page);
mm/vmalloc.c renamed from mm/vmalloc.c.old +1 -2
@@ -318,8 +318,7 @@
 		int i;
 
 		for (i = 0; i < area->nr_pages; i++) {
-			if (unlikely(!area->pages[i]))
-				BUG();
+			BUG_ON(!area->pages[i]);
 			__free_page(area->pages[i]);
 		}
mm/vmscan.c renamed from mm/vmscan.c.old +6 -12
@@ -564,8 +564,7 @@
 			prefetchw_prev_lru_page(page,
 						&zone->inactive_list, flags);
 
-			if (!TestClearPageLRU(page))
-				BUG();
+			BUG_ON(!TestClearPageLRU(page));
 			list_del(&page->lru);
 			if (get_page_testone(page)) {
 				/*

@@ -603,8 +602,7 @@
 		 */
 		while (!list_empty(&page_list)) {
 			page = lru_to_page(&page_list);
-			if (TestSetPageLRU(page))
-				BUG();
+			BUG_ON(TestSetPageLRU(page));
 			list_del(&page->lru);
 			if (PageActive(page))
 				add_page_to_active_list(zone, page);

@@ -662,8 +660,7 @@
 	while (pgscanned < nr_pages && !list_empty(&zone->active_list)) {
 		page = lru_to_page(&zone->active_list);
 		prefetchw_prev_lru_page(page, &zone->active_list, flags);
-		if (!TestClearPageLRU(page))
-			BUG();
+		BUG_ON(!TestClearPageLRU(page));
 		list_del(&page->lru);
 		if (get_page_testone(page)) {
 			/*

@@ -735,10 +732,8 @@
 	while (!list_empty(&l_inactive)) {
 		page = lru_to_page(&l_inactive);
 		prefetchw_prev_lru_page(page, &l_inactive, flags);
-		if (TestSetPageLRU(page))
-			BUG();
-		if (!TestClearPageActive(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
+		BUG_ON(!TestClearPageActive(page));
 		list_move(&page->lru, &zone->inactive_list);
 		pgmoved++;
 		if (!pagevec_add(&pvec, page)) {

@@ -764,8 +759,7 @@
 	while (!list_empty(&l_active)) {
 		page = lru_to_page(&l_active);
 		prefetchw_prev_lru_page(page, &l_active, flags);
-		if (TestSetPageLRU(page))
-			BUG();
+		BUG_ON(TestSetPageLRU(page));
 		BUG_ON(!PageActive(page));
 		list_move(&page->lru, &zone->active_list);
 		pgmoved++;
#12 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 39 lines · Adrian Bunk

The patch below does BUG -> BUG_ON conversions in mm/ .

diffstat output:

 mm/bootmem.c    |    6 ++----
 mm/filemap.c    |    6 ++----
 mm/highmem.c    |   15 +++++----------
 mm/memory.c     |   12 ++++--------
 mm/mempool.c    |    5 +++--
 mm/mmap.c       |   12 ++++--------
 mm/mprotect.c   |    3 +--
 mm/msync.c      |    3 +--
 mm/page_alloc.c |    3 +--
 mm/pdflush.c    |    3 +--
 mm/shmem.c      |    3 +--
 mm/slab.c       |   30 ++++++++++--------------------
 mm/swap.c       |   12 ++++--------
 mm/swap_state.c |    6 ++----
 mm/swapfile.c   |    6 ++----
 mm/vmalloc.c    |    3 +--
 mm/vmscan.c     |   18 ++++++------------
 17 files changed, 50 insertions(+), 96 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>

--- linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c.old 2004-08-28
16:25:18.000000000 +0200 +++
linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c 2004-08-28 16:26:48.000000000
+0200 @@ -125,8 +125,7 @@

mm/bootmem.c renamed from mm/bootmem.c.old +1 -2
 	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
 
 	for (i = sidx; i < eidx; i++) {
-		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
 	}
 }

mm/bootmem.c line 248

Quoted 6 lines
mm/bootmem.c renamed from mm/bootmem.c.old +1 -2
@@ -246,8 +245,7 @@
 	 * Reserve the area now:
 	 */
 	for (i = start; i < start+areasize; i++)
-		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));

BUG_ON is like assert(). It may be #defined to nothing. Do not place expression with side effects into it. --
vda

#13 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 39 lines · Denis Vlasenko
Quoted 38 lines · Adrian Bunk

The patch below does BUG -> BUG_ON conversions in mm/ .

diffstat output:

 mm/bootmem.c    |    6 ++----
 mm/filemap.c    |    6 ++----
 mm/highmem.c    |   15 +++++----------
 mm/memory.c     |   12 ++++--------
 mm/mempool.c    |    5 +++--
 mm/mmap.c       |   12 ++++--------
 mm/mprotect.c   |    3 +--
 mm/msync.c      |    3 +--
 mm/page_alloc.c |    3 +--
 mm/pdflush.c    |    3 +--
 mm/shmem.c      |    3 +--
 mm/slab.c       |   30 ++++++++++--------------------
 mm/swap.c       |   12 ++++--------
 mm/swap_state.c |    6 ++----
 mm/swapfile.c   |    6 ++----
 mm/vmalloc.c    |    3 +--
 mm/vmscan.c     |   18 ++++++------------
 17 files changed, 50 insertions(+), 96 deletions(-)
Signed-off-by Adrian Bunk <[email protected]>

--- linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c.old 2004-08-28
16:25:18.000000000 +0200 +++
linux-2.6.9-rc1-mm1-full-3.4/mm/bootmem.c 2004-08-28 16:26:48.000000000
+0200 @@ -125,8 +125,7 @@

mm/bootmem.c renamed from mm/bootmem.c.old +1 -2
 	sidx = start - (bdata->node_boot_start/PAGE_SIZE);
 
 	for (i = sidx; i < eidx; i++) {
-		if (unlikely(!test_and_clear_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(!test_and_clear_bit(i, bdata->node_bootmem_map));
 	}
 }
Quoted 10 lines

mm/bootmem.c line 248

Quoted 7 lines
mm/bootmem.c renamed from mm/bootmem.c.old +1 -2
@@ -246,8 +245,7 @@
 	 * Reserve the area now:
 	 */
 	for (i = start; i < start+areasize; i++)
-		if (unlikely(test_and_set_bit(i, bdata->node_bootmem_map)))
-			BUG();
+		BUG_ON(test_and_set_bit(i, bdata->node_bootmem_map));

BUG_ON is like assert(). It may be #defined to nothing. Do not place expression with side effects into it.

I've seen several write this, and I don't agree. I was the one that introduced BUG_ON, actually, with the original bio patches in 2.5.1-pre. I never intended it to be a nop, no more than making BUG() a nop would be stupid. It was just short-hand for adding the unlikely() without the readability problem.

BUG_ON(1); must always BUG(). That said, it's never wise to put expressions with side-effects into macros.

Signature · 1 line
Jens Axboe
author
#14 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 3 lines · Jens Axboe

BUG_ON(1); must always BUG(). That said, it's never wise to put expressions with side-effects into macros.

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

Quoted 1 line

Jens Axboe

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#15 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 9 lines · Adrian Bunk
Quoted 3 lines · Jens Axboe

BUG_ON(1); must always BUG(). That said, it's never wise to put expressions with side-effects into macros.

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

That only makes sense if you are using BUG incorrectly. A BUG() condition is something that is non-recoverable, undefining that doesn't make any sense regardless of the environment.

Signature · 1 line
Jens Axboe
#16 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:

Quoted 8 lines
Quoted 4 lines

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

That only makes sense if you are using BUG incorrectly. A BUG() condition is something that is non-recoverable, undefining that doesn't make any sense regardless of the environment.

Why not? Giving reports about unrecoverable errors is sensible only if the report can be read. On system this is not the case, you can just salvage the memory and let it crash.

Regards
Oliver

#17 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 6 lines · Oliver Neukum

Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:

Quoted 5 lines
Quoted 4 lines

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

Quoted 7 lines
Quoted 3 lines

That only makes sense if you are using BUG incorrectly. A BUG() condition is something that is non-recoverable, undefining that doesn't make any sense regardless of the environment.

Why not? Giving reports about unrecoverable errors is sensible only if the report can be read. On system this is not the case, you can just salvage the memory and let it crash.

"Unrecoverable" can quite easily mean "something really bad has happened, corruption imminent". So maybe you would want BUG/BUG_ON to restart the box there, the restart-on-panic should help you there.

You're mail is not making a case for defining BUG/BUG_ON to a nop, which is what the discussion is about.

Signature · 1 line
Jens Axboe
author
#18 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 11 lines · Jens Axboe
Quoted 10 lines · Oliver Neukum

Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:

Quoted 8 lines
Quoted 4 lines

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

That only makes sense if you are using BUG incorrectly. A BUG() condition is something that is non-recoverable, undefining that doesn't make any sense regardless of the environment.

Quoted 8 lines
Quoted 3 lines

Why not? Giving reports about unrecoverable errors is sensible only if the report can be read. On system this is not the case, you can just salvage the memory and let it crash.

"Unrecoverable" can quite easily mean "something really bad has happened, corruption imminent". So maybe you would want BUG/BUG_ON to restart the box there, the restart-on-panic should help you there.
...

The current sh/sh64 implementation doesn't seem to do any of the things you expect from BUG:

#define BUG() do { \
        printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
        asm volatile("nop"); \
} while (0)
Quoted 1 line

Jens Axboe

cu
Adrian

Signature · 4 lines
       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed
#19 lore raw

Re: [2.6 patch][3/3] mm/ BUG -> BUG_ON conversions

Quoted 21 lines · Adrian Bunk
Quoted 11 lines · Jens Axboe
Quoted 10 lines · Oliver Neukum

Am Sonntag, 29. August 2004 14:03 schrieb Jens Axboe:

Quoted 8 lines
Quoted 4 lines

The intention is, to add an option that lets BUG/BUG_ON/WARN_ON/PAGE_BUG do nothing. This option should be hidden under EMBEDDED.

In some environments, this seems to be desirable.

That only makes sense if you are using BUG incorrectly. A BUG() condition is something that is non-recoverable, undefining that doesn't make any sense regardless of the environment.

Quoted 8 lines
Quoted 3 lines

Why not? Giving reports about unrecoverable errors is sensible only if the report can be read. On system this is not the case, you can just salvage the memory and let it crash.

"Unrecoverable" can quite easily mean "something really bad has happened, corruption imminent". So maybe you would want BUG/BUG_ON to restart the box there, the restart-on-panic should help you there.
...

Quoted 7 lines

The current sh/sh64 implementation doesn't seem to do any of the things you expect from BUG:

#define BUG() do { \
        printk("kernel BUG at %s:%d!\n", __FILE__, __LINE__); \
        asm volatile("nop"); \
} while (0)

Well too bad for them, I'm glad I'm not trusting any data to a machine with that architecture.

Signature · 1 line
Jens Axboe