[neomutt-devel] NeoMutt 2023-11-03

[наб]

On Fri, Nov 03, 2023 at 10:33:05AM +0000, Richard Russon wrote:
> Behind the scenes, I added a lot of tests for the colour code.
> This revealed a handful of bugs that needed fixing.
Indeed. 20231103 still has one, but only if built with GCC
(bookworm, 12.2.0-14) and not with clang trunk (18.0.0 (++20231006112712+a16f6462d756-1~exp1~20231006112827.256)).

Test test_attr_colors... [ FAILED ]
 Case Common setup:
  attr.c:108: Check memcmp(&ac, &ac_copy, sizeof(ac_copy)) == 0...  failed

Adding a quick printf shows that, indeed (ac, then ac_copy):
  0000000000000000000000000000000000000000000000000000200000000000e08cb7c7e9550000000000000000000000000000000000000000000000000000
  0000000000000000000000000000000000000000000000000000200000000000e08cb7c7e9550000000026c7e955000000000000000000000000000000000000
  
  fg=000000000000000000000000
  bg=000000000000000000000000
  attrs=00002000
  curses_color=e08cb7c7e9550000
  ref_count=0000
  entries=00000000000000000000000000000000
  
  fg=000000000000000000000000
  bg=000000000000000000000000
  attrs=00002000
  curses_color=e08cb7c7e9550000
  ref_count=0000
  entries=00000000000000000000000000000000

The test looks like
  100     struct AttrColor ac_copy = attr_color_copy(&ac);
  101
  102     TEST_CHECK(memcmp(&ac, &ac_copy, sizeof(ac_copy)) == 0);
  103
  104     attr_color_clear(&ac);
vs
  struct AttrColor attr_color_copy(const struct AttrColor *ac)
  {
    struct AttrColor copy = { 0 };
    if (ac)
      copy = *ac;
  
    return copy;
  }

Naturally, the test is wrong, since it's also comparing padding,
and the value of the padding is undefined.

I'm assuming clang converts the assignment to a memcpy,
but gcc simply copies the fields.

Either way, trivial fix in scissor-patch below.

Best,
-- >8 --
Subject: [PATCH] color: attr_color_copy: return *orig or {0} directly

The tests use memcmp, and the previous variant led GCC to use padding
from the stack instead of zeroing it out or copying it from the original
---
 color/attr.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/color/attr.c b/color/attr.c
index 7846209cb..c389c3731 100644
--- a/color/attr.c
+++ b/color/attr.c
@@ -166,11 +166,10 @@ struct AttrColor *attr_color_list_find(struct AttrColorList *acl, color_t fg,
  */
 struct AttrColor attr_color_copy(const struct AttrColor *ac)
 {
-  struct AttrColor copy = { 0 };
   if (ac)
-    copy = *ac;
-
-  return copy;
+    return *ac;
+  else
+    return (struct AttrColor){ 0 };
 }
 
 /**
-- 
2.39.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://mailman.neomutt.org/pipermail/neomutt-devel-neomutt.org/attachments/20231103/d6887b31/attachment.sig>