0

I'm total noob in Assembly and I try to create function and use it in C. This function gets 3 variables a,x,ywhich are structure which contains two 64-bit int. I want to return a+x*y. Unfortunetely this piece of code is NASM causes a segfault

%define a1 [rdi]
%define a2 [rdi+8]
%define x1 [rsi]
%define x2 [rsi+8]
%define y1 [rdx]
%define y2 [rdx+8]
%define output1 rax
%define output2 rdx
%define res1 r8
%define res2 r9

global abc

abc:

mov output1, x1
mul qword y1
mov res1, output1
mov output1, x1
mul qword y2
mov res2, output1
mov output1, x2
mul qword y1
add res2, output1
mov output1, res1
mov output2, res2


ret

Deleting line with second and third mul allows me to run my program without getting segfault.

fra
  • 1
  • 1
    Step through the code in a debugger. Watch the registers closely. – Raymond Chen Mar 02 '17 at 15:49
  • 1
    I would suggest also to drop that %define hiding registers usage, makes very hard to follow the real machine code while reading, and as some instructions (like `mul`) have fixed registers, you have to constantly check if some define is not clashing with the instruction. In source with normal registers visible you just take a quick look above few lines. Those "names" of registers belong to comments IMHO. – Ped7g Mar 02 '17 at 17:33

1 Answers1

1

If you untangle the macro definitions above, you'll eventually result in this, which is what the processor is actually trying to execute:

mov rax, [rsi]
mul qword [rdx]      <--- no segfault here
mov r8, rax
mov rax, [rsi]
mul qword [rdx+8]    <--- segfault here
mov r9, rax
mov rax, [rsi+8]
mul qword [rdx]      <--- segfault here
add r8, rax
mov rax, r8
mov rdx, r9

You said that the segmentation faults occur on the marked lines above.

So let's do a little psychic debugging (one of Raymond Chen's favorite kinds of debugging). Consider what happens after the first mul instruction: rax is set to the low part of the product, and rdx is set to the high part of the product.

That means that after the first mul, rdx has changed! It's no longer a pointer to y1 or to y2, but is instead something having to do with whatever x1*y1 results in. Any successive attempts after this to use rdx as a pointer are guaranteed to fail, because it isn't one anymore.

So in order to fix this, you'll have to preserve rdx across the multiplication in another register. r10 isn't used by this code, and it's considered volatile, so we can safely use it to store a "backup" of the initial value of rdx. So something like this is likely sufficient to fix it:

%define a1 [rdi]
%define a2 [rdi+8]
%define x1 [rsi]
%define x2 [rsi+8]
%define y1 [r10]         ; Change which register we're using as the
%define y2 [r10+8]       ; pointer to 'r10'.
%define output1 rax
%define output2 rdx
%define res1 r8
%define res2 r9

global abc

abc:
    mov r10, rdx         ; Preserve the real pointer to 'y1' in 'r10'.

    mov output1, x1
    mul qword y1
    mov res1, output1
    mov output1, x1
    mul qword y2
    mov res2, output1
    mov output1, x2
    mul qword y1
    add res2, output1
    mov output1, res1
    mov output2, res2

    ret
Sean Werkema
  • 5,810
  • 2
  • 38
  • 42
  • 2
    Corrections: (1) This isn't psychic debugging. All the information needed is present in the question. (2) Psychic debugging is the worst kind of debugging. In psychic debugging, you are making wild guesses. Hardly a favorite pastime. (3) What we're doing here isn't psychic debugging. It's just mentally simulating a computer. – Raymond Chen Mar 03 '17 at 14:49
  • Well, okay, that's fair, this is really just straightforward code analysis. But I've been reading your blog for years, and I'll still make the claim that you *do* enjoy psychic debugging, based purely on the number of times you write articles on it ;-) – Sean Werkema Mar 03 '17 at 18:50
  • Normally you'd just use `imul rax, y1` or whatever, if you don't need the high-half result in RDX. See compiler output in [Why is imul used for multiplying unsigned numbers?](https://stackoverflow.com/q/42587607) for example. – Peter Cordes Sep 16 '22 at 22:17
  • @PeterCordes My goal was to make as few changes to his code as possible to get it to work, which is generally a good policy when fixing any code. For all we know, he may have *intended* it to use unsigned, extending multiplication. – Sean Werkema Sep 16 '22 at 23:47
  • Mostly commenting for the benefit of future readers who have the same bug (using `mul` when they just want the RAX result, and something in RDX gets overwritten). By far the most efficient fix is to use `imul reg,reg` if you don't want the high-half result. Your code doesn't leave the high-half result anywhere (`mov output2, res2` overwrites the last RDX result), so if they had wanted 128-bit outputs your answer isn't anywhere near doing that. The low 64 bits of the product of 64-bit integers is the same for `imul` vs. `mul`, which is why we can use `imul` on unsigned numbers. – Peter Cordes Sep 16 '22 at 23:59