bugThe FreeType Project - Bugs: bug #49949, pstables.h included twice, wasting...

 
 

bug #49949: pstables.h included twice, wasting data segment space

Submitted by:  Bruce Dawson <brucedawson>
Submitted on:  Wed 28 Dec 2016 07:08:52 PM UTC  
 
Severity: 3 - NormalItem Group: None
Status: FixedPrivacy: Public
Assigned to: Werner LEMBERG <wl>Open/Closed: Closed
Planned Release: 2.8.1

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue 12 Sep 2017 05:02:16 PM UTC, comment #6:

We finally rolled the latest freetype in Chrome and I verified that the duplicate array bug is fixed in today's canary build on Windows. See crbug.com/758197 for more details. Thanks for the fix.

Bruce Dawson <brucedawson>
Fri 21 Jul 2017 08:45:56 PM UTC, comment #5:

Basically all you have to do (and I have tested this on Chrome) is remove the write( &quot;#ifndef DEFINE_PS_TABLES\n&quot; ) calls that precede the &quot;extern/extern &quot;C&quot;&quot; writes from glnames.py, and the associated write( &quot;#ifdef DEFINE_PS_TABLES_DATA\n&quot; ) calls, so that the extern/extern &quot;C&quot; becomes unconditional. The remaining uses of DEFINE_PS_TABLES need to change to DEFINE_PS_TABLES_DATA (or something like that) except for the #ifdef around ft_get_adobe_glyph_index which should stay the same.

Then #define DEFINE_PS_TABLES_DATA needs to be added to psmodule.c, right beside the #define of DEFINE_PS_TABLES.

I also changed the file open type from w\n to wb. It seems to give identical results and wb works on Windows whereas w\n does not.

I can review any patches you create but unfortunately there's a bit of hassle involved in actually supplying a patch.

Bruce Dawson <brucedawson>
Fri 21 Jul 2017 08:34:24 PM UTC, comment #4:

I forgot to follow-up on this at the time and just noticed that the problem still exists, so I looked at the patch (6fb549ddab976e9b1993c6f14fdcee07d3397016). Unfortunately it is incorrect. The crucial diff is here:

- static const unsigned char ft_adobe_glyph_list[55997L] =
+#ifndef DEFINE_PS_TABLES
+#ifdef __cplusplus
+ extern &quot;C&quot;
+#else
+ extern
+#endif
+#endif
+ const unsigned char ft_adobe_glyph_list[55997L]
+#ifdef DEFINE_PS_TABLES
+ =

What this means is that if DEFINE_PS_TABLES is defined then we get:

const char ft_adobe_glyph_list[3696]
= {
....

and if it isn't defined then we get:
const char ft_adobe_glyph_list[3696];

What this means is that we end up with two arrays, one of which is initialized and the other one which isn't. This is because 'const' implies static and you need to override it with &quot;extern&quot; to avoid that.

The diff that we want is this:

- static const char ft_adobe_glyph_list[3696] =
+#ifdef __cplusplus
+ extern &quot;C&quot;
+#else
+ extern
+#endif
+ const char ft_adobe_glyph_list[3696]
+#ifdef DEFINE_PS_TABLES
+ =

so that we unconditionally get extern or extern &quot;C&quot;. That gives us:

extern &quot;C&quot; const char ft_adobe_glyph_list[3696]
= {
....

and:

extern &quot;C&quot; const char ft_adobe_glyph_list[3696];

The first one defines the array and gives it external linkage (const defaults to internal linkage). The second one declares the array instead of defining it. It's frustrating subtle, but this is the necessary magic.

The other problem is that DEFINE_PS_TABLES controls initialization of the ft_adobe_glyph_list array (and others) and generation of the ft_get_adobe_glyph_index function. The goal should be to give access to ft_get_adobe_glyph_index to all, while only defining (initializing) the arrays once. I think that a separate define, perhaps DEFINE_PS_TABLES_DATA, is needed to control whether or not the arrays are defined (and filled with data) or just declared (along with the function). I have a possible patch available. It shrinks chrome_child.dll's data segment by 67,200 bytes.

Bruce Dawson <brucedawson>
Wed 28 Dec 2016 09:24:35 PM UTC, comment #3:

Fixed in git. Thanks for the suggestion, and please test!

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Wed 28 Dec 2016 07:45:41 PM UTC, comment #2:

A patch should be simple enough but all of the possible patches require changing pstables.h and it has a comment at the top saying:

/* This file has been generated automatically -- do not edit! */

That said, I think that the easiest fix would be to modify the definition of ft_adobe_glyph_list in pstables.h like this:

#ifdef __cplusplus
extern "C"
#else
extern
#endif
const unsigned char ft_adobe_glyph_list[55997L]
#ifdef DEFINE_FT_ADOBE_GLYPH_LIST
=
{
...
}
#endif // DEFINE_FT_ADOBE_GLYPH_LIST
;

Then, in psmodule.c add the #define line:
#define DEFINE_FT_ADOBE_GLYPH_LIST
#include "pstables.h"

No other source files need to change.

The code snippets above are, effectively, a patch, but they need to be applied to whatever code generates pstables.h. Note that the issue potentially affects the other arrays in pstables.h but in practice the other arrays are not currently being affected, so my "patch" only addresses the one array.

As predicted this patch saves approximately 56,000 bytes from chrome_child.dll.

Bruce Dawson <brucedawson>
Wed 28 Dec 2016 07:10:53 PM UTC, comment #1:

Thanks for the report. Can you provide a patch?

Werner LEMBERG <wl>
Project AdministratorIn charge of this item.
Wed 28 Dec 2016 07:08:52 PM UTC, original submission:

When FreeType is used by pdfium which is used by chromium, pstables.h ends up being included twice, once by third_party\pdfium\third_party\freetype\src\psnames\psmodule.c and once by third_party\pdfium\core\fxge\freetype\fx_freetype.cpp. The second include from fx_freetype.cpp is done in order to gain access to ft_adobe_glyph_list and ft_get_adobe_glyph_index.

Unfortunately this causes the 55997 byte ft_adobe_glyph_list to be compiled into both source files, thus wasting 55,997 bytes in the data segment. This duplication is required by the C++ standard.

If the array was declared in the header file as "extern const" and then defined in a .c file as "extern const" then many source files could reference the same copy, thus avoiding this duplication. The C++ files would have to have an extern "C" to make the symbol names work.

This should be an easy fix that will save a noticeable amount of space for many projects.

Bruce Dawson <brucedawson>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by wl (Posted a comment)
  • -unavailable- added by brucedawson (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 12 Sep 2017 06:21:21 PM UTCwlPlanned Release2.7.1=>2.8.1
    Wed 28 Dec 2016 09:24:35 PM UTCwlPlanned ReleaseNone=>2.7.1
    Wed 28 Dec 2016 09:24:34 PM UTCwlStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Wed 28 Dec 2016 07:10:53 PM UTCwlAssigned toNone=>wl

    Back to the top


    Powered by Savane 3.1-cleanup1