Date: Mon, 23 Aug 2010 11:46:06 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dimitry Andric <dimitry@andric.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Rui Paulo <rpaulo@freebsd.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r211505 - head/contrib/gcc Message-ID: <20100823111937.D21446@delplex.bde.org> In-Reply-To: <4C6FDBCF.6070101@andric.com> References: <201008191259.o7JCxv3i004613@svn.freebsd.org> <20100820075236.L18914@delplex.bde.org> <4C6DC0F8.9040001@andric.com> <20100821054033.M19850@delplex.bde.org> <4C6FDBCF.6070101@andric.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 21 Aug 2010, Dimitry Andric wrote: > On 2010-08-20 22:36, Bruce Evans wrote: >> On Fri, 20 Aug 2010, Dimitry Andric wrote: > [...] >>> But will the casts not potentially hide problems, if you pass the wrong >>> types to those macros? Maybe it is better if the compiler complains >>> that some argument is of an incompatible type, than just forcing it to >>> cast? >> This is unclear. All integer types are compatible to some extent. >> Upcasting them always works and downcasting them works iff the value >> is not changed. Rui Paulo wrote in a another reply: > I think he meant that downcasting might not work. Yes, but note that for function calls with a prototype in scope, conversion always occurs and no dignostic is required for the dangerous downcasting (or sidecasting with a sign change?) cases. It is only with raw macros or raw asms that you even have a chance to see the original type in the callee. I guess my argument is essentially that since the original code wants to cast everything so as to get almost function call protocol for the macros, we shouldn't change this, except for the lvalues where using function calls would have made the bug obvious. > I meant this in the context of this llvm PR, about matching inline asm > input constraints with output constraints of an incompatible type: > > http://llvm.org/bugs/show_bug.cgi?id=3373 > > Clang is currently somewhat pickier about the arguments to inline asm, > which we also noticed in OpenSSL code, where a rotate-left macro is > defined (for i386 and amd64) as: > > # define ROTATE(a,n) ({ register unsigned int ret; \ > asm ( \ > "roll %1,%0" \ > : "=r"(ret) \ > : "I"(n), "0"(a) \ > : "cc"); \ > ret; \ > }) > > On amd64, it was being called with the 'a' argument being of unsigned > long type. Clang complained: > > crypto/openssl/crypto/md4/md4_dgst.c:117:2: > error: unsupported inline asm: input with type 'unsigned long' matching > output with type 'unsigned int' > R0(A,B,C,D,X( 0), 3,0); HOST_c2l(data,l); X( 2)=l; > ^~~~~~~~~~~~~~~~~~~~~~ > > In this case, the OpenSSL developers chose to explicitly cast 'a' to > 'unsigned int' (see <http://cvs.openssl.org/chngview?cn=19818>). Slightly wrong, since you want a macro named ROTATE() to be type-generic, but this one just doesn't work for 64-bit unsigned longs (and without the cast it may be be broken for [u_]chars and [u_]shorts) (and with the cast it is probably broken for signed chars and signed shorts with a negative value). But the authors may know that they only use it on 32-bit values. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100823111937.D21446>