These temporaries seem to been inadvertently changed to signed during a refactoring. Generally, bit shifting is frowned upon for signed values.
Return temporaries to being unsigned in secp256k1_fe_sqr_inner #1442
pull roconnor-blockstream wants to merge 1 commits into bitcoin-core:master from roconnor-blockstream:patch-7 changing 1 files +1 −1-
roconnor-blockstream commented at 2:29 PM on November 14, 2023: contributor
-
10271356c8
Return temporaries to being unsigned in secp256k1_fe_sqr_inner
These temporaries seem to been inadvertently changed to signed during a refactoring. Generally, bit shifting is frowned upon for signed values.
- roconnor-blockstream referenced this in commit 25b35c7ecb on Nov 14, 2023
- real-or-random added the label assurance on Nov 14, 2023
- real-or-random added the label refactor/smell on Nov 14, 2023
-
real-or-random commented at 2:38 PM on November 14, 2023: contributor
Generally, bit shifting is frowned upon for signed values.
True, at least for left shifting. In this case, the only relevant shift is
u0 << 4, and here we know that u0 is only 52 bits, so this is fine. But yes, this should be fixed, unsigned is just safer. Thanks for the PR. - real-or-random approved
-
real-or-random commented at 2:38 PM on November 14, 2023: contributor
utACK 10271356c8fc34395850ac70df5902571945fbea
-
sipa commented at 2:37 PM on November 15, 2023: contributor
Does this change the compiled code at all?
-
sipa commented at 2:43 PM on November 15, 2023: contributor
Compiled with default options and GCC 13.2.0, yes:
@@ -973,7 +973,7 @@ 2883: 4c 8b 46 18 mov 0x18(%rsi),%r8 2887: 48 8b 0e mov (%rsi),%rcx 288a: 48 8d 34 00 lea (%rax,%rax,1),%rsi - 288e: 48 89 44 24 d8 mov %rax,-0x28(%rsp) + 288e: 48 89 44 24 e8 mov %rax,-0x18(%rsp) 2893: 48 89 f8 mov %rdi,%rax 2896: 48 f7 e7 mul %rdi 2899: 4c 8d 2c 09 lea (%rcx,%rcx,1),%r13 @@ -1031,9 +1031,9 @@ 294b: 48 11 d7 adc %rdx,%rdi 294e: 49 f7 e0 mul %r8 2951: 49 89 f7 mov %rsi,%r15 - 2954: 4c 89 7c 24 e0 mov %r15,-0x20(%rsp) + 2954: 4c 89 7c 24 f0 mov %r15,-0x10(%rsp) 2959: 49 89 c2 mov %rax,%r10 - 295c: 48 8b 44 24 d8 mov -0x28(%rsp),%rax + 295c: 48 8b 44 24 e8 mov -0x18(%rsp),%rax 2961: 49 89 d3 mov %rdx,%r11 2964: 49 f7 e1 mul %r9 2967: 49 01 c2 add %rax,%r10 @@ -1046,14 +1046,14 @@ 2983: 49 11 fb adc %rdi,%r11 2986: 4c 89 d6 mov %r10,%rsi 2989: 4c 89 ff mov %r15,%rdi - 298c: 48 c1 ff 30 sar $0x30,%rdi + 298c: 48 c1 ef 30 shr $0x30,%rdi 2990: 48 c1 e6 04 shl $0x4,%rsi 2994: 48 21 c6 and %rax,%rsi 2997: 48 89 f8 mov %rdi,%rax 299a: 83 e0 0f and $0xf,%eax 299d: 48 09 c6 or %rax,%rsi 29a0: 4c 89 f0 mov %r14,%rax - 29a3: 48 f7 ee imul %rsi + 29a3: 48 f7 e6 mul %rsi 29a6: 48 89 c6 mov %rax,%rsi 29a9: 48 89 c8 mov %rcx,%rax 29ac: 48 89 d7 mov %rdx,%rdi @@ -1064,9 +1064,9 @@ 29bf: 4c 89 c8 mov %r9,%rax 29c2: 48 11 d7 adc %rdx,%rdi 29c5: 48 89 f2 mov %rsi,%rdx - 29c8: 48 89 74 24 e8 mov %rsi,-0x18(%rsp) + 29c8: 48 89 74 24 d8 mov %rsi,-0x28(%rsp) 29cd: 48 21 ca and %rcx,%rdx - 29d0: 48 89 7c 24 f0 mov %rdi,-0x10(%rsp) + 29d0: 48 89 7c 24 e0 mov %rdi,-0x20(%rsp) 29d5: 48 89 13 mov %rdx,(%rbx) 29d8: 49 f7 e4 mul %r12 29db: 48 89 c6 mov %rax,%rsi @@ -1086,7 +1086,7 @@ 2a08: 48 89 d0 mov %rdx,%rax 2a0b: 48 f7 e5 mul %rbp 2a0e: 49 89 c2 mov %rax,%r10 - 2a11: 48 8b 44 24 d8 mov -0x28(%rsp),%rax + 2a11: 48 8b 44 24 e8 mov -0x18(%rsp),%rax 2a16: 49 89 d3 mov %rdx,%r11 2a19: 49 f7 e5 mul %r13 2a1c: 48 89 c6 mov %rax,%rsi @@ -1094,21 +1094,21 @@ 2a22: 4c 89 c8 mov %r9,%rax 2a25: 4d 89 f9 mov %r15,%r9 2a28: 4c 01 d6 add %r10,%rsi - 2a2b: 4c 8b 54 24 e8 mov -0x18(%rsp),%r10 + 2a2b: 4c 8b 54 24 d8 mov -0x28(%rsp),%r10 2a30: 4c 11 df adc %r11,%rdi - 2a33: 4c 8b 5c 24 f0 mov -0x10(%rsp),%r11 + 2a33: 4c 8b 5c 24 e0 mov -0x20(%rsp),%r11 2a38: 4d 0f ac da 34 shrd $0x34,%r11,%r10 2a3d: 49 c1 eb 34 shr $0x34,%r11 2a41: 4c 01 d6 add %r10,%rsi 2a44: 48 89 f2 mov %rsi,%rdx 2a47: 4c 11 df adc %r11,%rdi 2a4a: 49 c1 e9 34 shr $0x34,%r9 - 2a4e: 4c 8b 5c 24 d8 mov -0x28(%rsp),%r11 + 2a4e: 4c 8b 5c 24 e8 mov -0x18(%rsp),%r11 2a53: 48 21 ca and %rcx,%rdx 2a56: 48 89 53 08 mov %rdx,0x8(%rbx) 2a5a: 49 f7 e0 mul %r8 2a5d: 4d 89 f0 mov %r14,%r8 - 2a60: 4c 8b 74 24 e0 mov -0x20(%rsp),%r14 + 2a60: 4c 8b 74 24 f0 mov -0x10(%rsp),%r14 2a65: 4d 0f ac f8 34 shrd $0x34,%r15,%r8 2a6a: 49 01 c0 add %rax,%r8 2a6d: 4c 89 e0 mov %r12,%raxI guess the
mulvsimulandshrvssarare expected. The stack layout change is strange, as it doesn't look like there is anything more or less on the stack... -
real-or-random commented at 7:26 PM on November 15, 2023: contributor
The stack layout change is strange, [...]
I guess my conclusion is simply that compilers are strange?
-
roconnor-blockstream commented at 7:27 PM on November 15, 2023: contributor
Maybe stack layout is arbitrary and the compiler sorts variables by type?
-
sipa commented at 7:29 PM on November 15, 2023: contributor
It's not reordering anything as far as I can see. Everything is just moved by 16 bytes...EDIT: nevermind, it actually just swapped some variables.
-
sipa commented at 9:28 PM on November 15, 2023: contributor
utACK 10271356c8fc34395850ac70df5902571945fbea
- real-or-random merged this on Nov 16, 2023
- real-or-random closed this on Nov 16, 2023
- roconnor-blockstream cross-referenced this on Nov 21, 2023 from issue Replace the 64-bit C field implementation by fiat-crypto output by dderjoel
- roconnor-blockstream deleted the branch on Nov 27, 2023
