Date: Fri, 06 Jan 2012 20:59:23 +0100 From: Andreas Tobler <andreast@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r229693 - in head/lib/libc: powerpc powerpc64 Message-ID: <4F07529B.2060608@FreeBSD.org> In-Reply-To: <20120106225728.G9027@besplex.bde.org> References: <201201060921.q069Lfi8081051@svn.freebsd.org> <20120106225728.G9027@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Bruce, thank you for the feedback. I wasn't aware about the fine details. I try to understand and implement what you suggest. It may take several iterations until I match everything. On 06.01.12 14:12, Bruce Evans wrote: > On Fri, 6 Jan 2012, Andreas Tobler wrote: > >> Log: >> Use the macro WEAK_ALIAS. Tested on 32 and 64-bit. > > This API should be fixed before using it extensively. It has the args > reversed relative to the C API __weak_reference(). Also, its name is > different, and the spelling of "=" in its implementation is different. > Perhaps the arg order makes sense for both, since for WEAK_ALIAS() the > alias is the first arg, while for __weak_reference() the symbol being > referred to to create the alias is the first arg. But this is still > confusing. > > The easiest way to fix this is to remove WEAK_ALIAS() and add an asm > API WEAK_REFERENCE(). Unfortunately, ALIAS is a better name than > REFERENCE. __weak_reference() is not so easy to change since it is > used extensively So, I started with a WEAK_REFERENCE macro in sys/powerpc/asm.h It is like the WEAK_ALIAS but with reversed arguments: +#define WEAK_REFERENCE(sym, alias) \ + .weak alias; \ + alias = sym + Here I do not have a preference for the "=" implementation, is it "=" or is it .set ..... If we find a final version I'll be able to delete the WEAK_ALIAS. > Similarly for STRONG_ALIAS() and __strong_reference(), except > STRONG_REFERENCE() doesn't exist for most arches and __strong_reference() > didn't exist until this week, so neither is used extensively. > > More details on current existence and use of these: > > In the kernel, WEAK_ALIAS is not defined for amd64 or sparc64, and is > never used. > > In libc, WEAK_ALIAS was only used for arm, ia64 and mips. Now it is used > for some i386 string functions. > > In the kernel, STRONG_ALIAS was only defined for mips, and was never used. > Now it is also defined for i386, and is never used. > > In libc, STRONG_ALIAS was not used. It was used for a few days in some > i386 string functions. This was a bug. Now WEAK_ALIAS is used instead, > and STRONG_ALIAS is not used again. It is another bug that these > "optimized" i386 string functions (strchr/index and strrchr/rindex) even > exist. amd64 doesn't have them. The MI versions are not very optimal, > but neither are the i386 ones. > >> Modified: head/lib/libc/powerpc/SYS.h >> ============================================================================== >> --- head/lib/libc/powerpc/SYS.h Fri Jan 6 09:17:34 2012 (r229692) >> +++ head/lib/libc/powerpc/SYS.h Fri Jan 6 09:21:40 2012 (r229693) >> @@ -44,10 +44,8 @@ >> .align 2; \ >> 2: b PIC_PLT(CNAME(HIDENAME(cerror))); \ >> ENTRY(__CONCAT(__sys_,x)); \ >> - .weak CNAME(x); \ >> - .set CNAME(x),CNAME(__CONCAT(__sys_,x)); \ >> - .weak CNAME(__CONCAT(_,x)); \ >> - .set CNAME(__CONCAT(_,x)),CNAME(__CONCAT(__sys_,x)); \ >> + WEAK_ALIAS(x,__CONCAT(__sys_,x)); \ >> + WEAK_ALIAS(__CONCAT(_,x),__CONCAT(__sys_,x)); \ >> _SYSCALL(x); \ >> bso 2b >> > > The style bugs in this should be fixed someday. This is still messed up > to support K&R. With ancient cpp's, you had to write __CONCAT(x,y) instead > of __CONCAT(x, y) to avoid getting a space between x and y. This was fixed > in Standard C 22 years ago, but all SYS.h files in libc except ia64's one > still use the ugly __CONCAT(x,y) in most places. ia64 hard-codes > __CONCAT(x, y) as x ## y instead. > > The missing space after the comma for the WEAK_ALIAS() parameters is even > less necessary. For ancient cpp's, it allowed WEAK_ALIAS to format the > asm directives without a space. With STDC cpp's, the formatting is > controlled by the macro, but it is still hard to produce nice formatting > because cpp may change whitespace. If I get the above right, the snippet from above should look like this, right? @@ -51,20 +51,17 @@ ld %r0,16(%r1); \ mtlr %r0; \ blr; \ -ENTRY(__CONCAT(__sys_,x)); \ - .weak CNAME(x); \ - .set CNAME(x),CNAME(__CONCAT(__sys_,x)); \ - .weak CNAME(__CONCAT(_,x)); \ - .set CNAME(__CONCAT(_,x)),CNAME(__CONCAT(__sys_,x)); \ +ENTRY(__CONCAT(__sys_, x)); \ + WEAK_REFERENCE(__CONCAT(__sys_, x), x); \ + WEAK_REFERENCE(__CONCAT(__sys_, x), __CONCAT(_, x)); \ _SYSCALL(x); \ bso 2b > > __weak_reference() also difference from WEAK_ALIAS() in the spelling > of "=". It uses ".equ". ".set" in the all the SYS.h's except ia64 and > mips (*) seems to be yet another spelling. > > (*) ia64 avoids the hard coded directive using WEAK_ALIAS(). mips > hard-codes "=" instead of ".set". > > Some other gratuitous differences in the SYS.h's: > - arm and mips use _C_LABEL() instead of CNAME() > - ia64 doesn't use either _C_LABEL() or CNAME() > - arm, ia64 and mips don't use HIDENAME() for cerror: > - arm uses CERROR and hard-codes this as _C_LABEL(cerror) > - ia64 hard-codes it as .cerror > - mips hard-codes it as __cerror. > - sparc64 doesn't use ENTRY() or END() and has to repeat things like .type > and .size found in those when it defines _SYSENTRY() and _SYSEND(). > It doesn't use CNAME() in these definitions. Underscores in these > names are bogus, since these names are even less public than ENTRY() > and END() which don't have them. > > Some of these differences may be due to FreeBSD's cleaning up or down > of the decomposition of SYS.h and<machine/asm.h> only for older arches. > Both of these files are MD so you can hard-code things in either, but > I think it is best put as much of the details as possible in the lowest > level, which is<machine.asm.h>. Using WEAK_ALIAS() from there is a > step in this direction. If we want to stay with WEAK_ALIAS I can at least fix the style bugs. Thanks, Andreas
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4F07529B.2060608>