18

In this code, for vector size, n >=32767, it gives segmentation fault, but upto 32766, it runs fine. What could be the error? This is full code.

#include<cstdio>
#include<cstring>
#include<cmath>
#include<queue>
#include<utility>
#include<algorithm>
#include<sys/time.h>
using namespace std;
#define MAX 100000

bool compare(pair<int,int> p1,pair<int,int> p2) {
    if(p1.second < p2.second)
        return 1;
    else if(p1.second > p2.second)
        return 0;
    if(p1.first <= p2.first)
        return 1;
    else
        return 0;
}

int main() {
    freopen("randomin.txt","r",stdin);
    int n;
    scanf("%d",&n);
    vector< pair<int,int> > p(n);
    for(int i=0;i<n;i++)
        scanf("%d%d",&p[i].first,&p[i].second);
    **printf("%d\n",(int)p.max_size()); // prints 536870911**
    sort(p.begin(),p.begin()+n,compare);

    //for(int i=0;i<n;i++)
        //printf("%d %d\n",p[i].first,p[i].second);
        printf("%.6f\n",(p[n-1].second+p[n-2].second)/(20.0+p[n-1].first+p[n-2].first));

    return 0;
}
avd
  • 13,993
  • 32
  • 78
  • 99
  • 1
    What compiler and OS you are using? Maybe it just has no enough memory? – maykeye Oct 09 '09 at 04:43
  • I compiled a slightly modified version (I didn't want to enter 35000 numbers from the console :-) ) and it ran fine using VS2008. I guess the problem is somewhere else. Post the code with which the problem is reproducible. – Naveen Oct 09 '09 at 04:43
  • Its GNU g++ with cygwin running on netbeans. I am using freopen and taking input from file. – avd Oct 09 '09 at 04:45
  • I have included the whole code. – avd Oct 09 '09 at 04:46
  • Never, ever, use `scanf()` to read user input without checking the return value. – DevSolar Sep 08 '21 at 08:25

3 Answers3

70

In C++, your compare predicate must be a strict weak ordering. In particular, compare(X,X) must return "false" for any X. In your compare function, if both pairs are identical, you hit the test (p1.first <= p2.first) , and return true. Therefore, this compare predicate does not impose a strict weak ordering, and the result of passing it to sort is undefined.

Audrius Meškauskas
  • 20,936
  • 12
  • 75
  • 93
Jim Lewis
  • 43,505
  • 7
  • 82
  • 96
  • Sir: Does that mean that that C++ checks implicitly that if two objects are same, it should return false. But why it was working for n<=32766. Sir: u r great. You have again helped me solving a problem. – avd Oct 09 '09 at 05:18
  • 1
    No deliberate implicit check - just a confused sort algorithm. Different input => differently confused. –  Oct 09 '09 at 05:21
  • 2
    +1 - nicely spotted! @aditya: Remember that STL compare functions are asking "is the first one less than the second" - not "are they equal". – Smashery Oct 09 '09 at 05:23
  • 2
    @Aditya: There are all sorts of reasons a non-strictly-conforming C++ program might work on some inputs, and fail on others. Consider the case where the library uses one algorithm for large containers (and dumps core if the strict weak ordering is violated), and uses a simpler (and more tolerant) algorithm for smaller values of N. – Jim Lewis Oct 09 '09 at 05:27
  • 1
    Perhaps it goes too deep into recursion (for example, because the faulty comparison function messes up the algorithm's worst case guarantees)? I would also like to point out that std::pair already defines comparison operators (except it uses first as the first criterion). – UncleBens Oct 09 '09 at 07:12
  • 4
    This was causing some pretty random segfaults for me. I still can't believe that a bad ordering mechanism can actually corrupt the stack. Great catch, thank you. – Jake Dec 24 '14 at 10:36
  • 12 years later this was an issue I was having in my code. Thanks stranger! – AlanWik Feb 11 '21 at 09:34
  • It is related to the segmentation fault, it is the cause of it. – Audrius Meškauskas Sep 08 '21 at 08:23
4

Try using all the values from n = 32766 up to 32770. I suspect you'll find that you are experiencing some sort of overflow. This is because 2^15 (32768) is the biggest number that can be represented using 16 bits (assuming you also allow negative numbers). You will have to use a different data type.

Suggestion:

Get it to output the vector's maxsize:

cout << p.max_size();

Let us know what that does. All things being normal, I'd expect it to be in the hundreds of millions (536870911 on my computer). But if it's more like 32768, then that could be the problem.

Smashery
  • 57,848
  • 30
  • 97
  • 128
  • How could there be overflow? I am just comparing. There is no arithmetic being performed here. – avd Oct 09 '09 at 04:41
  • Maybe your compiler is setting int to 16 bits ? – Andrew Keith Oct 09 '09 at 04:42
  • I am sure that on my system, int is 32 bits. I have checked it – avd Oct 09 '09 at 04:42
  • Its GNU g++ on netbeans with cygwin – avd Oct 09 '09 at 04:44
  • 5
    Your comparison function should return false on equality, not true. Not sure why this should cause a segv... – Keith Randall Oct 09 '09 at 04:47
  • I'm not saying "It is overflow." But if you try setting n to those few values, and it works up to about 32768, then you can be pretty sure it's some sort of overflow. At least try those values, and if I'm wrong, then awesome, we can rule out one suggestion. I've just added a bit of extra info in my response, try that. – Smashery Oct 09 '09 at 04:49
  • compare function returns 1 or 0 anyways. How can this generate fault? – avd Oct 09 '09 at 04:49
  • @Smashery: Yes,u r right. it generates fault for 32767. For 32766, it runs fine. But why this problem has arisen. I m using g++ – avd Oct 09 '09 at 04:52
  • @aditya: What does p.max_size() give you? – Smashery Oct 09 '09 at 04:53
  • 1
    aditya, Keith has a point. for n = 350000 where each pair is (0,0) program crashes. after adding if(p1==p2) return 0; it works fine. – maykeye Oct 09 '09 at 04:54
  • @Smashery:Yes, it gives 536870911. Please suggest a solution – avd Oct 09 '09 at 04:56
  • @aditya: No solution suggestion yet - just trying to gather data to work out what's going wrong. Have you tried Keith's suggestion (above)? – Smashery Oct 09 '09 at 04:59
  • @Smashery & @Keith: Even If I write a dummy compare function which always return 1 (without any comparison), then also it generates segmentation fault. – avd Oct 09 '09 at 05:06
  • 2
    @aditya, always 1 is not not a compararer function. Compare http://codepad.org/s3vvaai5 [works fine] and http://codepad.org/sD7kz0b4 [crashes] – maykeye Oct 09 '09 at 05:08
  • 1
    @Keith - Good point, but I'd be surprised if it's the real issue. May scramble the sort results, but a segfault? @aditya - to fix it, in compare, change the <= to <. –  Oct 09 '09 at 05:10
  • Why it is not a comparer? It just depends on the logic. – avd Oct 09 '09 at 05:12
  • @Steve314: Yes, it worked. I places < instead of <=. But why? please explain in detail in a new answer post. – avd Oct 09 '09 at 05:14
  • @Keith - OK, I think I believe - wierd. –  Oct 09 '09 at 05:16
  • @aditya, Jim Lewis already explained it. Check source code of sort functions for more details. – maykeye Oct 09 '09 at 05:17
  • 1
    @aditya - that's just Keiths thing done differently. You were violating the expectations that sort has for compare. I'm surprised it segfaults, but... –  Oct 09 '09 at 05:17
  • Oops - I've been calling maykeye Keith - sorry. –  Oct 09 '09 at 05:18
  • But why it was working for n<=32766? Where can I get source code of sort function. With .h files its just prototype. – avd Oct 09 '09 at 05:20
  • @Smashery & @Keith & @Steve314 & @Jim Lewis: Thank you all of u for ur great effort for solving this problem. – avd Oct 09 '09 at 05:26
  • @maykeye: when did you become Keith? ;-) @aditya: no worries, mate! Very strange about the 32766 limit – Smashery Oct 09 '09 at 05:31
1

Your overflow is possibly related to the stack overrun due to passing two vectors by value.

std::vector &vec - this is how you pass a vector by reference.

Change bool compare(pair<int,int> p1,pair<int,int> p2) to bool compare(pair<int,int> &p1,pair<int,int> &p2)

If you think this through, passing by reference will change the size of stack data from size (2*n*4)*2 bytes to 2x8 byte pointers

For n == 32767 you are reducing the stack frame by approx (2*32767*4)*2

As a rule of thumb, pass by reference unless your design requires otherwise.

moi
  • 467
  • 4
  • 19