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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
