Re: controversial fix or some errors breaking LINT
On Wed, 27 Feb 2002, Matthew Dillon wrote: :/* : * Note: the volatile below does not REQUIRE that the argument be : * volatile, but rather ony says that it is OK to use a volatile * i : * there. Same for the const. I know a const volatile sounds strange : * but it only indicates that either is acceptable. : */ :voidbcopy __P((volatile const void *from, volatile void *to, size_t :len)); I really don't like this. I mean, it will work but I really don't like this. Volatile in C will effectively prevent optimization of read and write accesses to the data the pointer is pointing at. Of course, this is bcopy(), which we implement in assembly, so again it will work for bcopy(). But it sets a very bad coding precedent. Volatile semantics also prevents optimizations in the assembly versions. i586_bcopy already does extensive optimizations involving out of order loads to prefetch cache lines. i586_bzero does even more volatile- unfriendly things for (dubious) alignment tricks. Even overlapping bcopies break volatile semantics (by copying backwards in some cases). Similarly for memcpy. bcopy and memcpy in the kernel should have the same interface as in userland. I think the correct fix is to always use bus space for hardware accesses, and only use volatile elements or whole volatile structs for non-hardware variables very rarely. The bus space functions avoid optimizations that are only valid for ordinary memory just by not using optimizations that are not usefule for device memory. Volatile variables don't work so well for SMP systems since you often need to lock them anyway and the lock makes them nonvolatile. BTW, C's volatile semantics do not protect you from invalidly bcopy'ing a non-volatile struct with volatile elements. C just looks at the qualifiers on the pointer to the struct and doesn't look inside the struct. Bcopy'ing such a struct gives undefined behaviour, so the compiler is permitted to detect this error. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
On Wed, 27 Feb 2002, Julian Elischer wrote: ok so I leave it to other people to fix LINT I'm not going near it any more one small example: ../../../dev/ie/if_ie.c:1471: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1480: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1483: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1508: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type it's so obviously the right way that I'm amazed that there can even be discussion. (except that I know Bruce didn't lik eit before) (he and jhb just actualy created more errors in LINT by removing a volatile on the definition of bzero in the kernel.) (the above fatal errors are new and brought to you by the people who just did that removal) We just unshot one of the messengers. Breaking bzero for the i386 case didn't affect other arches. remember folks.. this is the kernel here.. what Posix says doesn't count INSIDE the kernel. Actually, it does for bcopy. We use the same interface for all the string functions kernel so that experience with it them userland can be reused and vice versa. We deal wit devices inside the kernel. we have a LOT of volatile stuff. It may be worth creating a vbcopy and vbzero to handle volatile stuff then. (But I think it's a stupid thing to need to do) the bus-space mess should have stuff to do this but no-one who understands it has had teh energy to actually fix this stuff with it. so we are stuck with a broken LINT. Most drivers aren't affected by this. This is because: - most of them already use bus-space. - most of the drivers that don't use bus-space only support PIO devices, so they don't need volatile qualifiers for device memory. - most of the others (if there are any left other than if_le) don't use volatiles carefully to a fault like if_ie. Bruce To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
controversial fix or some errors breaking LINT
There are saveral places (e.g. if_ie.c) where data is copied out of a buffer that is shared with the hardware. The pointer to this is correctly labelled as volatile, though at the time we will copy the data out we know it to be stable. the problem is that it uses bcopy() to do this, and that generates teh error message ../../../dev/ie/if_ie.c:1232: warning: passing arg 1 of `bcopy' discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1232: warning: passing arg 2 of `bcopy' discards qualifiers from pointer target type which is now fatal. Now since it is also possible to copy data INTO a volatile shared buffer it seems logical to allow bcopy to do so. A while ago I proposed the following patch: /* * Note: the volatile below does not REQUIRE that the argument be * volatile, but rather ony says that it is OK to use a volatile * i * there. Same for the const. I know a const volatile sounds strange * but it only indicates that either is acceptable. */ voidbcopy __P((volatile const void *from, volatile void *to, size_t len)); NOW we can get rid of lots of UGLY casting tricks here and there that have been used to try UNVOLATILE things so thay they can use bcopy. I suggest a similar addition to a few other standard operations. Hopefully the BUS-space stuff should be used eventually but I'm not going to rewrite if_ie.c. are you? (I've also seem this used in the IPV6 code but I think thye've been (yukky) cast'ed by now. julian To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
:/* : * Note: the volatile below does not REQUIRE that the argument be : * volatile, but rather ony says that it is OK to use a volatile * i : * there. Same for the const. I know a const volatile sounds strange : * but it only indicates that either is acceptable. : */ :voidbcopy __P((volatile const void *from, volatile void *to, size_t :len)); : :NOW we can get rid of lots of UGLY casting tricks here and there that :have been used to try UNVOLATILE things so thay they can use bcopy. :I suggest a similar addition to a few other standard operations. : :Hopefully the BUS-space stuff should be used eventually :but I'm not going to rewrite if_ie.c. are you? :(I've also seem this used in the IPV6 code but I think :thye've been (yukky) cast'ed by now. : :julian I really don't like this. I mean, it will work but I really don't like this. Volatile in C will effectively prevent optimization of read and write accesses to the data the pointer is pointing at. Of course, this is bcopy(), which we implement in assembly, so again it will work for bcopy(). But it sets a very bad coding precedent. I would keep the casts. It reminds us that there is something funny going on. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
ok so I leave it to other people to fix LINT I'm not going near it any more one small example: ../../../dev/ie/if_ie.c:1471: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1480: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1483: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type ../../../dev/ie/if_ie.c:1508: warning: passing arg 1 of pointer to function discards qualifiers from pointer target type it's so obviously the right way that I'm amazed that there can even be discussion. (except that I know Bruce didn't lik eit before) (he and jhb just actualy created more errors in LINT by removing a volatile on the definition of bzero in the kernel.) (the above fatal errors are new and brought to you by the people who just did that removal) remember folks.. this is the kernel here.. what Posix says doesn't count INSIDE the kernel. We deal wit devices inside the kernel. we have a LOT of volatile stuff. It may be worth creating a vbcopy and vbzero to handle volatile stuff then. (But I think it's a stupid thing to need to do) the bus-space mess should have stuff to do this but no-one who understands it has had teh energy to actually fix this stuff with it. so we are stuck with a broken LINT. On Wed, 27 Feb 2002, Matthew Dillon wrote: :/* : * Note: the volatile below does not REQUIRE that the argument be : * volatile, but rather ony says that it is OK to use a volatile * i : * there. Same for the const. I know a const volatile sounds strange : * but it only indicates that either is acceptable. : */ :voidbcopy __P((volatile const void *from, volatile void *to, size_t :len)); : :NOW we can get rid of lots of UGLY casting tricks here and there that :have been used to try UNVOLATILE things so thay they can use bcopy. :I suggest a similar addition to a few other standard operations. : :Hopefully the BUS-space stuff should be used eventually :but I'm not going to rewrite if_ie.c. are you? :(I've also seem this used in the IPV6 code but I think :thye've been (yukky) cast'ed by now. : :julian I really don't like this. I mean, it will work but I really don't like this. Volatile in C will effectively prevent optimization of read and write accesses to the data the pointer is pointing at. Of course, this is bcopy(), which we implement in assembly, so again it will work for bcopy(). But it sets a very bad coding precedent. I would keep the casts. It reminds us that there is something funny going on. -Matt Matthew Dillon [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
: :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. -Matt To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
* Matthew Dillon [EMAIL PROTECTED] [020227 14:51] wrote: : :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. No. Leave it in, this will benifit us all in the long run. In fact it was the _only_ way I was able to get people clean up bad code at a former job and I strongly support keeping -Weerror enabled. -Alfred To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
No. Leave it in, this will benifit us all in the long run. Until we start hitting the broken/buggy warnings, which will cause people to write more obfuscated or harder to maintain code in order to avoid the warnings. Bill To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
At 1:27 PM -0800 2/27/02, Julian Elischer wrote: There are saveral places (e.g. if_ie.c) where data is copied out of a buffer that is shared with the hardware. The pointer to this is correctly labelled as volatile, though at the time we will copy the data out we know it to be stable. Note: at the time we will copy the data ... we know A while ago I proposed the following patch: /* * Note: the volatile below does not REQUIRE that the argument be * volatile, but rather ony says that it is OK to use a volatile * i * there. Same for the const. I know a const volatile sounds strange * but it only indicates that either is acceptable. */ voidbcopy __P((volatile const void *from, volatile void *to, size_t len)); This will always allow bcopy to do the copy to or from any volatile location, even if the call is done at a bad time. Any programmer calling bcopy should at least get a little flag waved at them if they are working with volatile arguments. How philosophically sickening would it be to create a macro: #define bcopy_volatile(x,y) bcopy((casts)x,(casts)y) so that you can have cleaner-looking source code, but still have it so the programmer has to *explicitly* say Yes, I know I am dealing with volatile memory here. -- Garance Alistair Drosehn= [EMAIL PROTECTED] Senior Systems Programmer or [EMAIL PROTECTED] Rensselaer Polytechnic Instituteor [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
Alfred Perlstein wrote: * Matthew Dillon [EMAIL PROTECTED] [020227 14:51] wrote: : :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. No. Leave it in, this will benifit us all in the long run. In fact it was the _only_ way I was able to get people clean up bad code at a former job and I strongly support keeping -Weerror enabled. If there are files that are too hard to fix, or vendor files, or the fix isn't clear, we should use the nowerror conf/files* flags. It is important that we stop new warnings turning up when the compile output is so damn large that it hides things. I will do a pass over things now and see what I can do. Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] All of this is for nothing if we don't go to the stars - JMS/B5 To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
On Wed, Feb 27, 2002 at 03:15:09PM -0800, Peter Wemm wrote: If there are files that are too hard to fix, or vendor files, or the fix isn't clear, we should use the nowerror conf/files* flags. It is important that we stop new warnings turning up when the compile output is so damn large that it hides things. I definatly agree. The warnings in gif(4) were mostly lame and should have been fixed, but no one bothered. I will do a pass over things now and see what I can do. Please make sure to test the !SMP case. The following slipped through the initial sweep: Index: kern_sig.c === RCS file: /usr/cvs/src/sys/kern/kern_sig.c,v retrieving revision 1.149 diff -u -r1.149 kern_sig.c --- kern_sig.c 23 Feb 2002 11:12:54 - 1.149 +++ kern_sig.c 26 Feb 2002 19:37:00 - @@ -1233,7 +1233,9 @@ register int prop; register sig_t action; struct thread *td; +#ifdef SMP struct ksegrp *kg; +#endif KASSERT(_SIG_VALID(sig), (psignal(): invalid signal %d\n, sig)); -- Brooks -- Any statement of the form X is the one, true Y is FALSE. PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4 msg35466/pgp0.pgp Description: PGP signature
Re: controversial fix or some errors breaking LINT
It's not my problem as jhb has volunteered to fix all these. :-) we just add an entry point for bcopy_volatile() next to that for bcopy. (I called it vbcopy in my suggestion.) bzero has the same thing bzero_volatile() I guess. Though I still feel that it breaks POLA to not be able to use bcopy and bzero. On Wed, 27 Feb 2002, Garance A Drosihn wrote: At 1:27 PM -0800 2/27/02, Julian Elischer wrote: There are saveral places (e.g. if_ie.c) where data is copied out of a buffer that is shared with the hardware. The pointer to this is correctly labelled as volatile, though at the time we will copy the data out we know it to be stable. Note: at the time we will copy the data ... we know A while ago I proposed the following patch: /* * Note: the volatile below does not REQUIRE that the argument be * volatile, but rather ony says that it is OK to use a volatile * i * there. Same for the const. I know a const volatile sounds strange * but it only indicates that either is acceptable. */ voidbcopy __P((volatile const void *from, volatile void *to, size_t len)); This will always allow bcopy to do the copy to or from any volatile location, even if the call is done at a bad time. Any programmer calling bcopy should at least get a little flag waved at them if they are working with volatile arguments. How philosophically sickening would it be to create a macro: #define bcopy_volatile(x,y) bcopy((casts)x,(casts)y) so that you can have cleaner-looking source code, but still have it so the programmer has to *explicitly* say Yes, I know I am dealing with volatile memory here. -- Garance Alistair Drosehn= [EMAIL PROTECTED] Senior Systems Programmer or [EMAIL PROTECTED] Rensselaer Polytechnic Instituteor [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
I presume then that you also feel that allowing bcopy to copy volatile regions is a bad idea? On Wed, 27 Feb 2002, Peter Wemm wrote: Alfred Perlstein wrote: * Matthew Dillon [EMAIL PROTECTED] [020227 14:51] wrote: : :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. No. Leave it in, this will benifit us all in the long run. In fact it was the _only_ way I was able to get people clean up bad code at a former job and I strongly support keeping -Weerror enabled. If there are files that are too hard to fix, or vendor files, or the fix isn't clear, we should use the nowerror conf/files* flags. It is important that we stop new warnings turning up when the compile output is so damn large that it hides things. I will do a pass over things now and see what I can do. Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] All of this is for nothing if we don't go to the stars - JMS/B5 To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
In message p05101410b8a31356bb54@[128.113.24.47] Garance A Drosihn writes: : How philosophically sickening would it be to create a macro: : : #define bcopy_volatile(x,y) bcopy((casts)x,(casts)y) How about just using bus_space in these drivers? That's the right solution and isn't too hard to do. I'm working on the wl driver right now. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
sure.. does bus-space have a bzero? On Wed, 27 Feb 2002, Warner Losh wrote: In message p05101410b8a31356bb54@[128.113.24.47] Garance A Drosihn writes: : How philosophically sickening would it be to create a macro: : : #define bcopy_volatile(x,y) bcopy((casts)x,(casts)y) How about just using bus_space in these drivers? That's the right solution and isn't too hard to do. I'm working on the wl driver right now. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
In message: [EMAIL PROTECTED] Julian Elischer [EMAIL PROTECTED] writes: : does bus-space have a bzero? Effectively yes. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
Peter Wemm wrote: Alfred Perlstein wrote: * Matthew Dillon [EMAIL PROTECTED] [020227 14:51] wrote: : :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. No. Leave it in, this will benifit us all in the long run. In fact it was the _only_ way I was able to get people clean up bad code at a former job and I strongly support keeping -Weerror enabled. If there are files that are too hard to fix, or vendor files, or the fix isn't clear, we should use the nowerror conf/files* flags. It is important that we stop new warnings turning up when the compile output is so damn large that it hides things. I will do a pass over things now and see what I can do. LINT now compiles, FWIW. Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] All of this is for nothing if we don't go to the stars - JMS/B5 To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
Re: controversial fix or some errors breaking LINT
* Peter Wemm [EMAIL PROTECTED] [020227 15:44] wrote: Alfred Perlstein wrote: * Matthew Dillon [EMAIL PROTECTED] [020227 14:51] wrote: : :ok so I leave it to other people to fix LINT :I'm not going near it any more It's the responsibility of whoever added -Werror to the default compile to unbreak the tree, either by fixing the problem or by backing out his commit. No. Leave it in, this will benifit us all in the long run. In fact it was the _only_ way I was able to get people clean up bad code at a former job and I strongly support keeping -Weerror enabled. If there are files that are too hard to fix, or vendor files, or the fix isn't clear, we should use the nowerror conf/files* flags. It is important that we stop new warnings turning up when the compile output is so damn large that it hides things. I will do a pass over things now and see what I can do. Agreed, any doofus that obfuscates code to mask a warning gets a kick in the pants at the next BSDcon. If you don't know then ask. (I know I'll be asking Bruce/Peter if I have a problem) -- -Alfred Perlstein [[EMAIL PROTECTED]] 'Instead of asking why a piece of software is using 1970s technology, start asking why software is ignoring 30 years of accumulated wisdom.' Tax deductible donations for FreeBSD: http://www.freebsdfoundation.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with unsubscribe freebsd-current in the body of the message
