On Mon, Oct 29, 2018 at 03:39:47PM +0000, Ben Dooks wrote: > This is an updated set with most of the comments now fixed, but > a few issues that could do with looking at before finalising the > code. > > 1- the ctype fix doesn't work for prefixed __attrbitue__ See my reply in patch 4/5. > 2- do we need an 'ASN-any' type for pointers? I don't think so (see also patch 4/5). > (the asn-any is for the case printf %p, where the value of the > pointer is printed, and it is not de-refrenced)... maybe using > asn:-1 for "any address space" is a good idea? > > The only things to discuss: > > - is the printf format parsing good enough? I think so, certainly as a first step. Maybe just also handle "%i". See also my suggestion about using a table. > - do we need to add a a __attribute__((linux-printk)) for printk fmts? Good question. I don't think so because in the kernel printk() itself is declared as using __attribute__((format(printf, 1, 2))). But a mecanism will be needed in the kernel headers to specifiy the extended conversion format and their corresponding types. I was thinking something like: #pragma extended_printf_formar "%pI4[bl]" const struct sockaddr_in * but: * it's just a suggestion * I'm sure some people will hate to use #pragma * sparse doesn't have real support of #pragma (but I have some code for it). > - should we add a -Wvariadic-format to force address-space safety? Probably (but with another name). In shorter term, the warning givi-en by this series should be conditionalized on -Wformat. > I think this is getting close to merging. Yes, it progress well and I'm all in favor to merge early the basic functionalities. At this stage, what's missing the most is some testcases. I also really think that the type checking can't be done by simply using a type (as returned by evaluate_printf_symbol()), a checking function is needed or at least use your own generic function instead of trying do let compatible_argument_type() do this check. Kind regards, -- Luc
