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 700157 - Vala-generated code leaks internal symbols
Vala-generated code leaks internal symbols
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Code Generator
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: Vala maintainers
Vala maintainers
: 669531 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-05-12 10:21 UTC by Maciej (Matthew) Piechotka
Modified: 2014-03-24 20:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-Hide-internal-methods-in-ABI.patch (32.11 KB, patch)
2013-05-12 10:21 UTC, Maciej (Matthew) Piechotka
needs-work Details | Review
0001-Hide-internal-methods-in-ABI.patch (28.83 KB, patch)
2013-05-14 22:15 UTC, Maciej (Matthew) Piechotka
none Details | Review
0001-Hide-internal-methods-in-ABI.patch (25.02 KB, patch)
2013-05-15 09:51 UTC, Maciej (Matthew) Piechotka
none Details | Review
0001-Hide-internal-methods-in-ABI.patch (23.69 KB, patch)
2013-08-16 19:46 UTC, Maciej (Matthew) Piechotka
none Details | Review
0001-Hide-internal-methods-in-ABI.patch (23.77 KB, patch)
2013-08-18 13:51 UTC, Maciej (Matthew) Piechotka
none Details | Review
Hide internal methods in ABI (26.72 KB, patch)
2013-08-21 18:32 UTC, Maciej (Matthew) Piechotka
needs-work Details | Review
Hide internal methods in ABI (26.59 KB, patch)
2013-08-21 20:11 UTC, Maciej (Matthew) Piechotka
none Details | Review
Hide internal methods in ABI (26.63 KB, patch)
2014-02-15 14:35 UTC, Maciej (Matthew) Piechotka
needs-work Details | Review
Hide internal methods in ABI (26.58 KB, patch)
2014-03-22 14:45 UTC, Maciej (Matthew) Piechotka
none Details | Review

Description Maciej (Matthew) Piechotka 2013-05-12 10:21:45 UTC
Created attachment 243893 [details] [review]
0001-Hide-internal-methods-in-ABI.patch

Vala generated code leaks symbols - if symbols is accessable from another compilation unit but it is internal it has public visibility. This creates some minor optimization problems as the function gets the PLT entry:

 - The compiler cannot optimize the code as the code might be overloaded by LD_PRELOAD. Marking symbol as internal or hidden disallows it
 - Each call results into call into PLT
 - The additional entries enlarge the relocation table slowing down startup and lookups
 - Deleting internal symbol will be visible outside and it makes using automatic ABI checkers harder

For example following code:

void f() __attribute__((visibility("hidden")));
void f() {}

void g() {}

static void h() {}

void test() {
    f();
    g();
    h();
}

Results in following assembly for test (simplified) compiled with -fPIC -O2:

test:
	xorl	%eax, %eax
	jmp	g@PLT

Please note that g call in test was not optimized as compiler don't know the code at compile time. There are even more missed optimization opportunities with LTO.

The patch attached marks all methods as hidden allowing the compiler to optimize the code (visibility internal disallows calls from outside assembly even by pointer). Please note that the fields support is missing.
Comment 1 Luca Bruno 2013-05-14 20:07:17 UTC
Review of attachment 243893 [details] [review]:

Thanks for the patch. What about using G_GNUC_INTERNAL?

::: codegen/valaccodebasemodule.vala
@@ +1628,3 @@
 				// accessor function should be private if the property is an internal symbol or it's a construct-only setter
 				function.modifiers |= CCodeModifiers.STATIC;
+			} else if (prop.is_private_symbol () || acc.access == SymbolAccessibility.INTERNAL) {

Why did you use is_private_symbol here?
Comment 2 Maciej (Matthew) Piechotka 2013-05-14 20:12:20 UTC
(In reply to comment #1)
> Review of attachment 243893 [details] [review]:
> 
> Thanks for the patch. What about using G_GNUC_INTERNAL?
> 

Because I missed it while searching the header files. I'll change it to behave as static is behaving right now.

> ::: codegen/valaccodebasemodule.vala
> @@ +1628,3 @@
>                  // accessor function should be private if the property is an
> internal symbol or it's a construct-only setter
>                  function.modifiers |= CCodeModifiers.STATIC;
> +            } else if (prop.is_private_symbol () || acc.access ==
> SymbolAccessibility.INTERNAL) {
> 
> Why did you use is_private_symbol here?

This is a typo.
Comment 3 Maciej (Matthew) Piechotka 2013-05-14 22:15:54 UTC
Created attachment 244252 [details] [review]
0001-Hide-internal-methods-in-ABI.patch

Updated
Comment 4 Luca Bruno 2013-05-15 07:31:57 UTC
Review of attachment 244252 [details] [review]:

What are valaccodefile.vala changes and valaccodemacroconditional.vala used for?
Comment 5 Maciej (Matthew) Piechotka 2013-05-15 09:51:58 UTC
Created attachment 244291 [details] [review]
0001-Hide-internal-methods-in-ABI.patch

(In reply to comment #4)
> Review of attachment 244252 [details] [review]:
> 
> What are valaccodefile.vala changes and valaccodemacroconditional.vala used
> for?

Ups. Sorry - leftovers from previous iteration. Fixed
Comment 6 Maciej (Matthew) Piechotka 2013-08-16 19:46:49 UTC
Created attachment 251937 [details] [review]
0001-Hide-internal-methods-in-ABI.patch

Fix the FIXME & correct the rest - external declaration does not need attribute.
Comment 7 Maciej (Matthew) Piechotka 2013-08-18 13:51:17 UTC
Created attachment 252116 [details] [review]
0001-Hide-internal-methods-in-ABI.patch

Small fix - one if was a bit too eager in pruning symbols.
Comment 8 Luca Bruno 2013-08-20 07:23:14 UTC
What about a valac --hide-internal-symbols ? Otherwise we break applications that rely on the current behavior (like for example calling functions from C, or using gmodule).
Comment 9 Maciej (Matthew) Piechotka 2013-08-20 07:57:39 UTC
(In reply to comment #8)
> What about a valac --hide-internal-symbols ? Otherwise we break applications
> that rely on the current behavior (like for example calling functions from C,
> or using gmodule).

I don't think that we need to preserve compatibility with equivalent of undefined behaviour. It is possible to get access to those functions but they are not present in headers and are marked as internal. Accessing them by either defining them in C or by GModule is IMHO equivalent of accessing private members of class[1][2].  It would work but it is inherently a hack as the method was already marked as 'internal' hence expressing explicit intend of library author that symbol should not be accessed from outside library. At the end the compatibility needs to be defined in term of defined behaviour of API and ABI[3].

I can add the switch to the patch but I don't believe it to be a good thing. Had the author wanted to expose the symbol (s)he should mark it as public. As (s)he marked it as internal (s)he might believe (s)he is free to change it anyway without affecting the API/ABI so the application depending on it is broken anyway.

[1] Changing private members of class would break the program:
// Vala library
class Test {
    private int i;
}

//// C program

struct _TestPrivate {
    gint i;
}

gint get_i (Test *t) {
    return t->priv->i;
}

[2] Mandatory XKCD comics: http://xkcd.com/1172/
[3] Also mentioned in Drepper's text on writing shared libraries - http://people.redhat.com/drepper/dsohowto.pdf - in chapter 3 "Maintaining APIs and ABIs", section 3.2 "Defining Stability".
Comment 10 Luca Bruno 2013-08-20 08:00:20 UTC
No the point is that current applications must not break. I'm not going to accept the patch otherwise sorry, even if it's not correct to not hide symbols or it's a hack or whatelse.
We have to keep compatibility whenever it's possible, and in this case is possible.
Comment 11 Maciej (Matthew) Piechotka 2013-08-20 12:00:38 UTC
(In reply to comment #10)
> No the point is that current applications must not break. I'm not going to
> accept the patch otherwise sorry, even if it's not correct to not hide symbols
> or it's a hack or whatelse.
> We have to keep compatibility whenever it's possible, and in this case is
> possible.

I am not sure if current Vala policy is a good one in long run. On one hand Vala does not guarantee that ABI would be preserved for libraries so we don't have benefits of doing so on another Vala doesn't change it losing the benefits of not guaranteeing it.

Having said that I do see your point and I will add the switch if it is requested by Vala maintainer (i.e. you). However it might be good to have a policy what is and what is not guaranteed and how the deprecation and default switch is handled.

(In reply to comment #9)
> (In reply to comment #8)
> Accessing them by either defining them in C

To clarify - defining them manually by user from outside assembly (i.e. program). Linking the Vala and C code into shared library would not be affected in any way by switch.
Comment 12 Luca Bruno 2013-08-20 17:56:44 UTC
To sum up, is there any even remote possibility that hiding internal symbols breaks any existing application (regardless of whether the application is doing something wrong)? If yes, the switch is required.
Comment 13 Maciej (Matthew) Piechotka 2013-08-20 20:39:58 UTC
I will update the patch by tomorrow with switch. The change is of course trivial but I'm not sure if the additional complexity of interaction is warranted. At the end decision is yours.

(In reply to comment #12)
> To sum up, is there any even remote possibility that hiding internal symbols
> breaks any existing application (regardless of whether the application is doing
> something wrong)? If yes, the switch is required.

Possibility - yes, chance - no. My guess would be that all people who have sufficient knowledge to do so would not do so. Furthermore such application must expect the breakage if not from Vala then from library itself when it changes the internal functions without care for such application or even knowledge about it.

I'd guess many developers (including myself) would push commit changing (adding/removing/changing meaning) the internal symbol in patch release if it fixed the problem. At least I in libgee don't care for maintaining internal ABI and I know that Phillip wanted to limit the 'internal' leaking so I'd say folks folks have similar attitude. I can't speak for Rygiel and GXml but I'd be surprised if they'd support using internal symbols. I could ask Zeeshan and Richard if you wish.

My guess is that while technically one could imagine (or even write) a program that would be broken by this patch a) I cannot imagine what the program would want to achieve as most of the internal functions are 'boring' b) the program might be broken by a next release of library anyway with maintainer not caring anyway c) I would strongly suspect that no such program was written in Vala anyway.
Comment 14 Luca Bruno 2013-08-20 21:03:45 UTC
I agree that there will be no such a program/library, as well as I don't think adding a switch to any build system will be that problematic, it's basically free for us to keep the compatibility.
If anybody cares about hiding internal symbols like you, (s)he will find the switch useful.
Anyway there's no hurry, if you want to wait some other opinion like from Jurg.
Comment 15 Jürg Billeter 2013-08-21 07:20:40 UTC
Unit tests in some projects use internal symbols (probably at least one of folks or rygel do this, iirc). Make sure that this change does not break those unit tests.
Comment 16 Maciej (Matthew) Piechotka 2013-08-21 10:57:13 UTC
(In reply to comment #15)
> Unit tests in some projects use internal symbols (probably at least one of
> folks or rygel do this, iirc). Make sure that this change does not break those
> unit tests.

After quick look - folks uses folks-internal library and it doesn't look like they use any type of internal vapi etc. I'll double check it but if anything do then it would be (one of) definite arguments for switch. I'll check it.

(In reply to comment #14)
> I agree that there will be no such a program/library, as well as I don't think
> adding a switch to any build system will be that problematic, it's basically
> free for us to keep the compatibility.

I'm currently looking on code generation to write patch for bug #697777 and, as person unfamiliar with code, I'm a bit taken back by complexity - also created by combination of various options.

> If anybody cares about hiding internal symbols like you, (s)he will find the
> switch useful.

In the ideal world the compiler should just work. The problem is not 'who cares about hiding symbols' - the problem is that having those symbols expose have a runtime cost. While in many cases not caring about performance is a right approach from programmers POV it is not necessary from language/compiler. We would all want a language which would be highier-level and fast - out of box if possible as there will be tyranny of default and programmers won't change the settings even when complaining about speed (in this case it is exaggeration of course but I'm more describing principle).

Of course we also want compatibility so there need to be balance between both. This is probably part of longer discussion which should be moved off the bugzilla (on e-mail, IRC or ML).
Comment 17 Maciej (Matthew) Piechotka 2013-08-21 18:32:22 UTC
Created attachment 252640 [details] [review]
Hide internal methods in ABI
Comment 18 Luca Bruno 2013-08-21 18:37:50 UTC
Oh wait, something you need to know: you can access context from within CCodeFunction with CodeContext.get(), no need for all those context.hide_internal checks in the codegen.
The patch looks good overall, thanks!
Comment 19 Maciej (Matthew) Piechotka 2013-08-21 18:50:41 UTC
(In reply to comment #15)
> Unit tests in some projects use internal symbols (probably at least one of
> folks or rygel do this, iirc). Make sure that this change does not break those
> unit tests.

Rygel and gxml do that (btw - they both fail to compile with valac master due to bug #706521 and bug #706525). I run folks tests successfully except the telepathy and libsocialweb backends which also might or might not contain such trick.

I have added the switch to newest patch.
Comment 20 Maciej (Matthew) Piechotka 2013-08-21 18:52:51 UTC
(In reply to comment #18)
> Oh wait, something you need to know: you can access context from within
> CCodeFunction with CodeContext.get(), no need for all those
> context.hide_internal checks in the codegen.
> The patch looks good overall, thanks!

I'm not sure I follow - how does switching from context.hide_internal to CodeContext.get ().hide_internal changes the code?
Comment 21 Luca Bruno 2013-08-21 19:48:16 UTC
Review of attachment 252640 [details] [review]:

That you can check it in CCodeFunction or CCodeDeclaration directly, but probably the current approach is better.
Also, is G_GNUC_UNUSED needed when hiding symbols?

::: codegen/valaccodestructmodule.vala
@@ +141,3 @@
 			if (st.is_private_symbol ()) {
 				function.modifiers = CCodeModifiers.STATIC;
+			} else if (st.is_internal_symbol ()) {

Here context.hide_internal is missing.

::: codegen/valagtypemodule.vala
@@ +123,3 @@
 				function.attributes = "G_GNUC_UNUSED";
+			} else if (cl.is_internal_symbol ()) {
+				function.modifiers = CCodeModifiers.INTERNAL;

Missing check.

@@ +981,2 @@
 			function.modifiers = CCodeModifiers.STATIC;
+		} else if (cl.is_internal_symbol ()) {

Missing check.
Comment 22 Maciej (Matthew) Piechotka 2013-08-21 20:11:24 UTC
Created attachment 252664 [details] [review]
Hide internal methods in ABI

> That you can check it in CCodeFunction or CCodeDeclaration directly, but
> probably the current approach is better.

I have thought about it before even looking at the CodeContext - my feeling
was that this was highier-level policy code generation policy so it should
belong to codegen instead of ccode. If vala will have any need for creating
internal symbol in future not from user code it will be ready ;)

> Also, is G_GNUC_UNUSED needed when hiding symbols?

Good catch. No as compiler does not now if it will be used.
Comment 23 Maciej (Matthew) Piechotka 2013-12-19 00:11:54 UTC
*** Bug 669531 has been marked as a duplicate of this bug. ***
Comment 24 Maciej (Matthew) Piechotka 2014-02-02 23:14:14 UTC
Last discussion (IRC, 21 Dec):

mpiechotka: juergbi: Could you take a look at bug #700157 (is there anything still needed for the patch)?
Services: Bug http://bugzilla.gnome.org/show_bug.cgi?id=700157 enhancement, Normal, ---, vala-maint, UNCONFIRMED, Vala-generated code leaks internal symbols
Lethalman: juergbi, so it's ok about the syntax? I'm going to review/tweak those patches to be simpler
juergbi: mpiechotka: i don't have time for a closer look right now
juergbi: i was just wondering whether we should default to the new behavior and simply disable it if header or vapi file is generated for the internal symbols
juergbi: or is there a way it could break things for projects that don't generate internal header/vapi?
juergbi: (ignoring hacks to access symbols)

I've noticed that I haven't answered it - I believe it would break only if they wrote header themselves/use extern which is partially a hack. In any case we can add the switch and change the default behaviour later.
Comment 25 Maciej (Matthew) Piechotka 2014-02-15 14:35:56 UTC
Created attachment 269197 [details] [review]
Hide internal methods in ABI
Comment 26 Maciej (Matthew) Piechotka 2014-02-15 14:37:45 UTC
Ups. I've forgot to pass -e.

Updated patch. I guess it might be still possible to have some setups where hiding internals is beneficial while headers are exported for the c files linked within the library.
Comment 27 Luca Bruno 2014-03-18 20:13:46 UTC
Review of attachment 269197 [details] [review]:

Coming back to this patch. It seems fine overall. Just a couple of doubts below for a missing hide_internal check.

::: codegen/valagtypemodule.vala
@@ +152,3 @@
 				// avoid C warning as this function is not always used
 				function.attributes = "G_GNUC_UNUSED";
+			} else if (cl.is_internal_symbol ()) {

context.hide_internal seems to be missing in the check here.

@@ +180,2 @@
 					function.modifiers = CCodeModifiers.STATIC;
+				} else if (cl.is_internal_symbol ()) {

also here
Comment 28 Maciej (Matthew) Piechotka 2014-03-22 14:45:39 UTC
Created attachment 272631 [details] [review]
Hide internal methods in ABI

Sorry for delay - I had a large backlog of emails.
Comment 29 Luca Bruno 2014-03-23 11:55:00 UTC
Review of attachment 272631 [details] [review]:

Only last thing about setting context.hide_internal, overlooked it before :S

::: compiler/valacompiler.vala
@@ +179,3 @@
 		context.checking = enable_checking;
 		context.deprecated = deprecated;
+		context.hide_internal = hide_internal || (internal_header_filename == null);

Can we drop the internal_header_filename == null check ? I'd like to not constraint the user from the choice of not hiding symbols when he doesn't generate an internal header. E.g. the internal header might be hand crafted. If you are ok with it, I can do this change while merging, no need for reupload.
Comment 30 Maciej (Matthew) Piechotka 2014-03-24 09:04:31 UTC
Go ahead. IIRC it was added on someone's request.
Comment 31 Luca Bruno 2014-03-24 20:05:45 UTC
commit 970f58989a2863faca11e30fdbcf4da1273a6acd
Author: Maciej Piechotka <uzytkownik2@gmail.com>
Date:   Sun May 12 11:18:27 2013 +0100

    Hide internal methods in ABI
    
    Fixes bug 700157

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.