Merge Sort: Segmentation error Core Dumped

Question

I read the theory of merge sort algorithm and based on that i wrote an implementation of merge sort in C++ using the STL Vector class.

I know that a copy paste from any one of the trillion articles on internet about merge sort can solve this, but I wanted to try this one on my own.

When I run the code i get segmentation fault core dumped error, which is caused by the recursive functions not terminating. Can anybody help me find the error in the code.

#include<iostream>
#include<vector>
void merge(std::vector<int>& arr,int last,int begin,int mid);
void mergeSort(std::vector<int>& arr,int begin, int last){ 
    std::cout<<"In merge sort";
    int mid; 
    if(begin<last){
      mid = (begin+last)/2;
      mergeSort(arr,begin,mid);
      mergeSort(arr,mid,last);
      merge(arr,last,begin,mid);
    } 
    return;
}
void merge(std::vector<int>& arr ,int last,int begin,int mid){
    if(last==begin){
        std::cout<<"Returned form finger";
        return; 
    }
    int b=begin,c=mid;
    std::vector<int> temp;
    while(b<mid&&c<last){
        if(arr[b]<arr[c]){
            temp.push_back(arr[b]);
            b++;
        }
        else{
            temp.push_back(arr[c]);
            c++;
        }
    }
    while(b<mid){temp.push_back(arr[b]);}
    while(c<last){temp.push_back(arr[c]);}
    arr.swap(temp);
    return;
}


int main(){
    std::vector<int> arr({2,4,1,5,3,6,2,4,3});   
    for(auto it=arr.begin();it!=arr.end();++it){
        std::cout<<*it<<' '; 
    }
    std::cout<<'\n';
   mergeSort(arr,0,8);
    for(auto it=arr.begin();it!=arr.end();++it){
        std::cout<<*it<<' '; 
    }
    return 0;
    }

I use gcc version 9.3.0 to compile the code using the terminal command

$ gcc filename.cpp -lstdc++

After analyzing the code i think the problem is in the merge function but i cant point out where it is. It would be helpful if anybody could help me and if possible suggest a few ways to optimize my code.

[How to implement classic sorting algorithms in modern C++](https://stackoverflow.com/questions/24650626/how-to-implement-classic-sorting-algorithms-in-modern-c) — PaulMcKenzie, Jun 05 '20 at 14:21
sorry, but I have to say it "...but I wanted to try this one on my own." and thats why you ask others to find the problem in your code. Allright :P. Did you already use a debugger? — 463035818_is_not_an_ai, Jun 05 '20 at 14:22
a debugger is a tool that lets you step through your code line by line to see where the problem is. If you don't know how to use it, now is the time — 463035818_is_not_an_ai, Jun 05 '20 at 14:23
Your interval division is wrong. If `begin` is 0 and `end` is 1, you get `mid` as 0 and recurse on `mergeSort(0,1)`. — molbdnilo, Jun 05 '20 at 14:32
Side note: one reason that we specify ranges as half-open intervals is that it makes interval division much easier. — molbdnilo, Jun 05 '20 at 14:34
@idclev463035818 What i meant is that i wanted to do something without looking at the original code itself, and get my head around the idea of recursions. I wanted to form my own code for it. no i didn't use a debugger (i use vim to write code) — Rohit Karunakaran, Jun 05 '20 at 14:36
yes that fine, and my comment was not meant as an offense, but rather suggesting to take the "on my own" one step further by using a debugger. Writing the code is only a tiny part of programming, debugging takes at least as much time and is an important skill — 463035818_is_not_an_ai, Jun 05 '20 at 14:43
There is one more problem. `arr` has 9 elements. You split it into two arrays `arr1` with 4 elements and `arr2` with 5 elements (or vice versa). Then you split `arr1` again into two arrays `arr11` with 2 elements and `arr12` with 2 elements. Then you merge these two arrays. You fill `temp` with these 4 elements and then you swap `arr.swap(temp);`. After this swap your array has 4 elements and every access is out of bounds. That's undefined behavior. Use `std::vector::at` instead of `std::vector::operator[]` for debug. — Thomas Sablik, Jun 05 '20 at 15:20

score 3 · Accepted Answer · answered Jun 05 '20 at 14:35

So your recursion is wrong. In main you have a vector of nine elements.

std::vector<int> arr({2,4,1,5,3,6,2,4,3});

and you call mergeSort like this

mergeSort(arr,0,8);

So the third parameter to mergeSort is the last valid index of the vector you are sorting (it would have been better to use arr.size() - 1). Put another way mergeSort is sorting the inclusive range (0, 8).

Now in merge_sort you have this

void mergeSort(std::vector<int>& arr,int begin, int last) { 
    ...
    mergeSort(arr,begin,mid);
    mergeSort(arr,mid,last);
    ...
}

Recall merge sort is sorting the inclusive range (begin, last). So your recursion is going to sort the two inclusive ranges (begin,mid) and (mid,last). In other words mid is being included twice.

Since (very admirably) you want to program this for yourself, I'll leave you to figure out the fix.

score 2 · Answer 2 · answered Jun 05 '20 at 14:47

In addition to the other answer(s) you have a serious problem in your merge function. The code you use to copy remaining elements to temp looks like:

while (b < mid) { temp.push_back(arr[b]); }
while (c < last) { temp.push_back(arr[c]); }

Notice that you never increment the b or c indices. That's an infinite loop that will eventually run out of memory calling push_back. The simple fix is something like:

while (b < mid) { temp.push_back(arr[b++]); }
while (c < last) { temp.push_back(arr[c++]); }

Merge Sort: Segmentation error Core Dumped

2 Answers2