bugThe FreeType Project - Bugs: bug #54753, Can't create an FT_MemoryRec_

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #54753: Can't create an FT_MemoryRec_

Submitter:  bungeman <bungeman>
Submitted:  Fri 28 Sep 2018 06:58:14 PM UTC
   
 
Severity:  3 - Normal Item Group:  None
Status:  Fixed Privacy:  Public
Assigned to:  podtelez Open/Closed:  Closed
Planned Release:  None

Jump to the original submission

Tue 30 Oct 2018 06:52:45 PM UTC, comment #14: 
Alexei Podtelezhnikov <podtelez>
Group Member
Fri 26 Oct 2018 05:56:55 AM UTC, comment #13: 

Yes, reverting seems to be the right solution, given the signed vs. unsigned issue is really an ABI problem.

Hin-Tak, what do you think of providing a Windows version of `ftdbgmem.c', to be put into `builds/windows'?  Would this be sufficient?  With some adjustments to the make system to handle this file similar to `ftdebug.c', the file could be tailored as necessary.

Werner LEMBERG <wl>
Group administrator
Thu 25 Oct 2018 06:54:06 PM UTC, comment #12: 

Putting this in very practical terms, bungeman is concerned because chromium/skia are
1) build with C++
2) build against either system (read old) or bundled (read git) copies of FreeType
3) actually use FT_Memory interfaces

I doubt that there are other products in this predicament. So indeed we have to make a tough choice between correctness and convenience.

Again I do not think we can or should be clever here. To revert or not to revert is the question.

We caused some Skia pain with padding changes. Everyone survived - we even provide bitmap dimensions without rendering now to support the font texture atlasing. This change appears less invasive, but perhaps I am wrong.

Werner, should we just revert it?

Alexei Podtelezhnikov <podtelez>
Group Member
Wed 24 Oct 2018 10:08:56 PM UTC, comment #11: 

Okay, I am re-reading comment #1. The function pointer suggestion actually doesn't quite work either - I have seen recent gcc complains about function pointer prototype mis-matches; it is bad practise to cast back and forth from (void *) so I think that gcc checking function pointer prototypes is a good thing.

I see your issue is with the sign switch from undeclared=signed to "unsigned", and I can see that some usage would complain about the sign change. The actual practical impact is perhaps insignificant though, since using the upper half of whichever range we are discussing (32/64) is rare.

For the signed/unsigned issue, I am inclined to just apologize to the user that they might need adjusting in a upcoming release notes, if they notice it. size_t (unsigned) is the correct type for those routines in the long term, and freetype needs to move towards it sooner or later. When that happens, is up to Werner/Alex. There is no rush to go there since those routines had been wrong for so long, so revert it and leave the issue to later is okay too.

Hin-Tak Leung <htl10>
Wed 24 Oct 2018 03:52:52 PM UTC, comment #10: 

Just to restate, my issue is that "size_t is not "long" on almost any platform, "size_t" is almost always "unsigned long" (except of course on win64). The signed vs unsigned issue causes c++ compilation errors (and c warnings) even when things should technically work at the bit level. I'm currently working around this in my c++ code by extracting the correct type from the function signatures, but this is not something every c++ user is going to want to do (and I'm not sure can be done in c).

bungeman <bungeman>
Sat 20 Oct 2018 08:43:42 PM UTC, comment #9: 

The current pain is mostly because we had it as "long" for a long time, and "size_t" is what it should be, for most of the memory related routines; on almost every platform, "long" is "size_t", so specialising win64 is the least disruptive choice. I spat a lot of "#ifdef _WIN64" while I was looking at my little problem, but maybe it does not need to be ugly if it can be centralized in one place as one redefinition of FT_SIZE_T . I think I am mostly against setting it with configure.ac, etc.

Hin-Tak Leung <htl10>
Sat 20 Oct 2018 08:34:20 PM UTC, comment #8: 

I am somewhat against introducing another FT_* types, actually. Because of the overhead of putting the other stuff in, configure.ac, cmake, visual studio projects, etc. Also we have already have a lot of types which are mostly identical on most platforms, like FT_Pos/FT_26dot6/FT_ULong :-(.

In this case, it probably makes sense to do it once uglily as in, something like this

#ifdef_WIN64
typedef FT_SIZE_T __int64
#else
the old stuff
#end

How about this?

Hin-Tak Leung <htl10>
Sat 20 Oct 2018 05:18:59 AM UTC, comment #7: 

Hin-Tak, see my comment #1 :-)

Werner LEMBERG <wl>
Group administrator
Fri 19 Oct 2018 08:21:55 PM UTC, comment #6: 

I am okay with reverting the change. When I first reported the problems about it, it was trying to debug an issue specific to win64. I knew that changing content of a public header that has been a while is likely to cause problems.

In my own solution, which went on in parallel while Alex researched on a better answer, I opted to do a lot of

#ifdef _WIN64
  new stuff
#else
  old stuff
#endif

which preserves existing behavior. For debugging, not introducing new changes unless strictly necessary, was the easy decision to make, on a private dev direction.

As we all agree, the only platform that is affected by the change is WIN64. On way or the other, it is broken anyway, either wrong prototype for 64-bit ness, or breaking ABI compatibility.

Should we simply revert the change, or do the ugly #ifdef thing like I did privately? I don't feel strongly about reverting / ugly "#ifdef _WIN64". There are a few more related modifications which affects the debug versions of those routines (which are not normally compiled in) sitting in my private stash.

Reverting is probably the easiest way out at the moment. I have a quick look at the code, those values are used as sizes and not offsets/addresses under normal usage for win64, so probably not important. (if you have to use >2G/4G inside FreeType on win64, you probably have other problems...). The usage in src/base/ftdbgmem.c is a bit more complicated.

Possibly reverting most of it, and do some ugly "ifdef _WIN64" in src/base/ftdbgmem.c ?

Hin-Tak Leung <htl10>
Tue 02 Oct 2018 05:54:59 PM UTC, comment #5: 

Experimenting with ftmemchk.c in freetype2-demos (which appears to be the one use of FT_New_Library in the repo) the difference appears to be when compiling as c++ vs c. In 'c' mode gcc and clang both warn as shown in comment #4; in 'c++' mode they both produce the same message but it is an error. It was possible to do this by taking the compile command for ftmemchk.c and replacing 'gcc' with 'g++' and '-std=c99' with '-std=c++11'.

bungeman <bungeman>
Mon 01 Oct 2018 06:45:47 PM UTC, comment #4: 

GCC gives me a WARNING, not an error, when compiling against old headers:

freetype2/src/base/ftsystem.c: In function ‘FT_New_Memory’:
freetype2/src/base/ftsystem.c:309: warning: assignment from incompatible pointer type
freetype2/src/base/ftsystem.c:310: warning: assignment from incompatible pointer type

Please post the error from your compiler and I will be happy to revert. I do not think it is worth any conditional macros to deal with it.

Once again, we know that the binary compatibility is only broken on LLP64 (win64). On other systems this just adds "unsigned" to "long". So let's just revert this after you admit that you are paranoid about an adjudicated warning.

Alexei Podtelezhnikov <podtelez>
Group Member
Mon 01 Oct 2018 05:08:12 PM UTC, comment #3: 

Sure, hiding things was ok, but this is about changing non-hidden API.

This isn't just a warning, this is a compile error since the types of the existing functions being used to initialize an FT_MemoryRec_ (in existing code) and the the types expected when initializing a 'new' FT_MemoryRec_ are not the same. Existing code can be updated to the 'new' types but then that will be an error when compiling against older headers.

And old headers last a long time. The reality is that most distros won't pick up 2.10 for another year and at least Ubuntu will still be supporting/using FreeType 2.5.2 into at least next year (RHEL is a bit worse I think). It can take up to ~10 years before distros move on and existing software no longer needs to compile with old headers. I can be convinced that after compilation everything is ok but I'm sure UBSAN and other sanitizers will complain when the types don't match. I can get away with magic C++ callable_traits and statically linking my own FreeType build for sanitizer builds, but I think C users will need to have an #ifdef around FREETYPE_XXX, which seems unfortunate.

bungeman <bungeman>
Fri 28 Sep 2018 08:39:35 PM UTC, comment #2: 

Thank you for your support of size_t. :)

I understand that the warning will be triggered, but the binary compatibility is preserved (except on LLP64) because we used to cast right back to size_t in ft_alloc.

This will be 2.10 after all. Can't we just live with the warning for a couple of months? We used to hide symbols from users and everybody survived.

Alexei Podtelezhnikov <podtelez>
Group Member
Fri 28 Sep 2018 08:06:14 PM UTC, comment #1: 

... so may we should indeed use FT_SIZE_T and a configuration macro to control what FT_SIZE_T actually is.

Werner LEMBERG <wl>
Group administrator
Fri 28 Sep 2018 06:58:14 PM UTC, original submission:  

I use FT_New_Library specifically for using particular malloc, free, and realloc implementations. The recent change from long to size_t (877aa1b2cc662978aae61ed4d5c6ea8ba56b2fe7) for memory management causes issues for some existing code. I have a wrapper allocation function which takes 'long' which used to match the signature of FT_Alloc_Func but 'size_t' is 'unsigned long' so the compiler complains that the wrapper function doesn't match the new declaration of FT_Alloc_Func when creating a FT_MemoryRec_. If I only compile with old headers I can leave things alone, but if I want to use new headers I need to change the signature but that is incompatible with old headers. The state of the ecosystem being what it is, it's fairly required to be able to build with older headers.

In practice the simplest way around this is to assume the binary representation is compatible between these two and just cast the function pointer like '(FT_Alloc_Func)sk_ft_alloc' when creating the FT_MemoryRec_, but casting function pointers doesn't seem like the greatest thing to do. In C++ it's possible to be super clever and figure out the types of the parameters and even assert everything is the right size, but I'm not sure everyone is going to want that.

I'm not sure I have a good solution to this, but I do support moving to using size_t where it is the right type. However, you should be aware of this issue with creating a FT_MemoryRec_.

bungeman <bungeman>

 

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

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by htl10 (Posted a comment)
  • -email is unavailable- added by podtelez (Posted a comment)
  • -email is unavailable- added by wl (Posted a comment)
  • -email is unavailable- added by bungeman (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-10-30 podtelez StatusNone Fixed
        Open/ClosedOpen Closed
    2018-09-28 wl Assigned toNone podtelez

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code