From owner-svn-src-all@freebsd.org Sat Jan 2 23:59:22 2016 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 20159A5E2B2; Sat, 2 Jan 2016 23:59:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id E06741DD8; Sat, 2 Jan 2016 23:59:21 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id D20EA3C5D6A; Sun, 3 Jan 2016 10:59:10 +1100 (AEDT) Date: Sun, 3 Jan 2016 10:59:10 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Lepore 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 In-Reply-To: <201601022255.u02Mtxe4043921@repo.freebsd.org> Message-ID: <20160103101219.C1033@besplex.bde.org> References: <201601022255.u02Mtxe4043921@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=R4L+YolX c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=kj9zAlcOel0A:10 a=YDqFnzWMhb-X08EIHl0A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.20 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, 02 Jan 2016 23:59:22 -0000 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