If it's a bug with clang then probably the people using the buggy compiler should add this to their own CFLAGS until clang fixes it?

Do you have a link to a bug report for clang?

Not yet, because I couldn't talk to them and confirm that this is a bug, and not a feature. using CFLAGS="-Wno-format-nonliteral" doesn't work, it's ignored because configure inserts -Wformat=2.

What are the compiler warnings it emits?

The relevant ones are in the following snippet:

  CC     gmessages.lo
gmessages.c:871:42: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
      size = _g_vsnprintf (buffer, 1024, format, args);
                                         ^~~~~~
gmessages.c:1059:3: warning: data argument not used by format string [-Wformat-extra-args]
         expression);
         ^
gmessages.c:1550:31: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  return _g_vsnprintf (&c, 1, format, args) + 1;
                              ^~~~~~
1 warning and 2 errors generated.

Link to clang bug:
http://llvm.org/bugs/show_bug.cgi?id=14935

After some discussion, it was decided that it's a feature, not a bug. clang will warn about non string literals even with va_args. clang behavior handles this case in a different way, as described in the following commit:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120220/053732.html

For short, the function that calls v*printf must have a format(printf) attribute:

__attribute__((__format__ (__printf__, 3, 0)))
void
g_logv (const gchar   *log_domain,
	GLogLevelFlags log_level,
	const gchar   *format,
	va_list	       args)
{
(...)
      size = _g_vsnprintf (buffer, 1024, format, args);

No errors in this case.

Completely reviewing this issue:

1) reversing the CFLAGS line to CFLAGS="$with_cflags $CFLAGS" solves the problem of the build system ignoring user flags.
2) clang requires G_GNUC_PRINTF on the functions calling v*printf to be able to keep the warning and still ignore them when the usage is correct.
3) even after adding G_GNUC_PRINTF on the relevant functions, some functions are actually breaking the warning. This is the case of g_ascii_formatd, which calls _g_snprintf without a literal and g_format_size_full, which calls g_string_append_printf. This means that gcc is ignoring the errors.
4) gcc is ignoring the errors because "-Werror=format=2" is not working. So this flag is already broken.
5) manually expanding -Werror=format=2 in configure.ac to -Werror=format-nonliteral -Werror=format-y2k -Werror=format-security fixes gcc handling the warnings (but also breaks the build)

I'm sending a patch for 1) and 2) because they're trivial. To fix the others it's necessary to deal with 3). Either the nonliteral usage in those cases are legitimate (so the warning should be removed), or a workaround for them should be made (however I think this is strange, because it's defeating the goal of the warning anyway).

I'll see if I can talk to gcc people to deal with 4).

Created attachment 233387 [details] [review]
0001-Fix-user-CFLAGS-handling-user-overwrites-defaults.patch

imho this is a pretty stupid 'feature' from clang... Not that we don't have a history of working around broken compilers already, though...

Review of attachment 233387 [details] [review]:

The code looks fine, but the commit message needs improvement and a link to this bug.  I fixed both those and pushed.

Review of attachment 233387 [details] [review]:

http://git.gnome.org/browse/glib/commit/?id=0864e3bd6fc17bab3f60b6f0e240efb44b6b5d43

(In reply to comment #9)
> imho this is a pretty stupid 'feature' from clang... Not that we don't have a

After hand writing the second patch and noticing that there are many legitimate cases for nonliterals in format strings, I actually think -Wformat-nonliteral is the stupid warning. clang just does a better job than gcc respecting this warning, so it's more annoying.

Created attachment 233390 [details] [review]
Add-G_GNUC_PRINTF-on-all-functions-with-format-strin

An ugly patch. The patch is not enough to make clang to compile without errors, there are still 10~20 real cases of non literal formatting. As soon as gcc side is fixed, compilation will break for both clang and gcc. If possible, consider not using -Wformat-nonliteral. Else, Next step will be to remove current erroneous cases.

Can you elaborate on 4) and 5)  ? I can't reproduce this with either:

RHEL 6.4: gcc version 4.4.7 20120313 (Red Hat 4.4.7-3) (GCC)   
Fedora 18: gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC)

I tried: http://git.gnome.org/browse/glib/commit/?h=wip/format-errors&id=de001225c39a4c4f5773b88b9805456f60d11374

Review of attachment 233390 [details] [review]:

This looks correct to me, thanks a lot for the patch!

(In reply to comment #14)
> Can you elaborate on 4) and 5)  ? I can't reproduce this with either:

In Ubuntu 12.10: error (gcc 4.7.2-2ubuntu1)
In Arch Linux: no error (gcc 4.7.2-3)

But Ubuntu gcc has -Wformat active by default and Arch Linux doesn't. The problem is that these warnings require -Wformat in configure:

cc1: error: -Wformat-nonliteral ignored without -Wformat [-Werror=format-nonliteral]

Try this instead:
CFLAGS="-Wformat -Werror=format-nonliteral" ./configure && make

For 4) I tried to ask in gcc IRC, but no one answered, so I filed a bug report at:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56048

Current list of clang warnings:
gcontenttype.c:428:42: warning: format string is not a string literal [-Wformat-nonliteral]
gcontenttype.c:431:36: warning: format string is not a string literal [-Wformat-nonliteral]
gcontenttype.c:433:46: warning: format string is not a string literal [-Wformat-nonliteral]
gmarkup.c:2432:31: warning: format string is not a string literal [-Wformat-nonliteral]
gmarkup.c:2439:31: warning: format string is not a string literal [-Wformat-nonliteral]
gstrfuncs.c:905:33: warning: format string is not a string literal [-Wformat-nonliteral]
gthreadedresolver.c:410:53: warning: format string is not a string literal [-Wformat-nonliteral]
gutils.c:2282:39: warning: format string is not a string literal [-Wformat-nonliteral]
test-printf.c:192:31: warning: format string is not a string literal [-Wformat-nonliteral]
test-printf.c:197:31: warning: format string is not a string literal [-Wformat-nonliteral]

Current list of gcc warnings:
gcontenttype.c:428:5: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
gcontenttype.c:431:3: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
gcontenttype.c:433:5: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
gdate.c:2497:7: warning: format not a string literal, format string not checked [-Wformat-nonliteral]
gstrfuncs.c:905:3: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
gthreadedresolver.c:410:7: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
gutils.c:2282:7: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
test-printf.c:192:3: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]
test-printf.c:197:3: warning: format not a string literal, argument types not checked [-Wformat-nonliteral]

Created attachment 233970 [details] [review]
Fix trivial non literal format uses

Trivial fixes for non literal usage in both gcc and clang. With this patch, the following uses remain:

gdate.c:2497
generates a format string to strftime using g_locale_from_utf8

gmarkup.c:2432
generates a custom g_string with a format based from the input format

gmarkup.c:2439
same as above

gstrfuncs.c:905
forwards the format

gutils.c:2282
uses format generated from g_dngettext

These should be checked by glib devs.

Any reviews expected for this patch ?

Review of attachment 233970 [details] [review]:

::: gio/gcontenttype.c
@@ +426,3 @@
   G_UNLOCK (gio_xdgmime);
   if (xdg_icon != NULL)
+    xdg_mimetype_icon = g_strdup_printf ("%s%s", xdg_icon, suffix);

this would be nicer as g_strconcat (xdg_icon, suffix, NULL)

@@ +429,3 @@
   xdg_mimetype_generic_icon = g_content_type_get_generic_icon_name (type);
 
+  mimetype_icon = g_strdup_printf ("%s%s", type, suffix);

Same here

@@ +432,2 @@
   if (xdg_mimetype_generic_icon)
+    generic_mimetype_icon = g_strdup_printf ("%s%s", xdg_mimetype_generic_icon, suffix);

And here

::: gio/gthreadedresolver.c
@@ +395,3 @@
           errnum = G_RESOLVER_ERROR_NOT_FOUND;
+          g_set_error (error, G_RESOLVER_ERROR, errnum,
+                       _("No DNS record of the requested type for '%s'"), rrname);

If we are going this way, please get the errnum as well.

*** Bug 701747 has been marked as a duplicate of this bug. ***

Just as an FYI, I just filed a bug on clang (rdar://14265147, in case anybody aboard the mothership is reading this); it mishandles the case where, if you have a test.h header file that contains

extern char *g_markup_vprintf_escaped(const char *format, va_list args)
    __attribute__((__format__(__printf__, 1, 0)));

extern char *g_strdup_vprintf(const char *format, va_list args)
    __attribute__((__format__(__printf__, 1, 0)));

and a test.c source file that contains

#include <stdlib.h>
#include <stdarg.h>
#include <stdio.h>

#include "test.h"

char *
g_strdup_vprintf_escaped(const char *format, va_list args)
{
        return g_strdup_vprintf(format, args);
}

char *
g_strdup_vprintf(const char *format, va_list args)
{
        char *result;

        result = malloc(1024);
        if (result == NULL)
                return NULL;
        vsnprintf(result, 1024, format, args);
        return result;
}

and try to compile it with "clang -c -Wall -W -Wformat-nonliteral test.c", you get:

test.c:10:26: warning: format string is not a string literal
      [-Wformat-nonliteral]
        return g_strdup_vprintf(format, args);
                                ^~~~~~

but *don't* get a warning about the vsnprintf() call.

You can probably guess from the routine names in the test files why I filed this bug :-); it causes

gmarkup.c:2432:31: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  output1 = g_strdup_vprintf (format1->str, args);
                              ^~~~~~~~~~~~
gmarkup.c:2439:31: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  output2 = g_strdup_vprintf (format2->str, args2);
                              ^~~~~~~~~~~~
2 errors generated.

when trying to build GLib.

So even if you fix all *your* issues, you may still run across this clang issue.  (I found it on OS X, but I suspect it applies on all platforms.)

(In reply to comment #24)
> Just as an FYI, I just filed a bug on clang (rdar://14265147, in case anybody
> aboard the mothership is reading this); it mishandles the case where, if you
> have a test.h header file that contains

...a typo, it *correctly* reports a problem.  The bug has been closed.

The problem here is different - clang doesn't know that the transformed string will take the same argument list as the input string.

You might have to move the transformation into a routine and tag that routine with __attribute__(format_arg) in order to squelch that warning.

The list of format-nonliteral errors with clang 5.0 is:
glib/gfileutils.c:1023
glib/gstrfuncs.c:912
glib/gutils.c:2280
gio/gthreadedresolver.c:546 & 575
gio/gcontenttype.c:428, 431, & 433

Colin has proposed a fix for the first one in bug 702516 by using #pragma GCC diagnostic ignore; unless those functions can be reworked, I think that's the only way forward.

I don't think that these can be fixed with __attribute__((__format__)) or __attribute__((__format_arg__)); at least I wasn't able to get the compiler to accept either of them on any of them.

This code in test-printf.c is causing some problems with clang on FreeBSD 10.0-BETA4, for somewhat obvious reasons:

  /* gcc emits warnings for the following formats, since the C spec
   * says some of the flags must be ignored. (The " " in "% +d" and
   * the "0" in "%-03d".) But we need to test that our printf gets
   * those rules right. So we fool gcc into not warning.
   */
  fmt = "% +d";
  res = g_snprintf (buf, 128, fmt, 5);
  g_assert_cmpint (res, ==, 2);
  g_assert_cmpstr (buf, ==, "+5");

  fmt = "%-03d";
  res = g_snprintf (buf, 128, fmt, -5);
  g_assert_cmpint (res, ==, 3);
  g_assert_cmpstr (buf, ==, "-5 ");

At a high level I don't mind too much backing off the -Werror=format family by default.

But it will conflict with both Debian and now Fedora doing -Werror=format-security by default, increasing the pain for those packagers.

fwiw, -Wformat-security would not be concerned about this issue.  It only finds the:

  printf(foo);

case, and the manpage mentions that it "is current a subset of what -Wformat-nonliteral warns about" but then goes on to mention that new warnings may be added in the future.

As with anything, changing meaning of -Wflags mixed with -Werror is a bridge we have to cross when we get there...

Created attachment 263769 [details] [review]
configure: back off -Werror on -Wformat=2

Don't treat non-literal format strings as hard errors.

We have a testcase that uses non-literal format strings like so:

  fmt = "...";
  printf (fmt, ...);

which GCC is clever enough to interpret as a literal, but clang is not
(and strictly speaking, it seems that clang is in the right here).
Stepping back the error to a warning lets the build proceed under clang.

fwiw, Henrique's patch to add the pragma to that specific testcase might actually be better.  I didn't notice it before.

Created attachment 263771 [details] [review]
[PATCH] Fix trivial non literal format uses

Based on a patch from Henrique Dante de Almeida <hdante@gmail.com>.


This addresses the comments in the review, as well as changing the
WIN32 path in the threaded resolver to be the same as the UNIX one is
now (which I tested by building under mingw32).

Review of attachment 263771 [details] [review]:

All looks good to me.

Attachment 263771 [details] pushed as ddf82a2 - [PATCH] Fix trivial non literal format uses