Обсуждение: [HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
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 modifyingthe 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