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 697354 - can't be built with -Werror=implicit
can't be built with -Werror=implicit
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2013-04-05 13:33 UTC by Simon McVittie
Modified: 2013-04-12 11:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Re-brand ERROR_CFLAGS to C_ERROR_CFLAGS (4.10 KB, patch)
2013-04-11 11:07 UTC, Simon McVittie
committed Details | Review
Include ERROR_CFLAGS everywhere we compile C code (10.41 KB, patch)
2013-04-11 11:08 UTC, Simon McVittie
committed Details | Review
Declare internal function folks_persona_store_set_is_user_set_default() (3.61 KB, patch)
2013-04-11 11:09 UTC, Simon McVittie
committed Details | Review
Ensure that the C bits of TestCase are pre-declared in a header (2.08 KB, patch)
2013-04-11 11:10 UTC, Simon McVittie
committed Details | Review
Add warnings (normally fatal) on use of archaic C features (2.40 KB, patch)
2013-04-11 11:11 UTC, Simon McVittie
committed Details | Review

Description Simon McVittie 2013-04-05 13:33:54 UTC
Missing declarations for functions are not diagnosed in Folks' build system. As a relic of C's 1970s origins, undeclared functions are "implicitly declared" as taking arguments like a varargs function, and returning int.

This is really bad if you return something larger than int, like a pointer on 64-bit platforms: depending what mood the compiler is in, it might work fine, or you might lose the top half of the bits of the pointer and get an invalid result.

Building every .c file (including those generated by Vala) with -Werror=implicit would diagnose this.

Unfortunately, adding that to CFLAGS reveals that the EDS backend calls the internal function folks_persona_store_set_is_user_set_default() via an implicit declaration.

I suspect the answer might be to change folks_persona_store_set_is_user_set_default from internal to protected?
Comment 1 Simon McVittie 2013-04-05 13:39:21 UTC
For what it's worth, "internal" appears to mean "is in the ABI, but not in the header" - so we're relying on C Folks users' use of -Werror=implicit to avoid things that are not considered to be public API working anyway. I was hoping it did tricks with G_GNUC_INTERNAL, but apparently not.

This appears to be sufficient to fix the build with -Werror=implicit:

-  public bool is_user_set_default { get; internal set; default = false; }
+  public bool is_user_set_default { get; protected set; default = false; }
Comment 2 Philip Withnall 2013-04-05 22:30:26 UTC
(In reply to comment #1)
> For what it's worth, "internal" appears to mean "is in the ABI, but not in the
> header" - so we're relying on C Folks users' use of -Werror=implicit to avoid
> things that are not considered to be public API working anyway. I was hoping it
> did tricks with G_GNUC_INTERNAL, but apparently not.
> 
> This appears to be sufficient to fix the build with -Werror=implicit:
> 
> -  public bool is_user_set_default { get; internal set; default = false; }
> +  public bool is_user_set_default { get; protected set; default = false; }

IIRC, is_user_set_default’s setter was intentionally created as ‘internal’ so that it wasn’t exposed in the header, since we don’t want client programs accidentally calling it. At the time I assumed that ‘internal’ visibility did some tricks with G_GNUC_INTERNAL, and didn’t look into it further.

The problem with changing it to ‘protected’ visibility is that it’s equivalent to ’public’ visibility as far as C code is concerned. We were trying to avoid that, since it seems unnecessary.

I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL properly. I’d prefer to see that than make is_user_set_default a protected setter.

In the mean time, please feel free to commit a change to folks which uses -Werror=implicit everywhere except in the EDS backend. I guess it would be best to add -Werror=implicit to (a new) ERROR_CFLAGS, and add -Wno-error=implicit (with a FIXME) to the EDS backend’s Makefile.am.
Comment 3 Simon McVittie 2013-04-11 10:14:37 UTC
(In reply to comment #2)
> I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL
> properly. I’d prefer to see that than make is_user_set_default a protected
> setter.

Sorry, that won't work. G_GNUC_INTERNAL means "local to this shared library", so libfolks-eds can't see G_GNUC_INTERNAL symbols from libfolks.

To have a symbol that is visible to libfolks-eds but not to "the general public", it would have to be ABI but not API: in other words, not G_GNUC_INTERNAL, but not in an installed header either. (folks_small_set_new() and folks_internal_equal_sets() are examples of such symbols.)

I'm not sure that is_user_set_default being internal actually gains us much in terms of symbol visibility, because as far as I can see, a library user can equally easily go via GObject properties (which have no visibility or access control):

    g_object_set (store,
        "is-user-set-default", TRUE,
        NULL);

(To prevent this it would have to be read-only or construct-only at the GObject level, but I don't think Vala gives us that much control - "internal set" seems to be translated into G_PARAM_WRITABLE.)
Comment 4 Simon McVittie 2013-04-11 10:29:45 UTC
(In reply to comment #2)
> I think the proper fix would be to improve Vala to use G_GNUC_INTERNAL
> properly.

That's Bug #669531, although if it's implemented, we'd have to stop using "internal" among different binaries from the same source tree. See also Bug #694809.

(In reply to comment #3)
> I'm not sure that is_user_set_default being internal actually gains us much in
> terms of symbol visibility, because as far as I can see, a library user can
> equally easily go via GObject properties (which have no visibility or access
> control):
> 
>     g_object_set (store,
>         "is-user-set-default", TRUE,
>         NULL);

That's Bug #653909 (but you already know that, since you reported it).
Comment 5 Simon McVittie 2013-04-11 11:07:43 UTC
Created attachment 241236 [details] [review]
Re-brand ERROR_CFLAGS to C_ERROR_CFLAGS

These are flags that we only use when compiling hand-written C code,
mostly originating from telepathy-glib. We can't be that strict with
the C code generated by Vala, because it's full of questionable things
(which the Vala compiler verifies are OK, but the C compiler still warns
about).
Comment 6 Simon McVittie 2013-04-11 11:08:07 UTC
Created attachment 241237 [details] [review]
Include ERROR_CFLAGS everywhere we compile C code

AM_CFLAGS was missing from a couple of foo_CFLAGS variables (it isn't
"inherited" automatically), so add that too.
Comment 7 Simon McVittie 2013-04-11 11:09:16 UTC
Created attachment 241238 [details] [review]
Declare internal function  folks_persona_store_set_is_user_set_default()

As explained in the comment, I can't find any better way to do this.
Implicit declarations are Bad, so we should be making them into errors
by default.

---

See the (long) comment in the code. I am not very happy with this but I can't see a way to do better.

See also Bug #694809.
Comment 8 Simon McVittie 2013-04-11 11:10:57 UTC
Created attachment 241239 [details] [review]
Ensure that the C bits of TestCase are pre-declared in a  header

This lets us verify that their Vala and C ABIs are the same: if they
are not, compilation will fail.

---

Caught by -Werror=missing-declaration, which checks that every function has a prior declaration (typically in a header file; but you can just declare it repetitively in its own .c file, like valac does, if you're desperate).
Comment 9 Simon McVittie 2013-04-11 11:11:14 UTC
Created attachment 241240 [details] [review]
Add warnings (normally fatal) on use of archaic C  features

We don't want these even in C code generated by Vala, and certainly not
in hand-written C code.
Comment 10 Philip Withnall 2013-04-11 20:19:03 UTC
Review of attachment 241236 [details] [review]:

++
Comment 11 Philip Withnall 2013-04-11 20:20:11 UTC
Review of attachment 241237 [details] [review]:

++
Comment 12 Philip Withnall 2013-04-11 20:26:21 UTC
Review of attachment 241238 [details] [review]:

I guess this is a reasonable solution, if a little onerous to maintain.

::: folks/redeclare-internal-api.h
@@ +30,3 @@
+ * header or the "public" one, never both. If we use the "internal"
+ * VAPI then libfolks-eds' "public" header ends up trying to include
+ * the "internal" header of libfolks, which is unacceptable.

I suppose there’s scope for Vala to grow a --internal-header-without-typedefs option which doesn’t contain redefinitions of public typedefs.

@@ +42,3 @@
+
+void folks_persona_store_set_is_user_set_default (FolksPersonaStore* self,
+    gboolean value);

Probably a good idea to add a comment in persona-store.vala pointing here.
Comment 13 Philip Withnall 2013-04-11 20:27:41 UTC
Review of attachment 241239 [details] [review]:

++
Comment 14 Philip Withnall 2013-04-11 20:28:29 UTC
Review of attachment 241240 [details] [review]:

++ \o/
Comment 15 Simon McVittie 2013-04-12 11:29:19 UTC
(In reply to comment #3)
> To have a symbol that is visible to libfolks-eds but not to "the general
> public", it would have to be ABI but not API: in other words, not
> G_GNUC_INTERNAL, but not in an installed header either. (folks_small_set_new()
> and folks_internal_equal_sets() are examples of such symbols.)

Side note on this: I do intend it to be possible for SmallSet and its unfinished relatives SmallMap and SmallMultiMap to be API one day (although it seems to be necessary to have them in their own .vapi for that to work) so that out-of-tree backends can use them, which is why small-set-internal.h exists.

However, it doesn't seem urgent to have them be API yet (and having them as non-API gives us the flexibility to change constructor parameters, etc. if desired). In particular, if they do become API I'd want to disable separate headers when FOLKS_COMPILATION is not defined, and add a meta-header <folks/folks-generics.h> or something, analogous to <telepathy-glib/telepathy-glib.h>.
Comment 16 Simon McVittie 2013-04-12 11:35:02 UTC
Everything applied, with a comment added as you suggested:

+  /* The setter folks_persona_store_set_is_user_set_default() is redeclared
+   * in folks/redeclare-internal-api.h so that libfolks-eds can use it.
+   * If you alter this property, check the generated C and update that
+   * header if necessary. https://bugzilla.gnome.org/show_bug.cgi?id=697354 */
   /**
    * Whether this {@link PersonaStore} has been marked as the default
    * store (in its backend) by the user. I.e.: a PersonaStore for the e-d-s