Replace XChar2b with unsigned in all font backends

Replace XChar2b with unsigned in all font backends

[martin rudalics]

After this commit, building master on Windows here produces



   CC       w32term.o
../../src/w32font.c: In function 'w32font_draw':
../../src/w32font.c:708:25: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
         s->char2b + from + i, 1, NULL);
         ~~~~~~~~~~~~~~~~~^~~
In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
                  from ../../src/w32font.c:20:
C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
                                                                                             ~~~~~~~~^~~~~~~~
../../src/w32font.c:711:57: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
      ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
                                                ~~~~~~~~~~^~~~~~
In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
                  from ../../src/w32font.c:20:
C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
                                                                                             ~~~~~~~~^~~~~~~~
   CC       w32xfns.o



and the resulting build is not usable.

martin

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Mon 20 May 2019, martin rudalics wrote:

> After this commit, building master on Windows here produces
>
>   CC       w32term.o
> ../../src/w32font.c: In function 'w32font_draw':
> ../../src/w32font.c:708:25: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
>         s->char2b + from + i, 1, NULL);
>         ~~~~~~~~~~~~~~~~~^~~
> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
>                  from ../../src/w32font.c:20:
> C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
>    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
>                                                                                             ~~~~~~~~^~~~~~~~
> ../../src/w32font.c:711:57: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
>      ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
>                                                ~~~~~~~~~~^~~~~~
> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
>                  from ../../src/w32font.c:20:
> C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
>    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
>                                                                                             ~~~~~~~~^~~~~~~~
>   CC       w32xfns.o
>
> and the resulting build is not usable.
>
> martin

Confirmed. This patch replaces use of XChar2b (a 16bit type) with
unsigned (usually 32bit) which seems wrong.

    AndyM

Re: Replace XChar2b with unsigned in all font backends

[Alex Gramiak]

Andy Moreton <andrewjmoreton@gmail.com> writes:

> Confirmed. This patch replaces use of XChar2b (a 16bit type) with
> unsigned (usually 32bit) which seems wrong.
>
>     AndyM

I originally used unsigned short for this patch, but Eli[1] nudged me
towards unsigned. Unsigned fits better with other parts of the font
system, e.g., *encode_char returns unsigned, and *text_extents takes a
pointer to unsigned.

[1] https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00457.html

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Mon 20 May 2019, Alex Gramiak wrote:

> Andy Moreton <andrewjmoreton@gmail.com> writes:
>
>> Confirmed. This patch replaces use of XChar2b (a 16bit type) with
>> unsigned (usually 32bit) which seems wrong.
>>
>>     AndyM
>
> I originally used unsigned short for this patch, but Eli[1] nudged me
> towards unsigned. Unsigned fits better with other parts of the font
> system, e.g., *encode_char returns unsigned, and *text_extents takes a
> pointer to unsigned.
>
> [1] https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00457.html

Yes, but Eli was concerned with efficiency, but correctness comes first.
If you are calling APIs that expect a pointer to an array of 16bit
values, then an array of 32bit values will not suffice.

Many of the comments around this code talk of 2-byte values, so
changing the code to use 4-byte values is surprising to the reader.

The changes also removed the explicit packing/unpacking of 16bit values,
which may give rise to endianness issues on some systems.

Please revert this patch to fix the build on master, and then revisit
these changes after that.

    AndyM

Re: Replace XChar2b with unsigned in all font backends

[Eli Zaretskii]

On May 20, 2019 7:29:34 PM GMT+01:00, Andy Moreton <andrewjmoreton@gmail.com> wrote:
> On Mon 20 May 2019, Alex Gramiak wrote:
> 
> > Andy Moreton <andrewjmoreton@gmail.com> writes:
> >
> >> Confirmed. This patch replaces use of XChar2b (a 16bit type) with
> >> unsigned (usually 32bit) which seems wrong.
> >>
> >>     AndyM
> >
> > I originally used unsigned short for this patch, but Eli[1] nudged
> me
> > towards unsigned. Unsigned fits better with other parts of the font
> > system, e.g., *encode_char returns unsigned, and *text_extents takes
> a
> > pointer to unsigned.
> >
> > [1]
> https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00457.html
> 
> Yes, but Eli was concerned with efficiency, but correctness comes
> first.
> If you are calling APIs that expect a pointer to an array of 16bit
> values, then an array of 32bit values will not suffice.
> 
> Many of the comments around this code talk of 2-byte values, so
> changing the code to use 4-byte values is surprising to the reader.
> 
> The changes also removed the explicit packing/unpacking of 16bit
> values,
> which may give rise to endianness issues on some systems.
> 
> Please revert this patch to fix the build on master, and then revisit
> these changes after that.
> 
>     AndyM

 You ask to revert because you don't think there's a way of fixing this without reverting?  Or fof some other reason?

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Mon 20 May 2019, Eli Zaretskii wrote:

> On May 20, 2019 7:29:34 PM GMT+01:00, Andy Moreton <andrewjmoreton@gmail.com> wrote:
>> Please revert this patch to fix the build on master, and then revisit
>> these changes after that.
>> 
>  You ask to revert because you don't think there's a way of fixing this without reverting?  Or fof some other reason?

The first order of business is to restore master to a usable state.
After that, any amount of time can be taken to revise the non-working
patch into something that does not cause the regression.

"Don't break the build" is an important rule. A recent Rx patch was
reverted by its author because it broke bootstrapping, and then a
revised version added later after the problem with the original patch
had been fixed. Why should this patch be any different ?

    AndyM

Re: Replace XChar2b with unsigned in all font backends

[Eli Zaretskii]

On May 20, 2019 11:52:30 PM GMT+01:00, Andy Moreton <andrewjmoreton@gmail.com> wrote:
> On Mon 20 May 2019, Eli Zaretskii wrote:
> 
> > On May 20, 2019 7:29:34 PM GMT+01:00, Andy Moreton
> <andrewjmoreton@gmail.com> wrote:
> >> Please revert this patch to fix the build on master, and then
> revisit
> >> these changes after that.
> >> 
> >  You ask to revert because you don't think there's a way of fixing
> this without reverting?  Or fof some other reason?
> 
> The first order of business is to restore master to a usable state.
> After that, any amount of time can be taken to revise the non-working
> patch into something that does not cause the regression.
> 
> "Don't break the build" is an important rule. A recent Rx patch was
> reverted by its author because it broke bootstrapping, and then a
> revised version added later after the problem with the original patch
> had been fixed. Why should this patch be any different ?
> 
>     AndyM

We are miscommunicating, I think.  I didn't mean the problem shouldn't be resolved ASAP, I meant it should be resolved by fixing the fallout from that commit rather then by reverting it.

One way of fixing that is by copying values from array of unsigned int to array of unsigned short.  Maybe there are others, I'm currently away of the sources and cannot say more.

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Tue 21 May 2019, Eli Zaretskii wrote:

> We are miscommunicating, I think. I didn't mean the problem shouldn't be
> resolved ASAP, I meant it should be resolved by fixing the fallout from that
> commit rather then by reverting it.

Indeed - the end goal is useful, but it seems we have different views on
how to get there. This patch has problems on Windows (described in this
thread), and on Linux (bug 35814).

Trying to fix it up in place will leave master broken for some time, so
I think an initial revert followed by reworking the patch in smaller
steps may be an easier way forward.

    AndyM

Re: Replace XChar2b with unsigned in all font backends

[Eli Zaretskii]

On May 21, 2019 12:50:59 PM GMT+01:00, Andy Moreton <andrewjmoreton@gmail.com> wrote:
> On Tue 21 May 2019, Eli Zaretskii wrote:
> 
> > We are miscommunicating, I think. I didn't mean the problem
> shouldn't be
> > resolved ASAP, I meant it should be resolved by fixing the fallout
> from that
> > commit rather then by reverting it.
> 
> Indeed - the end goal is useful, but it seems we have different views
> on
> how to get there. This patch has problems on Windows (described in
> this
> thread), and on Linux (bug 35814).
> 
> Trying to fix it up in place will leave master broken for some time,
> so
> I think an initial revert followed by reworking the patch in smaller
> steps may be an easier way forward.
> 
>     AndyM

 Indeed, I think that reverting is a destructive and generally less efficient way of fixing the problem when the changeset in question is mostly correct.

Re: Replace XChar2b with unsigned in all font backends

[Alex Gramiak]

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

Andy Moreton <andrewjmoreton@gmail.com> writes:

> On Mon 20 May 2019, Alex Gramiak wrote:
>
>> Andy Moreton <andrewjmoreton@gmail.com> writes:
>>
>>> Confirmed. This patch replaces use of XChar2b (a 16bit type) with
>>> unsigned (usually 32bit) which seems wrong.
>>>
>>>     AndyM
>>
>> I originally used unsigned short for this patch, but Eli[1] nudged me
>> towards unsigned. Unsigned fits better with other parts of the font
>> system, e.g., *encode_char returns unsigned, and *text_extents takes a
>> pointer to unsigned.
>>
>> [1] https://lists.gnu.org/archive/html/emacs-devel/2019-05/msg00457.html
>
> Yes, but Eli was concerned with efficiency, but correctness comes first.
> If you are calling APIs that expect a pointer to an array of 16bit
> values, then an array of 32bit values will not suffice.

Right, at least not without converting the array; the xfont backend does
this when the API expects an array of 8bit values.

> Many of the comments around this code talk of 2-byte values, so
> changing the code to use 4-byte values is surprising to the reader.

I agree, but at least the char2b name signifies that it's a 2-byte
value.

> The changes also removed the explicit packing/unpacking of 16bit values,
> which may give rise to endianness issues on some systems.

I haven't dealt with endianness issues before, but from a quick search
[1][2] it doesn't appear that it's an issue:

[1] https://developer.ibm.com/articles/au-endianc/
[2] https://stackoverflow.com/a/7184905

> Please revert this patch to fix the build on master, and then revisit
> these changes after that.

I think that the solution is just to do a conversion on the w32font_draw
side. Can you confirm if the following diff fixes the build for you?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: w32font v2 --]
[-- Type: text/x-patch, Size: 889 bytes --]

diff --git a/src/w32font.c b/src/w32font.c
index bd68e22cc9..d3ec2916a5 100644
--- a/src/w32font.c
+++ b/src/w32font.c
@@ -704,11 +704,20 @@ w32font_draw (struct glyph_string *s, int from, int to,
       int i;
 
       for (i = 0; i < len; i++)
-	ExtTextOutW (s->hdc, x + i, y, options, NULL,
-		     s->char2b + from + i, 1, NULL);
+        {
+          const wchar_t ch = s->char2b[from + i];
+          ExtTextOutW (s->hdc, x + i, y, options, NULL, &ch, 1, NULL);
+        }
     }
   else
-    ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
+    {
+      USE_SAFE_ALLOCA;
+      wchar_t *str = SAFE_ALLOCA (len * (sizeof (*str)));
+      for (int i = 0; i < len; ++i)
+        str[i] = s->char2b[from + i];
+      ExtTextOutW (s->hdc, x, y, options, NULL, str, len, NULL);
+      SAFE_FREE ();
+    }
 
   /* Restore clip region.  */
   if (s->num_clips > 0)

Re: Replace XChar2b with unsigned in all font backends

[Alex Gramiak]

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

martin rudalics <rudalics@gmx.at> writes:

> After this commit, building master on Windows here produces
>
>
>
>   CC       w32term.o
> ../../src/w32font.c: In function 'w32font_draw':
> ../../src/w32font.c:708:25: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
>         s->char2b + from + i, 1, NULL);
>         ~~~~~~~~~~~~~~~~~^~~
> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
>                  from ../../src/w32font.c:20:
> C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
>    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
>                                                                                             ~~~~~~~~^~~~~~~~
> ../../src/w32font.c:711:57: warning: passing argument 6 of 'ExtTextOutW' from incompatible pointer type [-Wincompatible-pointer-types]
>      ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
>                                                ~~~~~~~~~~^~~~~~
> In file included from C:/msys64/mingw64/x86_64-w64-mingw32/include/windows.h:71,
>                  from ../../src/w32font.c:20:
> C:/msys64/mingw64/x86_64-w64-mingw32/include/wingdi.h:3347:100: note: expected 'LPCWSTR' {aka 'const short unsigned int *'} but argument is of type 'unsigned int *'
>    WINGDIAPI WINBOOL WINAPI ExtTextOutW(HDC hdc,int x,int y,UINT options,CONST RECT *lprect,LPCWSTR lpString,UINT c,CONST INT *lpDx);
>                                                                                             ~~~~~~~~^~~~~~~~
>   CC       w32xfns.o
>
>
>
> and the resulting build is not usable.
>
> martin

Does the following diff work for you?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: w32font --]
[-- Type: text/x-patch, Size: 871 bytes --]

diff --git a/src/w32font.c b/src/w32font.c
index bd68e22cc9..fe85601380 100644
--- a/src/w32font.c
+++ b/src/w32font.c
@@ -704,11 +704,20 @@ w32font_draw (struct glyph_string *s, int from, int to,
       int i;
 
       for (i = 0; i < len; i++)
-	ExtTextOutW (s->hdc, x + i, y, options, NULL,
-		     s->char2b + from + i, 1, NULL);
+        {
+          const wchar_t ch = s->char2b[from + i];
+          ExtTextOutW (s->hdc, x + i, y, options, NULL, &ch, 1, NULL);
+        }
     }
   else
-    ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
+    {
+      USE_SAFE_ALLOCA;
+      wchar_t *str = SAFE_ALLOCA (len);
+      for (int i = 0; i < len; ++i)
+        str[i] = s->char2b[from + i];
+      ExtTextOutW (s->hdc, x, y, options, NULL, str, len, NULL);
+      SAFE_FREE ();
+    }
 
   /* Restore clip region.  */
   if (s->num_clips > 0)

Re: Replace XChar2b with unsigned in all font backends

[martin rudalics]

 > Does the following diff work for you?

The warnings go away but characters are still displayed as empty
boxes.

martin

Re: Replace XChar2b with unsigned in all font backends

[Alex Gramiak]

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

martin rudalics <rudalics@gmx.at> writes:

>> Does the following diff work for you?
>
> The warnings go away but characters are still displayed as empty
> boxes.

Right, that's not unexpected since I made a flub on that patch and
didn't multiply by sizeof (wchar_t) (I posted a version that fixed that
in another message but didn't CC you, sorry).

Here's a third version that uses SAFE_NALLOCA instead:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: w32font v3 --]
[-- Type: text/x-patch, Size: 886 bytes --]

diff --git a/src/w32font.c b/src/w32font.c
index bd68e22cc9..08d0f370bf 100644
--- a/src/w32font.c
+++ b/src/w32font.c
@@ -704,11 +704,21 @@ w32font_draw (struct glyph_string *s, int from, int to,
       int i;
 
       for (i = 0; i < len; i++)
-	ExtTextOutW (s->hdc, x + i, y, options, NULL,
-		     s->char2b + from + i, 1, NULL);
+        {
+          const wchar_t ch = s->char2b[from + i];
+          ExtTextOutW (s->hdc, x + i, y, options, NULL, &ch, 1, NULL);
+        }
     }
   else
-    ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
+    {
+      USE_SAFE_ALLOCA;
+      wchar_t *str;
+      SAFE_NALLOCA (str, 1, len);
+      for (int i = 0; i < len; ++i)
+        str[i] = s->char2b[from + i];
+      ExtTextOutW (s->hdc, x, y, options, NULL, str, len, NULL);
+      SAFE_FREE ();
+    }
 
   /* Restore clip region.  */
   if (s->num_clips > 0)

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Tue 21 May 2019, Alex Gramiak wrote:

> martin rudalics <rudalics@gmx.at> writes:
>
>>> Does the following diff work for you?
>>
>> The warnings go away but characters are still displayed as empty
>> boxes.
>
> Right, that's not unexpected since I made a flub on that patch and
> didn't multiply by sizeof (wchar_t) (I posted a version that fixed that
> in another message but didn't CC you, sorry).
>
> Here's a third version that uses SAFE_NALLOCA instead:
>
> diff --git a/src/w32font.c b/src/w32font.c
> index bd68e22cc9..08d0f370bf 100644
> --- a/src/w32font.c
> +++ b/src/w32font.c
> @@ -704,11 +704,21 @@ w32font_draw (struct glyph_string *s, int from, int to,
>        int i;
>  
>        for (i = 0; i < len; i++)
> -	ExtTextOutW (s->hdc, x + i, y, options, NULL,
> -		     s->char2b + from + i, 1, NULL);
> +        {
> +          const wchar_t ch = s->char2b[from + i];
> +          ExtTextOutW (s->hdc, x + i, y, options, NULL, &ch, 1, NULL);
> +        }
>      }
>    else
> -    ExtTextOutW (s->hdc, x, y, options, NULL, s->char2b + from, len, NULL);
> +    {
> +      USE_SAFE_ALLOCA;
> +      wchar_t *str;
> +      SAFE_NALLOCA (str, 1, len);
> +      for (int i = 0; i < len; ++i)
> +        str[i] = s->char2b[from + i];
> +      ExtTextOutW (s->hdc, x, y, options, NULL, str, len, NULL);
> +      SAFE_FREE ();
> +    }
>  
>    /* Restore clip region.  */
>    if (s->num_clips > 0)

This seems much better: no more mojibake, and `view-hello-file' works.

Thanks,

    AndyM

Re: Replace XChar2b with unsigned in all font backends

[martin rudalics]

 >> The warnings go away but characters are still displayed as empty
 >> boxes.
 >
 > Right, that's not unexpected since I made a flub on that patch and
 > didn't multiply by sizeof (wchar_t) (I posted a version that fixed that
 > in another message but didn't CC you, sorry).
 >
 > Here's a third version that uses SAFE_NALLOCA instead:

Works now, please install.

Thanks for the fix, martin

Re: Replace XChar2b with unsigned in all font backends

[Andy Moreton]

On Mon 20 May 2019, Alex Gramiak wrote:

> Does the following diff work for you?

That restores ordinary ASCII text to proper display.

However, "C-h h" (`view-hello-file') crashes emacs.

    AndyM