Seg faults in Binary search in NASM assembly

Question

Hi I'm struggling to find out why my binary search implementation is seg faulting (I'm new to NASM assembly)

Sorry I know its not much of a MVP but I cant think of an appropriate way to make one in assembly.

%define n [ebp+8]
%define list [ebp+12]
%define low [ebp+16]
%define high [ebp+20] ; Parameters loading from the stack
binary_search:
  push ebp
  mov ebp, esp

  mov ebx, n
  mov edi, list
  mov ecx, low
  mov edx, high

  cmp ecx, edx ; if (low > high)
  jg .FIRST

  mov eax, edx ; Next few lines for mid = low + (high - low)/2
  sub eax, ecx
  sar eax, 1 ; Will this give an appropriate index? (i.e is it floor division?)
  add eax, ecx
  lea esi, [edi+eax*4] ;Getting list[mid]
  cmp ebx, [esi]; if (n == list[mid])
  je .END
  jl .SECOND
  jg .THIRD

  jmp .END

.FIRST:
  mov eax, -1 ; return -1
  jmp .END

.SECOND:
  mov edx, eax ; return middle - 1
  dec edx
  jmp .CONTINUE

.THIRD:
  mov ecx, eax ; low = mid - 1
  dec ecx
  jmp .CONTINUE

.CONTINUE:
  push edx
  push ecx
  push edi
  push esi
  push ebx
  call binary_search ; recursive call, causing the segfault.
  pop ebx
  pop esi
  pop edi
  pop ecx
  pop edx
  jmp .END

.END:
  mov esp, ebp
  pop ebp
  ret

After commenting out different sections, I have determined that it is definitely something to do with my recursive call to binary_search that is causing the seg fault. (Found Inside .CONTINUE) What am I messing up insdie of the binary_search body that doesnt agree with multiple recursive calls?

The binary search algorithm:

binary_search(n, list, low, high)

    if (low > high)
        return -1

    mid = low + (high - low) / 2

    if (n == list[mid])
       return middle;
    if (n < list[mid])
       high = mid - 1
    else
        low = mid + 1

    return binary_search(n, list, low, high)

I know its a long shot, thanks :)

Edit: its 32-bit mode

Your recursive call doesn't pass any parameters on the stack as expected by your function. Maybe not the only problem but certainly one. — ecm, Oct 24 '20 at 14:19
I thought by pushing those registers before the call, I was passing the parameters? What am I missing? — liamod, Oct 24 '20 at 14:21
Check the declaration of you parameter indices. EBP points to the bottom of the stack, [EBP + 8] is where the previous EBP is (`push ebp`, at the very top) and [EBP + 16] is the return address (assuming we run in 64 bit mode). So n should probably be at [EBP + 24]. — PMF, Oct 24 '20 at 14:23
Would this still be a problem if I was running in 32-bit mode? — liamod, Oct 24 '20 at 14:30
To your comment, arithmetic right shift has the effect of rounding toward minus infinity, so yes, the floor. You can try some examples to confirm. — Nate Eldredge, Oct 24 '20 at 15:23
@PMF: You can't address 64-bit stack with `ebp` and `push ebp` is [not valid in 64-bit mode](https://stackoverflow.com/questions/52691517/push-ebp-operand-type-mismatch-for-push/52691683#52691683). — ecm, Oct 24 '20 at 17:38
@ecm: Eh, my bad. Forgot that _e_bp is still _only_ 32 bits. — PMF, Oct 24 '20 at 17:48
@liamod: You're right, in part. But you're passing ebx as n, esi as list, edi as high, and ecx as low. You're also pushing and popping edx but not using that value as a parameter. I think edi is not right for this. I'd have to check in more detail to find a fix though. — ecm, Oct 24 '20 at 17:50
You should drop the `push esi` and `pop esi`. I think that is necessary for correct recursive calls though probably not sufficient to fix your function for all corner cases. — ecm, Oct 24 '20 at 17:58
I think in `.THIRD` you probably should have `inc ecx` not `dec`. — ecm, Oct 24 '20 at 18:02
Thanks @ecm, the main problem was pushing / popping esi, could you maybe explain in more detail why this is the case? I would accept this as the answer. — liamod, Oct 24 '20 at 19:53

score 3 · Accepted Answer · answered Oct 24 '20 at 21:41

This part defines your function's protocol:

%define n [ebp+8]
%define list [ebp+12]
%define low [ebp+16]
%define high [ebp+20] ; Parameters loading from the stack
binary_search:
  push ebp
  mov ebp, esp

The dword [ebp] will hold the old ebp value, dword [ebp+4] the old eip to return to, and then your parameters follow. These are n, list, low, and high, in this order. As the stack grows downwards you need to push to the stack in the order so that the highest addressed parameter (which happens to be "high") is pushed first, then the second highest (low), and so on (list, n, eip, ebp).

The next part determines which registers you use to hold the variables initialised from these stack frame parameters:

  mov ebx, n
  mov edi, list
  mov ecx, low
  mov edx, high

(Either ecx or edx is modified before the recursive call. But they still act as the "low" and "high" variables in that code.)

Now to check your recursive call site. Essentially you want to pass all of the registers you're using to the function again. ebx = n, edi = list, ecx = low, edx = high. This is what you do:

.CONTINUE:
  push edx
  push ecx
  push edi
  push esi
  push ebx
  call binary_search ; recursive call, causing the segfault.
  pop ebx
  pop esi
  pop edi
  pop ecx
  pop edx

If you match up the pushed variables with the stack frame parameters expected by your function's protocol, you get:

  push edx    ; (unused)
  push ecx    ; high
  push edi    ; low
  push esi    ; list
  push ebx    ; n
  call binary_search

The variable represented by esi is only used internally by your function. It does not need to be passed to subsequent calls. What's more, it messes with your function's protocol. edx, ecx, and edi get shoved one stack slot higher each than they should be to pass these variables along to your recursive call. The solution is to drop push esi and pop esi here.

There are a few more potential problems with your code:

As I mentioned in the comments you should use inc ecx not dec ecx.
What is your program's calling convention used to call this function? You seem to be using a lot of registers and are only preserving ebp and esp.
If your calling convention allows to alter the stack frame parameter contents without limits, you could replace the literal call that you use to recurse, by a "tail call" with the parameters changed to those you currently pass to the call. And re-using the entire stack frame. It would look very similar to a loop at that point though. Perhaps you do want an actual (literally) recursive implementation. The tail-call-optimised or iterative approaches need O(1) of stack space though, as opposed to your O(log2 n).

Thanks @ecm, for the detailed answer, yes I changed the dec -> inc , was just a typo, we are using cdecl, am I not preserving all registers when I push them before the function call or is that just passing them? I'm aware of the ineffieciency of the approach, but It must be recursive. Thanks again! — liamod, Oct 25 '20 at 08:47
@liamod: It is actually both passing and preserving these 5 registers (or 4, if you delete the `esi` ops), given that the function does not modify the parameters on the stack. However, this only happens around the recursive call. It does not happen around the outermost function call (from your code not shown here). It appears that [you should preserve `ebx`, `esi`, and `edi`](https://stackoverflow.com/questions/9603003/what-registers-must-be-preserved-by-an-x86-function). To do that you should add the following: `push` each register after `mov ebp, esp` and `pop` each before `mov esp, ebp`. — ecm, Oct 25 '20 at 11:37

Seg faults in Binary search in NASM assembly

1 Answers1