From c7263571d217e0863d3e62629207659b6aaf2b8b Mon Sep 17 00:00:00 2001 From: Ekaitz Zarraga Date: Sat, 20 Jan 2024 23:36:45 +0100 Subject: [PATCH] riscv64-gen: Fix `load` and `store` type_size usage In `load` and `store` RISC-V gen used `type_size` to retrieve the size of the types being moved around, the problem with that function is it tries to obtain internal information of the type. When using `type_size` with a `char *` it returns 1, and emits a `lb` instruction when using a conditional expression like so (but probably also in other cases): char *a, *b; b = "hello"; a = x ? b : "" ; // this emits an `lb` That `lb` clobbers the pointer, only loading the latest byte of it: // if `b` was, say: 0x1f822e a = b; // now `a` is 0x00002e NOTE: We spotted this when building make-3.82, in `init_switches` function in the `main.c` file. This error made `make` unable to run long options like `make --help` (segfault when doing the `strlen` later) This happens because a `char *` is internally stored as a `char[1]` or something similar, it's result is the size of a `char` (1) times the size of the array `1`. This is not what we want, we are copying the pointer, not the array itself. Using `type_size` for this is not appropriate even if it works for some cases. If the conditional expression is rewritten to imperative style using an `if` it works properly, so it might be related with the fact the pointer is `load`ed to a register. `load` and `store` should only work with integral types, so reading the size of the array is not useful. This commit creates a simpler version of this function that only reads the integral type of what's working with: char * is considered just a pointer. So it makes an `ld` instead, and the code above works. --- riscv64-gen.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/riscv64-gen.c b/riscv64-gen.c index 8a17b701..7da87147 100644 --- a/riscv64-gen.c +++ b/riscv64-gen.c @@ -222,6 +222,48 @@ static void load_large_constant(int rr, int fc, uint32_t pi) EI(0x13, 1, rr, rr, 8); // slli RR, RR, 8 } +ST_FUNC int type_size_integral(CType *type, int *a){ + int bt; + bt = type->t & VT_BTYPE; + switch (bt){ + case VT_VOID: + case VT_BOOL: + case VT_BYTE: + *a = 1; + return 1; + case VT_SHORT: + *a = 2; + return 2; + case VT_LLONG: + case VT_DOUBLE: + *a = 8; + return 8; + case VT_PTR: + case VT_FUNC: + *a = PTR_SIZE; + return PTR_SIZE; + case VT_FLOAT: + case VT_INT: + *a = 4; + return 4; + case VT_LDOUBLE: + *a = LDOUBLE_ALIGN; + return LDOUBLE_SIZE; + case VT_QLONG: + case VT_QFLOAT: + *a = 8; + return 16; + case VT_ENUM: + *a = 4; + /* Enums might be incomplete, so don't just return '4' here. */ + return type->ref->c; + default: + /* VT_STRUCT and VT_UNION */ + tcc_error("load/store with a non integral"); + return type_size(type, a); + } +} + ST_FUNC void load(int r, SValue *sv) { int fr = sv->r; @@ -232,7 +274,7 @@ ST_FUNC void load(int r, SValue *sv) int align, size; if (fr & VT_LVAL) { int func3, opcode = is_freg(r) ? 0x07 : 0x03, br; - size = type_size(&sv->type, &align); + size = type_size_integral(&sv->type, &align); assert (!is_freg(r) || bt == VT_FLOAT || bt == VT_DOUBLE); if (bt == VT_FUNC) /* XXX should be done in generic code */ size = PTR_SIZE; @@ -313,7 +355,7 @@ ST_FUNC void load(int r, SValue *sv) EI(0x13, 0, rr, ireg(v), 0); // addi RR, V, 0 == mv RR, V else { int func7 = is_ireg(r) ? 0x70 : 0x78; - size = type_size(&sv->type, &align); + size = type_size_integral(&sv->type, &align); if (size == 8) func7 |= 1; assert(size == 4 || size == 8); @@ -373,7 +415,7 @@ ST_FUNC void store(int r, SValue *sv) int rr = is_ireg(r) ? ireg(r) : freg(r), ptrreg; int fc = sv->c.i; int bt = sv->type.t & VT_BTYPE; - int align, size = type_size(&sv->type, &align); + int align, size = type_size_integral(&sv->type, &align); assert(!is_float(bt) || is_freg(r) || bt == VT_LDOUBLE); /* long doubles are in two integer registers, but the load/store primitives only deal with one, so do as if it's one reg. */