Convert unicode index to long, not int, in get_char_index#7768
Convert unicode index to long, not int, in get_char_index#7768dopplershift merged 1 commit intomatplotlib:masterfrom
Conversation
There's an error in the `PyFT2Font.get_char_index()` method added in 2d56ffeb . The type for the unicode index to be sent to `FT_Get_Char_Index` is `FT_ULong` - an unsigned long - but the `PyArg_ParseTuple` call that converts it from Python used `I` in the format string, which converts a Python int to a C unsigned int, not a C unsigned long. This doesn't seem to cause a problem on little-endian arches, but it results in completely incorrect conversion on big-endian arches, which in turn would result in wrong glyphs, unfound glyphs, and even in an infinite recursion in `UnicodeFonts._get_glyph`. To get correct conversion we must use `k` not `I`, which is the specifier for a C unsigned long. Ref: https://docs.python.org/3/c-api/arg.html#numbers
|
BTW, I figured this out by modifying |
|
@mdboom (author of 2d56ffe) |
QuLogic
left a comment
QuLogic
left a comment
There was a problem hiding this comment.
Makes sense; on LE arches, the first 4 bytes would be filled, and assuming a zeroing stack, it should result in the same value. On BE arches, the first 4 bytes are the top-most bytes, so everything would be multiplied by 2**32.
Since it's an unsigned long, probably this worked fine on 32-bit BE arches (if any exist and were used.)
|
Note, there still appear to be several hundred other errors in the test suite on a ppc64 build. It seems like it would be a good idea to set up the CI to run the tests on a 64-bit be arch, if at all possible, so all those problems can be fixed and future ones won't be introduced... @QuLogic yeah, that adds up. The incorrect values were indeed very large. |
|
I'm not sure there are any GitHub integrations that support ppc64; maybe copr, but I never understood how their webhooks worked. |
Current coverage is 62.12% (diff: 100%)@@ master #7768 diff @@
==========================================
Files 174 174
Lines 56028 56028
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
Hits 34805 34805
Misses 21223 21223
Partials 0 0
|
|
hah, well, copr is a Fedora thing, so I guess that'd be my department...I could maybe talk to some people and see if we can hook anything up, I'll try and remember to give it a shot. |
|
Is there a public log of the test results? iirc, from looking at the failures from debian's build system any of the failures are in the spectra tests in mlab which are very sensitive to small differences in floating point math. |
|
Well, I did my test ppc64 build with the test suite enabled in a sandbox that fedora releng set up for me, but I can run it again and grab the log and put it up somewhere, I'll do that. A more long-term solution might be, as @QuLogic suggested, to set up a COPR repository which has a simple spec (and uses the bundled freetype and so on, not like the official Fedora package build) and just tries to run a build every time there's a commit to this repo (or just does it every day or something). |
|
Here's the build log of the ppc64 build with tests enabled: Looking at that, I'm seeing some more fun issues with integer types...digging into that right now. The freetype |
Convert unicode index to long, not int, in get_char_index
|
Backported to 2.x in bffe631 |
The preference seems to be (unsigned) int unless it's not 32-bit, but unless we're talking about embedded stuff, I'm not sure Fedora runs on anything that uses a 16-bit int. |
|
@AdamWill BTW, that build is with beta4; you should try with rc2. I believe there should be fixes for several of those size warnings. |
5 participants
There's an error in the
PyFT2Font.get_char_index()methodadded in 2d56ffeb . The type for the unicode index to be sent
to
FT_Get_Char_IndexisFT_ULong- an unsigned long - butthe
PyArg_ParseTuplecall that converts it from Python usedIin the format string, which converts a Python int to a Cunsigned int, not a C unsigned long. This doesn't seem to cause
a problem on little-endian arches, but it results in completely
incorrect conversion on big-endian arches, which in turn would
result in wrong glyphs, unfound glyphs, and even in an infinite
recursion in
UnicodeFonts._get_glyph.To get correct conversion we must use
knotI, which isthe specifier for a C unsigned long.
Ref: https://docs.python.org/3/c-api/arg.html#numbers