After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 691608 - Support compilation with clang 3.2
Support compilation with clang 3.2
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: build
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 701747 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-01-12 14:48 UTC by Henrique
Modified: 2013-12-08 19:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (947 bytes, patch)
2013-01-12 14:48 UTC, Henrique
rejected Details | Review
0001-Fix-user-CFLAGS-handling-user-overwrites-defaults.patch (835 bytes, patch)
2013-01-13 15:08 UTC, Henrique
committed Details | Review
Add-G_GNUC_PRINTF-on-all-functions-with-format-strin (15.45 KB, patch)
2013-01-13 16:11 UTC, Henrique
committed Details | Review
Fix trivial non literal format uses (3.86 KB, patch)
2013-01-20 23:04 UTC, Henrique
needs-work Details | Review
configure: back off -Werror on -Wformat=2 (1.20 KB, patch)
2013-12-08 18:53 UTC, Allison Karlitskaya (desrt)
none Details | Review
[PATCH] Fix trivial non literal format uses (5.23 KB, patch)
2013-12-08 19:29 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Henrique 2013-01-12 14:48:04 UTC
Created attachment 233303 [details] [review]
patch

Add parameter "-Wno-format-nonliteral" to standard CFLAGS to allow compilation with clang.

Note: this patch is most probably temporary, because it might be a bug in clang.
Comment 1 Allison Karlitskaya (desrt) 2013-01-12 15:35:25 UTC
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?
Comment 2 Henrique 2013-01-12 15:49:02 UTC
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.
Comment 3 Colin Walters 2013-01-12 16:37:29 UTC
What are the compiler warnings it emits?
Comment 4 Henrique 2013-01-12 18:43:27 UTC
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.
Comment 5 Henrique 2013-01-12 21:17:09 UTC
Link to clang bug:
http://llvm.org/bugs/show_bug.cgi?id=14935
Comment 6 Henrique 2013-01-12 22:48:32 UTC
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.
Comment 7 Henrique 2013-01-13 14:54:37 UTC
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).
Comment 8 Henrique 2013-01-13 15:08:14 UTC
Created attachment 233387 [details] [review]
0001-Fix-user-CFLAGS-handling-user-overwrites-defaults.patch
Comment 9 Allison Karlitskaya (desrt) 2013-01-13 15:10:51 UTC
imho this is a pretty stupid 'feature' from clang... Not that we don't have a history of working around broken compilers already, though...
Comment 10 Colin Walters 2013-01-13 15:48:33 UTC
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.
Comment 12 Henrique 2013-01-13 15:59:10 UTC
(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.
Comment 13 Henrique 2013-01-13 16:11:04 UTC
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.
Comment 14 Colin Walters 2013-01-13 17:27:37 UTC
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
Comment 15 Colin Walters 2013-01-13 17:31:12 UTC
Review of attachment 233390 [details] [review]:

This looks correct to me, thanks a lot for the patch!
Comment 16 Henrique 2013-01-13 21:24:36 UTC
(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
Comment 17 Henrique 2013-01-19 19:21:57 UTC
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
Comment 18 Henrique 2013-01-19 23:14:54 UTC
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]
Comment 19 Henrique 2013-01-19 23:15:25 UTC
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]
Comment 20 Henrique 2013-01-20 23:04:13 UTC
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.
Comment 21 Henrique 2013-01-26 10:28:10 UTC
Any reviews expected for this patch ?
Comment 22 Matthias Clasen 2013-03-18 02:55:04 UTC
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.
Comment 23 Daniel Macks 2013-06-06 15:30:48 UTC
*** Bug 701747 has been marked as a duplicate of this bug. ***
Comment 24 Guy Harris 2013-06-26 18:56:43 UTC
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.)
Comment 25 Guy Harris 2013-06-27 19:09:44 UTC
(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.
Comment 26 John Ralls 2013-06-27 21:55:06 UTC
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.
Comment 27 Allison Karlitskaya (desrt) 2013-12-08 00:06:28 UTC
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 ");
Comment 28 Colin Walters 2013-12-08 18:24:44 UTC
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.
Comment 29 Allison Karlitskaya (desrt) 2013-12-08 18:49:15 UTC
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...
Comment 30 Allison Karlitskaya (desrt) 2013-12-08 18:53:05 UTC
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.
Comment 31 Allison Karlitskaya (desrt) 2013-12-08 19:06:17 UTC
fwiw, Henrique's patch to add the pragma to that specific testcase might actually be better.  I didn't notice it before.
Comment 32 Allison Karlitskaya (desrt) 2013-12-08 19:29:06 UTC
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).
Comment 33 Colin Walters 2013-12-08 19:39:03 UTC
Review of attachment 263771 [details] [review]:

All looks good to me.
Comment 34 Allison Karlitskaya (desrt) 2013-12-08 19:50:32 UTC
Attachment 263771 [details] pushed as ddf82a2 - [PATCH] Fix trivial non literal format uses