Re: svn commit: r310171 - head/sys/sys
On Mon, 19 Dec 2016, Ravi Pokala wrote: -Original Message- From: on behalf of Ian Lepore Date: 2016-12-19, Monday at 11:20 To: Warner Losh , Ravi Pokala Cc: Sepherosa Ziehau , Dimitry Andric , src-committers , "svn-src-...@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r310171 - head/sys/sys On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote: ... Are there other precedence for avoiding the SCN macros in the tree as well, or is this new art? There was another commit recently the fixed the same kind of scanf error by making the variable fit the scanf type (changing uint64_t to an explicit long long unsigned, iirc). I don't know if that alone counts as a precedent, but IMO it's a more palatible fix than the SCN/PRI ugliness. That was apparently a bug, not a fix. Someone said that the variable needs to have type precisely uint64_t. (Extensive use of fixed-width types in FreeBSD is another bug. It asks for fixed width at all cost. Using the "fast" or "least" types would as for time or space efficiency instead. Or just use basic types whenever possible for simplicity. Fixed-width types should only be used on ABI boundaries.) I think there is an ABI boundary near the other commit. Some hardware wants uint64_t, and we should use uint_fast64_t or uint_least64_t, but we used uint64_t. These uses require translation at the ABI boundary, since the fast and least types may be larger than the fixed-width types. (In more complicated cases, the relevant fixed-width type does't exist. IIRC, POSIX requires only fixed width types of width 8, 16 and 32 to exist, since old badly designed ABIs like inet_addr() require a fixed width, but newer ABIs don't repeat the mistake for width 64. The C standard requires the "fast" and "least" types for widths 8, 16, 32 and 64 to exist. This is possible since they can all be [unsigned] long long or [u]intmax_t although these might be very far from being either fast or least. All fixed-width types are optional in C. So to support bad software ABIs and some hardware ABIs using portable code, you have to read write the data using some basic type (perhaps unsigned char) and convert it to a "fast" or "least" type (or maybe [u]intmax_t). We changed to using the unsigned long long abomination. This is basically a bad spelling of uintmax_t. All uint_fast64_t, uint_least64_t, unsigned long long and uintmax_t are at least as large asthe ABI type uint64_t, so they can be used to represent the ABI values, but all need translation at ABI booundaries since they may be strictly larger. The translation may be as simple as assignment, depending on how the code is written. Since the code wasn't written with this in mind, it might have type puns instead. Or the compiler might just warn about all implicit conversions that might lose bits. With all apologies to Churchill, SCN/PRI are the worst way to address this in a machine-independent way, except for all the other ways that have been tried from time to time. :-P No, there are much better ways. For PRI*, just cast to [u]intmax_t. The only thing wrong with this is that it lots wastes space and time when [u]intmax_t is very wide. We don't have this problem yet since [u]intmax_t is only 64 bits. That would be very wide on 8-bit systems but is merely wide on 32-bit systems and not wide on 63-bit systems. PRI* is not even available for an average typedef like uid_t. It is available for all "fast" and "least" typedefs. For SCN*, first don't use *scanf(). Use APIs that can actually handle errors, like strtol(). If you have to maintain bad code that uses *scanf(), then convert to uintmax_t. The conversions are not as short as casting for *printf(). They are much like what is needed at ABI boundaries: uintmax_t tmp; uint64_t val; /* * Good code does tmp = strtoumax() here. Then handle errors * reported by strtoumax(). Then check that the result is * representable in uint64_t. Then handle errors from that. * * Bad code did *sscanf(str, "%llx", &val) with no error checking * or handling of course. The type didn't't match the format in * general, and the behaviour was undefined on overflow. */ sscanf(str, "%jx", &tmp); /* keep null error handling */ val = tmp; /* preserve undefined on overflow */ /* * Overflow in sscanf() is less likely than before (uintmax_t * might be larger). Then overflow occurs much like before on * assignment (if the result is too large for the final type). */ Since the conversion is more elaborate for *scanf() than for *printf(), SCN* is slightly less bad than PRI*. SCN* is also no
Re: svn commit: r310171 - head/sys/sys
On Tue, Dec 20, 2016 at 4:37 AM, Ravi Pokala wrote: > -Original Message- >> From: on behalf of Ian Lepore >> >> Date: 2016-12-19, Monday at 11:20 >> To: Warner Losh , Ravi Pokala >> Cc: Sepherosa Ziehau , Dimitry Andric >> , src-committers , >> "svn-src-...@freebsd.org" , >> "svn-src-head@freebsd.org" >> Subject: Re: svn commit: r310171 - head/sys/sys >> >> On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote: >>> >>> ... >>> >>> Are there other precedence for avoiding the SCN macros in the tree as >>> well, or is this new art? >>> >>> Warner >> >> There was another commit recently the fixed the same kind of scanf >> error by making the variable fit the scanf type (changing uint64_t to >> an explicit long long unsigned, iirc). I don't know if that alone >> counts as a precedent, but IMO it's a more palatible fix than the >> SCN/PRI ugliness. > > With all apologies to Churchill, SCN/PRI are the worst way to address this in > a machine-independent way, except for all the other ways that have been tried > from time to time. :-P > If no objection comes, I'd commit the posted patch as it is tomorrow in my time. Thanks, sephe -- Tomorrow Will Never Die ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
-Original Message- > From: on behalf of Ian Lepore > > Date: 2016-12-19, Monday at 11:20 > To: Warner Losh , Ravi Pokala > Cc: Sepherosa Ziehau , Dimitry Andric > , src-committers , > "svn-src-...@freebsd.org" , > "svn-src-head@freebsd.org" > Subject: Re: svn commit: r310171 - head/sys/sys > > On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote: >> >> ... >> >> Are there other precedence for avoiding the SCN macros in the tree as >> well, or is this new art? >> >> Warner > > There was another commit recently the fixed the same kind of scanf > error by making the variable fit the scanf type (changing uint64_t to > an explicit long long unsigned, iirc). I don't know if that alone > counts as a precedent, but IMO it's a more palatible fix than the > SCN/PRI ugliness. With all apologies to Churchill, SCN/PRI are the worst way to address this in a machine-independent way, except for all the other ways that have been tried from time to time. :-P -Ravi (rpokala@) > -- Ian ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
On Mon, 2016-12-19 at 11:58 -0700, Warner Losh wrote: > On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala wrote: > > > > -Original Message- > > > > > > From: on behalf of Sepherosa > > > Ziehau > > > Date: 2016-12-18, Sunday at 23:02 > > > To: Dimitry Andric > > > Cc: , , > > -src-h...@freebsd.org> > > > Subject: Re: svn commit: r310171 - head/sys/sys > > > > > > The following patch unbreaks the LINT builds on amd64 for me > > > after this commit: > > > https://people.freebsd.org/~sephe/geom_sscanf.diff > > Wouldn't it be better to use the SCN macros? > Are there other precedence for avoiding the SCN macros in the tree as > well, or is this new art? > > Warner There was another commit recently the fixed the same kind of scanf error by making the variable fit the scanf type (changing uint64_t to an explicit long long unsigned, iirc). I don't know if that alone counts as a precedent, but IMO it's a more palatible fix than the SCN/PRI ugliness. -- Ian ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
On 19 Dec 2016, at 19:58, Warner Losh wrote: > > On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala wrote: >> -Original Message- >>> From: on behalf of Sepherosa Ziehau >>> >>> Date: 2016-12-18, Sunday at 23:02 >>> To: Dimitry Andric >>> Cc: , , >>> >>> Subject: Re: svn commit: r310171 - head/sys/sys >>> >>> The following patch unbreaks the LINT builds on amd64 for me after this >>> commit: >>> https://people.freebsd.org/~sephe/geom_sscanf.diff >> >> Wouldn't it be better to use the SCN macros? > > Are there other precedence for avoiding the SCN macros in the tree as > well, or is this new art? I personally don't have anything against using the PRI or SCN macros, but traditionally there has been some backlash against it, if I recall correctly. It also requires including either or , depending on circumstance or preference. -Dimitry signature.asc Description: Message signed with OpenPGP using GPGMail
Re: svn commit: r310171 - head/sys/sys
On Mon, Dec 19, 2016 at 1:39 AM, Ravi Pokala wrote: > -Original Message- >> From: on behalf of Sepherosa Ziehau >> >> Date: 2016-12-18, Sunday at 23:02 >> To: Dimitry Andric >> Cc: , , >> >> Subject: Re: svn commit: r310171 - head/sys/sys >> >> The following patch unbreaks the LINT builds on amd64 for me after this >> commit: >> https://people.freebsd.org/~sephe/geom_sscanf.diff > > Wouldn't it be better to use the SCN macros? Are there other precedence for avoiding the SCN macros in the tree as well, or is this new art? Warner >> Thanks, >> sephe >> >> >> On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric wrote: >>> Author: dim >>> Date: Fri Dec 16 19:49:22 2016 >>> New Revision: 310171 >>> URL: https://svnweb.freebsd.org/changeset/base/310171 >>> >>> Log: >>> Add __scanflike attributes to the kernel's sscanf() and vsscanf() >>> declarations. This should help to catch future mismatches between >>> format strings and arguments. >>> >>> MFC after:1 week >>> >>> Modified: >>> head/sys/sys/systm.h >>> >>> Modified: head/sys/sys/systm.h >>> == >>> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170) >>> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171) >>> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char >>> intvsnrprintf(char *, size_t, int, const char *, __va_list) >>> __printflike(4, 0); >>> intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0); >>> intttyprintf(struct tty *, const char *, ...) __printflike(2, 3); >>> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2); >>> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) >>> __nonnull(2); >>> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) >>> __scanflike(2, 3); >>> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) >>> __nonnull(2) __scanflike(2, 0); >>> long strtol(const char *, char **, int) __nonnull(1); >>> u_long strtoul(const char *, char **, int) __nonnull(1); >>> quad_t strtoq(const char *, char **, int) __nonnull(1); >>> ___ >>> svn-src-...@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" >> >> -- >> Tomorrow Will Never Die > > > > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
-Original Message- > From: on behalf of Sepherosa Ziehau > > Date: 2016-12-18, Sunday at 23:02 > To: Dimitry Andric > Cc: , , > > Subject: Re: svn commit: r310171 - head/sys/sys > > The following patch unbreaks the LINT builds on amd64 for me after this > commit: > https://people.freebsd.org/~sephe/geom_sscanf.diff Wouldn't it be better to use the SCN macros? -Ravi (rpokala@) > Please review it. > > Thanks, > sephe > > > On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric wrote: >> Author: dim >> Date: Fri Dec 16 19:49:22 2016 >> New Revision: 310171 >> URL: https://svnweb.freebsd.org/changeset/base/310171 >> >> Log: >> Add __scanflike attributes to the kernel's sscanf() and vsscanf() >> declarations. This should help to catch future mismatches between >> format strings and arguments. >> >> MFC after:1 week >> >> Modified: >> head/sys/sys/systm.h >> >> Modified: head/sys/sys/systm.h >> == >> --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170) >> +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171) >> @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char >> intvsnrprintf(char *, size_t, int, const char *, __va_list) >> __printflike(4, 0); >> intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0); >> intttyprintf(struct tty *, const char *, ...) __printflike(2, 3); >> -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2); >> -intvsscanf(const char *, char const *, __va_list) __nonnull(1) >> __nonnull(2); >> +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) >> __scanflike(2, 3); >> +intvsscanf(const char *, char const *, __va_list) __nonnull(1) >> __nonnull(2) __scanflike(2, 0); >> long strtol(const char *, char **, int) __nonnull(1); >> u_long strtoul(const char *, char **, int) __nonnull(1); >> quad_t strtoq(const char *, char **, int) __nonnull(1); >> ___ >> svn-src-...@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" > > -- > Tomorrow Will Never Die ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
The following patch unbreaks the LINT builds on amd64 for me after this commit: https://people.freebsd.org/~sephe/geom_sscanf.diff Please review it. Thanks, sephe On Sat, Dec 17, 2016 at 3:49 AM, Dimitry Andric wrote: > Author: dim > Date: Fri Dec 16 19:49:22 2016 > New Revision: 310171 > URL: https://svnweb.freebsd.org/changeset/base/310171 > > Log: > Add __scanflike attributes to the kernel's sscanf() and vsscanf() > declarations. This should help to catch future mismatches between > format strings and arguments. > > MFC after:1 week > > Modified: > head/sys/sys/systm.h > > Modified: head/sys/sys/systm.h > == > --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170) > +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171) > @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char > intvsnrprintf(char *, size_t, int, const char *, __va_list) > __printflike(4, 0); > intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0); > intttyprintf(struct tty *, const char *, ...) __printflike(2, 3); > -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2); > -intvsscanf(const char *, char const *, __va_list) __nonnull(1) > __nonnull(2); > +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) > __scanflike(2, 3); > +intvsscanf(const char *, char const *, __va_list) __nonnull(1) > __nonnull(2) __scanflike(2, 0); > long strtol(const char *, char **, int) __nonnull(1); > u_long strtoul(const char *, char **, int) __nonnull(1); > quad_t strtoq(const char *, char **, int) __nonnull(1); > ___ > svn-src-...@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" -- Tomorrow Will Never Die ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r310171 - head/sys/sys
Thanks! On Fri, Dec 16, 2016 at 11:49 AM, Dimitry Andric wrote: > Author: dim > Date: Fri Dec 16 19:49:22 2016 > New Revision: 310171 > URL: https://svnweb.freebsd.org/changeset/base/310171 > > Log: > Add __scanflike attributes to the kernel's sscanf() and vsscanf() > declarations. This should help to catch future mismatches between > format strings and arguments. > > MFC after:1 week > > Modified: > head/sys/sys/systm.h > > Modified: head/sys/sys/systm.h > == > --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170) > +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171) > @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char > intvsnrprintf(char *, size_t, int, const char *, __va_list) > __printflike(4, 0); > intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0); > intttyprintf(struct tty *, const char *, ...) __printflike(2, 3); > -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2); > -intvsscanf(const char *, char const *, __va_list) __nonnull(1) > __nonnull(2); > +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) > __scanflike(2, 3); > +intvsscanf(const char *, char const *, __va_list) __nonnull(1) > __nonnull(2) __scanflike(2, 0); > long strtol(const char *, char **, int) __nonnull(1); > u_long strtoul(const char *, char **, int) __nonnull(1); > quad_t strtoq(const char *, char **, int) __nonnull(1); > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r310171 - head/sys/sys
Author: dim Date: Fri Dec 16 19:49:22 2016 New Revision: 310171 URL: https://svnweb.freebsd.org/changeset/base/310171 Log: Add __scanflike attributes to the kernel's sscanf() and vsscanf() declarations. This should help to catch future mismatches between format strings and arguments. MFC after:1 week Modified: head/sys/sys/systm.h Modified: head/sys/sys/systm.h == --- head/sys/sys/systm.hFri Dec 16 19:09:57 2016(r310170) +++ head/sys/sys/systm.hFri Dec 16 19:49:22 2016(r310171) @@ -227,8 +227,8 @@ int vsnprintf(char *, size_t, const char intvsnrprintf(char *, size_t, int, const char *, __va_list) __printflike(4, 0); intvsprintf(char *buf, const char *, __va_list) __printflike(2, 0); intttyprintf(struct tty *, const char *, ...) __printflike(2, 3); -intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2); -intvsscanf(const char *, char const *, __va_list) __nonnull(1) __nonnull(2); +intsscanf(const char *, char const *, ...) __nonnull(1) __nonnull(2) __scanflike(2, 3); +intvsscanf(const char *, char const *, __va_list) __nonnull(1) __nonnull(2) __scanflike(2, 0); long strtol(const char *, char **, int) __nonnull(1); u_long strtoul(const char *, char **, int) __nonnull(1); quad_t strtoq(const char *, char **, int) __nonnull(1); ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"