Date: Sat, 25 Feb 2017 12:15:43 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314087 - head/sys/x86/x86 Message-ID: <20170225101543.GC2092@kib.kiev.ua> In-Reply-To: <20170225130549.C1026@besplex.bde.org> References: <201702220707.v1M7764i020598@repo.freebsd.org> <20170223053954.J1044@besplex.bde.org> <20170224125335.GV2092@kib.kiev.ua> <20170225130549.C1026@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Feb 25, 2017 at 02:17:23PM +1100, Bruce Evans wrote:
> 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 does not make a sense even more. long long is native compiler type,
while anything_t is a typename to provide MI fixed type. long long was
obviosvly choosen to extend types without requiring new keyword.
>
> 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.
If you replace long long with int there, the same logical structure of
sentences will hold. Does it mean that 'int' is abomination since we
have int32_t which allows everything to be done better ?
>
> >> 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.
And why it is bad ? Same is true for int and long, which are often used
in MD code to provide specific word size. In fact, there is not much for
a programmer to know: we and any other UNIX supports either ILP32 or LP64
for the given architecture.
> 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.
This clearly demonstrates why ULL/UL notation is superior to UINT64_C() or
any other obfuscation.
>
> >> 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?
Many Intel registers have a structure where bits N:12 denote a physical
address aligned at page boundary, and bits 11:0 are used for attributes
not directly related to addresses. We do not use PAGE_MASK there
traditionally, PAGE_MASK is only used to mask the page offset in actual
virtual address.
>
> 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.
Yes, yet another (and most useful) reason to use ULL and ignore a FUD about it.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170225101543.GC2092>