Date: Sat, 31 Oct 2015 18:59:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Justin Hibbits <jhibbits@freebsd.org> Cc: Conrad Meyer <cse.cem@gmail.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r290221 - head/sys/powerpc/powerpc Message-ID: <20151031180405.Y1413@besplex.bde.org> In-Reply-To: <39CCA88E-6D59-4F09-B054-DF765B8C9708@freebsd.org> References: <201510310208.t9V28dIh051810@repo.freebsd.org> <CAG6CVpWgvgZ-ubXcHP8Ky%2BqM6H3R%2BJRLFKdHmu6=du7m-fhKZA@mail.gmail.com> <39CCA88E-6D59-4F09-B054-DF765B8C9708@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 30 Oct 2015, Justin Hibbits wrote: > On Oct 30, 2015, at 9:27 PM, Conrad Meyer wrote: > >> Comments inline. >> >> On Fri, Oct 30, 2015 at 7:08 PM, Justin Hibbits <jhibbits@freebsd.org> >> wrote: >>> Author: jhibbits >>> Date: Sat Oct 31 02:08:39 2015 >>> New Revision: 290221 >>> URL: https://svnweb.freebsd.org/changeset/base/290221 >>> >>> Log: >>> Print unsigned memory sizes, to handle >2GB RAM on 32-bit powerpc. >>> ... >>> ========= >>> ===================================================================== >>> --- head/sys/powerpc/powerpc/machdep.c Sat Oct 31 02:07:30 2015 >>> (r290220) >>> +++ head/sys/powerpc/powerpc/machdep.c Sat Oct 31 02:08:39 2015 >>> (r290221) >>> @@ -176,12 +176,12 @@ cpu_startup(void *dummy) >>> #ifdef PERFMON >>> perfmon_init(); >>> #endif >>> - printf("real memory = %ld (%ld MB)\n", ptoa(physmem), >>> + printf("real memory = %lu (%lu MB)\n", ptoa(physmem), >>> ptoa(physmem) / 1048576); >> >> Shouldn't this be "real memory = %ju (%lu MB)\n" and >> (uintmax_t)ptoa(physmem), or it may overflow on >4GB RAM systems? No. Any overflow that may occur happens in ptoa() before this cast can effect it. Such overflow is supposed to be prevented by bogusly casting to the undocumented type unsigned long in the undocumented macro ptoa(). %lu is correct since it matches this type. Any mismatch (except the old sign mismatch) would be detected by printf format checking. Casting to uintmax_t would have no effect except to bloat the code from 32 bits to 64 bits in the 32-bit case and simplify matching the printf format with the type. There are about 100 different combinations of bogus types, bogus casts, no casts, and printf formats in various arches. Perhaps the bogus cast in powerpc32 ptoa() doesn't actually work. It only works up to 4GB. i386 used to have that limit and had to be changed for PAE. It now uses no cast in ptoa(). Callers must cast the arg to vm_paddr_t for efficiency or uintmax_t for simplicity. That is best since it forces them to be careful with types. The i386 code corresponding to the above uses uintmax_t for everything for simplicity. The bogus types start with physmem being long. That is more than large enough, at least on 32-bit arches, since its units are pages. But it is inconsistent with vm's page counters mostly having type u_int except where. On 64-bit arches, long is much longer than u_int, so using it tends to mask bugs, but on 32-bit arches it is shorter than u_int, so using it tends to unmask bugs. In the above, its signedness makes little difference since the bogus cast in the macro kills its sign bit. > Yes, it should, and it will. However, currently ptoa() casts to unsigned > long, and I didn't want to change that yet (I will eventually). In fact, the > machine I'm testing on has 8GB, which I've had to artificially limit to <4GB > because of the various memory accounting size limits. Similar to i386 with PAE. >>> realmem = physmem; >>> >>> if (bootverbose) >>> - printf("available KVA = %zd (%zd MB)\n", >>> + printf("available KVA = %zu (%zu MB)\n", >>> virtual_end - virtual_avail, >>> (virtual_end - virtual_avail) / 1048576); >>> This has logical type mismatches. The types are vm_offset_t, and it is assumed that this is the same as size_t so that it matches size_t. This defeats the reason for existence of fancy types like vm_offset_t. MD code can more easily just hard-code vm_offset_t and size_t as u_int ot u_long everywhere. This only works for virtual addresses. The types remain useful in MI code and for keeping really different things like physical memory sizes different. %zu is also logically wrong for the size in MB. size_t is only logically correct for sizes in bytes. The expression for the size in MB normally has type size_t too, but this is MD. vm has a vm_size_t type to add to the confusion. It is hard to remember to convert between offsets and sizes when they are physically identical. But IIRC, vm doesn't have a type for its page counts where the differences are physical. >>> @@ -199,7 +199,7 @@ cpu_startup(void *dummy) >>> #ifdef __powerpc64__ >>> printf("0x%016lx - 0x%016lx, %ld bytes (%ld >>> pages)\n", >>> #else >>> - printf("0x%08x - 0x%08x, %d bytes (%ld pages)\n", >>> + printf("0x%08x - 0x%08x, %u bytes (%lu pages)\n", >> >> It seems wrong that bytes is only %u here and pages is a %lu. I think >> bytes should probably be %ju too? What is the type of size1? This is perfectly backwards. Byte counts are large so they need to have type vm_paddr_t or larger. Page counts are small so they can be signed int. But surely the types are correct here since the printf format checker would have detected any mismatch except for signs? Bogus long format specifiers are likely to be needed to match the bogus long type of physmem. All of the code just before here has type errors. It starts with the style bugs of a nested declaration of vm_offset_t size1 and an initializer in this declaration. size1 is not an offset at all. It is actually a size, and its correct type is vm_size_t (or to be honestly sloppy, u_long). The expression for it is the difference of 2 vm_offset_t's which should also be vm_size_t's, but IIRC that is an old API bug, like the type of physmem. Most the types being printed are vm_offet_t's (which should be vm_size_t's)... > You're right. It should be long. However, it's 6-of-one in this case, since > on powerpc (32-bit) sizeof(long) == sizeof(int), so it doesn't matter too > much yet. size1 is a vm_offset_t, which is 32-bits in this case. > phys_avail[] is also an array of vm_offset_t's, and I intend to change it to > vm_paddr_t, and cast to uintmax_t for printf purposes, but again, that's > longer term (a lot of the powerpc bootup needs a cleanup, anyway). ... Not really. It is the long that is most bogus. The vm_offset_t's (which should be vm_size_t's) have the correct type. This is u_int on 32-bit arches, although u_long would work. The bogus long is from the type of PAGE_SIZE being bogusly long, so expressions using it tend to promote to long. sparc64 has the same bug. This is a bogus way of avoiding some overflow bugs. But it doesn't even avoid overflow on 32-bit arches in the 2G-4G range. The above divides by PAGE_SIZE so there is no problem. Multiplication by a signed int page count can give unnecessary overflow in the range. But muiltiplication by an unsigned int page count doesn't even do that. >>> @@ -208,7 +208,7 @@ cpu_startup(void *dummy) >>> >>> vm_ksubmap_init(&kmi); >>> >>> - printf("avail memory = %ld (%ld MB)\n", ptoa(vm_cnt.v_free_count), >>> + printf("avail memory = %lu (%lu MB)\n", ptoa(vm_cnt.v_free_count), >>> ptoa(vm_cnt.v_free_count) / 1048576); >> >> Same as with the first printf. > > See above. It's on my list, I just wanted something cleaner for now. sparc64 does this one even more bogusly by not using ptoa() but a direct multiplication by PAGE_SIZE with no cast. This asks for overflow, but works due to PAGE_SIZE being bogusly long. Hmm, division by PAGE_SIZE in the above is a style bug too. There is a macro for converting in that direction too. All of the related conversion macros are badly organized or named or duplicated, and undocumented. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151031180405.Y1413>