GNOME Bugzilla – Bug 720437
Vala doesn't cast int to pointer when passing Error-throwing method return value to generic class method
Last modified: 2018-05-22 15:02:32 UTC
I've only been able to reproduce this under specific circumstances, although it might happen under other conditions as well. We were recently notified of a build-policy red flag (in bug #720433) regarding 64-bit portability. (It's also discussed in bug #720433.) After investigation, it looks like Vala is not casting an int to a pointer under certain circumstances. Specifically, if a method that throws an Error returns an int and that int is passed directly to a generic class method (i.e. gee_collection_add), Vala won't cast the int to a pointer. Under most other similar circumstances, it will. I think the method's GError is what triggers this problem. This code works fine: int return_int() { return 1; } void main() { Gee.List<int> list = new Gee.ArrayList<int>(); list.add(return_int()); } generates: gee_collection_add (G_TYPE_CHECK_INSTANCE_CAST (list, GEE_TYPE_COLLECTION, GeeCollection), (gpointer) ((gintptr) _tmp1_)); This code produces a gcc warning and the build-policy violation: int return_int() throws Error { return 1; } void main() { Gee.List<int> list = new Gee.ArrayList<int>(); try { list.add(return_int()); } catch (Error err) { } } generates this code: gee_collection_add (G_TYPE_CHECK_INSTANCE_CAST (list, GEE_TYPE_COLLECTION, GeeCollection), _tmp1_); And these warnings: /home/jim/test/test.vala:43:10: warning: assignment makes integer from pointer without a cast [enabled by default] list.add(return_int()); ^ /home/jim/test/test.vala:43:3: warning: passing argument 2 of ‘gee_collection_add’ makes pointer from integer without a cast [enabled by default] list.add(return_int()); ^ In file included from /home/jim/test/test.c:39:0: /usr/include/gee-1.0/gee.h:827:10: note: expected ‘gconstpointer’ but argument is of type ‘gint’ gboolean gee_collection_add (GeeCollection* self, gconstpointer item); ^ Note that if the return value is stored in a local before being passed to Gee.Collection, the proper code is generated: int i = return_int(); list.add(i);
The bug was originally filed against Geary at bug #720433.
While fixing all the warnings in code is a good thing I don't think it is necessary for generated code to do so as long as it is safe. In this case the warning is because people might have stored an pointer inside an integer, which would cause problem on 64-bit platforms where usually sizeof(int) < sizeof(void *). On the other hand on most architectures storing integer inside pointer is safe[1] so above code is safe. In the end I don't think it's a goal of Vala to produce warning-less code even though it is goal to produce code which works. On the other hand warnings protect against programmers errors. For example the unused variable is often important warning but it would complicated the codegen without reason (the resulting assembly would be the same). (Disclaimer - I'm not a Vala project developer). [1] It's still an UB to store an invalid pointer value inside a pointer so code void *v = (void *)1 is technically incorrect C. However it's used in API all around in GLib so I don't think it will be fixed anytime soon - ideally the intptr_t would be used as universal storage instead of void *.
I'm not reporting this because Vala should produce warning-free code (although I do think that's a worthwhile goal), or even because a tool used by a major distro is flagging the code as a potential bug. I'm reporting this because a slight variation in the Vala code produces different C code in a way that doesn't seem related to the variation. From my examination (without comprehensive understand of the compiler, of course) it looks like Vala should be able to produce uniform code in all of the above examples. In other words, it doesn't seem like the method throwing an error should somehow impede Vala from producing warning-free code, which it's able to do in the other examples.
(In reply to comment #3) > I'm not reporting this because Vala should produce warning-free code (although > I do think that's a worthwhile goal), or even because a tool used by a major > distro is flagging the code as a potential bug. > In most cases production of warning-free code is just notifying the compiler 'yes, I really do want to do this and it's not a typo'. In case of writing code they are often useful ("I no longer need this variable - I can delete it so it won't clutter the code", "oops. I thought I can store pointer in int but what about 64-bit code", etc.). On the other hand in case of autogenerated code: - There is no issue of typo's etc. - The way of dealing with typo's is by silencing all errors, which is effectively the same as passing -w to compiler and forgetting about whole affair. However it comes at cost of clarity in compiler (codegen would need to become needlessly complicated). > I'm reporting this because a slight variation in the Vala code produces > different C code in a way that doesn't seem related to the variation. From my > examination (without comprehensive understand of the compiler, of course) it > looks like Vala should be able to produce uniform code in all of the above > examples. > Why? As long as meaning of the code is preserved the underlaying code is a detail. Comparing with CLang/LLVM - take following functions: int return_int() { return 1; } int t1() { return return_int(); } int t2() { try { return return_int(); } catch (...) { return 0; } } t1 will have straightforward code emitted, while t2 will be 'needlessly' complicated (basic blocks without predecessor, needless allocation, yet end block merged for some reason etc.). If the llvm-as was user-fronting program it WOULD emit warnings about it - but it isn't so it 'make sense' to push those things to llvm-as and/or opt which will do it anyway. On the other hand gcc and clang are user fronting programs. They expect that code is written by human so if it is 'awful' programmer will improve it by clearing it - but for Vala C is intermediary representation - all meaningful warnings should have been emitted already so we are at level 'trust me I know what I'm doing'. And seeing the code of codegen - it's hard to modify as it is without the extra need to produce warning-free code. I simply don't see any tangible benefit to have codegen producing warning-less code (to clarify - I do see benefit to add more warnings to Vala or fix warnings which appear in programs written in C) - at least not compared with extra bookkeeping that would be required. In fact I believe this would need to change the binding syntax as Gtk accepts GtkWidget * as parameter even for methods which expect subclasses - so it would require changes all around the stack... not giving anything as it's just a convenience for C programmers. PS. gee-1.0 is obsolete and unmaintained for 2 years or so. gee-0.8 is a current version (to be more specific 0.12 which uses 0.8 ABI). Unfortunately the naming is less then clear.
I'll say it one more time: I didn't report this bug because of the C warning messages. I feel I was pretty clear about that. I also never said Vala was producing unsafe code or indicate I was confused about why Vala was storing an int inside pointer-sized storage. Yet, even with all that, you felt the need to lecture me on compiler warnings and warning-less code and 64-bit vs. pointer and so on. I don't appreciate it. As for your question ("Why?" as in "Why produce uniform code?"), I'll leave that for the Vala developers to decide. I was merely bringing this situation to their attention. If the Vala devs decide this is a non-issue, they can close the ticket. I'm not demanding a solution or even a patch. And as far as your P.S., I'm well aware of gee-0.8 vs. 1.0. When I compiled the minimal test cases I was executing valac by hand and probably typed in the old Gee version by accident.
(In reply to comment #5) > I'll say it one more time: I didn't report this bug because of the C warning > messages. I feel I was pretty clear about that. I also never said Vala was > producing unsafe code or indicate I was confused about why Vala was storing an > int inside pointer-sized storage. Yet, even with all that, you felt the need > to lecture me on compiler warnings and warning-less code and 64-bit vs. pointer > and so on. I don't appreciate it. > I am really sorry if it sounded like a lecture. It was meant as an argument to "(although I do think that's a worthwhile goal)" - I probably needlessly concentrated on it. Regarding indication - unfortunately many people don't and I jumped to conclusion. Possibly I shouldn't but in Vala community there are people coming from HL languages which are not necessary familiar with C or its details. Many people is not aware of the difference between gee-0.8 and gee-1.0 (and frankly - gee-1.0 does look like newer version).
Jim is correct in that the produced code is inconsistent, regardless of whether we should cast or not. Thanks for the bug.
This require some refactoring to make it work, probably it even requires the wip/transform branch. The problem is that with error throwing bar(), vala splits foo(bar()) into tmp=bar(); foo(tmp) at the ast level. Here tmp is an int, and therefore at the codegen level we lost the necessary information about int->generics conversion. If bar() is not error throwing, foo(bar ()) split is done at the codegen level without affecting the ast. The plan, with wip/transform in particular, is to avoid ast transformations losing information.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/423.