Constifying numeric.c's local vars
Hi,
For JIT inlining currently functions can't be inlined if they reference
non-constant static variables. That's because there's no way, at least
none I know of, to link to the correct variable, instead of duplicating,
the linker explicitly renames symbols after all (that's the whole point
of static vars). There's a bunch of weird errors if you ignore that ;)
One large user of unnecessary non-constant static variables is
numeric.c. More out of curiosity - numeric is slow enough in itself to
make inlining not a huge win - I converted it to use consts.
before:
andres@alap4:~/build/postgres/dev-assert/vpath$ size --format=SysV src/backend/utils/adt/numeric.o |grep -v debug|grep -v '^.group'
src/backend/utils/adt/numeric.o :
section size addr
.text 68099 0
.data 64 0
.bss 2 0
.rodata 4256 0
.data.rel.local 224 0
.comment 29 0
.note.GNU-stack 0 0
.eh_frame 5976 0
Total 395590
after:
$ size --format=SysV src/backend/utils/adt/numeric.o |grep -v debug|grep -v '^.group'
src/backend/utils/adt/numeric.o :
section size addr
.text 68108 0
.data 0 0
.bss 0 0
.rodata 4288 0
.data.rel.ro.local 224 0
.comment 29 0
.note.GNU-stack 0 0
.eh_frame 5976 0
Total 395586
Nicely visible that the data is moved from a mutable segment to a
readonly one.
It's a bit ugly that some consts have to be casted away in the constant
definitions, but aside from just inlining the values, I don't quite see
a better solution?
Leaving JIT aside, I think stuff like this is worthwhile on its own...
Greetings,
Andres Freund
Attachments:
0001-Constify-numeric.c.patchtext/x-diff; charset=us-asciiDownload
From 2b114f2c91c7bbf80e48ef5c9653748c957bf15f Mon Sep 17 00:00:00 2001
From: Andres Freund <andres@anarazel.de>
Date: Sun, 10 Sep 2017 16:20:41 -0700
Subject: [PATCH] Constify numeric.c.
---
src/backend/utils/adt/numeric.c | 210 +++++++++++++++++++++-------------------
1 file changed, 110 insertions(+), 100 deletions(-)
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index bc01f3c284..ddc44d5179 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -367,59 +367,59 @@ typedef struct NumericSumAccum
* Some preinitialized constants
* ----------
*/
-static NumericDigit const_zero_data[1] = {0};
-static NumericVar const_zero =
-{0, 0, NUMERIC_POS, 0, NULL, const_zero_data};
+static const NumericDigit const_zero_data[1] = {0};
+static const NumericVar const_zero =
+{0, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_zero_data};
-static NumericDigit const_one_data[1] = {1};
-static NumericVar const_one =
-{1, 0, NUMERIC_POS, 0, NULL, const_one_data};
+static const NumericDigit const_one_data[1] = {1};
+static const NumericVar const_one =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_one_data};
-static NumericDigit const_two_data[1] = {2};
-static NumericVar const_two =
-{1, 0, NUMERIC_POS, 0, NULL, const_two_data};
+static const NumericDigit const_two_data[1] = {2};
+static const NumericVar const_two =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_two_data};
#if DEC_DIGITS == 4 || DEC_DIGITS == 2
-static NumericDigit const_ten_data[1] = {10};
-static NumericVar const_ten =
-{1, 0, NUMERIC_POS, 0, NULL, const_ten_data};
+static const NumericDigit const_ten_data[1] = {10};
+static const NumericVar const_ten =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_ten_data};
#elif DEC_DIGITS == 1
-static NumericDigit const_ten_data[1] = {1};
-static NumericVar const_ten =
-{1, 1, NUMERIC_POS, 0, NULL, const_ten_data};
+static const NumericDigit const_ten_data[1] = {1};
+static const NumericVar const_ten =
+{1, 1, NUMERIC_POS, 0, NULL, (NumericDigit *) const_ten_data};
#endif
#if DEC_DIGITS == 4
-static NumericDigit const_zero_point_five_data[1] = {5000};
+static const NumericDigit const_zero_point_five_data[1] = {5000};
#elif DEC_DIGITS == 2
-static NumericDigit const_zero_point_five_data[1] = {50};
+static const NumericDigit const_zero_point_five_data[1] = {50};
#elif DEC_DIGITS == 1
-static NumericDigit const_zero_point_five_data[1] = {5};
+static const NumericDigit const_zero_point_five_data[1] = {5};
#endif
-static NumericVar const_zero_point_five =
-{1, -1, NUMERIC_POS, 1, NULL, const_zero_point_five_data};
+static const NumericVar const_zero_point_five =
+{1, -1, NUMERIC_POS, 1, NULL, (NumericDigit *) const_zero_point_five_data};
#if DEC_DIGITS == 4
-static NumericDigit const_zero_point_nine_data[1] = {9000};
+static const NumericDigit const_zero_point_nine_data[1] = {9000};
#elif DEC_DIGITS == 2
-static NumericDigit const_zero_point_nine_data[1] = {90};
+static const NumericDigit const_zero_point_nine_data[1] = {90};
#elif DEC_DIGITS == 1
-static NumericDigit const_zero_point_nine_data[1] = {9};
+static const NumericDigit const_zero_point_nine_data[1] = {9};
#endif
-static NumericVar const_zero_point_nine =
-{1, -1, NUMERIC_POS, 1, NULL, const_zero_point_nine_data};
+static const NumericVar const_zero_point_nine =
+{1, -1, NUMERIC_POS, 1, NULL, (NumericDigit *) const_zero_point_nine_data};
#if DEC_DIGITS == 4
-static NumericDigit const_one_point_one_data[2] = {1, 1000};
+static const NumericDigit const_one_point_one_data[2] = {1, 1000};
#elif DEC_DIGITS == 2
-static NumericDigit const_one_point_one_data[2] = {1, 10};
+static const NumericDigit const_one_point_one_data[2] = {1, 10};
#elif DEC_DIGITS == 1
-static NumericDigit const_one_point_one_data[2] = {1, 1};
+static const NumericDigit const_one_point_one_data[2] = {1, 1};
#endif
-static NumericVar const_one_point_one =
-{2, 0, NUMERIC_POS, 1, NULL, const_one_point_one_data};
+static const NumericVar const_one_point_one =
+{2, 0, NUMERIC_POS, 1, NULL, (NumericDigit *) const_one_point_one_data};
-static NumericVar const_nan =
+static const NumericVar const_nan =
{0, 0, NUMERIC_NAN, 0, NULL, NULL};
#if DEC_DIGITS == 4
@@ -467,74 +467,84 @@ static const char *set_var_from_str(const char *str, const char *cp,
NumericVar *dest);
static void set_var_from_num(Numeric value, NumericVar *dest);
static void init_var_from_num(Numeric num, NumericVar *dest);
-static void set_var_from_var(NumericVar *value, NumericVar *dest);
-static char *get_str_from_var(NumericVar *var);
-static char *get_str_from_var_sci(NumericVar *var, int rscale);
+static void set_var_from_var(const NumericVar *value, NumericVar *dest);
+static char *get_str_from_var(const NumericVar *var);
+static char *get_str_from_var_sci(const NumericVar *var, int rscale);
-static Numeric make_result(NumericVar *var);
+static Numeric make_result(const NumericVar *var);
static void apply_typmod(NumericVar *var, int32 typmod);
-static int32 numericvar_to_int32(NumericVar *var);
-static bool numericvar_to_int64(NumericVar *var, int64 *result);
+static int32 numericvar_to_int32(const NumericVar *var);
+static bool numericvar_to_int64(const NumericVar *var, int64 *result);
static void int64_to_numericvar(int64 val, NumericVar *var);
#ifdef HAVE_INT128
-static bool numericvar_to_int128(NumericVar *var, int128 *result);
+static bool numericvar_to_int128(const NumericVar *var, int128 *result);
static void int128_to_numericvar(int128 val, NumericVar *var);
#endif
static double numeric_to_double_no_overflow(Numeric num);
-static double numericvar_to_double_no_overflow(NumericVar *var);
+static double numericvar_to_double_no_overflow(const NumericVar *var);
static Datum numeric_abbrev_convert(Datum original_datum, SortSupport ssup);
static bool numeric_abbrev_abort(int memtupcount, SortSupport ssup);
static int numeric_fast_cmp(Datum x, Datum y, SortSupport ssup);
static int numeric_cmp_abbrev(Datum x, Datum y, SortSupport ssup);
-static Datum numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss);
+static Datum numeric_abbrev_convert_var(const NumericVar *var,
+ NumericSortSupport *nss);
static int cmp_numerics(Numeric num1, Numeric num2);
-static int cmp_var(NumericVar *var1, NumericVar *var2);
+static int cmp_var(const NumericVar *var1, const NumericVar *var2);
static int cmp_var_common(const NumericDigit *var1digits, int var1ndigits,
int var1weight, int var1sign,
const NumericDigit *var2digits, int var2ndigits,
int var2weight, int var2sign);
-static void add_var(NumericVar *var1, NumericVar *var2, NumericVar *result);
-static void sub_var(NumericVar *var1, NumericVar *var2, NumericVar *result);
-static void mul_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
+static void add_var(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result);
+static void sub_var(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result);
+static void mul_var(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result,
int rscale);
-static void div_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
+static void div_var(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result,
int rscale, bool round);
-static void div_var_fast(NumericVar *var1, NumericVar *var2, NumericVar *result,
- int rscale, bool round);
-static int select_div_scale(NumericVar *var1, NumericVar *var2);
-static void mod_var(NumericVar *var1, NumericVar *var2, NumericVar *result);
-static void ceil_var(NumericVar *var, NumericVar *result);
-static void floor_var(NumericVar *var, NumericVar *result);
+static void div_var_fast(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result, int rscale, bool round);
+static int select_div_scale(const NumericVar *var1, const NumericVar *var2);
+static void mod_var(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result);
+static void ceil_var(const NumericVar *var, NumericVar *result);
+static void floor_var(const NumericVar *var, NumericVar *result);
-static void sqrt_var(NumericVar *arg, NumericVar *result, int rscale);
-static void exp_var(NumericVar *arg, NumericVar *result, int rscale);
-static int estimate_ln_dweight(NumericVar *var);
-static void ln_var(NumericVar *arg, NumericVar *result, int rscale);
-static void log_var(NumericVar *base, NumericVar *num, NumericVar *result);
-static void power_var(NumericVar *base, NumericVar *exp, NumericVar *result);
-static void power_var_int(NumericVar *base, int exp, NumericVar *result,
+static void sqrt_var(const NumericVar *arg, NumericVar *result, int rscale);
+static void exp_var(const NumericVar *arg, NumericVar *result, int rscale);
+static int estimate_ln_dweight(const NumericVar *var);
+static void ln_var(const NumericVar *arg, NumericVar *result, int rscale);
+static void log_var(const NumericVar *base, const NumericVar *num,
+ NumericVar *result);
+static void power_var(const NumericVar *base, const NumericVar *exp,
+ NumericVar *result);
+static void power_var_int(const NumericVar *base, int exp, NumericVar *result,
int rscale);
-static int cmp_abs(NumericVar *var1, NumericVar *var2);
+static int cmp_abs(const NumericVar *var1, const NumericVar *var2);
static int cmp_abs_common(const NumericDigit *var1digits, int var1ndigits,
int var1weight,
const NumericDigit *var2digits, int var2ndigits,
int var2weight);
-static void add_abs(NumericVar *var1, NumericVar *var2, NumericVar *result);
-static void sub_abs(NumericVar *var1, NumericVar *var2, NumericVar *result);
+static void add_abs(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result);
+static void sub_abs(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result);
static void round_var(NumericVar *var, int rscale);
static void trunc_var(NumericVar *var, int rscale);
static void strip_var(NumericVar *var);
static void compute_bucket(Numeric operand, Numeric bound1, Numeric bound2,
- NumericVar *count_var, NumericVar *result_var);
+ const NumericVar *count_var, NumericVar *result_var);
-static void accum_sum_add(NumericSumAccum *accum, NumericVar *var1);
-static void accum_sum_rescale(NumericSumAccum *accum, NumericVar *val);
+static void accum_sum_add(NumericSumAccum *accum, const NumericVar *var1);
+static void accum_sum_rescale(NumericSumAccum *accum, const NumericVar *val);
static void accum_sum_carry(NumericSumAccum *accum);
static void accum_sum_reset(NumericSumAccum *accum);
static void accum_sum_final(NumericSumAccum *accum, NumericVar *result);
@@ -1551,7 +1561,7 @@ width_bucket_numeric(PG_FUNCTION_ARGS)
*/
static void
compute_bucket(Numeric operand, Numeric bound1, Numeric bound2,
- NumericVar *count_var, NumericVar *result_var)
+ const NumericVar *count_var, NumericVar *result_var)
{
NumericVar bound1_var;
NumericVar bound2_var;
@@ -1883,7 +1893,7 @@ numeric_cmp_abbrev(Datum x, Datum y, SortSupport ssup)
#if NUMERIC_ABBREV_BITS == 64
static Datum
-numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss)
+numeric_abbrev_convert_var(const NumericVar *var, NumericSortSupport *nss)
{
int ndigits = var->ndigits;
int weight = var->weight;
@@ -1938,7 +1948,7 @@ numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss)
#if NUMERIC_ABBREV_BITS == 32
static Datum
-numeric_abbrev_convert_var(NumericVar *var, NumericSortSupport *nss)
+numeric_abbrev_convert_var(const NumericVar *var, NumericSortSupport *nss)
{
int ndigits = var->ndigits;
int weight = var->weight;
@@ -3002,7 +3012,7 @@ numeric_int4(PG_FUNCTION_ARGS)
* ereport(). The input NumericVar is *not* free'd.
*/
static int32
-numericvar_to_int32(NumericVar *var)
+numericvar_to_int32(const NumericVar *var)
{
int32 result;
int64 val;
@@ -4719,7 +4729,7 @@ numeric_stddev_internal(NumericAggState *state,
vsumX,
vsumX2,
vNminus1;
- NumericVar *comp;
+ const NumericVar *comp;
int rscale;
/* Deal with empty input and NaN-input cases */
@@ -5715,7 +5725,7 @@ init_var_from_num(Numeric num, NumericVar *dest)
* Copy one variable into another
*/
static void
-set_var_from_var(NumericVar *value, NumericVar *dest)
+set_var_from_var(const NumericVar *value, NumericVar *dest)
{
NumericDigit *newbuf;
@@ -5741,7 +5751,7 @@ set_var_from_var(NumericVar *value, NumericVar *dest)
* Returns a palloc'd string.
*/
static char *
-get_str_from_var(NumericVar *var)
+get_str_from_var(const NumericVar *var)
{
int dscale;
char *str;
@@ -5894,7 +5904,7 @@ get_str_from_var(NumericVar *var)
* Returns a palloc'd string.
*/
static char *
-get_str_from_var_sci(NumericVar *var, int rscale)
+get_str_from_var_sci(const NumericVar *var, int rscale)
{
int32 exponent;
NumericVar denominator;
@@ -5980,7 +5990,7 @@ get_str_from_var_sci(NumericVar *var, int rscale)
* a variable.
*/
static Numeric
-make_result(NumericVar *var)
+make_result(const NumericVar *var)
{
Numeric result;
NumericDigit *digits = var->digits;
@@ -6143,7 +6153,7 @@ apply_typmod(NumericVar *var, int32 typmod)
* If overflow, return FALSE (no error is raised). Return TRUE if okay.
*/
static bool
-numericvar_to_int64(NumericVar *var, int64 *result)
+numericvar_to_int64(const NumericVar *var, int64 *result)
{
NumericDigit *digits;
int ndigits;
@@ -6262,7 +6272,7 @@ int64_to_numericvar(int64 val, NumericVar *var)
* If overflow, return FALSE (no error is raised). Return TRUE if okay.
*/
static bool
-numericvar_to_int128(NumericVar *var, int128 *result)
+numericvar_to_int128(const NumericVar *var, int128 *result)
{
NumericDigit *digits;
int ndigits;
@@ -6406,7 +6416,7 @@ numeric_to_double_no_overflow(Numeric num)
/* As above, but work from a NumericVar */
static double
-numericvar_to_double_no_overflow(NumericVar *var)
+numericvar_to_double_no_overflow(const NumericVar *var)
{
char *tmp;
double val;
@@ -6438,7 +6448,7 @@ numericvar_to_double_no_overflow(NumericVar *var)
* truncated to no digits.
*/
static int
-cmp_var(NumericVar *var1, NumericVar *var2)
+cmp_var(const NumericVar *var1, const NumericVar *var2)
{
return cmp_var_common(var1->digits, var1->ndigits,
var1->weight, var1->sign,
@@ -6496,7 +6506,7 @@ cmp_var_common(const NumericDigit *var1digits, int var1ndigits,
* result might point to one of the operands too without danger.
*/
static void
-add_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
+add_var(const NumericVar *var1, const NumericVar *var2, NumericVar *result)
{
/*
* Decide on the signs of the two variables what to do
@@ -6613,7 +6623,7 @@ add_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
* result might point to one of the operands too without danger.
*/
static void
-sub_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
+sub_var(const NumericVar *var1, const NumericVar *var2, NumericVar *result)
{
/*
* Decide on the signs of the two variables what to do
@@ -6734,7 +6744,7 @@ sub_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
* in result. Result is rounded to no more than rscale fractional digits.
*/
static void
-mul_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
+mul_var(const NumericVar *var1, const NumericVar *var2, NumericVar *result,
int rscale)
{
int res_ndigits;
@@ -6763,7 +6773,7 @@ mul_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
*/
if (var1->ndigits > var2->ndigits)
{
- NumericVar *tmp = var1;
+ const NumericVar *tmp = var1;
var1 = var2;
var2 = tmp;
@@ -6931,7 +6941,7 @@ mul_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
* is truncated (towards zero) at that digit.
*/
static void
-div_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
+div_var(const NumericVar *var1, const NumericVar *var2, NumericVar *result,
int rscale, bool round)
{
int div_ndigits;
@@ -7216,8 +7226,8 @@ div_var(NumericVar *var1, NumericVar *var2, NumericVar *result,
* the correct answer is 1.
*/
static void
-div_var_fast(NumericVar *var1, NumericVar *var2, NumericVar *result,
- int rscale, bool round)
+div_var_fast(const NumericVar *var1, const NumericVar *var2,
+ NumericVar *result, int rscale, bool round)
{
int div_ndigits;
int res_sign;
@@ -7511,7 +7521,7 @@ div_var_fast(NumericVar *var1, NumericVar *var2, NumericVar *result,
* Returns the appropriate result scale for the division result.
*/
static int
-select_div_scale(NumericVar *var1, NumericVar *var2)
+select_div_scale(const NumericVar *var1, const NumericVar *var2)
{
int weight1,
weight2,
@@ -7580,7 +7590,7 @@ select_div_scale(NumericVar *var1, NumericVar *var2)
* Calculate the modulo of two numerics at variable level
*/
static void
-mod_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
+mod_var(const NumericVar *var1, const NumericVar *var2, NumericVar *result)
{
NumericVar tmp;
@@ -7609,7 +7619,7 @@ mod_var(NumericVar *var1, NumericVar *var2, NumericVar *result)
* on variable level
*/
static void
-ceil_var(NumericVar *var, NumericVar *result)
+ceil_var(const NumericVar *var, NumericVar *result)
{
NumericVar tmp;
@@ -7633,7 +7643,7 @@ ceil_var(NumericVar *var, NumericVar *result)
* on variable level
*/
static void
-floor_var(NumericVar *var, NumericVar *result)
+floor_var(const NumericVar *var, NumericVar *result)
{
NumericVar tmp;
@@ -7656,7 +7666,7 @@ floor_var(NumericVar *var, NumericVar *result)
* Compute the square root of x using Newton's algorithm
*/
static void
-sqrt_var(NumericVar *arg, NumericVar *result, int rscale)
+sqrt_var(const NumericVar *arg, NumericVar *result, int rscale)
{
NumericVar tmp_arg;
NumericVar tmp_val;
@@ -7729,7 +7739,7 @@ sqrt_var(NumericVar *arg, NumericVar *result, int rscale)
* Raise e to the power of x, computed to rscale fractional digits
*/
static void
-exp_var(NumericVar *arg, NumericVar *result, int rscale)
+exp_var(const NumericVar *arg, NumericVar *result, int rscale)
{
NumericVar x;
NumericVar elem;
@@ -7855,7 +7865,7 @@ exp_var(NumericVar *arg, NumericVar *result, int rscale)
* determine the appropriate rscale when computing natural logarithms.
*/
static int
-estimate_ln_dweight(NumericVar *var)
+estimate_ln_dweight(const NumericVar *var)
{
int ln_dweight;
@@ -7933,7 +7943,7 @@ estimate_ln_dweight(NumericVar *var)
* Compute the natural log of x
*/
static void
-ln_var(NumericVar *arg, NumericVar *result, int rscale)
+ln_var(const NumericVar *arg, NumericVar *result, int rscale)
{
NumericVar x;
NumericVar xx;
@@ -8040,7 +8050,7 @@ ln_var(NumericVar *arg, NumericVar *result, int rscale)
* Note: this routine chooses dscale of the result.
*/
static void
-log_var(NumericVar *base, NumericVar *num, NumericVar *result)
+log_var(const NumericVar *base, const NumericVar *num, NumericVar *result)
{
NumericVar ln_base;
NumericVar ln_num;
@@ -8100,7 +8110,7 @@ log_var(NumericVar *base, NumericVar *num, NumericVar *result)
* Note: this routine chooses dscale of the result.
*/
static void
-power_var(NumericVar *base, NumericVar *exp, NumericVar *result)
+power_var(const NumericVar *base, const NumericVar *exp, NumericVar *result)
{
NumericVar ln_base;
NumericVar ln_num;
@@ -8215,7 +8225,7 @@ power_var(NumericVar *base, NumericVar *exp, NumericVar *result)
* Raise base to the power of exp, where exp is an integer.
*/
static void
-power_var_int(NumericVar *base, int exp, NumericVar *result, int rscale)
+power_var_int(const NumericVar *base, int exp, NumericVar *result, int rscale)
{
double f;
int p;
@@ -8405,7 +8415,7 @@ power_var_int(NumericVar *base, int exp, NumericVar *result, int rscale)
* ----------
*/
static int
-cmp_abs(NumericVar *var1, NumericVar *var2)
+cmp_abs(const NumericVar *var1, const NumericVar *var2)
{
return cmp_abs_common(var1->digits, var1->ndigits, var1->weight,
var2->digits, var2->ndigits, var2->weight);
@@ -8483,7 +8493,7 @@ cmp_abs_common(const NumericDigit *var1digits, int var1ndigits, int var1weight,
* result might point to one of the operands without danger.
*/
static void
-add_abs(NumericVar *var1, NumericVar *var2, NumericVar *result)
+add_abs(const NumericVar *var1, const NumericVar *var2, NumericVar *result)
{
NumericDigit *res_buf;
NumericDigit *res_digits;
@@ -8568,7 +8578,7 @@ add_abs(NumericVar *var1, NumericVar *var2, NumericVar *result)
* ABS(var1) MUST BE GREATER OR EQUAL ABS(var2) !!!
*/
static void
-sub_abs(NumericVar *var1, NumericVar *var2, NumericVar *result)
+sub_abs(const NumericVar *var1, const NumericVar *var2, NumericVar *result)
{
NumericDigit *res_buf;
NumericDigit *res_digits;
@@ -8875,7 +8885,7 @@ accum_sum_reset(NumericSumAccum *accum)
* Accumulate a new value.
*/
static void
-accum_sum_add(NumericSumAccum *accum, NumericVar *val)
+accum_sum_add(NumericSumAccum *accum, const NumericVar *val)
{
int32 *accum_digits;
int i,
@@ -8996,7 +9006,7 @@ accum_sum_carry(NumericSumAccum *accum)
* accumulator, enlarge the buffers.
*/
static void
-accum_sum_rescale(NumericSumAccum *accum, NumericVar *val)
+accum_sum_rescale(NumericSumAccum *accum, const NumericVar *val)
{
int old_weight = accum->weight;
int old_ndigits = accum->ndigits;
--
2.14.1.2.g4274c698f4.dirty
Andres Freund <andres@anarazel.de> writes:
One large user of unnecessary non-constant static variables is
numeric.c. More out of curiosity - numeric is slow enough in itself to
make inlining not a huge win - I converted it to use consts.
LGTM.
It's a bit ugly that some consts have to be casted away in the constant
definitions, but aside from just inlining the values, I don't quite see
a better solution?
No, I don't either. I'm not sure that writing the constant inline would
produce the desired results - the compiler might well decide that it had
to be in read-write storage.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sep 11, 2017, at 5:10 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andres Freund <andres@anarazel.de> writes:
One large user of unnecessary non-constant static variables is
numeric.c. More out of curiosity - numeric is slow enough in itself to
make inlining not a huge win - I converted it to use consts.LGTM.
It's a bit ugly that some consts have to be casted away in the constant
definitions, but aside from just inlining the values, I don't quite see
a better solution?No, I don't either. I'm not sure that writing the constant inline would
produce the desired results - the compiler might well decide that it had
to be in read-write storage.regards, tom lane
This patch got committed as c1898c3e1e235ae35b4759d233253eff221b976a
on Sun Sep 10 16:20:41 2017 -0700, but I've only just gotten around to
reviewing it.
I believe this is wrong and should be reverted, at least in part.
The NumericVar struct has the field 'digits' as non-const:
typedef struct NumericVar
{
int ndigits; /* # of digits in digits[] - can be 0! */
int weight; /* weight of first digit */
int sign; /* NUMERIC_POS, NUMERIC_NEG, or NUMERIC_NAN */
int dscale; /* display scale */
NumericDigit *buf; /* start of palloc'd space for digits[] */
NumericDigit *digits; /* base-NBASE digits */
} NumericVar;
The static const data which is getting put in read only memory sets that data
by casting away const as follows:
static const NumericDigit const_zero_data[1] = {0};
static const NumericVar const_zero =
{0, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_zero_data};
This means that the const variable 'const_zero' contains a pointer that is
non-const, pointing at something that is static const, stored in read only
memory. Yikes.
The function set_var_from_var(const NumericVar *value, NumericVar *dest)
uses memcpy to copy the contents of value into dest. In cases where the value
is a static const variable (eg, const_zero), the memcpy is copying a pointer to
static const read only data into the dest and implicitly casting away const.
Since that static const data is stored in read only memory, this has undefined
semantics, and I believe could lead to a server crash, at least on some
architectures with some compilers.
The idea that set_var_from_var might be called on const_zero (or const_one,
etc.) is not hypothetical. It is being done in numeric.c.
If this is safe, somebody needs to be a lot clearer about why that is so. There
are no comments explaining it in the file, and the conversation in this thread
never got into any details about it either.
Mark Dilger
On February 21, 2018 8:49:51 AM PST, Mark Dilger <hornschnorter@gmail.com> wrote:
The idea that set_var_from_var might be called on const_zero (or
const_one,
etc.) is not hypothetical. It is being done in numeric.c.If this is safe, somebody needs to be a lot clearer about why that is
so. There
are no comments explaining it in the file, and the conversation in this
thread
never got into any details about it either.
Just getting started for the day, no coffee yet. But, uh, what you're appear to be saying is that we have code modifying the digits of the zero value? That's seems unlikely.
Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
This patch got committed as c1898c3e1e235ae35b4759d233253eff221b976a
on Sun Sep 10 16:20:41 2017 -0700, but I've only just gotten around to
reviewing it.I believe this is wrong and should be reverted, at least in part.
The NumericVar struct has the field 'digits' as non-const:
typedef struct NumericVar
{
int ndigits; /* # of digits in digits[] - can be 0! */
int weight; /* weight of first digit */
int sign; /* NUMERIC_POS, NUMERIC_NEG, or NUMERIC_NAN */
int dscale; /* display scale */
NumericDigit *buf; /* start of palloc'd space for digits[] */
NumericDigit *digits; /* base-NBASE digits */
} NumericVar;The static const data which is getting put in read only memory sets that data
by casting away const as follows:static const NumericDigit const_zero_data[1] = {0};
static const NumericVar const_zero =
{0, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_zero_data};This means that the const variable 'const_zero' contains a pointer that is
non-const, pointing at something that is static const, stored in read only
memory. Yikes.
I still believe this is unsafe.
The function set_var_from_var(const NumericVar *value, NumericVar *dest)
uses memcpy to copy the contents of value into dest. In cases where the value
is a static const variable (eg, const_zero), the memcpy is copying a pointer to
static const read only data into the dest and implicitly casting away const.
Since that static const data is stored in read only memory, this has undefined
semantics, and I believe could lead to a server crash, at least on some
architectures with some compilers.
This is probably safe, though, since NumericDigit seems to just be a typedef
to int16. I should have checked that definition before complaining about that
part.
Mark Dilger <hornschnorter@gmail.com> writes:
This means that the const variable 'const_zero' contains a pointer that is
non-const, pointing at something that is static const, stored in read only
memory. Yikes.
I still believe this is unsafe.
I'm with Andres: I don't see the problem. It's true that we've casted
away a chance for the compiler to notice a problem, but that's not the
only defense against problems. If we did try to modify const_zero,
what should happen now is that we get a SIGSEGV from trying to scribble
on read-only memory. But that's actually a step forward from before.
Before, we'd have successfully modified the value of const_zero and
thereby silently bollixed subsequent computations using it. Since,
in fact, the code should never try to modify const_zero, the SIGSEGV
should never happen. So effectively we have a hardware-enforced Assert
that we don't modify it, and that seems good.
As far as compiler-detectable mistakes go, Andres' changes to declare
various function inputs as const seem like a pretty considerable
improvement too, even if they aren't offering 100% coverage.
regards, tom lane