Hi, kernel list readers.
I have been building (and hopefully booting) ac-21 with gcc-3.0 snapshot
dated 20010312. I have cleared the 99% of the warnings that 3.0 issues
when building the kernel. Obviuosly, only in the main kernel part for
i386 and the drivers I use. I suppose other arch will require a similar
cleanup.
All are related to multiline strings in asm() sentences, that seem to have
been deprecated, and out: or default: labels at the end of blocks. Pathc
is inlined.
There are a couple more curious errors:
1) Is this a bug ?
make[1]: Entering directory `/usr/src/linux-2.4.2-ac21/arch/i386/kernel'
gcc ... -c -o setup.o setup.c
setup.c: In function `get_cpuinfo':
setup.c:2378: warning: unused variable `x86_udelay_tsc'
I have not patched this. Is a reminder of previous work, or should be used
for something and the use has flown erroneously ?
2)
gcc ... -c -o aic7xxx.o aic7xxx.c
aic7xxx.c: In function `ahc_print_scb':
aic7xxx.c:1335: warning: operation on `i' may be undefined
(nine times)
The piece of code is three reps of this:
printf(" %#02x %#02x %#02x %#02x\n",
hscb->shared_data.cdb[i++],
hscb->shared_data.cdb[i++],
hscb->shared_data.cdb[i++],
hscb->shared_data.cdb[i++]);
I suppose that gcc claims that the result is dependent on evaluation order
of the args to the printf(), so it is potentially dangerous. Just chaged it
to
hscb->shared_data.cdb[ 1],
hscb->shared_data.cdb[ 2],
hscb->shared_data.cdb[ 3],
etc.
If mantainers do not like the way I corrected this, at least it is a list
of thigs to look at.
BTW, after that changes, the kernel built and booted ok.
============ patch-gcc-3
--- linux-2.4.2-ac21/fs/smbfs/cache.c.orig Fri Mar 23 00:45:27 2001
+++ linux-2.4.2-ac21/fs/smbfs/cache.c Fri Mar 23 00:46:04 2001
@@ -34,7 +34,7 @@
page = grab_cache_page(&dir->i_data, 0);
if (!page)
- goto out;
+ return;
if (!Page_Uptodate(page))
goto out_unlock;
@@ -47,7 +47,6 @@
out_unlock:
UnlockPage(page);
page_cache_release(page);
-out:
}
/*
--- linux-2.4.2-ac21/fs/smbfs/ioctl.c.orig Fri Mar 23 00:46:22 2001
+++ linux-2.4.2-ac21/fs/smbfs/ioctl.c Fri Mar 23 00:46:56 2001
@@ -45,7 +45,7 @@
if (!copy_from_user(&opt, (void *)arg, sizeof(opt)))
result = smb_newconn(server, &opt);
break;
- default:
+ default:;
}
return result;
--- linux-2.4.2-ac21/include/asm-i386/string.h.orig Thu Mar 22 23:17:03
2001
+++ linux-2.4.2-ac21/include/asm-i386/string.h Thu Mar 22 23:20:40 2001
@@ -516,12 +516,12 @@
{
if (!size)
return addr;
- __asm__("repnz; scasb
- jnz 1f
- dec %%edi
-1: "
- : "=D" (addr), "=c" (size)
- : "0" (addr), "1" (size), "a" (c));
+ __asm__("repnz; scasb\n\t"
+ " jnz 1f\n\t"
+ " dec %%edi\n\t"
+ "1:"
+ : "=D" (addr), "=c" (size)
+ : "0" (addr), "1" (size), "a" (c));
return addr;
}
--- linux-2.4.2-ac21/include/asm-i386/system.h.orig Thu Mar 22 23:20:50
2001
+++ linux-2.4.2-ac21/include/asm-i386/system.h Thu Mar 22 23:21:47 2001
@@ -145,10 +145,10 @@
unsigned int low, unsigned int high)
{
__asm__ __volatile__ (
- "1: movl (%0), %%eax;
- movl 4(%0), %%edx;
- cmpxchg8b (%0);
- jnz 1b"
+ "1: movl (%0), %%eax;\n\t"
+ "movl 4(%0), %%edx;\n\t"
+ "cmpxchg8b (%0);\n\t"
+ "jnz 1b"
:: "D"(ptr),
"b"(low),
"c"(high)
--- linux-2.4.2-ac21/include/asm-i386/checksum.h.orig Thu Mar 22 23:21:58
2001
+++ linux-2.4.2-ac21/include/asm-i386/checksum.h Thu Mar 22 23:25:19 2001
@@ -69,25 +69,24 @@
unsigned int ihl) {
unsigned int sum;
- __asm__ __volatile__("
- movl (%1), %0
- subl $4, %2
- jbe 2f
- addl 4(%1), %0
- adcl 8(%1), %0
- adcl 12(%1), %0
-1: adcl 16(%1), %0
- lea 4(%1), %1
- decl %2
- jne 1b
- adcl $0, %0
- movl %0, %2
- shrl $16, %0
- addw %w2, %w0
- adcl $0, %0
- notl %0
-2:
- "
+ __asm__ __volatile__(
+" movl (%1), %0\n"
+" subl $4, %2\n"
+" jbe 2f\n"
+" addl 4(%1), %0\n"
+" adcl 8(%1), %0\n"
+" adcl 12(%1), %0\n"
+"1: adcl 16(%1), %0\n"
+" lea 4(%1), %1\n"
+" decl %2\n"
+" jne 1b\n"
+" adcl $0, %0\n"
+" movl %0, %2\n"
+" shrl $16, %0\n"
+" addw %w2, %w0\n"
+" adcl $0, %0\n"
+" notl %0\n"
+"2:"
/* Since the input registers which are loaded with iph and ipl
are modified, we must also specify them as outputs, or gcc
will assume they contain their original values. */
@@ -102,10 +101,9 @@
static inline unsigned int csum_fold(unsigned int sum)
{
- __asm__("
- addl %1, %0
- adcl $0xffff, %0
- "
+ __asm__(
+ "addl %1, %0\n"
+ "adcl $0xffff, %0\n"
: "=r" (sum)
: "r" (sum << 16), "0" (sum & 0xffff0000)
);
@@ -118,12 +116,11 @@
unsigned short proto,
unsigned int sum)
{
- __asm__("
- addl %1, %0
- adcl %2, %0
- adcl %3, %0
- adcl $0, %0
- "
+ __asm__(
+ "addl %1, %0\n"
+ "adcl %2, %0\n"
+ "adcl %3, %0\n"
+ "adcl $0, %0\n"
: "=r" (sum)
: "g" (daddr), "g"(saddr), "g"((ntohs(len)<<16)+proto*256), "0"(sum));
return sum;
@@ -158,19 +155,18 @@
unsigned short proto,
unsigned int sum)
{
- __asm__("
- addl 0(%1), %0
- adcl 4(%1), %0
- adcl 8(%1), %0
- adcl 12(%1), %0
- adcl 0(%2), %0
- adcl 4(%2), %0
- adcl 8(%2), %0
- adcl 12(%2), %0
- adcl %3, %0
- adcl %4, %0
- adcl $0, %0
- "
+ __asm__(
+ "addl 0(%1), %0\n"
+ "adcl 4(%1), %0\n"
+ "adcl 8(%1), %0\n"
+ "adcl 12(%1), %0\n"
+ "adcl 0(%2), %0\n"
+ "adcl 4(%2), %0\n"
+ "adcl 8(%2), %0\n"
+ "adcl 12(%2), %0\n"
+ "adcl %3, %0\n"
+ "adcl %4, %0\n"
+ "adcl $0, %0\n"
: "=&r" (sum)
: "r" (saddr), "r" (daddr),
"r"(htonl(len)), "r"(htonl(proto)), "0"(sum));
--- linux-2.4.2-ac21/include/asm-i386/floppy.h.orig Thu Mar 22 23:27:27
2001
+++ linux-2.4.2-ac21/include/asm-i386/floppy.h Thu Mar 22 23:28:37 2001
@@ -75,28 +75,28 @@
#ifndef NO_FLOPPY_ASSEMBLER
__asm__ (
- "testl %1,%1
- je 3f
-1: inb %w4,%b0
- andb $160,%b0
- cmpb $160,%b0
- jne 2f
- incw %w4
- testl %3,%3
- jne 4f
- inb %w4,%b0
- movb %0,(%2)
- jmp 5f
-4: movb (%2),%0
- outb %b0,%w4
-5: decw %w4
- outb %0,$0x80
- decl %1
- incl %2
- testl %1,%1
- jne 1b
-3: inb %w4,%b0
-2: "
+" testl %1,%1\n"
+" je 3f\n"
+"1: inb %w4,%b0\n"
+" andb $160,%b0\n"
+" cmpb $160,%b0\n"
+" jne 2f\n"
+" incw %w4\n"
+" testl %3,%3\n"
+" jne 4f\n"
+" inb %w4,%b0\n"
+" movb %0,(%2)\n"
+" jmp 5f\n"
+"4: movb (%2),%0\n"
+" outb %b0,%w4\n"
+"5: decw %w4\n"
+" outb %0,$0x80\n"
+" decl %1\n"
+" incl %2\n"
+" testl %1,%1\n"
+" jne 1b\n"
+"3: inb %w4,%b0\n"
+"2:"
: "=a" ((char) st),
"=c" ((long) virtual_dma_count),
"=S" ((long) virtual_dma_addr)
--- linux-2.4.2-ac21/net/ipv4/icmp.c.orig Thu Mar 22 23:39:22 2001
+++ linux-2.4.2-ac21/net/ipv4/icmp.c Thu Mar 22 23:42:23 2001
@@ -574,7 +574,7 @@
} else {
info = ip_rt_frag_needed(iph, ntohs(icmph->un.frag.mtu));
if (!info)
- goto out;
+ return;
}
break;
case ICMP_SR_FAILED:
@@ -585,7 +585,7 @@
break;
}
if (icmph->code>NR_ICMP_UNREACH)
- goto out;
+ return;
} else if (icmph->type == ICMP_PARAMETERPROB) {
info = ntohl(icmph->un.gateway)>>24;
}
@@ -613,7 +613,7 @@
if (net_ratelimit())
printk(KERN_WARNING "%u.%u.%u.%u sent an
invalid ICMP error to a broadcast.\n",
NIPQUAD(skb->nh.iph->saddr));
- goto out;
+ return;
}
}
@@ -621,7 +621,7 @@
* avoid additional coding at protocol handlers.
*/
if (!pskb_may_pull(skb, iph->ihl*4+8))
- goto out;
+ return;
iph = (struct iphdr *) skb->data;
protocol = iph->protocol;
@@ -668,7 +668,6 @@
ipprot = nextip;
}
-out:
}
@@ -879,7 +878,7 @@
case CHECKSUM_NONE:
if ((u16)csum_fold(skb_checksum(skb, 0, skb->len, 0)))
goto error;
- default:
+ default:;
}
if (!pskb_pull(skb, sizeof(struct icmphdr)))
--- linux-2.4.2-ac21/drivers/scsi/aic7xxx/aic7xxx.c.orig Fri Mar 23
01:01:40 2001
+++ linux-2.4.2-ac21/drivers/scsi/aic7xxx/aic7xxx.c Fri Mar 23 00:53:11
2001
@@ -1327,22 +1327,21 @@
hscb->scsiid,
hscb->lun,
hscb->cdb_len);
- i=0;
printf("Shared Data: %#02x %#02x %#02x %#02x\n",
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++]);
+ hscb->shared_data.cdb[ 0],
+ hscb->shared_data.cdb[ 1],
+ hscb->shared_data.cdb[ 2],
+ hscb->shared_data.cdb[ 3]);
printf(" %#02x %#02x %#02x %#02x\n",
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++]);
+ hscb->shared_data.cdb[ 4],
+ hscb->shared_data.cdb[ 5],
+ hscb->shared_data.cdb[ 6],
+ hscb->shared_data.cdb[ 7]);
printf(" %#02x %#02x %#02x %#02x\n",
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++],
- hscb->shared_data.cdb[i++]);
+ hscb->shared_data.cdb[ 8],
+ hscb->shared_data.cdb[ 9],
+ hscb->shared_data.cdb[10],
+ hscb->shared_data.cdb[11]);
printf(" dataptr:%#x datacnt:%#x sgptr:%#x tag:%#x\n",
ahc_le32toh(hscb->dataptr),
ahc_le32toh(hscb->datacnt),
--- linux-2.4.2-ac21/drivers/i2c/i2c-core.c.orig Fri Mar 23 00:42:08 2001
+++ linux-2.4.2-ac21/drivers/i2c/i2c-core.c Fri Mar 23 00:43:40 2001
@@ -378,13 +378,9 @@
if ((res = driver->
detach_client(client)))
{
- printk("i2c-core.o: while "
- "unregistering driver "
- "`%s', the client at "
- "address %02x of
- adapter `%s' could not
- be detached; driver
- not unloaded!",
+ printk("i2c-core.o: while unregistering
driver <%s>"
+ " the client at address %02x of
adapter <%s>"
+ " could not be detached; driver
not unloaded!",
driver->name,
client->addr,
adap->name);
--- linux-2.4.2-ac21/arch/i386/kernel/semaphore.c.orig Thu Mar 22
23:42:54 2001
+++ linux-2.4.2-ac21/arch/i386/kernel/semaphore.c Thu Mar 22 23:46:58
2001
@@ -231,49 +231,45 @@
);
asm(
-"
-.align 4
-.globl __down_read_failed
-__down_read_failed:
- pushl %edx
- pushl %ecx
- jnc 2f
-
-3: call down_read_failed_biased
-
-1: popl %ecx
- popl %edx
- ret
-
-2: call down_read_failed
- " LOCK "subl $1,(%eax)
- jns 1b
- jnc 2b
- jmp 3b
-"
+".align 4\n"
+".globl __down_read_failed\n"
+"__down_read_failed:\n"
+" pushl %edx\n"
+" pushl %ecx\n"
+" jnc 2f\n"
+"\n"
+"3: call down_read_failed_biased\n"
+"\n"
+"1: popl %ecx\n"
+" popl %edx\n"
+" ret\n"
+"\n"
+"2: call down_read_failed\n"
+LOCK "subl $1,(%eax)\n"
+" jns 1b\n"
+" jnc 2b\n"
+" jmp 3b\n"
);
asm(
-"
-.align 4
-.globl __down_write_failed
-__down_write_failed:
- pushl %edx
- pushl %ecx
- jnc 2f
-
-3: call down_write_failed_biased
-
-1: popl %ecx
- popl %edx
- ret
-
-2: call down_write_failed
- " LOCK "subl $" RW_LOCK_BIAS_STR ",(%eax)
- jz 1b
- jnc 2b
- jmp 3b
-"
+".align 4\n"
+".globl __down_write_failed\n"
+"__down_write_failed:\n"
+" pushl %edx\n"
+" pushl %ecx\n"
+" jnc 2f\n"
+"\n"
+"3: call down_write_failed_biased\n"
+"\n"
+"1: popl %ecx\n"
+" popl %edx\n"
+" ret\n"
+"\n"
+"2: call down_write_failed\n"
+LOCK "subl $" RW_LOCK_BIAS_STR ",(%eax)\n"
+" jz 1b\n"
+" jnc 2b\n"
+" jmp 3b\n"
);
struct rw_semaphore *FASTCALL(rwsem_wake_readers(struct rw_semaphore *sem));
@@ -384,23 +380,21 @@
}
asm(
-"
-.align 4
-.globl __rwsem_wake
-__rwsem_wake:
- pushl %edx
- pushl %ecx
-
- jz 1f
- call rwsem_wake_readers
- jmp 2f
-
-1: call rwsem_wake_writer
-
-2: popl %ecx
- popl %edx
- ret
-"
+".align 4\n"
+".globl __rwsem_wake\n"
+"__rwsem_wake:\n"
+" pushl %edx\n"
+" pushl %ecx\n"
+"\n"
+" jz 1f\n"
+" call rwsem_wake_readers\n"
+" jmp 2f\n"
+"\n"
+"1: call rwsem_wake_writer\n"
+"\n"
+"2: popl %ecx\n"
+" popl %edx\n"
+" ret\n"
);
/* Called when someone has done an up that transitioned from
@@ -425,30 +419,28 @@
#if defined(CONFIG_SMP)
asm(
-"
-.align 4
-.globl __write_lock_failed
-__write_lock_failed:
- " LOCK "addl $" RW_LOCK_BIAS_STR ",(%eax)
-1: cmpl $" RW_LOCK_BIAS_STR ",(%eax)
- jne 1b
-
- " LOCK "subl $" RW_LOCK_BIAS_STR ",(%eax)
- jnz __write_lock_failed
- ret
-
-
-.align 4
-.globl __read_lock_failed
-__read_lock_failed:
- lock ; incl (%eax)
-1: cmpl $1,(%eax)
- js 1b
-
- lock ; decl (%eax)
- js __read_lock_failed
- ret
-"
+".align 4\n"
+".globl __write_lock_failed\n"
+"__write_lock_failed:\n"
+LOCK "addl $" RW_LOCK_BIAS_STR ",(%eax)\n"
+"1: cmpl $" RW_LOCK_BIAS_STR ",(%eax)\n"
+" jne 1b\n"
+"\n"
+LOCK "subl $" RW_LOCK_BIAS_STR ",(%eax)\n"
+" jnz __write_lock_failed\n"
+" ret\n"
+"\n"
+"\n"
+".align 4\n"
+".globl __read_lock_failed\n"
+"__read_lock_failed:\n"
+" lock ; incl (%eax)\n"
+"1: cmpl $1,(%eax)\n"
+" js 1b\n"
+"\n"
+" lock ; decl (%eax)\n"
+" js __read_lock_failed\n"
+" ret\n"
);
#endif
--
J.A. Magallon # Let the source
mailto:[email protected] # be with you, Luke...
Linux werewolf 2.4.2-ac21 #5 SMP Thu Mar 22 23:47:26 CET 2001 i686
> page_cache_release(page);
> -out:
out:;
does that trick
> - default:
> + default:;
Agree - done
> --- linux-2.4.2-ac21/net/ipv4/icmp.c.orig Thu Mar 22 23:39:22 2001
> +++ linux-2.4.2-ac21/net/ipv4/icmp.c Thu Mar 22 23:42:23 2001
Again out:;
> goto error;
> - default:
> + default:;
Ok
The aic7xxx change looks right too. Someone with the hardware handy needs to
check that one though.
As to the asm - I'll apply it to -ac if you can verify the asm after changes
goes happily through the older gcc/binutils (should do) and send me a nice
clean diff of just those changes
On 03.23 Alan Cox wrote:
> > page_cache_release(page);
> > -out:
>
> out:;
>
Yes, a null sentence can shut up the compiler. But what is the purpose of
a jump to the end instead of a return ? Some optimization ?
> does that trick
>
> > - default:
> > + default:;
>
Same, I have not tested if gcc-3 will complain about a switch that not
covers all values (ie, no default:). But the logic thing would be to kill
the default: completely. Mmmm, and older compilers will eat it with no
default: ?
>
> The aic7xxx change looks right too. Someone with the hardware handy needs to
> check that one though.
>
It work on my 7880.
> As to the asm - I'll apply it to -ac if you can verify the asm after changes
> goes happily through the older gcc/binutils (should do) and send me a nice
> clean diff of just those changes
>
Is there a non-written standard for coding that asm's ?
For example:
" adcl 12(%1), %0\n"
"1: adcl 16(%1), %0\n"
" lea 4(%1), %1\n"
or
"adcl 12(%1), %0\n\t"
"1: adcl 16(%1), %0\n\t"
"lea 4(%1), %1\n\t"
--
J.A. Magallon # Let the source
mailto:[email protected] # be with you, Luke...
Linux werewolf 2.4.2-ac21 #5 SMP Thu Mar 22 23:47:26 CET 2001 i686
On Fri, Mar 23, 2001 at 01:38:00AM +0100, J . A . Magallon wrote:
> Yes, a null sentence can shut up the compiler. But what is the purpose of
> a jump to the end instead of a return ? Some optimization ?
So that when someone decides that the function needs to do some extra
initialisation at the beginning and some extra cleanup at the end,
they don't accidentally miss an exit point.
Tim.
*/
"J . A . Magallon" <[email protected]> said:
> I have been building (and hopefully booting) ac-21 with gcc-3.0 snapshot
> dated 20010312. I have cleared the 99% of the warnings that 3.0 issues
> when building the kernel. Obviuosly, only in the main kernel part for
> i386 and the drivers I use. I suppose other arch will require a similar
> cleanup.
>
> All are related to multiline strings in asm() sentences, that seem to have
> been deprecated, and out: or default: labels at the end of blocks. Pathc
> is inlined.
The problem with labels at the end of blocks, like so:
{
....
goto out;
....
out:
}
is that this is not legal C: The label should be part of a sentence, and
there is none. Just write (note the ';' after the label):
{
....
goto out;
....
out: ;
}
(Yes, this is ugly).
--
Dr. Horst H. von Brand mailto:[email protected]
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
Also sprach Alan Cox:
} > - default:
} > + default:;
}
} Agree - done
}
This kind of coding makes me want to cry. What's so wrong with:
default:
break;
instead? The ';' is hard to notice and, if people don't leave the
"default:" at the end, then bad things could happen...
--
|| Bill Wendling [email protected]
On Fri, 23 Mar 2001, Bill Wendling wrote:
> Also sprach Alan Cox:
>
> } > - default:
> } > + default:;
> }
> } Agree - done
> }
> This kind of coding makes me want to cry. What's so wrong with:
>
> default:
> break;
>
> instead? The ';' is hard to notice and
I agree. I'd much prefer that syntax also.
Or just remove the "default:" altogether, when it doesn't make any
difference.
Linus
On 03.23 Linus Torvalds wrote:
>
> I agree. I'd much prefer that syntax also.
>
> Or just remove the "default:" altogether, when it doesn't make any
> difference.
>
Well, at last some sense. The same is with that ugly out: at the end
of the function. Just change all that 'goto out' for a return.
It does not matter, -O2 is going to do what it wants.
And the missing return 0 at the end of functions that call a 'noreturn'
function. gcc 2.96 still wants them. But it looks like a religious matter
to put ot not to put that stupid return just to shut up the compiler.
As I understand, the noreturn says that the function that is marked as
noreturn is allowed to have missing correct return paths, and the compiler
can build, for example <panic>, without worring about the global state
once it has entered <panic>. But <info gcc> says nothing about functions
that call a 'noreturn' function. So I see as INCORRECT to omit a return path
in a function that calls <panic>.
And if people is so worried about fast paths, begin to use 'const' or
'pure' functions. I think that can help the compiler to generate fast code
more than trying to do hancrafted fast paths that the compiler will reorganize.
--
J.A. Magallon # Let the source
mailto:[email protected] # be with you, Luke...
Linux werewolf 2.4.2-ac22 #3 SMP Fri Mar 23 02:06:00 CET 2001 i686
On Fri, Mar 23, 2001 at 01:38:00AM +0100, J . A . Magallon wrote:
> Is there a non-written standard for coding that asm's ?
> For example:
> " adcl 12(%1), %0\n"
> "1: adcl 16(%1), %0\n"
> " lea 4(%1), %1\n"
>
> or
>
> "adcl 12(%1), %0\n\t"
^[1]
> "1: adcl 16(%1), %0\n\t"
> "lea 4(%1), %1\n\t"
The first one is better readable and the latter one is more
portable (since the first may contain tabs in the string, instead
of spaces and no one sees this).
You'll see, what I mean with readable, if you omit the tab in [1].
Regards
Ingo Oeser
--
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
<<<<<<<<<<<< been there and had much fun >>>>>>>>>>>>
On Fri, Mar 23, 2001 at 11:59:09PM +0100, J . A . Magallon wrote:
>
> On 03.23 Linus Torvalds wrote:
> >
> > I agree. I'd much prefer that syntax also.
> >
> > Or just remove the "default:" altogether, when it doesn't make any
> > difference.
> >
>
> Well, at last some sense. The same is with that ugly out: at the end
> of the function. Just change all that 'goto out' for a return.
> It does not matter, -O2 is going to do what it wants.
>
This has nothing to do with fastpathing and object code optimization. C
doesn't have exception handling, so you either have to remember to undo
allocations etc. in failure cases all through the code, or you stick your
undo code at the end of the function and have all failure cases jump to the
relevant label. It's not pretty, but it's much less error-prone e.g.
func()
{
error = 0;
a = alloc_something();
if (some failure) {
error = XXX;
goto out;
}
b = alloc_something_else();
if (some other failure) {
error = YYY;
goto out1;
}
...
out1:
dealloc(b);
out:
dealloc(a);
return(error);
}
This is arguably easier to follow and less likely to get broken than the
alternative of embedding all the unwind code at each error point.
Tim
--
Tim Wright - [email protected] or [email protected] or [email protected]
IBM Linux Technology Center, Beaverton, Oregon
Interested in Linux scalability ? Look at http://lse.sourceforge.net/
"Nobody ever said I was charming, they said "Rimmer, you're a git!"" RD VI
"J . A . Magallon" wrote:
>
> The same is with that ugly out: at the end
> of the function. Just change all that 'goto out' for a return.
Oh no, no, no. Please, no.
Multiple return statements are a maintenance nightmare.
Go back and look at the "checker" reports. Think about them.
-
On 03.24 Andrew Morton wrote:
> "J . A . Magallon" wrote:
> >
> > The same is with that ugly out: at the end
> > of the function. Just change all that 'goto out' for a return.
>
> Oh no, no, no. Please, no.
>
> Multiple return statements are a maintenance nightmare.
>
Well, I do not want this to restart a religion war.
The real thing is: gcc 3.0 (ISO C 99) does not like that practice
(let useless things there for someday using them ?). And there can be
other languaje issues also (I'm just thinkin of some issues with case and
no default:) And gcc-3 is what we will
have to live with. I suppose people will like to see a kernel build
without tons of wanings. They hide real errors.
I think its a good thing to decide what to do (and start doing), than wait
until gcc2.95 is buried.
--
J.A. Magallon # Let the source
mailto:[email protected] # be with you, Luke...
Linux werewolf 2.4.2-ac22 #3 SMP Fri Mar 23 02:06:00 CET 2001 i686
At 04:31 PM 3/23/01 -0800, you wrote:
>This has nothing to do with fastpathing and object code optimization. C
>doesn't have exception handling, so you either have to remember to undo
>allocations etc. in failure cases all through the code, or you stick your
>undo code at the end of the function and have all failure cases jump to the
>relevant label. It's not pretty, but it's much less error-prone e.g.
Really? I have a "cleanup" function that can be called during failure
cases (and success cases -- but you didn't mention that) so that the cost
is very low and I don't have to code ANY labels.
But then again, I'm a double-pipe abuser, in that I tend to code "atomic"
sequences as
if ((a) || (b) || (c) || (d) || (e) || (f) || (g) || ... ) { something
failed} else {it all worked!}
and make sure that the failure value is non-zero for each a, b, c, d, and
so forth.
I remember looking at the generated code from one compiler for x86 and
seeing a series of short jumps to short jumps to short jumps... to the
failure case, which in that particular sequence saved about 100 bytes. I
haven't looked at GCC output yet to see what it does, but working in a
32-bit system instead of a 16-bit system I tend to care a little less about
"efficiency".
Does that mean that I avoid "goto"? No. Like every other construct in the
C language, there is a valid and appropriate use for every single
thing. The key is recognizing when the goto is appropriate.
Another thing you will see in my code is resource pointers being
initialized to zero on entry, set to non-zero values as resources are
allocated, and then conditionally released based on whether the value is
zero or non-zero. It makes recovery from malloc failures easier, for one
thing.
Satch. the || Abuser.
On Fri, 23 Mar 2001 23:59:09 +0100, J . A . Magallon <[email protected]> wrote:
>
>
> On 03.23 Linus Torvalds wrote:
>>
>> I agree. I'd much prefer that syntax also.
>>
>> Or just remove the "default:" altogether, when it doesn't make any
>> difference.
>>
>
> Well, at last some sense. The same is with that ugly out: at the end
> of the function. Just change all that 'goto out' for a return.
No, no. Hell no. Multiple return paths in a function are a sure recipe
for errors creeping in later.
Just change the
out:;
into
out:
return;
and be done with it. Heck, it even looks like C code for a change. :-)
Ion
--
It is better to keep your mouth shut and be thought a fool,
than to open it and remove all doubt.
On Sat, Mar 24, 2001 at 01:55:15AM +0100, J . A . Magallon wrote:
>
> On 03.24 Andrew Morton wrote:
> > "J . A . Magallon" wrote:
> > >
> > > The same is with that ugly out: at the end
> > > of the function. Just change all that 'goto out' for a return.
> >
> > Oh no, no, no. Please, no.
> >
> > Multiple return statements are a maintenance nightmare.
> >
>
> Well, I do not want this to restart a religion war.
>
> The real thing is: gcc 3.0 (ISO C 99) does not like that practice
> (let useless things there for someday using them ?).
The GCC warning has nothing to do with the (good) practice of having a
single exit point. It is the difference between this:
...
out:
}
and this:
...
out:
return;
}
I think that the latter looks better, and the C standard says that
it's also the only one that's correct.
You are the one arguing about coding religion, by saying that
_neither_ of them is any good.
Tim.
*/
On Fri, Mar 23, 2001 at 05:16:26PM -0800, Stephen Satchell wrote:
[...]
> Really? I have a "cleanup" function that can be called during failure
> cases (and success cases -- but you didn't mention that) so that the cost
> is very low and I don't have to code ANY labels.
>
> But then again, I'm a double-pipe abuser, in that I tend to code "atomic"
> sequences as
>
> if ((a) || (b) || (c) || (d) || (e) || (f) || (g) || ... ) { something
> failed} else {it all worked!}
>
> and make sure that the failure value is non-zero for each a, b, c, d, and
> so forth.
>
Sorry, my example was too simplistic. Replace simple allocations with e.g.
allocate();
grab lock;
set flag;
allocate();
or something similar. Yes it's possible to code a state variable to remember
where you got to, or to e.g. add an extra boolean variable to indicate that
you grabbed the lock, but I'd argue that this obfuscates the code as well as
making it less efficient. It's no good looking to see if the lock has been
grabbed - if you failed at the first stage, it may still be locked by a
different CPU.
Tim
--
Tim Wright - [email protected] or [email protected] or [email protected]
IBM Linux Technology Center, Beaverton, Oregon
Interested in Linux scalability ? Look at http://lse.sourceforge.net/
"Nobody ever said I was charming, they said "Rimmer, you're a git!"" RD VI