Hans, can you check HEAD again?  I don't claim I understand what that error message means, but I fixed break.c to initialize the struct by leaving it to compiler to clear all members.

About being a ABI change, that's up for discussion.  As for API change, it definitely is API change, but one that I expect not breaks many apps, if any.

Created attachment 71052 [details] [review]
Patch to make it at least compile

The ABI change breaks every GDK using application on win32. I hope this is not up to discussion. At least until now the ABI guarantees of GTK+ were very strong.

But maybe I just was not clear enough in describing the problem. From my understanding the bitfield packaging rules don't allow to resuse some 24 bits left between the guin8 and the following pointer. Instead the new PangoGravity field is aligned at the same place where the language pointer used to be.
Probably because there is some rule with the (msvc) compiler to not share padding between different sized types.

One could probably mess around with something like:

guint level : 8;
guint gravity : 8;

But I think the solution I originally proposed is cleaner; at least I do not know if the bitfields games would not be binary incompatible on different endianness ...

BTW: initializing with empty braces seems GCC specific as well.

Given that the main point of this bug is breaking ABI - still unsolved - I'm reopening.

Hans, your patch contains a lot of noise.  Can you update your cvs checkout and attach a better patch please?

What I see in the patch:

  - Replacing {} with {0}.  Fair enough.  Will pick.

  - Replacing M_PI with G_PI.  Which standard is G_PI defined in?  That doesn't work on gcc, and not found in 'man math.h'.  We should define M_PI and friends if not defined already.


With the above change, are you still seeing any problem?  Does it compile?  On gcc I double checked that ABI is not changed, as sizeof (PangoAnalysis) didn't change after adding new fileds (24).  It may be different on other systems though, and I have a vague memory that bitfields may start word-aligned.  Got to check the standard (not that the standard means much here).  Can you check sizeof (PangoAnalysis) before/after?  I think using bitfield for level is what I'll do.  I will try to check on big/little endian machines and verify that both still work.

If I do go on and use "guint gravity : 8" (infact I'll allocate only 3 bits in that case), doesn't MSVC err converting between guint and PangoGravity?  Does such a thing need a cast in C++?  Does MSVC compile in C mode?

Thanks

Hans is saying that it *is different*. No ifs, ands buts. That's the
main point of this bug.

The "lot of noise" is not a matter of cvs update, but cvs commit (just done):

2006-08-17  Hans Breuer  <hans@breuer.org>

	* pango/makefile.msc pango/pango.def : updated
	* pango/break.c(1630) : error C2059: syntax error : '}' Intializing a 
	struct with empty braces is not supported with MSVC, instead use 0.
	* pango/pangocairo.def : removed pango_fc_font_(map_)get_type
	pangocairo has either win32 fonts or freetype. The former is more usual.
	* pango/pango-utils.c : replaced M_PI by G_PI (from glib/gtypes.h).
	
The ABI breakage remains. And it will if the code in question is not changed. I'm not doing an abstract discussion here, but have compiled and tested both variants.
Your bitset-after-guint8 hack crashes in gtk+/gdk/gdkpango.c(gdk_pango_renderer_prepare_run).


> If I do go on and use "guint gravity : 8" (infact I'll allocate only 3 bits in
> that case), 
with "guint level : 3" you'll still change ABI, as tried to outline above.

> doesn't MSVC err converting between guint and PangoGravity?  Does
> such a thing need a cast in C++?  Does MSVC compile in C mode?
Converting from enum to int is always possible no matter if C or C++. The other way around requires a cast in C++. 
The MSVC build of Pango is in "C mode" but with some warnings turned to errors. 
See glib/msvc_recommended_pragmas.h

The following patch does indeed fix the crash. If it keeps the ABI on all Pango supporting platforms I'm not sure (again see above: "endianness").

diff --exclude-from=c:\util\tool\diff.ign -u --recursive from-cvs/pango/pango/pango-item.h my-gtk/pango/pango/pango-item.h
--- from-cvs/pango/pango/pango-item.h	Wed Aug 16 22:39:12 2006
+++ my-gtk/pango/pango/pango-item.h	Thu Aug 17 07:47:45 2006
@@ -35,7 +35,7 @@
   PangoEngineLang  *lang_engine;
   PangoFont *font;
 
-  guint8 level;
+  guint level : 8;
   /* nastiest hack ever.  stuff new items in the padding after the guint8 */
   PangoGravity gravity       : 8;
   gboolean centered_baseline : 1;

(In reply to comment #6)
> The "lot of noise" is not a matter of cvs update, but cvs commit (just done):

This part of your patch and commit reverted some of my fixes:

  http://cvs.gnome.org/viewcvs/pango/pango/pango.def?r1=1.45&r2=1.46

That's what I referred to as noise.  Fixed it now.

> 2006-08-17  Hans Breuer  <hans@breuer.org>
> 
>         * pango/makefile.msc pango/pango.def : updated
>         * pango/break.c(1630) : error C2059: syntax error : '}' Intializing a 
>         struct with empty braces is not supported with MSVC, instead use 0.
>         * pango/pangocairo.def : removed pango_fc_font_(map_)get_type
>         pangocairo has either win32 fonts or freetype. The former is more
> usual.
>         * pango/pango-utils.c : replaced M_PI by G_PI (from glib/gtypes.h).

Thanks.

> The ABI breakage remains. And it will if the code in question is not changed.
> I'm not doing an abstract discussion here, but have compiled and tested both
> variants.
> Your bitset-after-guint8 hack crashes in
> gtk+/gdk/gdkpango.c(gdk_pango_renderer_prepare_run).

Ok, that's where my confusion is coming from.  You didn't mention this in the original report, so I assumed the only breakage you were experiencing was the initializer.

Fixed now.  Changed to bitfields for level.  I'll test on bigendian systems later.

This can be closed I guess.  No breaks were reported.

Reopening because of recent discussions about bitfields on Windows.  Got to make sure different compilers don't generate different layouts.

Hans, do you have access to MS and gcc compilers on Windows?  Can you check if the following struct works.  That is, this one:

struct _PangoAnalysis
{
  PangoEngineShape *shape_engine;
  PangoEngineLang  *lang_engine;
  PangoFont *font;

  guint8 level;
  guint8 gravity; /* PangoGravity */
  guint8 flags;

  PangoLanguage *language;
  GSList *extra_attrs;
};

and the old one:

struct _PangoAnalysis
{
  PangoEngineShape *shape_engine;
  PangoEngineLang  *lang_engine;
  PangoFont *font;

  guint8 level;

  PangoLanguage *language;
  GSList *extra_attrs;
};

have the same size, and offsetof all common members, using both compilers?

Note that the oficial win32 builds of glib/pango/GTK+ use MSVC bitfield
arrangement by passing a flag to the compiler (-ms-bitfields or something
like that.)

(In reply to comment #11)
> Note that the oficial win32 builds of glib/pango/GTK+ use MSVC bitfield
> arrangement by passing a flag to the compiler (-ms-bitfields or something
> like that.)

Hans is saying that (in a cairo list message) GTK+ does that, but not Pango.  So I'm not sure if we should go on and introduce it now.  We sure have bitfields in public API, for the logattrs.

Created attachment 79289 [details]
a small bitfields test program

Back in August I wrote a small test program - now attached - to better understand the issue. In the comment there is included the output from two platforms (x86, ppc) two compilers (msvc, gcc) and with/without one compiler option (-mms-bitfileds).

The only case different between gcc (without --mms-bitfileds) and msvc is not currently used by Pango public API/ABI (it was with the original issue of the bug report, but that would not have been fixed by -mms-bitfileds). 

A bitfield following a type smaller than the alignment is fitted by gcc into the gap. With msvc the alignment of the bitfields underlying int takes place before packing the struct.

So the change suggested in comment #10 is safe, with or without -mms-bifileds.

Pango really has to be compiled with -mms-bitfields, or you won't be able
to (fully) use the DLLs from applications compiled with MSVC.

cairo just doesn't matter since the bitfields aren't part of the public
ABO.

(Adding Tor to the Cc: since this has drifted off topic)

Tor, there's still the question of whether pango should use -mms-bitfields.

2007-01-08  Behdad Esfahbod  <behdad@gnome.org>

        * docs/pango-sections.txt:
        * docs/tmpl/glyphs.sgml:
        * docs/tmpl/main.sgml:
        * pango/pango-context.c (itemize_state_add_character):
        * pango/pango-item.h:
        * pango/pango-layout.c (pango_layout_run_get_extents):
        * pango/pango-renderer.c (pango_renderer_draw_layout_line):
        Don't introduce bitfields in public struct and rename PangoAnalysis.
        centered_baseline to PangoAnalysis.flags and introduce
        PANGO_ANALYSIS_FLAGS_CENTERED_BASELINE.

If there are bitfields in the API, yes it should. The configure.in should presumably turn in on automatically when using gcc, like it does for gtk+.