bug#37756: [PATCH] Wrong initialization of fringe bitmap

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Carlos Pita]

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

In fringe.c:1606 you have:

  xfb = xmalloc (sizeof fb + fb.height * BYTES_PER_BITMAP_ROW);
  fb.bits = b = ((unsigned short *)
ptr_bounds_clip (xfb + 1, fb.height * BYTES_PER_BITMAP_ROW));
  xfb = ptr_bounds_clip (xfb, sizeof *xfb);
  memset (b, 0, fb.height);

I might be wrong but it seems to me that the last line should be:

  memset (b, 0, fb.height * BYTES_PER_BITMAP_ROW);

instead.

I've attached a patch that does exactly that.

Best regards
--
Carlos

[-- Attachment #2: 0001-Fix-zero-initialization-of-fringe-bitmap.patch --]
[-- Type: text/x-patch, Size: 895 bytes --]

From fe3f040d6614ee4b990f3f4252c89d9a17c52845 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Mon, 14 Oct 2019 23:33:37 -0300
Subject: [PATCH] Fix zero initialization of fringe bitmap

* src/fringe.c: the size in bytes of fb.bits is
    fb.height * BYTES_PER_BITMAP_ROW
  and not simply the number of rows fb.height.
---
 src/fringe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fringe.c b/src/fringe.c
index 22f3bdc..db91209 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1607,7 +1607,7 @@ list (ALIGN PERIODIC) where PERIODIC non-nil specifies that the bitmap
   fb.bits = b = ((unsigned short *)
 		 ptr_bounds_clip (xfb + 1, fb.height * BYTES_PER_BITMAP_ROW));
   xfb = ptr_bounds_clip (xfb, sizeof *xfb);
-  memset (b, 0, fb.height);
+  memset (b, 0, fb.height * BYTES_PER_BITMAP_ROW);
 
   j = 0;
   while (j < fb.height)
-- 
2.20.1

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Mon, 14 Oct 2019 23:39:19 -0300
> 
> In fringe.c:1606 you have:
> 
>   xfb = xmalloc (sizeof fb + fb.height * BYTES_PER_BITMAP_ROW);
>   fb.bits = b = ((unsigned short *)
> ptr_bounds_clip (xfb + 1, fb.height * BYTES_PER_BITMAP_ROW));
>   xfb = ptr_bounds_clip (xfb, sizeof *xfb);
>   memset (b, 0, fb.height);
> 
> I might be wrong but it seems to me that the last line should be:
> 
>   memset (b, 0, fb.height * BYTES_PER_BITMAP_ROW);
> 
> instead.

Can you explain your reasoning?  The loop after that does initialize
the structure as needed, doesn't it?

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Carlos Pita]

> Can you explain your reasoning?  The loop after that does initialize

Ok, TBH I hadn't looked at the loop. But anyway:

> >   xfb = xmalloc (sizeof fb + fb.height * BYTES_PER_BITMAP_ROW);

This allocates fb.height * BYTES_PER_BITMAP_ROW for bits = b.

> >   memset (b, 0, fb.height);

This only initializes fb.height bytes to zero.

And then the loop indeed initializes all fb.height *
BYTES_PER_BITMAP_ROW bytes (b being an unsigned short pointer):

  j = 0;
  while (j < fb.height)
    {
       ...
       b[j++] = something;
       ...
    }

So instead of memset (b, 0, fb.height) being incomplete I would now
say that it is redundant and misleading.

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Carlos Pita]

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

Here is a patch that removes the superfluous line with a detailed
explanation in the commit message.

I've tested it in combination (also in combination with my "hidpi
fringe" patches) and got no segv yet ;)

[-- Attachment #2: 0001-Remove-redundant-and-incomplete-initialization-of-fr.patch --]
[-- Type: application/x-patch, Size: 1047 bytes --]

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 16 Oct 2019 14:28:29 -0300
> Cc: 37756@debbugs.gnu.org
> 
> Here is a patch that removes the superfluous line with a detailed
> explanation in the commit message.

Thanks, but please format the log message according to CONTRIBUTE: it
should name the function in which the change is being made, start with
a capital letter, and keep 2 spaces between sentences.  Also, there's
no need to describe in such detail what the code does, when the code
speaks for itself.  You could just say "Remove redundant zeroing of
fb.bits".

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Carlos Pita]

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

I've done as you asked, Eli. Sorry but I'm still trying to catch up
with many of your conventions.

I'm not sure about how to designate the modified function in the
changelog, since it's a DEFUN.

[-- Attachment #2: 0001-Remove-redundant-and-incomplete-initialization-of-fr.patch --]
[-- Type: text/x-patch, Size: 847 bytes --]

From dd69833c5a1aa6a89cac4cc3b9419c1dabbe50ed Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Wed, 16 Oct 2019 13:44:00 -0300
Subject: [PATCH] Remove redundant and incomplete initialization of fringe
 bitmap

* src/fringe.c (define-fringe-bitmap): Remove redundant zeroing
of fb.bits that only zeroed half of the array anyway. Bug#37756.
---
 src/fringe.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/fringe.c b/src/fringe.c
index 22f3bdc..08bf271 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1607,7 +1607,6 @@ list (ALIGN PERIODIC) where PERIODIC non-nil specifies that the bitmap
   fb.bits = b = ((unsigned short *)
 		 ptr_bounds_clip (xfb + 1, fb.height * BYTES_PER_BITMAP_ROW));
   xfb = ptr_bounds_clip (xfb, sizeof *xfb);
-  memset (b, 0, fb.height);
 
   j = 0;
   while (j < fb.height)
-- 
2.20.1

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 16 Oct 2019 15:54:01 -0300
> Cc: 37756@debbugs.gnu.org
> 
> I've done as you asked, Eli. Sorry but I'm still trying to catch up
> with many of your conventions.

Thanks for persevering.

> I'm not sure about how to designate the modified function in the
> changelog, since it's a DEFUN.

Use one of the related Emacs commands, and Emacs will do that for you.
Those commands are described in CONTRIBUTE under "Generating ChangeLog
entries".

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Carlos Pita]

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

> Use one of the related Emacs commands, and Emacs will do that for you.
> Those commands are described in CONTRIBUTE under "Generating ChangeLog
> entries".

I'm having some issues doing that because I'm using magit to amend /
reword previously committed changes, so the instructions there don't
quite match my situation. Will try harder later, for now I just
modified the message after some examples in your git log. Hope it's ok
now.

[-- Attachment #2: 0001-Remove-redundant-initialization-of-fringe-bitmap-Bug.patch --]
[-- Type: text/x-patch, Size: 833 bytes --]

From dcb47d7cfb83fb4b5a28c7f909aea8e5aab11893 Mon Sep 17 00:00:00 2001
From: memeplex <carlosjosepita@gmail.com>
Date: Wed, 16 Oct 2019 13:44:00 -0300
Subject: [PATCH] Remove redundant initialization of fringe bitmap (Bug#37756)

* src/fringe.c (Fdefine-fringe-bitmap): Remove redundant zeroing
of fb.bits that only zeroed half of the array anyway.
---
 src/fringe.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/fringe.c b/src/fringe.c
index 22f3bdc..08bf271 100644
--- a/src/fringe.c
+++ b/src/fringe.c
@@ -1607,7 +1607,6 @@ list (ALIGN PERIODIC) where PERIODIC non-nil specifies that the bitmap
   fb.bits = b = ((unsigned short *)
 		 ptr_bounds_clip (xfb + 1, fb.height * BYTES_PER_BITMAP_ROW));
   xfb = ptr_bounds_clip (xfb, sizeof *xfb);
-  memset (b, 0, fb.height);
 
   j = 0;
   while (j < fb.height)
-- 
2.20.1

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Alan Third]

On Wed, Oct 16, 2019 at 05:01:11PM -0300, Carlos Pita wrote:
> > Use one of the related Emacs commands, and Emacs will do that for you.
> > Those commands are described in CONTRIBUTE under "Generating ChangeLog
> > entries".
> 
> I'm having some issues doing that because I'm using magit to amend /
> reword previously committed changes, so the instructions there don't
> quite match my situation. Will try harder later, for now I just
> modified the message after some examples in your git log. Hope it's ok
> now.

In the Magit diff view that is displayed when comitting you should be
able to just hit ‘C’ with point in the code you want to add to the
commit message.
-- 
Alan Third

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Robert Pluim]

>>>>> On Thu, 17 Oct 2019 12:53:51 +0100, Alan Third <alan@idiocy.org> said:

    Alan> On Wed, Oct 16, 2019 at 05:01:11PM -0300, Carlos Pita wrote:
    >> > Use one of the related Emacs commands, and Emacs will do that for you.
    >> > Those commands are described in CONTRIBUTE under "Generating ChangeLog
    >> > entries".
    >> 
    >> I'm having some issues doing that because I'm using magit to amend /
    >> reword previously committed changes, so the instructions there don't
    >> quite match my situation. Will try harder later, for now I just
    >> modified the message after some examples in your git log. Hope it's ok
    >> now.

    Alan> In the Magit diff view that is displayed when comitting you should be
    Alan> able to just hit ‘C’ with point in the code you want to add to the
    Alan> commit message.

That doesnʼt auto-snarf the function names the way C-x 4 A or C-c C-w
do in vc.

Robert

bug#37756: [PATCH] Wrong initialization of fringe bitmap

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 16 Oct 2019 17:01:11 -0300
> Cc: 37756@debbugs.gnu.org
> 
> I'm having some issues doing that because I'm using magit to amend /
> reword previously committed changes, so the instructions there don't
> quite match my situation. Will try harder later, for now I just
> modified the message after some examples in your git log. Hope it's ok
> now.

Thanks, pushed.  Closing the bug.

> From: memeplex <carlosjosepita@gmail.com>
        ^^^^^^^^
Is this really how you want your name to appear in the Git log?