GNOME Bugzilla – Bug 700157
Vala-generated code leaks internal symbols
Last modified: 2014-03-24 20:05: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.
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?
(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.
Created attachment 244252 [details] [review] 0001-Hide-internal-methods-in-ABI.patch Updated
Review of attachment 244252 [details] [review]: What are valaccodefile.vala changes and valaccodemacroconditional.vala used for?
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
Created attachment 251937 [details] [review] 0001-Hide-internal-methods-in-ABI.patch Fix the FIXME & correct the rest - external declaration does not need attribute.
Created attachment 252116 [details] [review] 0001-Hide-internal-methods-in-ABI.patch Small fix - one if was a bit too eager in pruning symbols.
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).
(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".
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.
(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.
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.
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.
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.
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.
(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).
Created attachment 252640 [details] [review] Hide internal methods in ABI
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!
(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.
(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?
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.
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.
*** Bug 669531 has been marked as a duplicate of this bug. ***
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.
Created attachment 269197 [details] [review] Hide internal methods in ABI
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.
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
Created attachment 272631 [details] [review] Hide internal methods in ABI Sorry for delay - I had a large backlog of emails.
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.
Go ahead. IIRC it was added on someone's request.
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.