bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Handle-nil-FONT_WEIGHT_INDEX.patch --]
[-- Type: text/x-patch, Size: 3017 bytes --]

From b7eff2c3e1d09f44b1d5790275fc92c11e9be88f Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Wed, 29 Dec 2021 21:18:07 -0700
Subject: [PATCH] Handle nil FONT_WEIGHT_INDEX

* src/font.c (font_delete_unmatched, font_score): Handle nil entity
property.  In particular, FONT_WEIGHT_INDEX is nil for some variable
weight fonts.
---
 src/font.c | 42 ++++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 18 deletions(-)

diff --git a/src/font.c b/src/font.c
index fa831f2861..cb057abca6 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2183,7 +2183,8 @@ font_score (Lisp_Object entity, Lisp_Object *spec_prop)
 
   /* Score three style numeric fields.  Maximum difference is 127. */
   for (i = FONT_WEIGHT_INDEX; i <= FONT_WIDTH_INDEX; i++)
-    if (! NILP (spec_prop[i]) && ! EQ (AREF (entity, i), spec_prop[i]))
+    if (! NILP (spec_prop[i]) && ! NILP (AREF (entity, i))
+	&& ! EQ (AREF (entity, i), spec_prop[i]))
       {
 	EMACS_INT diff = ((XFIXNUM (AREF (entity, i)) >> 8)
 			  - (XFIXNUM (spec_prop[i]) >> 8));
@@ -2764,26 +2765,31 @@ font_delete_unmatched (Lisp_Object vec, Lisp_Object spec, int size)
 	{
 	  if (FIXNUMP (AREF (spec, prop)))
 	    {
-	      int required = XFIXNUM (AREF (spec, prop)) >> 8;
-	      int candidate = XFIXNUM (AREF (entity, prop)) >> 8;
+	      if (NILP (AREF (entity, prop)))
+		  prop = FONT_SPEC_MAX;
+	      else {
+		int required = XFIXNUM (AREF (spec, prop)) >> 8;
+		int candidate = XFIXNUM (AREF (entity, prop)) >> 8;
 
-	      if (candidate != required
+		if (candidate != required
 #ifdef HAVE_NTGUI
-		  /* A kludge for w32 font search, where listing a
-		     family returns only 4 standard weights: regular,
-		     italic, bold, bold-italic.  For other values one
-		     must specify the font, not just the family in the
-		     :family attribute of the face.  But specifying
-		     :family in the face attributes looks for regular
-		     weight, so if we require exact match, the
-		     non-regular font will be rejected.  So we relax
-		     the accuracy of the match here, and let
-		     font_sort_entities find the best match.  */
-		  && (prop != FONT_WEIGHT_INDEX
-		      || eabs (candidate - required) > 100)
+		    /* A kludge for w32 font search, where listing a
+		       family returns only 4 standard weights:
+		       regular, italic, bold, bold-italic.  For other
+		       values one must specify the font, not just the
+		       family in the :family attribute of the face.
+		       But specifying :family in the face attributes
+		       looks for regular weight, so if we require
+		       exact match, the non-regular font will be
+		       rejected.  So we relax the accuracy of the
+		       match here, and let font_sort_entities find the
+		       best match.  */
+		    && (prop != FONT_WEIGHT_INDEX
+			|| eabs (candidate - required) > 100)
 #endif
-		  )
-		prop = FONT_SPEC_MAX;
+		    )
+		  prop = FONT_SPEC_MAX;
+	      }
 	    }
 	}
       if (prop < FONT_SPEC_MAX
-- 
2.30.2

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 52888@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

Hello,

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

>> From: Sean Whitton <spwhitton@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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Skip-virtual-FcPattern-entries-for-variable-weight-f.patch --]
[-- Type: text/x-patch, Size: 1976 bytes --]

From 4747fe3072c6fca0385203cef9b6e2661badaaba Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 4 Jan 2022 19:07:29 -0700
Subject: [PATCH] Skip virtual FcPattern entries for variable weight fonts

* src/ftfont.c (ftfont_list): Pass FC_VARIABLE to FcObjectSetBuild.
* src/ftfont.c (ftfont_pattern_entity): Skip meta/virtual FcPattern
entries for variable weight fonts (Bug#52888).
---
 src/ftfont.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/ftfont.c b/src/ftfont.c
index cf592759ab..73276fdc86 100644
--- 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;
 
   file = (char *) str;
   key = Fcons (build_unibyte_string (file), make_fixnum (idx));
@@ -853,7 +864,7 @@ ftfont_list (struct frame *f, Lisp_Object spec)
     adstyle = Qnil;
   objset = FcObjectSetBuild (FC_FOUNDRY, FC_FAMILY, FC_WEIGHT, FC_SLANT,
 			     FC_WIDTH, FC_PIXEL_SIZE, FC_SPACING, FC_SCALABLE,
-			     FC_STYLE, FC_FILE, FC_INDEX,
+			     FC_STYLE, FC_FILE, FC_INDEX, FC_VARIABLE,
 #ifdef FC_CAPABILITY
 			     FC_CAPABILITY,
 #endif	/* FC_CAPABILITY */
-- 
2.30.2

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Robert Pluim]

>>>>> On Wed, 05 Jan 2022 14:37:56 +0200, Eli Zaretskii <eliz@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
-- 

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Sean Whitton <spwhitton@spwhitton.name>,  52888@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-Skip-virtual-FcPattern-entries-for-variable-weigh.patch --]
[-- Type: text/x-patch, Size: 1789 bytes --]

From d88f23882b66243b42d79e0fe897f99f8125c7c4 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 4 Jan 2022 19:07:29 -0700
Subject: [PATCH v2] Skip virtual FcPattern entries for variable weight fonts

* src/ftfont.c (ftfont_list): Pass FC_VARIABLE to FcObjectSetBuild.
* src/ftfont.c (ftfont_pattern_entity): Skip meta/virtual FcPattern
entries for variable weight fonts (Bug#52888).
---
 src/ftfont.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/ftfont.c b/src/ftfont.c
index 2bdcce306b..6bfbc50ffa 100644
--- a/src/ftfont.c
+++ b/src/ftfont.c
@@ -189,6 +189,19 @@ ftfont_pattern_entity (FcPattern *p, Lisp_Object extra)
     return Qnil;
   if (FcPatternGetInteger (p, FC_INDEX, 0, &idx) != FcResultMatch)
     return Qnil;
+#ifdef FC_VARIABLE
+  FcRange *range;
+  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;
+#endif	/* FC_VARIABLE */
 
   file = (char *) str;
   key = Fcons (build_unibyte_string (file), make_fixnum (idx));
@@ -863,6 +876,9 @@ ftfont_list (struct frame *f, Lisp_Object spec)
 #if defined HAVE_XFT && defined FC_COLOR
                              FC_COLOR,
 #endif
+#ifdef FC_VARIABLE
+			     FC_VARIABLE,
+#endif	/* FC_VARIABLE */
 			     NULL);
   if (! objset)
     goto err;
-- 
2.30.2

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 52888@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v3-0001-Skip-virtual-FcPattern-entries-for-variable-weigh.patch --]
[-- Type: text/x-patch, Size: 2029 bytes --]

From 8f2c3b969af77e707429ac4a51fa439831c5d075 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Tue, 4 Jan 2022 19:07:29 -0700
Subject: [PATCH v3] Skip virtual FcPattern entries for variable weight fonts

* src/ftfont.c (ftfont_list): Pass FC_VARIABLE to FcObjectSetBuild.
* src/ftfont.c (ftfont_pattern_entity): Skip meta/virtual FcPattern
entries for variable weight fonts (Bug#52888).
---
 src/ftfont.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/src/ftfont.c b/src/ftfont.c
index 2bdcce306b..5797300d23 100644
--- a/src/ftfont.c
+++ b/src/ftfont.c
@@ -189,6 +189,24 @@ ftfont_pattern_entity (FcPattern *p, Lisp_Object extra)
     return Qnil;
   if (FcPatternGetInteger (p, FC_INDEX, 0, &idx) != FcResultMatch)
     return Qnil;
+#ifdef FC_VARIABLE
+  /* 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.
+
+     Fontconfig started generating virtual/meta patterns for variable
+     weight fonts in the same release that FC_VARIABLE was added, so
+     we conditionalize on that constant.  This also ensures that
+     FcPatternGetRange is available.  */
+  FcRange *range;
+  if (FcPatternGetRange (p, FC_WEIGHT, 0, &range) == FcResultMatch
+      && FcPatternGetBool (p, FC_VARIABLE, 0, &b) == FcResultMatch
+      && b == FcTrue)
+    return Qnil;
+#endif	/* FC_VARIABLE */
 
   file = (char *) str;
   key = Fcons (build_unibyte_string (file), make_fixnum (idx));
@@ -863,6 +881,9 @@ ftfont_list (struct frame *f, Lisp_Object spec)
 #if defined HAVE_XFT && defined FC_COLOR
                              FC_COLOR,
 #endif
+#ifdef FC_VARIABLE
+			     FC_VARIABLE,
+#endif	/* FC_VARIABLE */
 			     NULL);
   if (! objset)
     goto err;
-- 
2.30.2

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 52888@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Sean Whitton]

Hello,

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

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Cc: 52888@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

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 52888@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@spwhitton.name>
> >> Cc: 52888@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.

bug#52888: 29.0.50; font_{delete_unmatched,score} do not handle nil FONT_WEIGHT_INDEX

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@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.

Can't set conditional break point using AREF/NILP

[Sean Whitton]

[moving to emacs-devel]

On Sat 01 Jan 2022 at 08:56AM +02, Eli Zaretskii wrote:

>> From: Sean Whitton <spwhitton@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.

Can confirm mine is.

> Perhaps try
>
>   condition NN AREF (entity, FONT_WEIGHT_INDEX) == Qnil
>
> instead.

This fails in the same way.  This is what I see:

Breakpoint 2 at 0x335d30: file ftfont.c, line 231.
(gdb) condition 2 AREF (entity, FONT_WEIGHT_INDEX) == Qnil
(gdb) r
`/home/spwhitton/src/emacs/src/emacs' has changed; re-reading symbols.
Starting program: /home/spwhitton/src/emacs/src/emacs
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff15c4700 (LWP 2073615)]
[New Thread 0x7ffff0bca700 (LWP 2073616)]
[New Thread 0x7fffebfff700 (LWP 2073617)]
[New Thread 0x7fffeb254700 (LWP 2073618)]
[New Thread 0x7fffea8c3700 (LWP 2073619)]
[New Thread 0x7fffe9f32700 (LWP 2073620)]
Error in testing breakpoint condition:
Couldn't get registers: No such process.
An error occurred while in a function called from GDB.
Evaluation of the expression containing the function
(AREF) will be abandoned.
When the function is done executing, GDB will silently stop.
Selected thread is running.
(gdb) [Switching to thread 1 (Thread 0x7ffff237b040 (LWP 2073608))]
#0  ftfont_pattern_entity (p=0x55555643d660, extra=XIL(0x555555f91953)) at ftfont.c:230
230	  if (FcPatternGetInteger (p, FC_SLANT, 0, &numeric) == FcResultMatch)

And then gdb does break at the break point, but it's badly broken:

(gdb) p AREF (entity, FONT_WEIGHT_INDEX) == Qnil
[Thread 0x7fffea8c3700 (LWP 2073619) exited]
[Thread 0x7fffe9f32700 (LWP 2073620) exited]
Attempt to take address of value not located in memory.
(gdb)

-- 
Sean Whitton

Re: Can't set conditional break point using AREF/NILP

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sat, 01 Jan 2022 14:49:05 -0700
> 
> >>     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.
> 
> Can confirm mine is.
> 
> > Perhaps try
> >
> >   condition NN AREF (entity, FONT_WEIGHT_INDEX) == Qnil
> >
> > instead.
> 
> This fails in the same way.  This is what I see:
> 
> Breakpoint 2 at 0x335d30: file ftfont.c, line 231.
> (gdb) condition 2 AREF (entity, FONT_WEIGHT_INDEX) == Qnil
> (gdb) r
> `/home/spwhitton/src/emacs/src/emacs' has changed; re-reading symbols.
> Starting program: /home/spwhitton/src/emacs/src/emacs
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> [New Thread 0x7ffff15c4700 (LWP 2073615)]
> [New Thread 0x7ffff0bca700 (LWP 2073616)]
> [New Thread 0x7fffebfff700 (LWP 2073617)]
> [New Thread 0x7fffeb254700 (LWP 2073618)]
> [New Thread 0x7fffea8c3700 (LWP 2073619)]
> [New Thread 0x7fffe9f32700 (LWP 2073620)]
> Error in testing breakpoint condition:
> Couldn't get registers: No such process.

This error message already says it won't work, although I don't
understand why.  The "No such process" part is especially worrisome.
It works here, FWIW.

What version of GDB is that, and was it built with Python support?
I have GDB 11.1 here.

Also, did you perhaps use the --enable-check-lisp-object-type
configure-time switch?  If so, please rebuild without it.

Re: Can't set conditional break point using AREF/NILP

[Sean Whitton]

Hello,

On Sun 02 Jan 2022 at 08:42AM +02, Eli Zaretskii wrote:

> What version of GDB is that, and was it built with Python support?
> I have GDB 11.1 here.

I have the one in Debian bullseye: GNU gdb (Debian 10.1-1.7)
10.1.90.20210103-git.  Looks like it has Python support:

    ~ % gdb --configuration | grep python
                 --with-python=/usr (relocatable)
                 --with-python-libdir=/usr/lib (relocatable)

> Also, did you perhaps use the --enable-check-lisp-object-type
> configure-time switch?  If so, please rebuild without it.

I did have that on, but removing it does not help.  I did

    ./configure
        --config-cache --with-native-compilation --with-pgtk
        --enable-checking='yes,glyphs' CFLAGS='-O0 -g3' && make

and I get a similar-looking error:

    Breakpoint 2 at 0x345830: file ftfont.c, line 230.
    (gdb) condition 2 AREF (entity, FONT_WEIGHT_INDEX) == Qnil
    (gdb) r
    Starting program: /home/spwhitton/src/emacs/src/emacs 
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
    [New Thread 0x7fffef6a2700 (LWP 2427493)]
    [New Thread 0x7fffee7a8700 (LWP 2427494)]
    [New Thread 0x7fffeddee700 (LWP 2427495)]
    [New Thread 0x7fffed032700 (LWP 2427496)]
    [New Thread 0x7fffdffff700 (LWP 2427497)]
    [New Thread 0x7fffdf66e700 (LWP 2427498)]
    Error in testing breakpoint condition:
    Couldn't get registers: No such process.
    An error occurred while in a function called from GDB.
    Evaluation of the expression containing the function
    (AREF) will be abandoned.
    When the function is done executing, GDB will silently stop.
    Selected thread is running.
    (gdb) [Switching to thread 1 (Thread 0x7ffff046e400 (LWP 2427486))]
    #0  ftfont_pattern_entity (p=0x5555564dff20, extra=XIL(0x55555620bdf3)) at ftfont.c:230
    230	  if (FcPatternGetInteger (p, FC_SLANT, 0, &numeric) == FcResultMatch)

-- 
Sean Whitton

Re: Can't set conditional break point using AREF/NILP

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sun, 02 Jan 2022 15:38:52 -0700
> 
>     ./configure
>         --config-cache --with-native-compilation --with-pgtk
>         --enable-checking='yes,glyphs' CFLAGS='-O0 -g3' && make
> 
> and I get a similar-looking error:
> 
>     Breakpoint 2 at 0x345830: file ftfont.c, line 230.
>     (gdb) condition 2 AREF (entity, FONT_WEIGHT_INDEX) == Qnil
>     (gdb) r
>     Starting program: /home/spwhitton/src/emacs/src/emacs 
>     [Thread debugging using libthread_db enabled]
>     Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>     [New Thread 0x7fffef6a2700 (LWP 2427493)]
>     [New Thread 0x7fffee7a8700 (LWP 2427494)]
>     [New Thread 0x7fffeddee700 (LWP 2427495)]
>     [New Thread 0x7fffed032700 (LWP 2427496)]
>     [New Thread 0x7fffdffff700 (LWP 2427497)]
>     [New Thread 0x7fffdf66e700 (LWP 2427498)]
>     Error in testing breakpoint condition:
>     Couldn't get registers: No such process.
>     An error occurred while in a function called from GDB.
>     Evaluation of the expression containing the function
>     (AREF) will be abandoned.
>     When the function is done executing, GDB will silently stop.
>     Selected thread is running.

Then maybe it's a problem with your system architecture, and you need
to expand the macro "by hand" for GDB to swallow it.  Sorry I couldn't
be of more help.

Re: Can't set conditional break point using AREF/NILP

[Sean Whitton]

Hello,

On Mon 03 Jan 2022 at 02:28PM +02, Eli Zaretskii wrote:

> Then maybe it's a problem with your system architecture, and you need
> to expand the macro "by hand" for GDB to swallow it.  Sorry I couldn't
> be of more help.

No problem.  Next time, I'll try expanding the macro by hand.

When you asked me to remove --enable-check-lisp-object-type, was that
purely for debugging purposes, or is using the macros in break point
conditions with that switch on expected not to work?  If the latter, I'd
like to document it in etc/DEBUG.

Ftr my system is very standard Debian bullseye.

-- 
Sean Whitton

Re: Can't set conditional break point using AREF/NILP

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: emacs-devel@gnu.org
> Date: Tue, 04 Jan 2022 14:57:32 -0700
> 
> When you asked me to remove --enable-check-lisp-object-type, was that
> purely for debugging purposes, or is using the macros in break point
> conditions with that switch on expected not to work?  If the latter, I'd
> like to document it in etc/DEBUG.

Comparisons with structures, like foo == Qnil, aren't guaranteed to
work in GDB.  --enable-check-lisp-object-type makes Lisp objects be
structures rather than simple scalar values.

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Sean Whitton]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Hello,

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

> Comparisons with structures, like foo == Qnil, aren't guaranteed to
> work in GDB.  --enable-check-lisp-object-type makes Lisp objects be
> structures rather than simple scalar values.

Thanks -- how about the attached?

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-etc-DEBUG-Say-more-about-enable-check-lisp-object-ty.patch --]
[-- Type: text/x-patch, Size: 2139 bytes --]

From eaf40f04614063010e60d14c9ba8ba48e4b842bc Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Thu, 6 Jan 2022 11:17:44 -0700
Subject: [PATCH] etc/DEBUG: Say more about --enable-check-lisp-object-type

* etc/DEBUG: Explain how --enable-check-lisp-object-type can obstruct
certain debugging strategies.  Don't claim that this switch has no
effect on debugging with GDB.
---
 etc/DEBUG | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/etc/DEBUG b/etc/DEBUG
index dd33b42f19..6d0e27cbce 100644
--- a/etc/DEBUG
+++ b/etc/DEBUG
@@ -28,12 +28,21 @@ exceptionally hard.
 Older versions of GCC may need more than just the -g3 flag.  For more,
 search for "analyze failed assertions" below.
 
-The 2 --enable-* switches are optional.  They don't have any effect on
-debugging with GDB, but will compile additional code that might catch
-the problem you are debugging much earlier, in the form of assertion
-violation.  The --enable-checking option also enables additional
-functionality useful for debugging display problems; see more about
-this below under "Debugging Emacs redisplay problems".
+The 2 --enable-* switches are optional.  They compile additional code
+that might catch the problem you are debugging much earlier, in the
+form of assertion violation.  The --enable-checking option also
+enables additional functionality useful for debugging display
+problems; see more about this below under "Debugging Emacs redisplay
+problems".
+
+You should be aware that the --enable-check-lisp-object-type option
+changes Lisp objects from scalar values into structs, and this can
+obstruct certain debugging strategies.  In particular, asking GDB to
+compare Lisp objects might not work.  For example, consider this
+attempt to conditionalize a break point: "condition 2 NILP (foo)".  In
+this case you are requesting that GDB make the comparison "foo ==
+Qnil", and you might need to recompile without
+--enable-check-lisp-object-type for that to work.
 
 Emacs needs not be installed to be debugged, you can debug the binary
 created in the 'src' directory.
-- 
2.30.2

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Thu, 06 Jan 2022 11:20:19 -0700
> 
> > Comparisons with structures, like foo == Qnil, aren't guaranteed to
> > work in GDB.  --enable-check-lisp-object-type makes Lisp objects be
> > structures rather than simple scalar values.
> 
> Thanks -- how about the attached?

Thanks, but the text there is inaccurate: NILP should still work with
structures, only == comparison probably won't.

I'm actually not sure we should describe such subtleties in etc/DEBUG.

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Sean Whitton]

Hello,

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

>> From: Sean Whitton <spwhitton@spwhitton.name>
>> Date: Thu, 06 Jan 2022 11:20:19 -0700
>>
>> > Comparisons with structures, like foo == Qnil, aren't guaranteed to
>> > work in GDB.  --enable-check-lisp-object-type makes Lisp objects be
>> > structures rather than simple scalar values.
>>
>> Thanks -- how about the attached?
>
> Thanks, but the text there is inaccurate: NILP should still work with
> structures, only == comparison probably won't.
>
> I'm actually not sure we should describe such subtleties in etc/DEBUG.

Fair -- what do you think about just saying that certain gdb features
might not work, so one thing that you can try is to drop the flag?  The
claim that it doesn't affect debugging at all is what threw me off
recently, so that's the thing I'd really like to improve, I guess.

-- 
Sean Whitton

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 53058@debbugs.gnu.org
> Date: Thu, 06 Jan 2022 16:46:24 -0700
> 
> Hello,
> 
> On Thu 06 Jan 2022 at 10:11PM +02, Eli Zaretskii wrote:
> 
> >> From: Sean Whitton <spwhitton@spwhitton.name>
> >> Date: Thu, 06 Jan 2022 11:20:19 -0700
> >>
> >> > Comparisons with structures, like foo == Qnil, aren't guaranteed to
> >> > work in GDB.  --enable-check-lisp-object-type makes Lisp objects be
> >> > structures rather than simple scalar values.
> >>
> >> Thanks -- how about the attached?
> >
> > Thanks, but the text there is inaccurate: NILP should still work with
> > structures, only == comparison probably won't.
> >
> > I'm actually not sure we should describe such subtleties in etc/DEBUG.
> 
> Fair -- what do you think about just saying that certain gdb features
> might not work, so one thing that you can try is to drop the flag?  The
> claim that it doesn't affect debugging at all is what threw me off
> recently, so that's the thing I'd really like to improve, I guess.

But it really doesn't affect the debugging.  You just need to
understand what it does, and if we describe this in etc/DEBUG, we'd
need to describe gobs of other similar factoids about the various
build options.

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Sean Whitton]

Hello,

On Fri 07 Jan 2022 at 08:58AM +02, Eli Zaretskii wrote:

> But it really doesn't affect the debugging.  You just need to
> understand what it does, and if we describe this in etc/DEBUG, we'd
> need to describe gobs of other similar factoids about the various
> build options.

Well, it doesn't affect the debugging in the way that the CFLAGS
discussed there do, sure, but from a less experienced gdb user's point
of view, it does indeed affect the debugging.  In my case, I encountered
a problem setting a conditional break point, and I wouldn't have even
considered revisiting that flag because I had just read text which I
took to be telling me that the flag wasn't connected with the way gdb
works.  I had to wait for your input to learn that it is connected, in a
relevant sense.

To me, conditionalising break points is a fundamental debugging action,
something that a beginner is likely to want to do.  And it's Emacs, so
chances are those conditions will involve Lisp objects.  So having
instructions aimed at someone just getting started telling you to use a
flag which makes that basic debugging action much less likely to work
seems worth improving.  Do you have any other ideas as to how that could
be done?

Thanks.

-- 
Sean Whitton

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 53058@debbugs.gnu.org
> Date: Fri, 07 Jan 2022 13:41:52 -0700
> 
> To me, conditionalising break points is a fundamental debugging action,
> something that a beginner is likely to want to do.  And it's Emacs, so
> chances are those conditions will involve Lisp objects.  So having
> instructions aimed at someone just getting started telling you to use a
> flag which makes that basic debugging action much less likely to work
> seems worth improving.  Do you have any other ideas as to how that could
> be done?

The reasons for the problem in your case is not known: NILP should
have worked, as did the comparison with Qnil.  Until we understand why
those didn't work, I don't see how we can say anything in etc/DEBUG
that would both be useful and important/general enough to have there.

My best advice is to try to understand why those conditions didn't
work on your system.  Maybe someone here knowns, or maybe you should
ask on the GDB mailing list (gdb@sourceware.org).

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Sean Whitton]

Hello,

On Sat 08 Jan 2022 at 08:55am +02, Eli Zaretskii wrote:

> The reasons for the problem in your case is not known: NILP should
> have worked, as did the comparison with Qnil.  Until we understand why
> those didn't work, I don't see how we can say anything in etc/DEBUG
> that would both be useful and important/general enough to have there.
>
> My best advice is to try to understand why those conditions didn't
> work on your system.  Maybe someone here knowns, or maybe you should
> ask on the GDB mailing list (gdb@sourceware.org).

If I understand you correctly: we don't yet know exactly what was going
wrong on my machine when I tried to set the conditional break point, so
we lack a concrete case in which --enable-check-lisp-object-type
affected debugging with GDB, and we are thus not in a position to assert
in etc/DEBUG that --enable-check-lisp-object-type has the potential to
have an effect on debugging with gdb?

When you wrote that comparisons with structs are not guaranteed to work
with gdb, you were /not/ going as far as saying that comparisons with
structs are never expected to work (because that would imply that
--enable-check-lisp-object-type would often lead to issues when
debugging)?

If this summary is right, I think this bug should be closed.

Thanks.

-- 
Sean Whitton

bug#53058: etc/DEBUG could say more about --enable-check-lisp-object-type

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Cc: 53058@debbugs.gnu.org
> Date: Wed, 02 Feb 2022 17:19:04 -0700
> 
> Hello,
> 
> On Sat 08 Jan 2022 at 08:55am +02, Eli Zaretskii wrote:
> 
> > The reasons for the problem in your case is not known: NILP should
> > have worked, as did the comparison with Qnil.  Until we understand why
> > those didn't work, I don't see how we can say anything in etc/DEBUG
> > that would both be useful and important/general enough to have there.
> >
> > My best advice is to try to understand why those conditions didn't
> > work on your system.  Maybe someone here knowns, or maybe you should
> > ask on the GDB mailing list (gdb@sourceware.org).
> 
> If I understand you correctly: we don't yet know exactly what was going
> wrong on my machine when I tried to set the conditional break point, so
> we lack a concrete case in which --enable-check-lisp-object-type
> affected debugging with GDB, and we are thus not in a position to assert
> in etc/DEBUG that --enable-check-lisp-object-type has the potential to
> have an effect on debugging with gdb?

Yes.

> When you wrote that comparisons with structs are not guaranteed to work
> with gdb, you were /not/ going as far as saying that comparisons with
> structs are never expected to work (because that would imply that
> --enable-check-lisp-object-type would often lead to issues when
> debugging)?

Yes.

> If this summary is right, I think this bug should be closed.

OK, closing.

bug#52888:

[André Silva]

[-- Attachment #1.1: Type: text/plain, Size: 248 bytes --]

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.

[-- Attachment #1.2: Type: text/html, Size: 301 bytes --]

[-- Attachment #2: emacs-fix-font.patch --]
[-- Type: text/x-patch, Size: 640 bytes --]

diff --git a/src/font.c b/src/font.c
index 56a921da9..64dd12abc 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2751,9 +2751,9 @@ font_delete_unmatched (Lisp_Object vec, Lisp_Object spec, int size)
 	}
       for (prop = FONT_WEIGHT_INDEX; prop < FONT_SIZE_INDEX; prop++)
 	if (FIXNUMP (AREF (spec, prop))
-	    && ! (FIXNUMP (AREF (entity, prop))
-		  && ((XFIXNUM (AREF (spec, prop)) >> 8)
-		      == (XFIXNUM (AREF (entity, prop)) >> 8))))
+	    && FIXNUMP (AREF (entity, prop))
+	    && ((XFIXNUM (AREF (spec, prop)) >> 8)
+		!= (XFIXNUM (AREF (entity, prop)) >> 8)))
 	  prop = FONT_SPEC_MAX;
       if (prop < FONT_SPEC_MAX
 	  && size

bug#52888:

[Eli Zaretskii]

> From: André Silva <andre.beat@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?

bug#52888:

[Eli Zaretskii]

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

> From: André Silva <andre.beat@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?

bug#52888:

[Eli Zaretskii]

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

> From: André Silva <andre.beat@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.