From owner-svn-src-all@freebsd.org Sat Feb 25 03:17:32 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E0831CEB815; Sat, 25 Feb 2017 03:17:32 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 911B61D62; Sat, 25 Feb 2017 03:17:32 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 63B3ED4DFD4; Sat, 25 Feb 2017 14:17:23 +1100 (AEDT) Date: Sat, 25 Feb 2017 14:17:23 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Bruce Evans , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314087 - head/sys/x86/x86 In-Reply-To: <20170224125335.GV2092@kib.kiev.ua> Message-ID: <20170225130549.C1026@besplex.bde.org> References: <201702220707.v1M7764i020598@repo.freebsd.org> <20170223053954.J1044@besplex.bde.org> <20170224125335.GV2092@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=KeqiiUQD c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=Wprkr_MR3Ar5J1-hRocA:9 a=yrhAz4GM2nK4HfPb:21 a=No_fGCopjTXwDP6x:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Feb 2017 03:17:33 -0000 On Fri, 24 Feb 2017, Konstantin Belousov wrote: > On Thu, Feb 23, 2017 at 06:33:43AM +1100, Bruce Evans wrote: >> On Wed, 22 Feb 2017, Konstantin Belousov wrote: >> >>> Log: >>> More fixes for regression in r313898 on i386. >>> Use long long constants where needed. >> >> The long long abomination is never needed, and is always a style bug. > I never saw any explanation behind this claim. Esp. the first part > of it, WRT 'never needed'. I hope I wrote enough about this in log messages when I cleaned up the long longs 20 years ago :-). long long was a hack to work around intmax_t not existing and long being unexpandable in practice because it was used in ABIs. It should have gone away when intmax_t was standardized. Unfortunately, long long was standardised too. It is "never needed" since anything that can be done with it can be done better using intmax_t or intN_t or int_fastN_T or int_leastN_t. Except, there is no suffix for explicit intmax_t constants, so you would have to write such constants using INTMAX_C() or better a cast to intmax_t if the constant is not needed in a cpp expression. >> I don't like using explicit long constants either. Here the number of bits >> in the register is fixed by the hardware at 64. The number of bits in a >> long on amd64 and a long on i386 is only fixed by ABI because the ABI is >> broken for historical reasons. > I really cannot make any sense of this statement. To know that the ULL suffix is correct for 64-bit types, you have to now that long longs are 64 bits on all arches supported by the code. Then to use this suffix, you have to hard-code this knowledge. Then to read the code, the reader has to translate back to 64 bits. The translations were easier 1 arch at a time. Casting to uint64_t is clearer, but doesn't work in cpp expressions. In cpp expressions, use UINT64_C(). Almost no one knows about it uses this. There are 5 examples of using it in /sys (3 in arm64 pte.h, 1 in powerpc pte.h, and 1 in mips xlr_machdep.c, where the use is unnecessary but interesting: it is ~UINT64_C(0). We used to have squillions of magic ~0's for the rman max limit. This was spelled ~0U, ~0UL and perhaps even plain ~0. Plain ~0 worked best except on unsupported 1's complement machines, since it normally gets sign extended to as many bits as necessary. Now this is spelled RM_MAX_END, which is implemented non-magically using a cast: (~(rman_res_t)0). Grepping for ~0[uU] in dev/* shows only 1 obvious unconverted place. >> Only very MD code can safely assume the >> size of long and long long. This code was MD enough before it was merged, >> but now it shouldn't use long since that varies between amd64 and i386, >> and it shouldn't use long long since that is a style bug. > > Well, I do not see anything wrong with long long, at least until > explained. > > Anyway, below is the patch to use uint64_t cast in important place, > and removal of LL suffix in unimportant expression. This is correct. > diff --git a/sys/x86/x86/x86_mem.c b/sys/x86/x86/x86_mem.c > index d639224f840..8bc4d3917a0 100644 > --- a/sys/x86/x86/x86_mem.c > +++ b/sys/x86/x86/x86_mem.c > @@ -260,7 +260,7 @@ x86_mrfetch(struct mem_range_softc *sc) > > /* Compute the range from the mask. Ick. */ > mrd->mr_len = (~(msrv & mtrr_physmask) & > - (mtrr_physmask | 0xfffLL)) + 1; > + (mtrr_physmask | 0xfff)) + 1; > if (!mrvalid(mrd->mr_base, mrd->mr_len)) > mrd->mr_flags |= MDF_BOGUS; > > @@ -638,7 +638,8 @@ x86_mrinit(struct mem_range_softc *sc) > * Determine the size of the PhysMask and PhysBase fields in > * the variable range MTRRs. > */ > - mtrr_physmask = (((uint64_t)1 << cpu_maxphyaddr) - 1) & ~0xfffULL; > + mtrr_physmask = (((uint64_t)1 << cpu_maxphyaddr) - 1) & > + ~(uint64_t)0xfff; > > /* If fixed MTRRs supported and enabled. */ > if ((mtrrcap & MTRR_CAP_FIXED) && (mtrrdef & MTRR_DEF_FIXED_ENABLE)) { Now I wonder what these magic 0xfff's are. Are they PAGE_MASK, where the register is encoded like a page table to put metadata in the low bits? There is already a macro MTRR_PHYSBASE_PHYSMASK which looks like it should be used here, except it is zero in the top 12 bits too. There is no macro for 0xfff, but you get that by ORing the bits for other macros. 0xfff is just as readable. The MTRR_* macros are in x86/specialreg.h, and are spelled without ULL suffixes. I prefer the latter, but seem to rememeber bugs elsewhere caused by using expressions like ~FOO where FOO happens to be small. Actually the problems are mostly when FOO happens to be between INT_MAX+1U and UINT_MAX. When FOO is small and has no suffix, e.g., if it is 0, then its type is int and ~FOO has type int and sign-extends to 64 bits if necessary. But if FOO is say 0x80000000, it has type u_int so ~FOO doesn't sign-extend. (Decimal constants without a suffix never have an unsigned type and the hex constant here lets me write this number and automatically give it an unsigned type. Normally this type is best.) Explicit type suffixes mainly hide these problems. If FOO is 0x80000000ULL, then it has the correct type for ~FOO to work in expressions where everything has type unsigned long long, but in other expressions a cast might still be needed. Bruce