GNU bug report logs - #52888
29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 52888 in the body.
You can then email your comments to 52888 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; font_{delete_unmatched,score} do not handle nil
 FONT_WEIGHT_INDEX
Date: Wed, 29 Dec 2021 22:28:37 -0700

[Message part 1 (text/plain, inline)]

Hello,

On my system I have a variable weight .ttf font[1] installed somewhere.
When I build with --enable-check-lisp-object-type, the line

    int candidate = XFIXNUM (AREF (entity, prop)) >> 8;

in font_delete_unmatched and the expression

   EMACS_INT diff = ((XFIXNUM (AREF (entity, i)) >> 8)
                     - (XFIXNUM (spec_prop[i]) >> 8));

in font_score have failed assertions because the FONT_WEIGHT_INDEX for
this .ttf file is nil:

    #<font-entity ftcrhb CYRE Inconsolata nil iso10646-1 nil normal nil
     0 nil 100 0 ((:font-entity
     "/usr/share/fonts/inconsolata/Inconsolata-VariableFont_wdth,wght.ttf"
     . 0))>

I don't believe Emacs really knows how to handle these multi-weight .ttf
files?  Thus I propose the attached patch, to handle the value.

[1]  https://github.com/googlefonts/Inconsolata/tree/main/fonts/variable

-- 
Sean Whitton

[0001-Handle-nil-FONT_WEIGHT_INDEX.patch (text/x-patch, attachment)]

Message #8 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50;
 font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX
Date: Thu, 30 Dec 2021 09:33:17 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Wed, 29 Dec 2021 22:28:37 -0700
> 
> On my system I have a variable weight .ttf font[1] installed somewhere.
> When I build with --enable-check-lisp-object-type, the line
> 
>     int candidate = XFIXNUM (AREF (entity, prop)) >> 8;
> 
> in font_delete_unmatched and the expression
> 
>    EMACS_INT diff = ((XFIXNUM (AREF (entity, i)) >> 8)
>                      - (XFIXNUM (spec_prop[i]) >> 8));
> 
> in font_score have failed assertions because the FONT_WEIGHT_INDEX for
> this .ttf file is nil:
> 
>     #<font-entity ftcrhb CYRE Inconsolata nil iso10646-1 nil normal nil
>      0 nil 100 0 ((:font-entity
>      "/usr/share/fonts/inconsolata/Inconsolata-VariableFont_wdth,wght.ttf"
>      . 0))>
> 
> I don't believe Emacs really knows how to handle these multi-weight .ttf
> files?  Thus I propose the attached patch, to handle the value.

Is the patch supposed to allow Emacs to handle these fonts, or is it
just the protection against assertion violations?  If the latter,
isn't it better to teach the font driver to handle these fonts
correctly?

AFAIU, your patch basically will cause Emacs to reject such fonts and
not use them, which is tantamount to telling users to configure Emacs
to ignore them via, say, face-ignored-fonts.  Is that right, or am I
missing something?

Thanks.

Message #11 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Thu, 30 Dec 2021 10:13:13 -0700

Hello,

On Thu 30 Dec 2021 at 09:33am +02, Eli Zaretskii wrote:

> Is the patch supposed to allow Emacs to handle these fonts, or is it
> just the protection against assertion violations?

The latter -- the code implicitly assumes that the weight will always be
a fixnum, but that is not so.  I want to fix that implicit assumption.

> If the latter, isn't it better to teach the font driver to handle
> these fonts correctly?
>
> AFAIU, your patch basically will cause Emacs to reject such fonts and
> not use them, which is tantamount to telling users to configure Emacs
> to ignore them via, say, face-ignored-fonts.  Is that right, or am I
> missing something?

I don't think it is equivalent to face-ignored-fonts.  The weight field
in the entity vector is examined only when the weight field in the font
spec is non-nil.  So my code does not categorically reject these fonts:
it rejects them only when the user requested a specific weight, AFAICT.

I don't know enough about these variable weight TTFs to judge whether it
is worth anyone's time adding better support for them in Emacs.  In the
case of Inconsolata-VariableFont_wdth,wght.ttf, the font authors provide
separate .ttf files for each weight too, so there doesn't seem to be an
expectation that applications know how to read the combined file.

Thanks for looking!

-- 
Sean Whitton

Message #14 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Thu, 30 Dec 2021 20:39:28 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: 52888 <at> debbugs.gnu.org
> Date: Thu, 30 Dec 2021 10:13:13 -0700
> 
> > Is the patch supposed to allow Emacs to handle these fonts, or is it
> > just the protection against assertion violations?
> 
> The latter -- the code implicitly assumes that the weight will always be
> a fixnum, but that is not so.  I want to fix that implicit assumption.
> 
> > If the latter, isn't it better to teach the font driver to handle
> > these fonts correctly?
> >
> > AFAIU, your patch basically will cause Emacs to reject such fonts and
> > not use them, which is tantamount to telling users to configure Emacs
> > to ignore them via, say, face-ignored-fonts.  Is that right, or am I
> > missing something?
> 
> I don't think it is equivalent to face-ignored-fonts.  The weight field
> in the entity vector is examined only when the weight field in the font
> spec is non-nil.  So my code does not categorically reject these fonts:
> it rejects them only when the user requested a specific weight, AFAICT.

Does it really make sense to accept these fonts in some situations,
but not in others?  AFAIU, what you suggest would cause Emacs to
accept these fonts when :weight is not mentioned (and so defaults to
'normal'), but to reject them if the 'normal' weight is specified
explicitly, is that right?  If so, it's confusing, and users will
complain.  Rejecting such fonts outright is at least consistent, and
thus better than semi-support.

> I don't know enough about these variable weight TTFs to judge whether it
> is worth anyone's time adding better support for them in Emacs.  In the
> case of Inconsolata-VariableFont_wdth,wght.ttf, the font authors provide
> separate .ttf files for each weight too, so there doesn't seem to be an
> expectation that applications know how to read the combined file.

I installed on the release branch a temporary fix, similar to what you
suggested, to avoid undefined behavior with those fonts, but I don't
think we should install something like that on master.  On master, I
think ftfont.c and its ilk should be fixed to handle these fonts
correctly, or reject them if we cannot DTRT with them for some reason.
I think the fact that we create invalid font entities from such fonts
is a clear sign that the font backend mishandles them, and if so,
that's where this problem should be corrected: we should create valid
font entities to begin with, with ;weight and other similar attributes
having numerical values, as expected.

Thanks.

Message #17 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>, 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Fri, 31 Dec 2021 17:30:35 -0700

Hello,

On Thu 30 Dec 2021 at 08:39PM +02, Eli Zaretskii wrote:

> Does it really make sense to accept these fonts in some situations,
> but not in others?  AFAIU, what you suggest would cause Emacs to
> accept these fonts when :weight is not mentioned (and so defaults to
> 'normal'), but to reject them if the 'normal' weight is specified
> explicitly, is that right?  If so, it's confusing, and users will
> complain.  Rejecting such fonts outright is at least consistent, and
> thus better than semi-support.

Ah, yes, that would indeed be confusing.

> I installed on the release branch a temporary fix, similar to what you
> suggested, to avoid undefined behavior with those fonts, but I don't
> think we should install something like that on master.  On master, I
> think ftfont.c and its ilk should be fixed to handle these fonts
> correctly, or reject them if we cannot DTRT with them for some reason.
> I think the fact that we create invalid font entities from such fonts
> is a clear sign that the font backend mishandles them, and if so,
> that's where this problem should be corrected: we should create valid
> font entities to begin with, with ;weight and other similar attributes
> having numerical values, as expected.

I spent some more time in gdb and learned the following.

The ftcrhb backend returns a FcPattern for each of the weights contained
in Inconsolata-VariableFont_wdth,wght.ttf.  So Emacs does not need to
learn anything special about these variable weight files in order to
support them, I think.  However, this code in ftfont_pattern_entity can
sometimes set FONT_WEIGHT_INDEX to nil:

  if (FcPatternGetInteger (p, FC_WEIGHT, 0, &numeric) == FcResultMatch)
    {
      FONT_SET_STYLE (entity, FONT_WEIGHT_INDEX, make_fixnum (numeric));
    }

I haven't yet determined exactly when this can happen, but it suggests
the problem is within FONT_SET_STYLE, as all the backend-specific code
is doing is upplying 'numeric'.

If I might ask a gdb question: to try to determine when this code can
set FONT_WEIGHT_INDEX to nil, I set a breakpoint right after it and then
tried

    condition NN NILP (AREF (entity, FONT_WEIGHT_INDEX))

but this didn't work -- is it possible to do something like that?

Thanks.

-- 
Sean Whitton

Message #20 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>, 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Fri, 31 Dec 2021 19:35:53 -0700

Hello,

On Fri 31 Dec 2021 at 05:30PM -07, Sean Whitton wrote:

> The ftcrhb backend returns a FcPattern for each of the weights contained
> in Inconsolata-VariableFont_wdth,wght.ttf.  So Emacs does not need to
> learn anything special about these variable weight files in order to
> support them, I think.  However, this code in ftfont_pattern_entity can
> sometimes set FONT_WEIGHT_INDEX to nil:
>
>   if (FcPatternGetInteger (p, FC_WEIGHT, 0, &numeric) == FcResultMatch)
>     {
>       FONT_SET_STYLE (entity, FONT_WEIGHT_INDEX, make_fixnum (numeric));
>     }

... or FcPatternGetInteger returns something other than FcResultMatch,
which is in fact what is happening.

I tried FcPatternGetDouble in an else branch and that fails too, so it
seems fontconfig cannot determine a weight for the variant in question.

So, perhaps each of the if (FcPatternGetInteger (...)) conditionals that
corresponds to one of the FONT_* fields that the font.c functions assume
are fixnums should have an else branch to return Qnil?

-- 
Sean Whitton

Message #23 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Sat, 01 Jan 2022 08:56:15 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Fri, 31 Dec 2021 17:30:35 -0700
> 
> If I might ask a gdb question: to try to determine when this code can
> set FONT_WEIGHT_INDEX to nil, I set a breakpoint right after it and then
> tried
> 
>     condition NN NILP (AREF (entity, FONT_WEIGHT_INDEX))
> 
> but this didn't work -- is it possible to do something like that?

It should be possible if your Emacs was compiled with -g3.
Perhaps try

  condition NN AREF (entity, FONT_WEIGHT_INDEX) == Qnil

instead.

Message #26 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Sat, 01 Jan 2022 09:15:18 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Fri, 31 Dec 2021 19:35:53 -0700
> 
> >   if (FcPatternGetInteger (p, FC_WEIGHT, 0, &numeric) == FcResultMatch)
> >     {
> >       FONT_SET_STYLE (entity, FONT_WEIGHT_INDEX, make_fixnum (numeric));
> >     }
> 
> ... or FcPatternGetInteger returns something other than FcResultMatch,
> which is in fact what is happening.
> 
> I tried FcPatternGetDouble in an else branch and that fails too, so it
> seems fontconfig cannot determine a weight for the variant in question.
> 
> So, perhaps each of the if (FcPatternGetInteger (...)) conditionals that
> corresponds to one of the FONT_* fields that the font.c functions assume
> are fixnums should have an else branch to return Qnil?

Maybe.

However, the question that I think we should ask ourselves at this
point is whether there's a reasonable way to process these fonts into
a font spec that Emacs expects.  So if FcPatternGetInteger and
FcPatternGetDouble don't do the job, perhaps there's another
fontconfig function that does, or maybe these fonts need to be
processed in some slightly different ways (like in a loop, for
example) to produce the weight matches from them?  Can you perhaps ask
the developers of this font to help?  Or maybe we could ask on the
fontconfig forum?

If it turns out that there's no way we could adapt our code to these
fonts (which I'd find hard to believe), we should then look for a way
of detecting them so that we could reject them on the font backend
level, before they get into the platform-independent code in font.c,
e.g. with the 'else' branch that you suggest.  However, I suspect that
the fact we don't have such branches is not just because we blindly
assume that can "never happen", so maybe doing so would break
something.

In any case, trying to accept those fonts instead of rejecting them
would be a better solution, if we can reasonably code it.

Thanks.

Message #29 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>, 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Sat, 01 Jan 2022 15:31:15 -0700

Hello,

On Sat 01 Jan 2022 at 09:15am +02, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton <at> spwhitton.name>
>> Date: Fri, 31 Dec 2021 19:35:53 -0700
>>
>> >   if (FcPatternGetInteger (p, FC_WEIGHT, 0, &numeric) == FcResultMatch)
>> >     {
>> >       FONT_SET_STYLE (entity, FONT_WEIGHT_INDEX, make_fixnum (numeric));
>> >     }
>>
>> ... or FcPatternGetInteger returns something other than FcResultMatch,
>> which is in fact what is happening.
>>
>> I tried FcPatternGetDouble in an else branch and that fails too, so it
>> seems fontconfig cannot determine a weight for the variant in question.
>>
>> So, perhaps each of the if (FcPatternGetInteger (...)) conditionals that
>> corresponds to one of the FONT_* fields that the font.c functions assume
>> are fixnums should have an else branch to return Qnil?
>
> Maybe.
>
> However, the question that I think we should ask ourselves at this
> point is whether there's a reasonable way to process these fonts into
> a font spec that Emacs expects.  So if FcPatternGetInteger and
> FcPatternGetDouble don't do the job, perhaps there's another
> fontconfig function that does, or maybe these fonts need to be
> processed in some slightly different ways (like in a loop, for
> example) to produce the weight matches from them?  Can you perhaps ask
> the developers of this font to help?  Or maybe we could ask on the
> fontconfig forum?

I've filed a bug[1] and posted to the fontconfig mailing list.

To be clear, we are generating valid font specs for a number of weights
available in this multi-weight .ttf.  It's just that some of the
variants that are in there do not seem to have a valid weight, or
something like that.  So if we give up on those variants, we wouldn't be
giving up on the whole .ttf, so it's not a complete loss.

[1]  https://github.com/googlefonts/Inconsolata/issues/69

-- 
Sean Whitton

Message #32 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Sun, 02 Jan 2022 19:04:51 -0700

On Sat 01 Jan 2022 at 03:31PM -07, Sean Whitton wrote:

> I've filed a bug[1] and posted to the fontconfig mailing list.

<https://lists.freedesktop.org/archives/fontconfig/2022-January/006855.html>

for reference.

-- 
Sean Whitton

Message #35 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>, 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Tue, 04 Jan 2022 19:10:46 -0700

[Message part 1 (text/plain, inline)]

Hello,

On Sat 01 Jan 2022 at 09:15AM +02, Eli Zaretskii wrote:

> However, the question that I think we should ask ourselves at this
> point is whether there's a reasonable way to process these fonts into
> a font spec that Emacs expects.  So if FcPatternGetInteger and
> FcPatternGetDouble don't do the job, perhaps there's another
> fontconfig function that does, or maybe these fonts need to be
> processed in some slightly different ways (like in a loop, for
> example) to produce the weight matches from them?

I received a helpful reply from Akira TAGOH on the fontconfig list.  The
problematic entries are not in fact additional font weights available to
Emacs, but meta or virtual entries from which applications can extract
information about the range of weights provided by the variable weight
.ttf file.

As we do not need to know the range of weights explicitly, the virtual
entries are no use to us.  We can just skip over them and process the
non-virtual FcPattern entries we get for each weight.  I'm attaching a
patch which detects these virtual entries and skips over them.  Maybe we
could revert and replace the temporary fix with my patch?

As a test, I tried calling FcPatternGetRange on the virtual entry for
Inconsolata-VariableFont_wdth,wght.ttf and I got a range of 40 to 210.
I confirmed that the weights of all the non-virtual entries fall within
this range, inclusive.

-- 
Sean Whitton

[0001-Skip-virtual-FcPattern-entries-for-variable-weight-f.patch (text/x-patch, attachment)]

Message #38 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 05 Jan 2022 14:37:56 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Date: Tue, 04 Jan 2022 19:10:46 -0700
> 
> I received a helpful reply from Akira TAGOH on the fontconfig list.  The
> problematic entries are not in fact additional font weights available to
> Emacs, but meta or virtual entries from which applications can extract
> information about the range of weights provided by the variable weight
> .ttf file.
> 
> As we do not need to know the range of weights explicitly, the virtual
> entries are no use to us.  We can just skip over them and process the
> non-virtual FcPattern entries we get for each weight.  I'm attaching a
> patch which detects these virtual entries and skips over them.

Thanks, very good news!

> Maybe we could revert and replace the temporary fix with my patch?

According to what I see in the documentation, FcPatternGetRange exists
only since version 2.11.91 of Fontconfig, whereas we support 2.2.0 and
up.  So we cannot unconditionally use that API, and some part of the
temporary solution will have to be left in the source.  I'd need to
see the final patch with that in mind, before I can decide whether it
is simple/safe enough for the release branch.

> --- a/src/ftfont.c
> +++ b/src/ftfont.c
> @@ -184,11 +184,22 @@ ftfont_pattern_entity (FcPattern *p, Lisp_Object extra)
>    int numeric;
>    double dbl;
>    FcBool b;
> +  FcRange *range;
>  
>    if (FcPatternGetString (p, FC_FILE, 0, &str) != FcResultMatch)
>      return Qnil;
>    if (FcPatternGetInteger (p, FC_INDEX, 0, &idx) != FcResultMatch)
>      return Qnil;
> +  if (FcPatternGetRange (p, FC_WEIGHT, 0, &range) == FcResultMatch
> +      && FcPatternGetBool (p, FC_VARIABLE, 0, &b) == FcResultMatch
> +      && b == FcTrue)
> +    /* This is a virtual/meta FcPattern for a variable weight font,
> +       from which it is possible to extract an FcRange value
> +       specifying the minimum and maximum weights available in this
> +       file.  We don't need to know that information explicitly, so
> +       skip it.  We will be called with an FcPattern for each actually
> +       available, non-virtual weight.  */
> +    return Qnil;

I'm far from being an expert on Fontconfig programming, but the above
use of 'range' looks strange: it's a pointer that starts pointing to
some random (potentially invalid) address, and you pass its address to
FcPatternGetRange, which presumably assigns to it a valid point.  But
doesn't that valid pointer need to be released somehow after we use
it?  Or does it point to static area(s)?  I cannot find anything in
the Fontconfig documentation about the memory-management protocols for
FcValue objects, but maybe we should call FcValueDestroy on it after
we are done with it?  Or maybe this is not needed, as this passage
from the docs says near the end:

  void FcValueDestroy (FcValue v)
      Frees any memory referenced by `v'. Values of type FcTypeString,
      FcTypeMatrix and FcTypeCharSet reference memory, the other types
      do not.

Thanks.

Message #41 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52888 <at> debbugs.gnu.org, Sean Whitton <spwhitton <at> spwhitton.name>
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 05 Jan 2022 14:55:02 +0100

>>>>> On Wed, 05 Jan 2022 14:37:56 +0200, Eli Zaretskii <eliz <at> gnu.org> said:

    Eli> I'm far from being an expert on Fontconfig programming, but the above
    Eli> use of 'range' looks strange: it's a pointer that starts pointing to
    Eli> some random (potentially invalid) address, and you pass its address to
    Eli> FcPatternGetRange, which presumably assigns to it a valid point.  But
    Eli> doesn't that valid pointer need to be released somehow after we use
    Eli> it?  Or does it point to static area(s)?  I cannot find anything in
    Eli> the Fontconfig documentation about the memory-management protocols for
    Eli> FcValue objects, but maybe we should call FcValueDestroy on it after
    Eli> we are done with it?  Or maybe this is not needed, as this passage
    Eli> from the docs says near the end:

    Eli>   void FcValueDestroy (FcValue v)
    Eli>       Frees any memory referenced by `v'. Values of type FcTypeString,
    Eli>       FcTypeMatrix and FcTypeCharSet reference memory, the other types
    Eli>       do not.

Based on an admittedly quick reading of the fontconfig code, itʼs not
our responsibility to deallocate the FcRange here; it ends up pointing
to memory previously allocated internally by fontconfig.

Robert
-- 

Message #44 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 52888 <at> debbugs.gnu.org, spwhitton <at> spwhitton.name
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 05 Jan 2022 16:08:04 +0200

> From: Robert Pluim <rpluim <at> gmail.com>
> Cc: Sean Whitton <spwhitton <at> spwhitton.name>,  52888 <at> debbugs.gnu.org
> Date: Wed, 05 Jan 2022 14:55:02 +0100
> 
> Based on an admittedly quick reading of the fontconfig code, itʼs not
> our responsibility to deallocate the FcRange here; it ends up pointing
> to memory previously allocated internally by fontconfig.

OK, thanks.

Message #47 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 05 Jan 2022 22:41:55 -0700

[Message part 1 (text/plain, inline)]

Hello,

On Wed 05 Jan 2022 at 02:37PM +02, Eli Zaretskii wrote:

> According to what I see in the documentation, FcPatternGetRange exists
> only since version 2.11.91 of Fontconfig, whereas we support 2.2.0 and
> up.

Ah, right.  Is this version bound written down anywhere other than the
conditionals in configure.ac?  Sorry I didn't think to look there.

> So we cannot unconditionally use that API, and some part of the
> temporary solution will have to be left in the source.  I'd need to
> see the final patch with that in mind, before I can decide whether it
> is simple/safe enough for the release branch.

My patch requires FC_VARIABLE which was not added until 2.12.91, so in
the attached revision I've set up preprocessor conditionals based on
FC_VARIABLE.  Checking that FC_VARIABLE is true is how we know it's one
of the virtual entries.

It looks like fontconfig commit 83b41611 introduced the meta patterns
under discussion.  The first tagged release including that commit is
2.12.91.  So AFAICT nothing of the old fix need remain.

> I'm far from being an expert on Fontconfig programming, but the above
> use of 'range' looks strange: it's a pointer that starts pointing to
> some random (potentially invalid) address, and you pass its address to
> FcPatternGetRange, which presumably assigns to it a valid point.  But
> doesn't that valid pointer need to be released somehow after we use
> it?  Or does it point to static area(s)?  I cannot find anything in
> the Fontconfig documentation about the memory-management protocols for
> FcValue objects, but maybe we should call FcValueDestroy on it after
> we are done with it?  Or maybe this is not needed, as this passage
> from the docs says near the end:

Yes, I was surprised too, but as has been mentioned, the FcPatternGet*
functions are documented as supplying pointers to storage which must not
be freed.

-- 
Sean Whitton

[v2-0001-Skip-virtual-FcPattern-entries-for-variable-weigh.patch (text/x-patch, attachment)]

Message #50 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Thu, 06 Jan 2022 14:29:34 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: 52888 <at> debbugs.gnu.org
> Date: Wed, 05 Jan 2022 22:41:55 -0700
> 
> > According to what I see in the documentation, FcPatternGetRange exists
> > only since version 2.11.91 of Fontconfig, whereas we support 2.2.0 and
> > up.
> 
> Ah, right.  Is this version bound written down anywhere other than the
> conditionals in configure.ac?  Sorry I didn't think to look there.

No, I don't think we document the minimum versions of prerequisites
anywhere.

> > So we cannot unconditionally use that API, and some part of the
> > temporary solution will have to be left in the source.  I'd need to
> > see the final patch with that in mind, before I can decide whether it
> > is simple/safe enough for the release branch.
> 
> My patch requires FC_VARIABLE which was not added until 2.12.91, so in
> the attached revision I've set up preprocessor conditionals based on
> FC_VARIABLE.  Checking that FC_VARIABLE is true is how we know it's one
> of the virtual entries.

OK, but (a) we need a comment there explaining why FC_VARIABLE is used
as the condition, and (b) we'd also need to merge the temporary fix in
font.c to master.

Thanks.

Message #53 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Thu, 06 Jan 2022 11:10:12 -0700

[Message part 1 (text/plain, inline)]

Hello,

On Thu 06 Jan 2022 at 02:29PM +02, Eli Zaretskii wrote:

> OK, but (a) we need a comment there explaining why FC_VARIABLE is used
> as the condition, and (b) we'd also need to merge the temporary fix in
> font.c to master.

Here's an updated patch.  I don't think I can help with (b) but let me
know if there's something I can do.

Thanks!

-- 
Sean Whitton

[v3-0001-Skip-virtual-FcPattern-entries-for-variable-weigh.patch (text/x-patch, attachment)]

Message #58 received at 52888-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888-done <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 12 Jan 2022 16:56:44 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: 52888 <at> debbugs.gnu.org
> Date: Thu, 06 Jan 2022 11:10:12 -0700
> 
> > OK, but (a) we need a comment there explaining why FC_VARIABLE is used
> > as the condition, and (b) we'd also need to merge the temporary fix in
> > font.c to master.
> 
> Here's an updated patch.  I don't think I can help with (b) but let me
> know if there's something I can do.

Thanks, I installed this, and also installed a followup change that is
the "temporary fix" from emacs-28 adapted to the code on master.

And with that, I'm closing this bug.  Thanks for working on this.

Message #61 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Sean Whitton <spwhitton <at> spwhitton.name>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Wed, 12 Jan 2022 14:41:33 -0700

Hello,

On Wed 12 Jan 2022 at 04:56pm +02, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton <at> spwhitton.name>
>> Cc: 52888 <at> debbugs.gnu.org
>> Date: Thu, 06 Jan 2022 11:10:12 -0700
>> 
>> > OK, but (a) we need a comment there explaining why FC_VARIABLE is used
>> > as the condition, and (b) we'd also need to merge the temporary fix in
>> > font.c to master.
>> 
>> Here's an updated patch.  I don't think I can help with (b) but let me
>> know if there's something I can do.
>
> Thanks, I installed this, and also installed a followup change that is
> the "temporary fix" from emacs-28 adapted to the code on master.

Thank you for reviewing and installing.

The temporary fix can't do any harm, I suppose, but it is not clear to
me why it is necessary -- fontconfig doesn't generate the virtual/meta
FcPatterns in versions older than 2.12.91.  Sorry if I didn't make that
clearer earlier, or if I am missing some other reason why it is
necessary.

-- 
Sean Whitton

Message #64 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Sean Whitton <spwhitton <at> spwhitton.name>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle
 nil FONT_WEIGHT_INDEX
Date: Thu, 13 Jan 2022 08:52:22 +0200

> From: Sean Whitton <spwhitton <at> spwhitton.name>
> Cc: 52888 <at> debbugs.gnu.org
> Date: Wed, 12 Jan 2022 14:41:33 -0700
> 
> Hello,
> 
> On Wed 12 Jan 2022 at 04:56pm +02, Eli Zaretskii wrote:
> 
> >> From: Sean Whitton <spwhitton <at> spwhitton.name>
> >> Cc: 52888 <at> debbugs.gnu.org
> >> Date: Thu, 06 Jan 2022 11:10:12 -0700
> >> 
> >> > OK, but (a) we need a comment there explaining why FC_VARIABLE is used
> >> > as the condition, and (b) we'd also need to merge the temporary fix in
> >> > font.c to master.
> >> 
> >> Here's an updated patch.  I don't think I can help with (b) but let me
> >> know if there's something I can do.
> >
> > Thanks, I installed this, and also installed a followup change that is
> > the "temporary fix" from emacs-28 adapted to the code on master.
> 
> Thank you for reviewing and installing.
> 
> The temporary fix can't do any harm, I suppose, but it is not clear to
> me why it is necessary -- fontconfig doesn't generate the virtual/meta
> FcPatterns in versions older than 2.12.91.  Sorry if I didn't make that
> clearer earlier, or if I am missing some other reason why it is
> necessary.

Yes, you should have said that earlier.  But the defensive programming
in that place cannot hurt, so I will leave it there.

Message #67 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: André Silva <andre.beat <at> gmail.com>
To: 52888 <at> debbugs.gnu.org
Subject: bug#52888
Date: Thu, 13 Jan 2022 11:54:02 +0000

[Message part 1 (text/plain, inline)]

FWIW the patch included in emacs-28.0.91 seems to lead to a regression for
me where the incorrect font is used in certain cases or maybe just some
font attributes are not applied (e.g. in the modeline). The attached patch
fixes the problem for me.

[Message part 2 (text/html, inline)]

[emacs-fix-font.patch (text/x-patch, attachment)]

Message #70 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: André Silva <andre.beat <at> gmail.com>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888:
Date: Thu, 13 Jan 2022 18:40:54 +0200

> From: André Silva <andre.beat <at> gmail.com>
> Date: Thu, 13 Jan 2022 11:54:02 +0000
> 
> FWIW the patch included in emacs-28.0.91 seems to lead to a regression for me where the incorrect font is
> used in certain cases or maybe just some font attributes are not applied (e.g. in the modeline). The attached
> patch fixes the problem for me.

Do you have a test case where the problem happens?

Message #73 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: André Silva <andre.beat <at> gmail.com>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888:
Date: Thu, 13 Jan 2022 20:13:16 +0200

[Please use Reply All to keep the bug tracker on the CC list.]

> From: André Silva <andre.beat <at> gmail.com>
> Date: Thu, 13 Jan 2022 17:04:34 +0000
> 
> I don't, and I'm using doom-emacs so creating a minimal test case would take some effort (and probably
> also depends on the font I'm using). That said if you look at the fix you committed if `FIXNUMP (AREF (entity,
> prop))` returns false then the rest of the expression will get short-circuited (since it's an &&) and we will
> negate the result therefore entering the if statement. I believe this is contrary to what we want.

No, I think that's exactly what we want: fonts for which the attribute
is not a number should be ignored.  Your change will cause Emacs to
use them, right?

Message #76 received at 52888 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: André Silva <andre.beat <at> gmail.com>
Cc: 52888 <at> debbugs.gnu.org
Subject: Re: bug#52888:
Date: Thu, 13 Jan 2022 21:49:04 +0200

[Forwarding to the bug tracker.  Please use Reply All.]

> From: André Silva <andre.beat <at> gmail.com>
> Date: Thu, 13 Jan 2022 18:20:20 +0000
> 
> OK then maybe my expectations were wrong. My change restores the visual behavior I was seeing before
> these font attributes checks were introduced (but maybe it was wrong to begin with). It seems that now in
> certain places different font styles are being used (they seem to be thinner in some places although I'm not
> 100% sure whether it's the same font or not). The font I am using is JetBrains Mono. I am sorry for the
> noise, as the behavior changed I just assumed that this was buggy. Since I am not providing much
> information I understand that this is not entirely actionable right now so feel free to ignore it.

Thanks.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.