Date: Sat, 02 Jan 2016 17:28:54 -0700 From: Ian Lepore <ian@freebsd.org> 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: r293064 - head/sys/boot/uboot/lib Message-ID: <1451780934.1369.129.camel@freebsd.org> In-Reply-To: <20160103101219.C1033@besplex.bde.org> References: <201601022255.u02Mtxe4043921@repo.freebsd.org> <20160103101219.C1033@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 2016-01-03 at 10:59 +1100, Bruce Evans wrote:
> On Sat, 2 Jan 2016, Ian Lepore wrote:
>
> > Log:
> > Cast pointer through uintptr_t on the way to uint64_t to squelch a warning.
> >
> > Modified: head/sys/boot/uboot/lib/copy.c
> > ==============================================================================
> > --- head/sys/boot/uboot/lib/copy.c Sat Jan 2 22:31:14 2016 (r293063)
> > +++ head/sys/boot/uboot/lib/copy.c Sat Jan 2 22:55:59 2016 (r293064)
> > @@ -100,7 +100,7 @@ uboot_loadaddr(u_int type, void *data, u
> >
> > biggest_block = 0;
> > biggest_size = 0;
> > - subldr = rounddown2((uint64_t)_start, KERN_ALIGN);
> > + subldr = rounddown2((uint64_t)(uintptr_t)_start, KERN_ALIGN);
> > eubldr = roundup2((uint64_t)uboot_heap_end, KERN_ALIGN);
> > for (i = 0; i < si->mr_no; i++) {
> > if (si->mr[i].flags != MR_ATTR_DRAM)
>
> This gives 1 more bogus cast here:
> - _start is a function pointer, so it should be cast to uintfptr_t
> - rounddown2() acts on any integer and subldr is an integer, and both of the
> integers are unsigned, and KERN_ALIGN is a small signed int, and so there
> is no need for any further cast, except possibily on arches with
> uintfptr_t larger than uint64_t or the type of subldr where the compiler
> warns about downwards cast, but then the downcast may be a bug since it
> just breaks the warning
>
> - if there is a need for a further cast, then it should be of rounddown2(),
> not of one of its args, so that the args and the internals of rounddown2()
> don't have to be analysed for promotions. I did some of did analysis
> above -- KERN_ALIGN is a small int, so its promotion is int and that is
> presumably smaller than uintfptr_t.
>
> rounddown2() is of course undocumented, but everyone knows that macros
> like it are supposed to mix the args without any casts and end up with
> a type related to the arg types.
>
> - subldr actually has type uintptr_t, so the old cast to uint64_t was
> just a double type mismatch (mismatched with both the source and target)
> and leaving it makes it just change the correct type to a wrong one
> before converting back to the correct type. Since you cast to uintptr_t
> and not uintfptr_t and KERN_ALIGN is small int, the rvalue would already
> have the same type as the lvalue unless it is messed up by the uint64_t
> cast.
>
> - not quite similarly on the next line:
> - the lvalue has type uintptr_t
> - casting to uint64_t is wrong, as in the new version of the previous
> line. uboot_heap_end already has type uintptr_t, and casting it to
> uint64_t just breaks it or complicates the analysis:
> - if uintptr_t is 64 bits, then casting to uint64_t has no effect
> - if uintptr_t is 128 bits, then casting to uint64_t just breaks things
> - if uintptr_t is 32, bits, then casting to uint64_t doesn't even
> break things.
>
> There is an overflow problem in theory, but the bogus cast doesn't
> fix the problem. roundup2() normally overflows if the address is
> at a nonzero offset on the last "page" (of size KERN_ALIGN bytes
> here). Then roundup2() should overflow to 0. On 32-bit arches,
> the bogus cast avoids the overflow and gives a result of
> 0x100000000. But since the rvalue has type uintptr_t, it cannot
> represent this result, and the value overflows to 0 on assignment
> instead of in roundup2().
>
> roundup2() and its overflow problems are of course undocumented.
>
This analysis is at least partly wrong because you've missed the fact
that the prior commit (that caused the warning I had to fix) changed
the type of subldr, eubdlr, and friends to uint64_t. With the
uintptr_t types, the theoretical overflow you mention was in fact an
actual overflow on 32-bit hardware people use, which is why I'm fixing
it (however ineptly).
So while a rigorous analysis would have to conclude that rounddown2()
can't create the overflow and thus the cast of its arg to 64 bits is
unneeded, I wasn't really thinking that way at the time so I just cast
the args in both of the rounding invocations to 64 bits.
I had no idea there was such a thing as uintfptr_t, it does seem like
that's the right one to use on _start.
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1451780934.1369.129.camel>