On Tue, Oct 30, 2018 at 11:06:03AM +0000, Ben Dooks wrote:
> On 29/10/18 22:08, Luc Van Oostenryck wrote:
> > 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.
>
> ok, i'm thinking something like:
>
> static void fmt_validate_str(void *data, struct expression **expr,
> struct symbol *sym); // todo //
>
> tatic void fmt_validate_ptr(void *data, struct expression **expr,
> struct symbol *sym); // todo //
>
> static void fmt_validate_type(void *data, struct expression **expr,
> struct symbol *sym)
> {
> struct symbol *source = degenerate(*expr);
> struct symbol *target = data;
>
> /* check source vs. target */
> /* note, we need to be fairly strict, long != long long, etc */
>
> }
>
> struct format_string {
> const char *fmt,
> void (validate)(void *data, struct expression *expr, struct symbol *sym);
> void *data
> } fmts[] = {
> { "s", .validate = fmt_validate_str, },
> { "p", .validate = fmt_validate_ptr, },
> { "d", .validate = fmt_validate_type, .data = &int_ctype },
> { "ld", .validate = fmt_validate_type, .data = &long_type },
> { "lld", .validate = fmt_validate_type, .data = &llong_type },
> { }
> }
>
> where the best match is searched for, and then that function is executed.
It's essentially what I had in mind.
I was thinking about a perfect match, not a best match, so that later we
can use a hash table or simply reuse lookup_symbol() with a new namespace.
But it's a detail.
> > > - 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).
>
> I sort of like that, we should probably start a discussion about
> this on the kernel lists..
IMO, better to first come with something already working.
I'm myself not really excited of using #pragma but it has the
advantage to directly address the problem. All other solutions
I can see are indirect and try to reuse another mechanism to
add this format-to-type information as a side-effect.
> > > - 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.
>
> Ok, added.
>
> > > 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.
>
> I'll look into how to do this better.
>
> Is there any chance of getting a ptrlist patch to allow fetching
> pointers from arbitrary positions in the list, it would mean we
> can move the code that's doing the variadic bits out of the main
> checking loop altogether.
I just sent one but, most probably, it will only apply to the tree
I maintain @ git://github.com/lucvoo/sparse.git
> For when you want to merge, is it ok to send patches again or
> do you prefer a mergable branch somewhere?
I'm fine with both with a slight preference for a git branch.
Kind regards,
-- Luc
