curl_Xprintf() question

[gvanem]

I see in lib/strerror.c that a %u is used for a DWORD value:

const char *Curl_winapi_strerror(DWORD err, char *buf, size_t buflen)
{
...
#ifndef CURL_DISABLE_VERBOSE_STRINGS
  if(!get_winapi_error(err, buf, buflen)) {
    msnprintf(buf, buflen, "Unknown error %u (0x%08X)", err, err);
  }

I would assume curl_msnprintf() should be printf() compatible. Now this compiles w/o a warning since there is no __attribute__((format(printf,3,4)) on this function. So adding support for that, I now get from clang-cl:

strerror.c(941,57): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
    msnprintf(buf, buflen, "Unknown error %u (0x%08X)", err, err);
                                          ~~            ^~~
                                          %lu
strerror.c(941,62): warning: format specifies type 'unsigned int' but the argument has type 'DWORD' (aka 'unsigned long') [-Wformat]
    msnprintf(buf, buflen, "Unknown error %u (0x%08X)", err, err);
                                                ~~~~         ^~~
                                                %08lX

So would it be an idea to add stuff like this:

#if defined(__GNUC__) || defined(__clang__)
#define CURL_ATTR_PRINTF(_1, _2) __attribute__ ((format(printf, _1, _2)))
#else
#define CURL_ATTR_PRINTF(_1, _2)
#endif

(and _Printf_format_string_ for MSVC) for all curl_*printf() functions?

[gvanem]

I found another such warning:

vtls/x509asn1.c(976,44): warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned long long')
      [-Wformat]
      (void)msnprintf(q, sizeof(q), "%lu", len);
                                     ~~~   ^~~
                                     %zu
vtls/x509asn1.c(1010,38): warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned long long')
      [-Wformat]
      msnprintf(r, sizeof(r), "%lu", len);
                               ~~~   ^~~
                               %zu

[bagder]

Do you want to make a PR for these, or should I?

[gvanem]

Please do.

[bagder]

I see in lib/strerror.c that a %u is used for a DWORD value:

%u is for unsigned ints, a DWORD is according to docs I find "a 32-bit unsigned integer", which incidentally on Windows is exactly the same size and an unsigned it.

I believe DWORD is usually typedef'ed as an unsigned long, but that is also 32 bits on Windows so %u or %lu makes no difference to the printf() code. On Windows, ints and longs are virtually the same. It seems next to silly to generate a warning for this.

But yes, if that code runs on other platforms than windows then %u vs %lu diff is meaningful.

I would assume curl_msnprintf() should be printf() compatible

They're actually close to compatible but not quite. The biggest difference is probably in return code, but there might also be some subtle differences in flag treatment. This minor incompatibility is one of the reasons why I've hesitated to make our own *printf() code conditional and instead stick to using it everywhere.

That's also why adding __attribute__ might cause false positives. But maybe we can work around them?

[gvanem]

But yes, if that code runs on other platforms than windows then %u vs %lu diff is meaningful.

Like Cygwin (a LP64 platform). I've not tried it. But I'd assume it would compile with a lot of noise if such __attribute__ were added.

[bagder]

Using the __attribute__ unfortunately causes problems like this and I don't know how to work around it easily:

transfer.c: In function ‘Curl_fillreadbuffer’:
transfer.c:314:26: error: ISO C90 does not support the ‘z’ gnu_printf length modifier [-Werror=format=]
  314 |                          "%zx%s", nread, endofline_native);
      |                          ^~~~~~~

[gvanem]

Never heard of %zx. Is it the same as:

      hexlen = msnprintf(hexbuffer, sizeof(hexbuffer), "%.*s", (int)nread, endofline_native);

[bagder]

'z' - A following integer conversion corresponds to a size_t or ssize_t argument, or a following n conversion corresponds to a pointer to a size_t argument.

[bagder]

see #11589

[gvanem]

Looks okay to me. But adding this for va_list functions, a clang-cl -Wall produces this new warning:

../include\curl/mprintf.h(61,17): warning: GCC requires a function with the 'format' attribute to be variadic [-Wgcc-compat]
CURL_EXTERN int curl_mvprintf(const char *format, va_list args)
                ^

Adding -Wno-gcc-compat to the already long (56) list squelches this warning.

[bagder]

To comment on the PR I think you should put the comment in #11589 ...

[bagder]

attributes have been added!