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: contributorThese temporaries seem to been inadvertently changed to signed during a refactoring. Generally, bit shifting is frowned upon for signed values.
-
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: contributorutACK 10271356c8fc34395850ac70df5902571945fbea
-
sipa commented at 2:37 pm on November 15, 2023: contributorDoes 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:
0@@ -973,7 +973,7 @@ 1 2883: 4c 8b 46 18 mov 0x18(%rsi),%r8 2 2887: 48 8b 0e mov (%rsi),%rcx 3 288a: 48 8d 34 00 lea (%rax,%rax,1),%rsi 4- 288e: 48 89 44 24 d8 mov %rax,-0x28(%rsp) 5+ 288e: 48 89 44 24 e8 mov %rax,-0x18(%rsp) 6 2893: 48 89 f8 mov %rdi,%rax 7 2896: 48 f7 e7 mul %rdi 8 2899: 4c 8d 2c 09 lea (%rcx,%rcx,1),%r13 9@@ -1031,9 +1031,9 @@ 10 294b: 48 11 d7 adc %rdx,%rdi 11 294e: 49 f7 e0 mul %r8 12 2951: 49 89 f7 mov %rsi,%r15 13- 2954: 4c 89 7c 24 e0 mov %r15,-0x20(%rsp) 14+ 2954: 4c 89 7c 24 f0 mov %r15,-0x10(%rsp) 15 2959: 49 89 c2 mov %rax,%r10 16- 295c: 48 8b 44 24 d8 mov -0x28(%rsp),%rax 17+ 295c: 48 8b 44 24 e8 mov -0x18(%rsp),%rax 18 2961: 49 89 d3 mov %rdx,%r11 19 2964: 49 f7 e1 mul %r9 20 2967: 49 01 c2 add %rax,%r10 21@@ -1046,14 +1046,14 @@ 22 2983: 49 11 fb adc %rdi,%r11 23 2986: 4c 89 d6 mov %r10,%rsi 24 2989: 4c 89 ff mov %r15,%rdi 25- 298c: 48 c1 ff 30 sar $0x30,%rdi 26+ 298c: 48 c1 ef 30 shr $0x30,%rdi 27 2990: 48 c1 e6 04 shl $0x4,%rsi 28 2994: 48 21 c6 and %rax,%rsi 29 2997: 48 89 f8 mov %rdi,%rax 30 299a: 83 e0 0f and $0xf,%eax 31 299d: 48 09 c6 or %rax,%rsi 32 29a0: 4c 89 f0 mov %r14,%rax 33- 29a3: 48 f7 ee imul %rsi 34+ 29a3: 48 f7 e6 mul %rsi 35 29a6: 48 89 c6 mov %rax,%rsi 36 29a9: 48 89 c8 mov %rcx,%rax 37 29ac: 48 89 d7 mov %rdx,%rdi 38@@ -1064,9 +1064,9 @@ 39 29bf: 4c 89 c8 mov %r9,%rax 40 29c2: 48 11 d7 adc %rdx,%rdi 41 29c5: 48 89 f2 mov %rsi,%rdx 42- 29c8: 48 89 74 24 e8 mov %rsi,-0x18(%rsp) 43+ 29c8: 48 89 74 24 d8 mov %rsi,-0x28(%rsp) 44 29cd: 48 21 ca and %rcx,%rdx 45- 29d0: 48 89 7c 24 f0 mov %rdi,-0x10(%rsp) 46+ 29d0: 48 89 7c 24 e0 mov %rdi,-0x20(%rsp) 47 29d5: 48 89 13 mov %rdx,(%rbx) 48 29d8: 49 f7 e4 mul %r12 49 29db: 48 89 c6 mov %rax,%rsi 50@@ -1086,7 +1086,7 @@ 51 2a08: 48 89 d0 mov %rdx,%rax 52 2a0b: 48 f7 e5 mul %rbp 53 2a0e: 49 89 c2 mov %rax,%r10 54- 2a11: 48 8b 44 24 d8 mov -0x28(%rsp),%rax 55+ 2a11: 48 8b 44 24 e8 mov -0x18(%rsp),%rax 56 2a16: 49 89 d3 mov %rdx,%r11 57 2a19: 49 f7 e5 mul %r13 58 2a1c: 48 89 c6 mov %rax,%rsi 59@@ -1094,21 +1094,21 @@ 60 2a22: 4c 89 c8 mov %r9,%rax 61 2a25: 4d 89 f9 mov %r15,%r9 62 2a28: 4c 01 d6 add %r10,%rsi 63- 2a2b: 4c 8b 54 24 e8 mov -0x18(%rsp),%r10 64+ 2a2b: 4c 8b 54 24 d8 mov -0x28(%rsp),%r10 65 2a30: 4c 11 df adc %r11,%rdi 66- 2a33: 4c 8b 5c 24 f0 mov -0x10(%rsp),%r11 67+ 2a33: 4c 8b 5c 24 e0 mov -0x20(%rsp),%r11 68 2a38: 4d 0f ac da 34 shrd $0x34,%r11,%r10 69 2a3d: 49 c1 eb 34 shr $0x34,%r11 70 2a41: 4c 01 d6 add %r10,%rsi 71 2a44: 48 89 f2 mov %rsi,%rdx 72 2a47: 4c 11 df adc %r11,%rdi 73 2a4a: 49 c1 e9 34 shr $0x34,%r9 74- 2a4e: 4c 8b 5c 24 d8 mov -0x28(%rsp),%r11 75+ 2a4e: 4c 8b 5c 24 e8 mov -0x18(%rsp),%r11 76 2a53: 48 21 ca and %rcx,%rdx 77 2a56: 48 89 53 08 mov %rdx,0x8(%rbx) 78 2a5a: 49 f7 e0 mul %r8 79 2a5d: 4d 89 f0 mov %r14,%r8 80- 2a60: 4c 8b 74 24 e0 mov -0x20(%rsp),%r14 81+ 2a60: 4c 8b 74 24 f0 mov -0x10(%rsp),%r14 82 2a65: 4d 0f ac f8 34 shrd $0x34,%r15,%r8 83 2a6a: 49 01 c0 add %rax,%r8 84 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: contributorMaybe 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: contributorutACK 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
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 17:15 UTC
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 17:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
More mirrored repositories can be found on mirror.b10c.me