GNOME Bugzilla – Bug 660338
test_size_of_struct_with_array_of_anon_unions fails on powerpc/armel
Last modified: 2015-02-07 16:44:15 UTC
Version 1.30 now has a test suite failure on armel and powerpc: Successfully found GCancellable PASS: gitestrepo PASS: gitestthrows ** ERROR:./gitypelibtest.c:124:test_size_of_struct_with_array_of_anon_unions: assertion failed: (g_struct_info_get_size (struct_info) == sizeof (GType) + 2*sizeof (gint64)) /bin/bash: line 5: 18738 Aborted env GI_TYPELIB_PATH=../..:../../gir:../../tests:../../tests/scanner: XDG_DATA_DIRS="../../gir:" ${dir}$tst FAIL: gitypelibtest ============================================================================================= 1 of 3 tests failed Please report to http://bugzilla.gnome.org/enter_bug.cgi?product=glib&component=introspection ============================================================================================= make[6]: *** [check-TESTS] Error 1 1.29.17 still built fine everywhere. I'm investigating this now.
This was introduced with the fix for bug 657040: http://git.gnome.org/browse/gobject-introspection/commit/?id=ea68ec234ad31b0a1d92adbf0c911a5dd541b647
Using g_assert_cmpint() instead of g_assert() shows ERROR:./gitypelibtest.c:124:test_size_of_struct_with_array_of_anon_unions: assertion failed (g_struct_info_get_size (struct_info) == sizeof (GType) + 2*sizeof (gint64)): (24 == 20) I. e. the compiler added some extra padding there. This is harmless, of course, but breaks the test.
Created attachment 197637 [details] [review] Account for padding in struct size check Simple patch. Torsten, does this look acceptable to you? OK to push this?
Yes, the patch looks fine to me. Thanks for tracking this down.
Johan ack'ed in #introspection. Pushed to master and gnome-3-2 branch.
Review of attachment 197637 [details] [review]: Two comments: 1) Why are we not just using sizeof (TestStructE) here? 2) We could add another 64 bit value, then it'd be a nice even 32 bytes (as opposed to 24) which I doubt any platform would pad.
Argh, reopening.
1) I'm not sure about this, I'll leave this question to Torsten. 2) Fine for me. I can propose patch for this and test it on i386, x86_64, armel, and powerpc if you want.
(In reply to comment #6) > 1) Why are we not just using sizeof (TestStructE) here? I tried this back then, I think. The problem was that, if we don't want to duplicate the structure definition, we'd have to pull in regress.h, which needs cairo.h. The tests' Makefile was not set up to handle that.
Created attachment 197798 [details] [review] Tests: Expand RegressTestStructE to be 32 bytes to avoid padding At 24 bytes it's possible some architectures add padding, and this creates a problem since we want to assert on the size of the structure. Expand it to 32, which shouldn't be padded.
Can you test Martin?
On powerpc this now fails with ERROR:./gitypelibtest.c:124:test_size_of_struct_with_array_of_anon_unions: assertion failed (g_struct_info_get_size (struct_info) == sizeof (GType) + 3*sizeof (gint64)): (32 == 28) It seems sizeof(GType) is just 4 bytes? This is actually consistent with comment 2. It does work on x86_64, so I suppose GType is a "long int"?
Maybe this test is just not worth all this hassle. That the scanner understands the array of union is tested with Regress-1.0-expected.gir. That the struct size calculation is done correctly is tested with the g_struct_info_get_size (GValue) == sizeof (GValue) assertion. So the TestStructE test is redundant. Let's just remove it?
(In reply to comment #13) > Let's just remove it? Well there's actually already a large number of tests specifically for g_struct_info_get_size() versus sizeof() in tests/offsets written by Owen. We could just add this exact struct there. I'll do that.
(In reply to comment #14) > (In reply to comment #13) > > > Let's just remove it? > > Well there's actually already a large number of tests specifically for > g_struct_info_get_size() versus sizeof() in tests/offsets written by Owen. We > could just add this exact struct there. That turns out to be quite hard because we use regexps to parse the C source =/ Oh well, let's just remove it per comment 13.
Oh, but now you also removed the sizeof (GValue) test: - struct_info = g_irepository_find_by_name (repo, "GObject", "Value"); - if (!struct_info) - g_error ("Could not find GObject.Value"); - g_assert_cmpuint (g_struct_info_get_size (struct_info), ==, sizeof (GValue)); - g_base_info_unref (struct_info); That's the bit I referred to when I said: | That the struct size calculation is done correctly is tested with the | g_struct_info_get_size (GValue) == sizeof (GValue) assertion.
Thanks for your attention on this Torsten! I've readded the test. Sorry about that =)
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]