From a4e523bcd216473abee909f7e7316ba841606278 Mon Sep 17 00:00:00 2001 From: Nick Gasson Date: Tue, 25 Oct 2022 21:26:24 +0100 Subject: [PATCH] Detect negative overflow with negation operator --- NEWS.md | 2 ++ lib/std.08/standard.vhd | 4 +-- lib/std/standard.vhd | 4 +-- src/cgen.c | 1 + src/jit/jit-exits.c | 23 +++++++++----- src/jit/jit-irgen.c | 33 +++++++++++++++++++ src/lexer.l | 24 +++++++------- src/lower.c | 13 ++++++-- src/object.c | 2 +- src/vcode.c | 70 +++++++++++++++++++++++++++-------------- src/vcode.h | 2 ++ test/parse/literal.vhd | 3 ++ test/regress/elab27.vhd | 2 +- test/test_jit.c | 2 ++ test/test_lower.c | 4 ++- test/test_parse.c | 11 ++++++- 16 files changed, 146 insertions(+), 54 deletions(-) diff --git a/NEWS.md b/NEWS.md index 13db9db5..a88ae8c3 100644 --- a/NEWS.md +++ b/NEWS.md @@ -19,6 +19,8 @@ - The new `--gtkw` run option writes a `.gtkw` save file for GtkWave containing all the signals in the design (suggested by @amb5l). - `libffi` is now a build-time dependency. +- Negation of the smallest negative value of a type such as + `-integer'left` now produces an error. ## Version 1.7.2 - 2022-10-16 - Fixed build on FreeBSD/arm (#534). diff --git a/lib/std.08/standard.vhd b/lib/std.08/standard.vhd index f7aac1a3..48145345 100644 --- a/lib/std.08/standard.vhd +++ b/lib/std.08/standard.vhd @@ -1,5 +1,5 @@ -- -------------------------------------------------*- coding: latin-1; -*----- --- Copyright (C) 2011-2021 Nick Gasson +-- Copyright (C) 2011-2022 Nick Gasson -- -- Licensed under the Apache License, Version 2.0 (the "License"); -- you may not use this file except in compliance with the License. @@ -76,7 +76,7 @@ package STANDARD is type REAL is range -1.7976931348623157e308 to 1.7976931348623157e308; - type TIME is range -9223372036854775808 to 9223372036854775807 + type TIME is range -9223372036854775807 - 1 to 9223372036854775807 units fs; ps = 1000 fs; diff --git a/lib/std/standard.vhd b/lib/std/standard.vhd index 039ae70f..1a276c6a 100644 --- a/lib/std/standard.vhd +++ b/lib/std/standard.vhd @@ -1,5 +1,5 @@ -- -------------------------------------------------*- coding: latin-1; -*----- --- Copyright (C) 2011-2021 Nick Gasson +-- Copyright (C) 2011-2022 Nick Gasson -- -- Licensed under the Apache License, Version 2.0 (the "License"); -- you may not use this file except in compliance with the License. @@ -76,7 +76,7 @@ package STANDARD is type REAL is range -1.7976931348623157e308 to 1.7976931348623157e308; - type TIME is range -9223372036854775808 to 9223372036854775807 + type TIME is range -9223372036854775807 - 1 to 9223372036854775807 units fs; ps = 1000 fs; diff --git a/src/cgen.c b/src/cgen.c index ae7284b2..e80442d1 100644 --- a/src/cgen.c +++ b/src/cgen.c @@ -3588,6 +3588,7 @@ static void cgen_op(int i, cgen_ctx_t *ctx) cgen_op_div(i, ctx); break; case VCODE_OP_NEG: + case VCODE_OP_TRAP_NEG: cgen_op_neg(i, ctx); break; case VCODE_OP_EXP: diff --git a/src/jit/jit-exits.c b/src/jit/jit-exits.c index b95cb900..1abb4fed 100644 --- a/src/jit/jit-exits.c +++ b/src/jit/jit-exits.c @@ -271,18 +271,27 @@ void x_exponent_fail(int32_t value, tree_t where) void x_overflow(int64_t lhs, int64_t rhs, tree_t where) { - const char *op = "??"; + LOCAL_TEXT_BUF tb = tb_new(); if (tree_kind(where) == T_FCALL) { switch (tree_subkind(tree_ref(where))) { - case S_ADD: op = "+"; break; - case S_MUL: op = "*"; break; - case S_SUB: op = "-"; break; + case S_ADD: + tb_printf(tb, "%"PRIi64" + %"PRIi64, lhs, rhs); + break; + case S_MUL: + tb_printf(tb, "%"PRIi64" * %"PRIi64, lhs, rhs); + break; + case S_SUB: + tb_printf(tb, "%"PRIi64" - %"PRIi64, lhs, rhs); + break; + case S_NEGATE: + tb_printf(tb, "-(%"PRIi64")", lhs); + break; } } - jit_msg(tree_loc(where), DIAG_FATAL, "result of %"PRIi64" %s %"PRIi64 - " cannot be represented as %s", lhs, op, rhs, - type_pp(tree_type(where))); + jit_msg(tree_loc(where), DIAG_FATAL, + "result of %s cannot be represented as %s", + tb_get(tb), type_pp(tree_type(where))); } void x_null_deref(tree_t where) diff --git a/src/jit/jit-irgen.c b/src/jit/jit-irgen.c index efe5adcd..eb43cae7 100644 --- a/src/jit/jit-irgen.c +++ b/src/jit/jit-irgen.c @@ -1364,6 +1364,36 @@ static void irgen_op_neg(jit_irgen_t *g, int op) g->map[result] = j_neg(g, arg0); } +static void irgen_op_trap_neg(jit_irgen_t *g, int op) +{ + vcode_reg_t result = vcode_get_result(op); + vcode_type_t vtype = vcode_reg_type(result); + + jit_value_t arg0 = irgen_get_arg(g, op, 0); + jit_value_t locus = irgen_get_arg(g, op, 1); + + int64_t cmp; + switch (irgen_jit_size(vtype)) { + case JIT_SZ_8: cmp = INT8_MIN; break; + case JIT_SZ_16: cmp = INT16_MIN; break; + case JIT_SZ_32: cmp = INT32_MIN; break; + case JIT_SZ_64: cmp = INT64_MIN; break; + default: cmp = 0; + } + + irgen_label_t *cont = irgen_alloc_label(g); + j_cmp(g, JIT_CC_GT, arg0, jit_value_from_int64(cmp)); + j_jump(g, JIT_CC_T, cont); + + j_send(g, 0, arg0); + j_send(g, 1, jit_value_from_int64(0)); + j_send(g, 2, locus); + macro_exit(g, JIT_EXIT_OVERFLOW); + + irgen_bind_label(g, cont); + g->map[result] = j_neg(g, arg0); +} + static void irgen_op_abs(jit_irgen_t *g, int op) { vcode_reg_t result = vcode_get_result(op); @@ -3182,6 +3212,9 @@ static void irgen_block(jit_irgen_t *g, vcode_block_t block) case VCODE_OP_NEG: irgen_op_neg(g, i); break; + case VCODE_OP_TRAP_NEG: + irgen_op_trap_neg(g, i); + break; case VCODE_OP_ABS: irgen_op_abs(g, i); break; diff --git a/src/lexer.l b/src/lexer.l index 239f3988..edaadce0 100644 --- a/src/lexer.l +++ b/src/lexer.l @@ -462,14 +462,23 @@ static int parse_decimal_literal(const char *str) char *val = strtok(tmp, "eE"); char *exp = strtok(NULL, "eE"); - yylval.n = atoll(val); + errno = 0; + yylval.n = strtoll(val, NULL, 10); + bool overflow = (errno == ERANGE); long long int e = (exp ? atoll(exp) : 0); if (e >= 0) { // Minus sign forbidden for an integer literal - for (; e > 0; e--) yylval.n *= 10; + for (; e > 0; e--) { + if (__builtin_mul_overflow(yylval.n, INT64_C(10), &yylval.n)) + overflow = true; + } tok = (sign == NULL) ? tINT : tERROR; } + + if (overflow) + error_at(&yylloc, "value %s is outside implementation defined range " + "of universal_integer", str); } else { yylval.d = strtod(tmp, NULL); @@ -479,17 +488,6 @@ static int parse_decimal_literal(const char *str) TOKEN(tok); } -/** - * \fn static int parse_based_literal (const char *str) - * \brief - * - * \param str Formatted string as specified in LRM 13.4.2 - * (based_literal ::= base [#:] based_integer [.based_integer] [#:] [exponent]). - * \return TOKEN(tINT) if no '.' found and no '-' found, - * TOKEN(tERROR) if no '.' found and '-' found, - * TOKEN(tERROR) if the base is not at least 2 or at most 16, - * else TOKEN(tREAL). - */ static int parse_based_literal (const char *str) { // Transform a string into a literal as specified in LRM 13.4.2 diff --git a/src/lower.c b/src/lower.c index f3e5277c..9a622769 100644 --- a/src/lower.c +++ b/src/lower.c @@ -1837,7 +1837,14 @@ static vcode_reg_t lower_builtin(tree_t fcall, subprogram_kind_t builtin, return lower_arith(fcall, S_EXP, r0, r1); } case S_NEGATE: - return emit_neg(r0); + { + if (type_is_integer(r0_type)) { + vcode_reg_t locus = lower_debug_locus(fcall); + return emit_trap_neg(r0, locus); + } + else + return emit_neg(r0); + } case S_ABS: return emit_abs(r0); case S_IDENTITY: @@ -5206,7 +5213,7 @@ static void lower_signal_assign(tree_t stmt) delay_reg = lower_rvalue(delay); } else - delay_reg = emit_const(vtype_int(INT64_MIN, INT64_MAX), 0); + delay_reg = emit_const(vtype_time(), 0); vcode_reg_t reject_reg; if (i == 0 && tree_has_reject(stmt)) { @@ -5221,7 +5228,7 @@ static void lower_signal_assign(tree_t stmt) } else { // All but the first waveform have zero reject time - reject_reg = emit_const(vtype_int(INT64_MIN, INT64_MAX), 0); + reject_reg = emit_const(vtype_time(), 0); } vcode_var_t tmp_var = VCODE_INVALID_VAR; diff --git a/src/object.c b/src/object.c index b312b74c..a8261b1f 100644 --- a/src/object.c +++ b/src/object.c @@ -342,7 +342,7 @@ void object_one_time_init(void) // Increment this each time a incompatible change is made to the // on-disk format not expressed in the object items table - const uint32_t format_fudge = 26; + const uint32_t format_fudge = 27; format_digest += format_fudge * UINT32_C(2654435761); diff --git a/src/vcode.c b/src/vcode.c index 4663d8cf..b4b9350b 100644 --- a/src/vcode.c +++ b/src/vcode.c @@ -215,28 +215,29 @@ static void *dump_arg = NULL; static inline int64_t sadd64(int64_t a, int64_t b) { - if (a > 0) { - if (b > INT64_MAX - a) - return INT64_MAX; - } - else if (b < INT64_MIN - a) - return INT64_MIN; + int64_t result; + if (__builtin_add_overflow(a, b, &result)) + return b < 0 ? INT64_MIN : INT64_MAX; + + return result; +} + +static inline int64_t ssub64(int64_t a, int64_t b) +{ + int64_t result; + if (__builtin_sub_overflow(a, b, &result)) + return b > 0 ? INT64_MIN : INT64_MAX; - return a + b; + return result; } static inline int64_t smul64(int64_t a, int64_t b) { - if ((a > 0 && b > 0) || (a < 0 && b < 0)) { - if ((b > INT32_MAX && a > INT32_MAX) - || (b < INT32_MIN && a < INT32_MIN)) - return INT64_MAX; - } - else if ((b < INT32_MIN && a > INT32_MAX) - || (b > INT32_MAX && a < INT32_MIN)) - return INT64_MIN; + int64_t result; + if (__builtin_mul_overflow(a, b, &result)) + return (a > 0 && b > 0) || (a < 0 && b < 0) ? INT64_MAX : INT64_MIN; - return a * b; + return result; } static vcode_reg_t vcode_add_reg(vcode_type_t type) @@ -482,6 +483,7 @@ void vcode_heap_allocate(vcode_reg_t reg) case VCODE_OP_TRAP_ADD: case VCODE_OP_TRAP_SUB: case VCODE_OP_TRAP_MUL: + case VCODE_OP_TRAP_NEG: // Result cannot reference pointer break; @@ -931,6 +933,7 @@ const char *vcode_op_string(vcode_op_t op) "push scope", "pop scope", "alias signal", "trap add", "trap sub", "trap mul", "force", "release", "link instance", "unreachable", "package init", "strconv", "canon value", "convstr", + "trap neg" }; if ((unsigned)op >= ARRAY_LEN(strs)) return "???"; @@ -958,8 +961,6 @@ static int vcode_pretty_print_int(int64_t n) return printf("2^63-1"); else if (n == INT64_MIN) return printf("-2^63"); - else if (n == INT64_MIN + 1) - return printf("-2^63"); // XXX: bug in lexer/parser else if (n == INT32_MAX) return printf("2^31-1"); else if (n == INT32_MIN) @@ -1738,6 +1739,7 @@ void vcode_dump_with_mark(int mark_op, vcode_dump_fn_t callback, void *arg) break; case VCODE_OP_NEG: + case VCODE_OP_TRAP_NEG: case VCODE_OP_ABS: case VCODE_OP_RESOLVED: case VCODE_OP_LAST_VALUE: @@ -1745,6 +1747,10 @@ void vcode_dump_with_mark(int mark_op, vcode_dump_fn_t callback, void *arg) col += vcode_dump_reg(op->result); col += printf(" := %s ", vcode_op_string(op->kind)); col += vcode_dump_reg(op->args.items[0]); + if (op->args.count > 1) { + col += printf(" locus "); + col += vcode_dump_reg(op->args.items[1]); + } vcode_dump_result_type(col, op); } break; @@ -2523,8 +2529,7 @@ vcode_type_t vtype_offset(void) vcode_type_t vtype_time(void) { - // XXX: should be INT64_MIN - return vtype_int(INT64_MIN + 1, INT64_MAX); + return vtype_int(INT64_MIN, INT64_MAX); } vcode_type_t vtype_char(void) @@ -3856,9 +3861,8 @@ static vcode_reg_t emit_sub_op(vcode_op_t op, vcode_reg_t lhs, vcode_reg_t rhs, vtype_t *bl = vcode_type_data(lhs_r->bounds); vtype_t *br = vcode_type_data(rhs_r->bounds); - // XXX: this is wrong - see TO_UNSIGNED - int64_t rbl = sadd64(bl->low, -br->high); - int64_t rbh = sadd64(bl->high, -br->low); + int64_t rbl = ssub64(bl->low, br->high); + int64_t rbh = ssub64(bl->high, br->low); vtype_repr_t repr = vtype_repr(lhs_r->type); if (op == VCODE_OP_TRAP_SUB && vtype_clamp_to_repr(repr, &rbl, &rbh)) { @@ -4125,6 +4129,26 @@ vcode_reg_t emit_neg(vcode_reg_t lhs) return op->result; } +vcode_reg_t emit_trap_neg(vcode_reg_t lhs, vcode_reg_t locus) +{ + int64_t lconst; + if (vcode_reg_const(lhs, &lconst) && lconst >= 0) + return emit_const(vcode_reg_type(lhs), -lconst); + else if (vcode_type_data(vcode_reg_data(lhs)->bounds)->low >= 0) + return emit_neg(lhs); // Cannot overflow + + op_t *op = vcode_add_op(VCODE_OP_TRAP_NEG); + vcode_add_arg(op, lhs); + vcode_add_arg(op, locus); + + VCODE_ASSERT(vcode_reg_kind(locus) == VCODE_TYPE_DEBUG_LOCUS, + "locus argument to trap neg must be a debug locus"); + VCODE_ASSERT(vcode_reg_kind(lhs) == VCODE_TYPE_INT, + "trapping neg may only be used with integer types"); + + return (op->result = vcode_add_reg(vcode_reg_type(lhs))); +} + vcode_reg_t emit_abs(vcode_reg_t lhs) { int64_t lconst; diff --git a/src/vcode.h b/src/vcode.h index be458732..c4814217 100644 --- a/src/vcode.h +++ b/src/vcode.h @@ -152,6 +152,7 @@ typedef enum { VCODE_OP_STRCONV, VCODE_OP_CANON_VALUE, VCODE_OP_CONVSTR, + VCODE_OP_TRAP_NEG, } vcode_op_t; typedef enum { @@ -410,6 +411,7 @@ void emit_disconnect(vcode_reg_t nets, vcode_reg_t nnets, vcode_reg_t reject, vcode_reg_t after); void emit_cond(vcode_reg_t test, vcode_block_t btrue, vcode_block_t bfalse); vcode_reg_t emit_neg(vcode_reg_t lhs); +vcode_reg_t emit_trap_neg(vcode_reg_t lhs, vcode_reg_t locus); vcode_reg_t emit_abs(vcode_reg_t lhs); void emit_comment(const char *fmt, ...) __attribute__((format(printf, 1, 2))); vcode_reg_t emit_select(vcode_reg_t test, vcode_reg_t rtrue, diff --git a/test/parse/literal.vhd b/test/parse/literal.vhd index 790bc659..e2e5eacd 100644 --- a/test/parse/literal.vhd +++ b/test/parse/literal.vhd @@ -32,6 +32,9 @@ ARCHITECTURE aa OF ee IS subtype lowercase is character range 'a' to 'z'; type my_string is array (lowercase range <>) of character; constant w : my_string := "hello"; + + constant too_big : integer := 9223372036854775808; -- Error + constant way_too_big : integer := 235423414124e124124; -- Error BEGIN END ARCHITECTURE; diff --git a/test/regress/elab27.vhd b/test/regress/elab27.vhd index 97e539e4..720d0537 100644 --- a/test/regress/elab27.vhd +++ b/test/regress/elab27.vhd @@ -1,5 +1,5 @@ entity sub is - port ( c : out integer; + port ( c : out integer := 0; a, b : in integer ); end entity; diff --git a/test/test_jit.c b/test/test_jit.c index b2c3c1c0..1f6b4f99 100644 --- a/test/test_jit.c +++ b/test/test_jit.c @@ -570,6 +570,7 @@ START_TEST(test_arith1) const error_t expect[] = { { 25, "division by zero" }, { 45, "negative exponent -1 only allowed for floating-point types" }, + { 50, "result of -(-2147483648) cannot be represented as INTEGER" }, { -1, NULL }, }; expect_errors(expect); @@ -606,6 +607,7 @@ START_TEST(test_arith1) jit_handle_t negi = compile_for_test(j, "WORK.ARITH1.NEG(I)I"); ck_assert_int_eq(jit_call(j, negi, NULL, 2).integer, -2); ck_assert_int_eq(jit_call(j, negi, NULL, -124).integer, 124); + fail_if(jit_try_call(j, negi, &result, NULL, INT32_MIN)); jit_handle_t negr = compile_for_test(j, "WORK.ARITH1.NEG(R)R"); ck_assert_double_eq(jit_call(j, negr, NULL, 2.0).real, -2.0); diff --git a/test/test_lower.c b/test/test_lower.c index 50b77ef9..aaec5b4a 100644 --- a/test/test_lower.c +++ b/test/test_lower.c @@ -247,6 +247,7 @@ static void check_bb(int bb, const check_bb_t *expect, int len) case VCODE_OP_TRAP_ADD: case VCODE_OP_TRAP_SUB: case VCODE_OP_TRAP_MUL: + case VCODE_OP_TRAP_NEG: break; case VCODE_OP_CONST_ARRAY: @@ -837,7 +838,8 @@ START_TEST(test_arith1) { VCODE_OP_CMP, .cmp = VCODE_CMP_GEQ }, { VCODE_OP_DEBUG_LOCUS }, { VCODE_OP_ASSERT }, - { VCODE_OP_NEG }, + { VCODE_OP_DEBUG_LOCUS }, + { VCODE_OP_TRAP_NEG }, { VCODE_OP_CONST, .value = -3 }, { VCODE_OP_CMP, .cmp = VCODE_CMP_EQ }, { VCODE_OP_DEBUG_LOCUS }, diff --git a/test/test_parse.c b/test/test_parse.c index 444527f7..22c3aa50 100644 --- a/test/test_parse.c +++ b/test/test_parse.c @@ -893,6 +893,15 @@ START_TEST(test_literal) input_from_file(TESTDIR "/parse/literal.vhd"); + const error_t expect[] = { + { 36, "value 9223372036854775808 is outside implementation defined " + "range of universal_integer" }, + { 37, "value 235423414124e124124 is outside implementation defined " + "range of universal_integer" }, + { -1, NULL } + }; + expect_errors(expect); + e = parse(); fail_if(e == NULL); fail_unless(tree_kind(e) == T_ENTITY); @@ -1164,7 +1173,7 @@ START_TEST(test_literal) a = parse(); fail_unless(a == NULL); - fail_if_errors(); + check_expected_errors(); } END_TEST -- 2.39.2