public inbox for [email protected]
help / color / mirror / Atom feedFrom: Alexander Lakhin <[email protected]>
To: Nathan Bossart <[email protected]>
Cc: Joseph Koshakow <[email protected]>
Cc: jian he <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Matthew Kim <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Cc: Andres Freund <[email protected]>
Subject: Re: Remove dependence on integer wrapping
Date: Wed, 21 Aug 2024 10:00:00 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZsUI3QJdUOIpPkZ5@nathan>
References: <Zr5M3KxOZlEZRZIN@nathan>
<Zr50ZpSyrBx0EHfR@nathan>
<CAAvxfHfkHS1n0+wMbtKwj6_GE6+aSJ9JjnsCOOdZPhTKRs-Yhw@mail.gmail.com>
<Zr-DwBxep-IaayQF@nathan>
<[email protected]>
<Zr-b1c6x-Yf31TCZ@nathan>
<Zr-yiLatqDlen3oH@nathan>
<CAAvxfHd43nT6Va20wgftJ9XQuNPpmYSjR4WRc1jxTv=U4qOZgg@mail.gmail.com>
<CAAvxfHeWeETpiwxn11NvWkWd4o0curubg4cTXuk6yOGtixzAdg@mail.gmail.com>
<[email protected]>
<ZsUI3QJdUOIpPkZ5@nathan>
Hello Nathan,
21.08.2024 00:21, Nathan Bossart wrote:
> I've combined all the current proposed changes into one patch. I've also
> introduced signed versions of the negation functions into int.h to avoid
> relying on multiplication.
>
Thank you for taking care of this!
I'd like to add some info to show how big the iceberg is.
Beside other trap-triggered places in date/time conversion functions, I
also discovered:
1)
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-2147483648] = '0';
#4 0x00007f15ab00d7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00005570113b2591 in __addvsi3 ()
#6 0x00005570111d55a0 in push_null_elements (ps=0x7fff37385fb8, num=-2147483648) at jsonfuncs.c:1707
#7 0x00005570111d5749 in push_path (st=0x7fff37385fb8, level=0, path_elems=0x55701300c880, path_nulls=0x55701300d520,
path_len=2, newval=0x7fff37386030) at jsonfuncs.c:1770
The "problematic" code:
while (num-- > 0)
*ps = 0;
looks innocent to me, but is not for good enough for -ftrapv.
I think there could be other similar places and this raises two questions:
can they be reached with INT_MIN and what to do if so?
By the way, the same can be seen with CC=clang CPPFLAGS="-ftrapv". Please
look at the code produced by both compilers for x86_64:
https://godbolt.org/z/vjszjf4b3
(clang generates ud1, while gcc uses call __addvsi3)
The aside question is: should jsonb subscripting accept negative indexes
when the target array is not initialized yet?
Compare:
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
SELECT * FROM jt;
j
-------
[[0]]
with
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[[]]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
ERROR: path element at position 2 is out of range: -1
2)
SELECT x, lag(x, -2147483648) OVER (ORDER BY x) FROM (SELECT 1) x;
#4 0x00007fa7d00f47f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x00005623a7336851 in __negvsi2 ()
#6 0x00005623a726ae35 in leadlag_common (fcinfo=0x7ffd59cca950, forward=false, withoffset=true, withdefault=false)
at windowfuncs.c:551
#7 0x00005623a726af19 in window_lag_with_offset (fcinfo=0x7ffd59cca950) at windowfuncs.c:594
As to 32-bit Debian, I wrote about before, I use gcc (Debian 12.2.0-14).
Please look at the demo code (and it's assembly, produced with
gcc -S -ftrapv t.c) attached:
gcc -Wall -Wextra -fsanitize=signed-integer-overflow -Wstrict-overflow=5 \
-O0 -ftrapv t.c -o t && ./t
Aborted (core dumped)
#4 0xb762226a in __GI_abort () at ./stdlib/abort.c:79
#5 0x00495077 in __mulvdi3.cold ()
#6 0x00495347 in pg_mul_s64_overflow ()
(It looks like -Wstrict-overflow can't help with the static analysis
desired in such cases.)
Moreover, I got `make check` failed with -ftrapv on aarch64 (using gcc 8.3)
as follows:
#1 0x0000007e1edc48e8 in __GI_abort () at abort.c:79
#2 0x0000005ee66b71cc in __subvdi3 ()
#3 0x0000005ee6560e24 in int8gcd_internal (arg1=-9223372036854775808, arg2=1) at int8.c:623
#4 0x0000005ee62f576c in ExecInterpExpr (state=0x5eeaba9d18, econtext=0x5eeaba95f0, isnull=<optimized out>)
at execExprInterp.c:770
...
#13 0x0000005ee64e5d84 in exec_simple_query (
query_string=query_string@entry=0x5eeaac7500 "SELECT a, b, gcd(a, b), gcd(a, -b), gcd(b, a), gcd(-b, a)\nFROM
(VALUES (0::int8, 0::int8),\n", ' ' <repeats 13 times>, "(0::int8, 29893644334::int8),\n", ' ' <repeats 13 times>,
"(288484263558::int8, 29893644334::int8),\n", ' ' <repeats 12 times>...) at postgres.c:1284
So I wonder whether enabling -ftrapv can really help us prepare the code
for -fno-wrapv?
Best regards,
Alexander
.file "t.c"
.text
.globl __mulvdi3
.type pg_mul_s64_overflow, @function
pg_mul_s64_overflow:
.LFB0:
.cfi_startproc
pushl %ebp
.cfi_def_cfa_offset 8
.cfi_offset 5, -8
movl %esp, %ebp
.cfi_def_cfa_register 5
pushl %edi
pushl %esi
pushl %ebx
subl $76, %esp
.cfi_offset 7, -12
.cfi_offset 6, -16
.cfi_offset 3, -20
call __x86.get_pc_thunk.ax
addl $_GLOBAL_OFFSET_TABLE_, %eax
movl %eax, -52(%ebp)
movl 8(%ebp), %eax
movl %eax, -32(%ebp)
movl 12(%ebp), %eax
movl %eax, -28(%ebp)
movl 16(%ebp), %eax
movl %eax, -40(%ebp)
movl 20(%ebp), %eax
movl %eax, -36(%ebp)
movl $0, -64(%ebp)
movl $0, -60(%ebp)
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %eax, -48(%ebp)
movl %edx, -44(%ebp)
movl -32(%ebp), %eax
movl %eax, %ecx
sarl $31, %ecx
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, %esi
movl %edx, %edi
movl %edi, %esi
movl %esi, %edi
sarl $31, %edi
movl -40(%ebp), %eax
sarl $31, %eax
movl -48(%ebp), %ebx
cmpl %ebx, %ecx
jne .L4
cmpl %esi, %eax
jne .L5
movl -40(%ebp), %eax
imull -32(%ebp)
jmp .L2
.L5:
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -32(%ebp), %eax
movl %eax, -48(%ebp)
jmp .L6
.L4:
cmpl %esi, %eax
jne .L7
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -48(%ebp), %esi
movl -40(%ebp), %eax
movl %eax, -48(%ebp)
.L6:
movl -40(%ebp), %edi
movl %edi, %eax
mull -32(%ebp)
movl %eax, -80(%ebp)
movl %edx, -76(%ebp)
movl -48(%ebp), %edi
movl %edi, %eax
mull %esi
movl %eax, %ecx
movl %edx, %ebx
testl %esi, %esi
jns .L8
movl %edi, %eax
movl $0, %edx
movl %eax, %edx
movl $0, %eax
movl %eax, %esi
movl %edx, %edi
movl %ecx, %eax
movl %ebx, %edx
subl %esi, %eax
sbbl %edi, %edx
movl %eax, %ecx
movl %edx, %ebx
.L8:
cmpl $0, -48(%ebp)
jns .L9
movl %ecx, %eax
movl %ebx, %edx
subl -72(%ebp), %eax
sbbl -68(%ebp), %edx
movl %eax, %ecx
movl %edx, %ebx
.L9:
movl -80(%ebp), %eax
movl -76(%ebp), %edx
movl %edx, %eax
xorl %edx, %edx
addl %ecx, %eax
adcl %ebx, %edx
movl %eax, %ecx
movl %edx, %ebx
movl %ecx, %eax
movl %ebx, %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %ecx, %esi
sarl $31, %esi
cmpl %eax, %esi
jne .L10
movl %ecx, %ebx
movl $0, %ecx
movl %ecx, %eax
movl %ebx, %edx
movl -80(%ebp), %ecx
movl $0, %ebx
orl %ecx, %eax
orl %ebx, %edx
jmp .L2
.L7:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
movl -48(%ebp), %ebx
leal 1(%ebx), %ecx
cmpl $1, %ecx
ja .L3
leal 1(%esi), %ecx
cmpl $1, %ecx
ja .L3
cmpl %esi, -48(%ebp)
jne .L11
movl $0, %ebx
movl $0, %ecx
cmpl %eax, %ebx
sbbl %edx, %ecx
jge .L3
jmp .L2
.L11:
testl %edx, %edx
jns .L3
jmp .L2
.L10:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
.L3:
movl $1, -64(%ebp)
movl $0, -60(%ebp)
.L2:
movl 24(%ebp), %ecx
movl %eax, (%ecx)
movl %edx, 4(%ecx)
movl -64(%ebp), %eax
movl -60(%ebp), %edx
andl $1, %eax
leal -12(%ebp), %esp
popl %ebx
.cfi_restore 3
popl %esi
.cfi_restore 6
popl %edi
.cfi_restore 7
popl %ebp
.cfi_restore 5
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE0:
.size pg_mul_s64_overflow, .-pg_mul_s64_overflow
.section .rodata
.LC0:
.string "r: %d, c: %lld\n"
.text
.globl main
.type main, @function
main:
.LFB1:
.cfi_startproc
leal 4(%esp), %ecx
.cfi_def_cfa 1, 0
andl $-16, %esp
pushl -4(%ecx)
pushl %ebp
movl %esp, %ebp
.cfi_escape 0x10,0x5,0x2,0x75,0
pushl %ebx
pushl %ecx
.cfi_escape 0xf,0x3,0x75,0x78,0x6
.cfi_escape 0x10,0x3,0x2,0x75,0x7c
subl $32, %esp
call __x86.get_pc_thunk.bx
addl $_GLOBAL_OFFSET_TABLE_, %ebx
movl $-1, -16(%ebp)
movl $2147483647, -12(%ebp)
movl $2, -24(%ebp)
movl $0, -20(%ebp)
movl $0, -40(%ebp)
movl $0, -36(%ebp)
subl $12, %esp
leal -40(%ebp), %eax
pushl %eax
pushl -20(%ebp)
pushl -24(%ebp)
pushl -12(%ebp)
pushl -16(%ebp)
call pg_mul_s64_overflow
addl $32, %esp
movb %al, -25(%ebp)
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movsbl -25(%ebp), %ecx
pushl %edx
pushl %eax
pushl %ecx
leal .LC0@GOTOFF(%ebx), %eax
pushl %eax
call printf@PLT
addl $16, %esp
movl $0, %eax
leal -8(%ebp), %esp
popl %ecx
.cfi_restore 1
.cfi_def_cfa 1, 0
popl %ebx
.cfi_restore 3
popl %ebp
.cfi_restore 5
leal -4(%ecx), %esp
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE1:
.size main, .-main
.section .text.__x86.get_pc_thunk.ax,"axG",@progbits,__x86.get_pc_thunk.ax,comdat
.globl __x86.get_pc_thunk.ax
.hidden __x86.get_pc_thunk.ax
.type __x86.get_pc_thunk.ax, @function
__x86.get_pc_thunk.ax:
.LFB2:
.cfi_startproc
movl (%esp), %eax
ret
.cfi_endproc
.LFE2:
.section .text.__x86.get_pc_thunk.bx,"axG",@progbits,__x86.get_pc_thunk.bx,comdat
.globl __x86.get_pc_thunk.bx
.hidden __x86.get_pc_thunk.bx
.type __x86.get_pc_thunk.bx, @function
__x86.get_pc_thunk.bx:
.LFB3:
.cfi_startproc
movl (%esp), %ebx
ret
.cfi_endproc
.LFE3:
.ident "GCC: (Debian 12.2.0-14) 12.2.0"
.section .note.GNU-stack,"",@progbits
Attachments:
[text/x-csrc] t.c (349B, ../[email protected]/2-t.c)
download | inline:
#include <stdio.h>
typedef char bool;
typedef long long int int64;
static inline bool
pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
{
return __builtin_mul_overflow(a, b, result);
}
int main()
{
int64 a = 9223372036854775807L;
int64 b = 2L;
int64 c = 0;
bool r;
r = pg_mul_s64_overflow(a, b, &c);
printf("r: %d, c: %lld\n", r, c);
}
[text/plain] t.x86.s (5.0K, ../[email protected]/3-t.x86.s)
download | inline:
.file "t.c"
.text
.globl __mulvdi3
.type pg_mul_s64_overflow, @function
pg_mul_s64_overflow:
.LFB0:
.cfi_startproc
pushl %ebp
.cfi_def_cfa_offset 8
.cfi_offset 5, -8
movl %esp, %ebp
.cfi_def_cfa_register 5
pushl %edi
pushl %esi
pushl %ebx
subl $76, %esp
.cfi_offset 7, -12
.cfi_offset 6, -16
.cfi_offset 3, -20
call __x86.get_pc_thunk.ax
addl $_GLOBAL_OFFSET_TABLE_, %eax
movl %eax, -52(%ebp)
movl 8(%ebp), %eax
movl %eax, -32(%ebp)
movl 12(%ebp), %eax
movl %eax, -28(%ebp)
movl 16(%ebp), %eax
movl %eax, -40(%ebp)
movl 20(%ebp), %eax
movl %eax, -36(%ebp)
movl $0, -64(%ebp)
movl $0, -60(%ebp)
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %eax, -48(%ebp)
movl %edx, -44(%ebp)
movl -32(%ebp), %eax
movl %eax, %ecx
sarl $31, %ecx
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, %esi
movl %edx, %edi
movl %edi, %esi
movl %esi, %edi
sarl $31, %edi
movl -40(%ebp), %eax
sarl $31, %eax
movl -48(%ebp), %ebx
cmpl %ebx, %ecx
jne .L4
cmpl %esi, %eax
jne .L5
movl -40(%ebp), %eax
imull -32(%ebp)
jmp .L2
.L5:
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -32(%ebp), %eax
movl %eax, -48(%ebp)
jmp .L6
.L4:
cmpl %esi, %eax
jne .L7
movl -32(%ebp), %eax
movl -28(%ebp), %edx
movl %eax, -72(%ebp)
movl %edx, -68(%ebp)
movl -48(%ebp), %esi
movl -40(%ebp), %eax
movl %eax, -48(%ebp)
.L6:
movl -40(%ebp), %edi
movl %edi, %eax
mull -32(%ebp)
movl %eax, -80(%ebp)
movl %edx, -76(%ebp)
movl -48(%ebp), %edi
movl %edi, %eax
mull %esi
movl %eax, %ecx
movl %edx, %ebx
testl %esi, %esi
jns .L8
movl %edi, %eax
movl $0, %edx
movl %eax, %edx
movl $0, %eax
movl %eax, %esi
movl %edx, %edi
movl %ecx, %eax
movl %ebx, %edx
subl %esi, %eax
sbbl %edi, %edx
movl %eax, %ecx
movl %edx, %ebx
.L8:
cmpl $0, -48(%ebp)
jns .L9
movl %ecx, %eax
movl %ebx, %edx
subl -72(%ebp), %eax
sbbl -68(%ebp), %edx
movl %eax, %ecx
movl %edx, %ebx
.L9:
movl -80(%ebp), %eax
movl -76(%ebp), %edx
movl %edx, %eax
xorl %edx, %edx
addl %ecx, %eax
adcl %ebx, %edx
movl %eax, %ecx
movl %edx, %ebx
movl %ecx, %eax
movl %ebx, %edx
movl %edx, %eax
movl %eax, %edx
sarl $31, %edx
movl %ecx, %esi
sarl $31, %esi
cmpl %eax, %esi
jne .L10
movl %ecx, %ebx
movl $0, %ecx
movl %ecx, %eax
movl %ebx, %edx
movl -80(%ebp), %ecx
movl $0, %ebx
orl %ecx, %eax
orl %ebx, %edx
jmp .L2
.L7:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
movl -48(%ebp), %ebx
leal 1(%ebx), %ecx
cmpl $1, %ecx
ja .L3
leal 1(%esi), %ecx
cmpl $1, %ecx
ja .L3
cmpl %esi, -48(%ebp)
jne .L11
movl $0, %ebx
movl $0, %ecx
cmpl %eax, %ebx
sbbl %edx, %ecx
jge .L3
jmp .L2
.L11:
testl %edx, %edx
jns .L3
jmp .L2
.L10:
pushl -36(%ebp)
pushl -40(%ebp)
pushl -28(%ebp)
pushl -32(%ebp)
movl -52(%ebp), %ebx
call __mulvdi3@PLT
addl $16, %esp
.L3:
movl $1, -64(%ebp)
movl $0, -60(%ebp)
.L2:
movl 24(%ebp), %ecx
movl %eax, (%ecx)
movl %edx, 4(%ecx)
movl -64(%ebp), %eax
movl -60(%ebp), %edx
andl $1, %eax
leal -12(%ebp), %esp
popl %ebx
.cfi_restore 3
popl %esi
.cfi_restore 6
popl %edi
.cfi_restore 7
popl %ebp
.cfi_restore 5
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE0:
.size pg_mul_s64_overflow, .-pg_mul_s64_overflow
.section .rodata
.LC0:
.string "r: %d, c: %lld\n"
.text
.globl main
.type main, @function
main:
.LFB1:
.cfi_startproc
leal 4(%esp), %ecx
.cfi_def_cfa 1, 0
andl $-16, %esp
pushl -4(%ecx)
pushl %ebp
movl %esp, %ebp
.cfi_escape 0x10,0x5,0x2,0x75,0
pushl %ebx
pushl %ecx
.cfi_escape 0xf,0x3,0x75,0x78,0x6
.cfi_escape 0x10,0x3,0x2,0x75,0x7c
subl $32, %esp
call __x86.get_pc_thunk.bx
addl $_GLOBAL_OFFSET_TABLE_, %ebx
movl $-1, -16(%ebp)
movl $2147483647, -12(%ebp)
movl $2, -24(%ebp)
movl $0, -20(%ebp)
movl $0, -40(%ebp)
movl $0, -36(%ebp)
subl $12, %esp
leal -40(%ebp), %eax
pushl %eax
pushl -20(%ebp)
pushl -24(%ebp)
pushl -12(%ebp)
pushl -16(%ebp)
call pg_mul_s64_overflow
addl $32, %esp
movb %al, -25(%ebp)
movl -40(%ebp), %eax
movl -36(%ebp), %edx
movsbl -25(%ebp), %ecx
pushl %edx
pushl %eax
pushl %ecx
leal .LC0@GOTOFF(%ebx), %eax
pushl %eax
call printf@PLT
addl $16, %esp
movl $0, %eax
leal -8(%ebp), %esp
popl %ecx
.cfi_restore 1
.cfi_def_cfa 1, 0
popl %ebx
.cfi_restore 3
popl %ebp
.cfi_restore 5
leal -4(%ecx), %esp
.cfi_def_cfa 4, 4
ret
.cfi_endproc
.LFE1:
.size main, .-main
.section .text.__x86.get_pc_thunk.ax,"axG",@progbits,__x86.get_pc_thunk.ax,comdat
.globl __x86.get_pc_thunk.ax
.hidden __x86.get_pc_thunk.ax
.type __x86.get_pc_thunk.ax, @function
__x86.get_pc_thunk.ax:
.LFB2:
.cfi_startproc
movl (%esp), %eax
ret
.cfi_endproc
.LFE2:
.section .text.__x86.get_pc_thunk.bx,"axG",@progbits,__x86.get_pc_thunk.bx,comdat
.globl __x86.get_pc_thunk.bx
.hidden __x86.get_pc_thunk.bx
.type __x86.get_pc_thunk.bx, @function
__x86.get_pc_thunk.bx:
.LFB3:
.cfi_startproc
movl (%esp), %ebx
ret
.cfi_endproc
.LFE3:
.ident "GCC: (Debian 12.2.0-14) 12.2.0"
.section .note.GNU-stack,"",@progbits
view thread (19+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Remove dependence on integer wrapping
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox