Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 17 Dec 2016 12:33:33 -0800
From:      John Baldwin <jhb@freebsd.org>
To:        Eric van Gyzen <vangyzen@freebsd.org>
Cc:        Warner Losh <imp@bsdimp.com>, Dimitry Andric <dim@freebsd.org>, Baptiste Daroussin <bapt@freebsd.org>, "Conrad E. Meyer" <cem@freebsd.org>, src-committers <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>
Subject:   Re: svn commit: r310138 - head/lib/libc/stdio
Message-ID:  <49460793.UcUNovQMDa@ralph.baldwin.cx>
In-Reply-To: <e2056221-26ad-5108-3385-865abff12d78@FreeBSD.org>
References:  <201612160144.uBG1ipjW016736@repo.freebsd.org> <CANCZdfrNu0=qPuMX8SA2H-MUXGb5rei06_dQg9XGp-Miu3Xsfg@mail.gmail.com> <e2056221-26ad-5108-3385-865abff12d78@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <jhb@freebsd.org> 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 <bapt@FreeBSD.org> 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<b20>"
                        "\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<b16>"
                                "\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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?49460793.UcUNovQMDa>