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 660338 - test_size_of_struct_with_array_of_anon_unions fails on powerpc/armel
test_size_of_struct_with_array_of_anon_unions fails on powerpc/armel
Status: RESOLVED FIXED
Product: gobject-introspection
Classification: Platform
Component: general
1.30.x
Other Linux
: Normal normal
: ---
Assigned To: Martin Pitt
gobject-introspection Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2011-09-28 07:53 UTC by Martin Pitt
Modified: 2015-02-07 16:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Account for padding in struct size check (1.71 KB, patch)
2011-09-28 08:29 UTC, Martin Pitt
reviewed Details | Review
Tests: Expand RegressTestStructE to be 32 bytes to avoid padding (3.05 KB, patch)
2011-09-29 19:00 UTC, Colin Walters
none Details | Review

Description Martin Pitt 2011-09-28 07:53:54 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.
Comment 2 Martin Pitt 2011-09-28 08:24:44 UTC
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.
Comment 3 Martin Pitt 2011-09-28 08:29:51 UTC
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?
Comment 4 Torsten Schoenfeld 2011-09-28 19:37:40 UTC
Yes, the patch looks fine to me.  Thanks for tracking this down.
Comment 5 Martin Pitt 2011-09-29 11:51:34 UTC
Johan ack'ed in #introspection. Pushed to master and gnome-3-2 branch.
Comment 6 Colin Walters 2011-09-29 13:04:19 UTC
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.
Comment 7 Colin Walters 2011-09-29 13:04:42 UTC
Argh, reopening.
Comment 8 Martin Pitt 2011-09-29 15:25:35 UTC
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.
Comment 9 Torsten Schoenfeld 2011-09-29 18:41:26 UTC
(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.
Comment 10 Colin Walters 2011-09-29 19:00:41 UTC
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.
Comment 11 Colin Walters 2011-09-29 19:02:37 UTC
Can you test Martin?
Comment 12 Martin Pitt 2011-09-30 04:35:09 UTC
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"?
Comment 13 Torsten Schoenfeld 2011-09-30 06:40:00 UTC
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?
Comment 14 Colin Walters 2011-09-30 13:38:55 UTC
(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.
Comment 15 Colin Walters 2011-09-30 14:44:15 UTC
(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.
Comment 16 Torsten Schoenfeld 2011-09-30 19:42:00 UTC
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.
Comment 17 Colin Walters 2011-10-03 14:17:06 UTC
Thanks for your attention on this Torsten!  I've readded the test.
Sorry about that =)
Comment 18 André Klapper 2015-02-07 16:44:15 UTC
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]