Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 3 Jan 2016 10:59:10 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Ian Lepore <ian@freebsd.org>
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:  <20160103101219.C1033@besplex.bde.org>
In-Reply-To: <201601022255.u02Mtxe4043921@repo.freebsd.org>
References:  <201601022255.u02Mtxe4043921@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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.

       Another of your recent commits reminded me about this overflow
       problem.  I will complain about that separately :-).

       rounddown2() cannot overflow, and is also missing the bug of
       being named in lower case despite being an unsafe macro (one
       which evaluates its args more than once).  It happens to be
       safe because it only needs to evaluate its args more than once,
       but its name is just because it folows the bad example of
       howmany().  Even howmany() is undocumented.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160103101219.C1033>