From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9762 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix atomic_arch.h for MIPS32 R6 Date: Mon, 28 Mar 2016 22:19:27 -0400 Message-ID: <20160329021927.GK21636@brightrain.aerifal.cx> References: <20160321173754.GC21636@brightrain.aerifal.cx> <20160322212211.GG21636@brightrain.aerifal.cx> <20160323150302.GK21636@brightrain.aerifal.cx> <20160328130451.GH21636@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="wzJLGUyc3ArbnUjN" X-Trace: ger.gmane.org 1459217988 32279 80.91.229.3 (29 Mar 2016 02:19:48 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Tue, 29 Mar 2016 02:19:48 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9775-gllmg-musl=m.gmane.org@lists.openwall.com Tue Mar 29 04:19:47 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1akjFx-0007ea-Hu for gllmg-musl@m.gmane.org; Tue, 29 Mar 2016 04:19:45 +0200 Original-Received: (qmail 22384 invoked by uid 550); 29 Mar 2016 02:19:42 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 22363 invoked from network); 29 Mar 2016 02:19:41 -0000 Content-Disposition: inline In-Reply-To: <20160328130451.GH21636@brightrain.aerifal.cx> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9762 Archived-At: --wzJLGUyc3ArbnUjN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Mar 28, 2016 at 09:04:51AM -0400, Rich Felker wrote: > On Mon, Mar 28, 2016 at 05:07:39AM +0000, Jaydeep Patil wrote: > > >> >I was just saying it makes the code less cluttered to use them > > >> >spuriously even though we don't need to: > > >> > > > >> > ".set push ; " > > >> >#if __mips_isa_rev < 6 > > >> > ".set mips2 ; " > > >> >#endif > > >> > "ll %0, %1 ; .set pop" > > >> > > > >> >or similar. > > >> > > > >> >It's also not clear to me whether the "m" constraint is valid anymore > > >> >for the R6 ll/sc instructions since they take a 9-bit offset now instead of a > > >16-bit offset. > > >> >The compiler could generate an address expression whose offset part > > >> >does not fit in 9 bits. In that case we may need to #if the whole > > >> >function (or at least the __asm__ statement) separately rather than just > > >skipping the .set mips2.... > > >> > > > >> > > >> The "m" constrain is still valid here, as the offset will be 0 in this case.. > > > > > >How can you assume the offset will be 0? It's the compiler's choice what to > > >use. For instance, a_cas(&foo->bar, t, s) is likely to have an offset equal to > > >offsetof(__typeof__(foo),bar). AFAIK this happens in practice with small > > >offsets in mutex structures, etc. so the bug may be unlikely to be hit, but I > > >think it's still an incorrect-constraint bug. > > > > Compiler generates appropriate LL/SC based on the offset. > > Compiler adds the offset to the base register if it does not fit 9bits. > > The compiler has no way of knowing that the operand will be used with > ll with the 9-bit offset restriction; as far as it knows, it will be > used in a normal context where a 16-bit offset is valid. I don't have > a toolchain that will target r6, but you can try the following program > which produces an offset of 4096 for loading p[1024]: > > unsigned ll1k(volatile unsigned *p) > { > unsigned val; > __asm__ __volatile__ ("ll %0, %1" : "=r"(val) : "m"(p[1024]) : "memory" ); > return val; > } > > I would expect this to produce errors at assembly time on r6. Indeed, the ZC constraint seems to have been added to address this; see: https://gcc.gnu.org/onlinedocs/gcc/Machine-Constraints.html It's not available on old gcc versions (pre-mipsr6) so I think the attached patch would be appropriate. Let me know if it works for you. We still need to add r6 detection in configure and the dynamic linker name in reloc.h, and fix the pthread_arch.h issue. Rich --wzJLGUyc3ArbnUjN Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="mipsr6-atomic.diff" diff --git a/arch/mips/atomic_arch.h b/arch/mips/atomic_arch.h index ce2823b..ad534f2 100644 --- a/arch/mips/atomic_arch.h +++ b/arch/mips/atomic_arch.h @@ -2,11 +2,17 @@ static inline int a_ll(volatile int *p) { int v; +#if __mips_isa_rev < 6 __asm__ __volatile__ ( ".set push ; .set mips2\n\t" "ll %0, %1" "\n\t.set pop" : "=r"(v) : "m"(*p)); +#else + __asm__ __volatile__ ( + "ll %0, %1" + : "=r"(v) : "ZC"(*p)); +#endif return v; } @@ -14,24 +20,29 @@ static inline int a_ll(volatile int *p) static inline int a_sc(volatile int *p, int v) { int r; +#if __mips_isa_rev < 6 __asm__ __volatile__ ( ".set push ; .set mips2\n\t" "sc %0, %1" "\n\t.set pop" : "=r"(r), "=m"(*p) : "0"(v) : "memory"); +#else + __asm__ __volatile__ ( + "sc %0, %1" + : "=r"(r), "=ZC"(*p) : "0"(v) : "memory"); +#endif return r; } #define a_barrier a_barrier static inline void a_barrier() { +#if __mips_isa_rev < 2 /* mips2 sync, but using too many directives causes * gcc not to inline it, so encode with .long instead. */ __asm__ __volatile__ (".long 0xf" : : : "memory"); -#if 0 - __asm__ __volatile__ ( - ".set push ; .set mips2 ; sync ; .set pop" - : : : "memory"); +#else + __asm__ __volatile__ ("sync" : : : "memory"); #endif } --wzJLGUyc3ArbnUjN--