svn commit: r293064 - head/sys/boot/uboot/lib

Ian Lepore ian at freebsd.org
Sun Jan 3 00:28:58 UTC 2016


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



More information about the svn-src-all mailing list