From owner-svn-src-head@freebsd.org Mon Dec 19 17:02:29 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 536D7C88393; Mon, 19 Dec 2016 17:02:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [IPv6:2001:470:1f11:75::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 1371C101E; Mon, 19 Dec 2016 17:02:29 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (c-73-231-226-104.hsd1.ca.comcast.net [73.231.226.104]) by mail.baldwin.cx (Postfix) with ESMTPSA id 9587D10A746; Mon, 19 Dec 2016 12:02:27 -0500 (EST) From: John Baldwin To: Eric van Gyzen Cc: Warner Losh , Dimitry Andric , Baptiste Daroussin , "Conrad E. Meyer" , src-committers , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r310138 - head/lib/libc/stdio Date: Sat, 17 Dec 2016 12:33:33 -0800 Message-ID: <49460793.UcUNovQMDa@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.0-PRERELEASE; KDE/4.14.10; amd64; ; ) In-Reply-To: References: <201612160144.uBG1ipjW016736@repo.freebsd.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Mon, 19 Dec 2016 12:02:27 -0500 (EST) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Mon, 19 Dec 2016 17:02:29 -0000 On Friday, December 16, 2016 07:31:28 PM Eric van Gyzen wrote: > On 12/16/2016 17:44, Warner Losh wrote: > > On Fri, Dec 16, 2016 at 3:07 PM, John Baldwin wrote: > >> On Friday, December 16, 2016 04:53:04 PM Eric van Gyzen wrote: > >>> On 12/16/2016 16:45, John Baldwin wrote: > >>>> On Friday, December 16, 2016 08:53:26 PM Dimitry Andric wrote: > >>>>> On 16 Dec 2016, at 20:31, Baptiste Daroussin wrote: > >>>>>> > >>>>>> On Fri, Dec 16, 2016 at 01:44:51AM +0000, Conrad E. Meyer wrote: > >>>>>>> Author: cem > >>>>>>> Date: Fri Dec 16 01:44:50 2016 > >>>>>>> New Revision: 310138 > >>>>>>> URL: https://svnweb.freebsd.org/changeset/base/310138 > >>>>>>> > >>>>>>> Log: > >>>>>>> vfprintf(3): Add support for kernel %b format > >>>>>>> > >>>>>>> This is a direct port of the kernel %b format. > >>>>>>> > >>>>>>> I'm unclear on if (more) non-portable printf extensions will be a > >>>>>>> problem. I think it's desirable to have userspace formats include all > >>>>>>> kernel formats, but there may be competing goals I'm not aware of. > >>>>>>> > >>>>>>> Reviewed by: no one, unfortunately > >>>>>>> Sponsored by: Dell EMC Isilon > >>>>>>> Differential Revision: https://reviews.freebsd.org/D8426 > >>>>>>> > >>>>>> > >>>>>> I really don't think it is a good idea, if used in userland it would be make > >>>>>> more of our code difficult to port elsewhere. > >>>>> > >>>>> Indeed, this is a bad idea. These custom format specifiers should be > >>>>> eliminated, not multiplied. :-) > >>>>> > >>>>> > >>>>>> Other than that, it makes more difficult to use vanilla gcc with out userland. > >>>>>> and it is adding more complexity to be able to build freebsd from a non freebsd > >>>>>> system which some people are working on. > >>>>>> > >>>>>> Personnaly I would prefer to see those extensions removed from the kernel rather > >>>>>> than see them available in userland. > >>>>> > >>>>> Same here. > >>>>> > >>>>> > >>>>>> Can't we use simple helper function instead? > >>>>> > >>>>> Yes, please. Just take the snprintb(3) function from NetBSD: > >>>>> > >>>>> http://netbsd.gw.com/cgi-bin/man-cgi?snprintb+3+NetBSD-current > >>>> > >>>> In general I agree with something like this instead, but it is quite a bit more > >>>> tedious to use as you have to run it once to determine the length, allocate a > >>>> buffer, and then run it again. Calling malloc() for that buffer isn't always > >>>> convenient in the kernel (though it should be fine in userland). Having it live > >>>> in printf() itself means the output is generated to the stream without having to > >>>> manage a variable-sized intermediate buffer. > >>> > >>> I imagine most callers can simply use a char[sizeof(fmt)+C] on the stack, where > >>> C is some constant that I haven't taken the time to calculate, at the risk of > >>> making myself look foolish and unprofessional. > >> > >> Hmm, that might work, but it is still cumbersome. Probably to make things readable > >> we'd end up with a wrapper: > >> > >> printb(uint val, const char *fmt) > >> { > >> char buf[strlen(fmt) + C]; > >> > >> snprintb(...); > >> printf("%s", buf); > >> } > > > > Sadly this "cure" is worse than the disease. > > How about this cure? > > printf("reg=%b\n", value, FORMAT); > > // versus > > char buf[BITMASK_BUFFER_SIZE(FORMAT)]; > printf("reg=%s\n", format_bitmask(buf, sizeof(buf), value, FORMAT)); > > That doesn't seem so bad. The trick here is giving FORMAT twice. For code that often uses %b that is untenable. You would have to make it a macro (or use printb which only accepts it once which is why I suggested that approach instead). But a macro moves its definition out of context. Here's an example to think about: /* * Here we should probably set up flags indicating * whether or not various features are available. * The interesting ones are probably VME, PSE, PAE, * and PGE. The code already assumes without bothering * to check that all CPUs >= Pentium have a TSC and * MSRs. */ printf("\n Features=0x%b", cpu_feature, "\020" "\001FPU" /* Integral FPU */ "\002VME" /* Extended VM86 mode support */ "\003DE" /* Debugging Extensions (CR4.DE) */ "\004PSE" /* 4MByte page tables */ "\005TSC" /* Timestamp counter */ "\006MSR" /* Machine specific registers */ "\007PAE" /* Physical address extension */ "\010MCE" /* Machine Check support */ "\011CX8" /* CMPEXCH8 instruction */ "\012APIC" /* SMP local APIC */ "\013oldMTRR" /* Previous implementation of MTRR */ "\014SEP" /* Fast System Call */ "\015MTRR" /* Memory Type Range Registers */ "\016PGE" /* PG_G (global bit) support */ "\017MCA" /* Machine Check Architecture */ "\020CMOV" /* CMOV instruction */ "\021PAT" /* Page attributes table */ "\022PSE36" /* 36 bit address space support */ "\023PN" /* Processor Serial number */ "\024CLFLUSH" /* Has the CLFLUSH instruction */ "\025" "\026DTS" /* Debug Trace Store */ "\027ACPI" /* ACPI support */ "\030MMX" /* MMX instructions */ "\031FXSR" /* FXSAVE/FXRSTOR */ "\032SSE" /* Streaming SIMD Extensions */ "\033SSE2" /* Streaming SIMD Extensions #2 */ "\034SS" /* Self snoop */ "\035HTT" /* Hyperthreading (see EBX bit 16-23) */ "\036TM" /* Thermal Monitor clock slowdown */ "\037IA64" /* CPU can execute IA64 instructions */ "\040PBE" /* Pending Break Enable */ ); if (cpu_feature2 != 0) { printf("\n Features2=0x%b", cpu_feature2, "\020" "\001SSE3" /* SSE3 */ "\002PCLMULQDQ" /* Carry-Less Mul Quadword */ "\003DTES64" /* 64-bit Debug Trace */ "\004MON" /* MONITOR/MWAIT Instructions */ "\005DS_CPL" /* CPL Qualified Debug Store */ "\006VMX" /* Virtual Machine Extensions */ "\007SMX" /* Safer Mode Extensions */ "\010EST" /* Enhanced SpeedStep */ "\011TM2" /* Thermal Monitor 2 */ "\012SSSE3" /* SSSE3 */ "\013CNXT-ID" /* L1 context ID available */ "\014SDBG" /* IA32 silicon debug */ "\015FMA" /* Fused Multiply Add */ "\016CX16" /* CMPXCHG16B Instruction */ "\017xTPR" /* Send Task Priority Messages*/ "\020PDCM" /* Perf/Debug Capability MSR */ "\021" "\022PCID" /* Process-context Identifiers*/ "\023DCA" /* Direct Cache Access */ "\024SSE4.1" /* SSE 4.1 */ "\025SSE4.2" /* SSE 4.2 */ "\026x2APIC" /* xAPIC Extensions */ "\027MOVBE" /* MOVBE Instruction */ "\030POPCNT" /* POPCNT Instruction */ "\031TSCDLT" /* TSC-Deadline Timer */ "\032AESNI" /* AES Crypto */ "\033XSAVE" /* XSAVE/XRSTOR States */ "\034OSXSAVE" /* OS-Enabled State Management*/ "\035AVX" /* Advanced Vector Extensions */ "\036F16C" /* Half-precision conversions */ "\037RDRAND" /* RDRAND Instruction */ "\040HV" /* Hypervisor */ ); } (and several more of these) -- John Baldwin