Re: [PATCH v1 6/6] vsnprintf: Mark va_format() with __printf() attribute

[Andy Shevchenko]

On Fri, Mar 21, 2025 at 03:09:20PM +0100, Rasmus Villemoes wrote:
> On Thu, Mar 20 2025, Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

...

> I'm sorry, but this is broken in so many ways I don't know where to
> start.

You shouldn't be sorry, my guts feeling was on the same page, I was sending it
with the expectation that someone will direct me, so thank you!

(And that's why there is a paragraph about this rather hackish patch)

> The format string that va_format actually passes on is va_fmt->fmt, and
> that is _not_ at all the same thing as va_fmt cast to (const char*),
> even if ->fmt is the first member, so the static_assert doesn't do what
> you think it does. So of course the ptr variable (which is (void*)) can
> be passed as a (const char*) argument just as well as it can be passed
> as a (struct va_format *) argument, and sure, the callee can take that
> arbitrary pointer and cast to the real type of that argument. But it
> seems you did that only to have _some_ random const char* parameter to
> attach that __printf attribute to.

True, and again, I felt that what I'm doing here is all not right.

> So, since the format string that va_format() passes to vsnprintf() is
> not one of va_format()'s own parameters, there is no parameter to attach
> that __printf() attribute to. So I actually consider this somewhat of a
> gcc bug. But can't we just silence that false positive with the tool
> that gcc provides for this very purpose:
> 
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wsuggest-attribute=format"
> static char *va_format(char *buf, char *end, struct va_format *va_fmt,
> ...
> }
> #pragma GCC diagnostic pop
> 
> with whatever added sauce to also make it work for clang.

clang doesn't produce this warning to me. But I will check again.

> Then you don't need to annotate pointer(),

Sure, I also felt that that one is hack to satisfy a broken tool.

> and then you don't need to annotate the BINARY_PRINTF users of pointer(),
> though I think those two do make sense.

I prefer to have them marked as they really like printf():s.

Thanks for the suggestion, I will experiment and send the result in v2.

-- 
With Best Regards,
Andy Shevchenko