From owner-svn-src-head@freebsd.org Sun Jan 3 00:28:58 2016 Return-Path: Delivered-To: svn-src-head@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 D160AA530C6 for ; Sun, 3 Jan 2016 00:28:58 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from pmta2.delivery6.ore.mailhop.org (pmta2.delivery6.ore.mailhop.org [54.200.129.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id A74D31D9E for ; Sun, 3 Jan 2016 00:28:58 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from ilsoft.org (unknown [73.34.117.227]) by outbound2.ore.mailhop.org (Halon Mail Gateway) with ESMTPSA; Sun, 3 Jan 2016 00:29:26 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.14.9/8.14.9) with ESMTP id u030StfE026951; Sat, 2 Jan 2016 17:28:55 -0700 (MST) (envelope-from ian@freebsd.org) Message-ID: <1451780934.1369.129.camel@freebsd.org> Subject: Re: svn commit: r293064 - head/sys/boot/uboot/lib From: Ian Lepore To: Bruce Evans Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Date: Sat, 02 Jan 2016 17:28:54 -0700 In-Reply-To: <20160103101219.C1033@besplex.bde.org> References: <201601022255.u02Mtxe4043921@repo.freebsd.org> <20160103101219.C1033@besplex.bde.org> Content-Type: text/plain; charset="us-ascii" X-Mailer: Evolution 3.16.5 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Jan 2016 00:28:58 -0000 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