[Valgrind-developers] [valgrind] s390x: Minor fix for vbit-test

[Florian K. <fk...@so...>]

https://sourceware.org/cgit/valgrind/commit/?id=ce46882e02b38493fe921611cb3ee2d64bde8c30

commit ce46882e02b38493fe921611cb3ee2d64bde8c30
Author: Florian Krohm <fl...@ei...>
Date:   Mon Jul 7 13:51:29 2025 +0000

    s390x: Minor fix for vbit-test
    
    Add a big comment as to why Ity_I1 type values are stored in a
    32-bit entity.

Diff:
---
 VEX/priv/ir_inject.c                |  7 ++++---
 memcheck/tests/vbit-test/util.c     |  2 +-
 memcheck/tests/vbit-test/valgrind.c |  6 +++---
 memcheck/tests/vbit-test/vbits.h    | 14 +++++++++++++-
 4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/VEX/priv/ir_inject.c b/VEX/priv/ir_inject.c
index 26e8ca2787..e62a2d8506 100644
--- a/VEX/priv/ir_inject.c
+++ b/VEX/priv/ir_inject.c
@@ -67,7 +67,8 @@ load_aux(IREndness endian, IRType type, IRExpr *addr)
                   IRExpr_Load(endian, Ity_I64, addr));
    }
    if (type == Ity_I1) {
-      /* A Boolean value is stored as a 32-bit entity (see store_aux). */
+      /* A Boolean value is stored as a 32-bit entity. For an explanation
+         see comment in vbit-test/vbits.h */
       return unop(Iop_32to1, IRExpr_Load(endian, Ity_I32, addr));
    }
 
@@ -131,8 +132,8 @@ store_aux(IRSB *irsb, IREndness endian, IRExpr *addr, IRExpr *data)
       data = unop(Iop_ReinterpD64asI64, data);
    }
    if (typeOfIRExpr(irsb->tyenv, data) == Ity_I1) {
-      /* We cannot store a single bit. So we store it in a 32-bit container.
-         See also load_aux. */
+      /* A Boolean value is stored as a 32-bit entity. For an explanation
+         see comment in vbit-test/vbits.h */
       data = unop(Iop_1Uto32, data);
    }
    stmt(irsb, IRStmt_Store(endian, addr, data));
diff --git a/memcheck/tests/vbit-test/util.c b/memcheck/tests/vbit-test/util.c
index 63eabc8993..3e57cfa703 100644
--- a/memcheck/tests/vbit-test/util.c
+++ b/memcheck/tests/vbit-test/util.c
@@ -86,7 +86,7 @@ static void
 print_value(FILE *fp, value_t val, unsigned num_bits)
 {
    switch (num_bits) {
-   case 1:  fprintf(fp, "%02x",   val.u8);  break;
+   case 1:  fprintf(fp, "%01x",   val.u32); break; // see comment in vbits.h
    case 8:  fprintf(fp, "%02x",   val.u8);  break;
    case 16: fprintf(fp, "%04x",   val.u16); break;
    case 32: fprintf(fp, "%08x",   val.u32); break;
diff --git a/memcheck/tests/vbit-test/valgrind.c b/memcheck/tests/vbit-test/valgrind.c
index c24a06d649..55ff3647da 100644
--- a/memcheck/tests/vbit-test/valgrind.c
+++ b/memcheck/tests/vbit-test/valgrind.c
@@ -64,7 +64,7 @@ valgrind_set_vbits(opnd_t *opnd)
 {
    unsigned rc, num_bytes;
    
-   /* 1-bit wide values cannot be read. So we read a 4 bytes here */
+   /* 1-bit wide values cannot be read. So we read 4 bytes here */
    num_bytes = opnd->type == Ity_I1 ? 4 : sizeof_irtype(opnd->type);
    rc = VALGRIND_SET_VBITS(&opnd->value, &opnd->vbits.bits, num_bytes);
    assert(rc == 1);
@@ -83,8 +83,8 @@ valgrind_get_vbits(opnd_t *opnd)
 {
    unsigned rc, num_bytes;
 
-   /* 1-bit wide values cannot be stored. So we store them by writing a
-      single byte */
+   /* 1-bit wide values cannot be stored. So we store them by writing
+      4 bytes which is what ir_inject.c expects. */
    num_bytes = opnd->type == Ity_I1 ? 4 : sizeof_irtype(opnd->type);
    opnd->vbits.num_bits = bitsof_irtype(opnd->type);
    rc = VALGRIND_GET_VBITS(&opnd->value, &opnd->vbits.bits, num_bytes);
diff --git a/memcheck/tests/vbit-test/vbits.h b/memcheck/tests/vbit-test/vbits.h
index 53ba328aa0..7d0002d32f 100644
--- a/memcheck/tests/vbit-test/vbits.h
+++ b/memcheck/tests/vbit-test/vbits.h
@@ -48,7 +48,19 @@ typedef struct {
 
 /* A type large enough to hold any IRType'd value. At this point
    we do not expect to test with specific floating point values.
-   So we don't need to represent them. */
+   So we don't need to represent them.
+
+   NOTE: Values of type Ity_I1 are stored in the u32 variant. This is
+   inconsistent with the way such values are stored elsewhere in VEX,
+   namely, in an 8-bit container. Why is that?
+   The reason is that libvex_ir.h does not provide an Iop_8to1 operator.
+   However, that would be needed in ir_inject.c when loading a 1-bit value
+   from memory (see function load_aux there). Instead of today's
+      return unop(Iop_32to1, IRExpr_Load(endian, Ity_I32, addr));
+   we'd like to write
+      return unop(Iop_8to1, IRExpr_Load(endian, Ity_I8, addr));
+   But cannot do. Grrrrr...
+ */
 typedef union {
    uint8_t   u8;
    uint16_t  u16;