double free or corruption (out)



  • Hallo,

    Ich erhalte folgende Fehlermeldung:
    *** Error in `./a.out': double free or corruption (out): 0x000000000207b0f0 ***
    Aborted (core dumped)

    Diese tritt auf wenn die Funktion 'MUCA_itertation' verlassen wird

    #include <sstream>
    #include <iostream>
    #include <fstream>
    #include <iomanip> 
    #include <cstdlib>
    #include <cmath>
    #include <vector>
    #include <boost/random.hpp>
    
    #define couplingconst 1 
    
    vector < vector <int> > grid;
    vector < double > W;
    
    void MUCA_iteration(int N,int part_run, double beta, int flag,int run)
    {
      double variance=1;
      double Pave=0, Pave2=0;
      vector <double> P_n(N+1,0); 
      vector <double> m(N+1,0); 
      vector <double> g(N+1,0);
      vector <double> R(N,0);
      double Eg;
      int pos,ii=0,pos2, M=0;
      double exponent, P; 
      int cond=0,cont;
    
      Eg = -couplingconst*energy_grid();
      M = mag();
    
      while(ii < run ){
        ii++;
        for (int ittherm=0; ittherm<part_run; ittherm++)	{       
    
         for (int n=0; n<N; n++){
            Eg+=update_step(Eg, M, beta, flag);                          
            }
        pos=(M+N)/2;
        pos2=(Eg+2*N)/4;
        m[pos]+=1;
        g[pos2]+=1; 
        }  
    
        for (int k=0; k<N; k++){  //accumulative MUCA 
          if(m[k]==0 || m[k+1]==0) 
          {
            exponent = 0;
            P = 0;
          }
          else
          { 
            P = m[k]*m[k+1]/(m[k]+m[k+1]);
            P_n[k] += P;
    	exponent = P/P_n[k];
            R[k] = R[k] + exponent * log(m[k]/m[k+1]); // log R
          }
          W[k+1] = W[k] + R[k];
          Pave2=(Pave2*k+m[k]*m[k])/(double)(k+1);
          m[k] = 0;  
          g[k] = 0;
        }   
        Pave2=(Pave2*N+m[N]*m[N])/(double)(N+1.0);
        Pave = part_run/(double)(N+1);
        variance =sqrt( Pave2 - Pave*Pave )/part_run;
        cout<<ii<<"    "<<variance<<endl;
        m[N]=0;
        g[N]=0;
        Pave=0;
        Pave2=0;
      }
      cout<< "here" <<endl;
    }
    
    int main (int argc, char *argv[])
    {  
      double beta;
      double T=;
      int part_run;	
      int Eg = 0, M=0;
      int run=0;
      int l;
    
      T=1.5;
      l=10;
      part_run=10000;
      run= 10;
    
      beta=1./T;
      int N=l*l;
    
      W.resize(N+1,0);
    
      grid_init(1./T);
    
      MUCA_iteration(N, part_run, beta, flag,run);
      cout<< "not here..." << endl;
    
    }
    

    Das letzte 'cout' wird nicht mehr ausgegeben.. Der Fehler soll (soweit ich weiß) darauf hinweisen dass irgendetwas geschlossen wurde, dabei nutze ich alloc, new, delete usw gar nicht..
    Vllt könnt ihr mir ja helfen.

    Vielen lieben Dank!



  • 1. Das ist C++, nicht C.
    2. Fehlt da ein using namespace std;
    3. Kennen wir einen Haufen Symbole nicht:

    xxx.cpp:31:35: Fehler: »energy_grid« wurde in diesem Gültigkeitsbereich nicht definiert
    xxx.cpp:32:11: Fehler: »mag« wurde in diesem Gültigkeitsbereich nicht definiert
    xxx.cpp:39:42: Fehler: »update_step« wurde in diesem Gültigkeitsbereich nicht definiert
    xxx.cpp:100:17: Fehler: »grid_init« wurde in diesem Gültigkeitsbereich nicht definiert
    xxx.cpp:102:37: Fehler: »flag« wurde in diesem Gültigkeitsbereich nicht definiert
    

    4. Du machst eine kaputte Zuweisung:

    double T=;
    

    5. Meine Vermutung ist, dass du irgendwo in Speicher schreibst, in den du nicht schreiben sollst (zur Info: wenn du Vektoren verwendest, oder generell Container aus der STL, dann verwendest du meistens ohne es zu wissen new/delete ). Irgendwo reinschreiben, wo du faktisch zwar ohne Segfault schreiben darfst, aber nicht solltest, ist genauso tödlich wie ein Segfault. Prüfen geht aber nicht, weil ich's nicht kompiliert bekomme.

    6. Und durchlesen tue ich das nicht - habe Leute schon für schlechtere Codequalität gewürgt. 😉



  • Huch, ich habe ja schon ein Account.

    Sorry, ich dachte ich hab einen standard Fehler (den ich noch nicht kenne) und habe deshalb das Programm auf die wesentlichen Elemente runter gekürzt.

    Hier noch mal die lange Version(zum kompilieren wird boost benötigt. )

    #include <sstream>
    #include <iostream>
    #include <fstream>
    #include <iomanip> 
    #include <cstdlib>
    #include <cmath>
    #include <vector>
    #include <boost/random.hpp>
    
    #define couplingconst 1 
    
    boost::mt19937 drand(300);
    
    using namespace std;
    
    int l;
    vector < vector <int> > grid;
    vector < double > W;
    double update_prop[5]; 
    
    void grid_init(double beta)    	
    {
      grid.resize(l);
      for (int k=0; k<l; k++) grid[k].resize(l);
    
      for (int i=0 ; i<l ; i++)
      {
        for (int j=0 ; j<l ; j++)
        {
          boost::random::uniform_int_distribution<> dist(0, 1);
          grid[i][j] = dist(drand);
          if(grid[i][j]==0) grid[i][j]=-1;
        }
      }
      for (int na=0 ; na<5 ; na++)
      {
        update_prop[na]=exp(-beta*couplingconst*(4*na-8)); // -8, -4, 0, 4, 8
      }
    
    }
    
    int energy_grid()         		
    {
      int spin_sum = 0;
      for (int i=0; i<l; i++)
      {
        for (int j=0; j<l; j++)
        {
          int ir = ( (i+1==l) ? 0 : i+1 ); 
          int ju = ( (j-1<0) ? l-1 : j-1 );
          spin_sum += grid[i][j]*(grid[ir][j] + grid[i][ju]);
        }
      }
      return (spin_sum);
    }
    
    int energy_single_spin(int i, int j) 	
    {
      int spin_sum = 0;
      int ir = ( (i+1==l) ? 0 : i+1 );		
      int il = ( (i-1<0) ? l-1 : i-1 );
      int ju = ( (j-1<0) ? l-1 : j-1 );
      int jd = ( (j+1==l) ? 0 : j+1 );
      spin_sum = grid[i][j]*(grid[ir][j] + grid[il][j] + grid[i][ju] + grid[i][jd]);
      return(spin_sum);
    }
    
    int flip_spin(int a, int b)   	
    {
      int dM;
      if (grid[a][b] == 1) {dM=-2;}
        else {dM=2;}
      grid[a][b] = grid[a][b] * (-1);
      return dM;
    }
    
    int mag()
    {
       int mag = 0;
       for (int i=0; i<l; i++){
          for (int j=0; j<l; j++){
             mag +=grid[i][j];
             }
          }
       return mag;
    }
    
    int update_step(int Eg,int &M, double beta, int flag)  
    {
      int energy_old_spin, energy_new_spin;
      int dEs=0,dM=0;			
      int i,j,pos,pos2;
      int spin_old;
      int N=l*l;
      double p,d,acceptance;
    
      boost::random::uniform_int_distribution<> dist(0, (l-1));
      i=dist(drand);
      j=dist(drand);
      spin_old = grid[i][j];
      energy_old_spin = energy_single_spin(i,j);
      dM = flip_spin(i,j);
      energy_new_spin = energy_single_spin(i,j);
      dEs = energy_new_spin - energy_old_spin;
    
      d = beta * dEs + W[(M+N+dM)/2] - W[(M + N)/2];
    
      if (d > 0) {
        //cout << "accepted < 0";
        M =  M + dM;
        return couplingconst*dEs;      
      }
      else {
        //acceptance test
    
        boost::random::uniform_real_distribution<double> distDouble(0,1);
        double r = distDouble(drand);
    
        acceptance = exp(d);
    
        if (acceptance > r) {
          //cout << "accepted\t";
          M =  M + dM;
          return couplingconst*dEs;
        }
        else {
        grid[i][j] = spin_old;
        //cout << "rejected ";
        return 0;
        }      
      } 
    }  
    
    void MUCA_iteration(int N,int part_run, double beta, int flag,int run)
    {
      double variance=1;
      double Pave=0, Pave2=0;
      vector <double> P_n(N+1,0); 
      vector <double> m(N+1,0); 
      vector <double> g(N+1,0);
      vector <double> R(N,0);
      double Eg;
      int pos,ii=0,pos2, M=0;
      double exponent, P; //For accumulative MUCA rekursion
      int cond=0,cont;
    
      Eg = -couplingconst*energy_grid();
      M = mag();
    
      while(ii < run ){
        ii++;
        for (int ittherm=0; ittherm<part_run; ittherm++)	{      
         for (int n=0; n<N; n++){
            Eg+=update_step(Eg, M, beta, flag);                          
            }
        pos=(M+N)/2;
        pos2=(Eg+2*N)/4;
        m[pos]+=1;
        g[pos2]+=1; 
        }  
    
        for (int k=0; k<N; k++){  //accumulative MUCA 
          if(m[k]==0 || m[k+1]==0) 
          {
            exponent = 0;
            P = 0;
          }
          else
          { 
            P = m[k]*m[k+1]/(m[k]+m[k+1]);
            P_n[k] += P;
    	exponent = P/P_n[k];
            //cout<<k<<"   "<< P << "   "<< exponent<<endl;
            R[k] = R[k] + exponent * log(m[k]/m[k+1]); // log R
          }
          W[k+1] = W[k] + R[k];
          Pave2=(Pave2*k+m[k]*m[k])/(double)(k+1);
          m[k] = 0;  
          g[k] = 0;
        }   
        Pave2=(Pave2*N+m[N]*m[N])/(double)(N+1.0);
        Pave = part_run/(double)(N+1);
        variance =sqrt( Pave2 - Pave*Pave )/part_run;
        cout<<ii<<"    "<<variance<<endl;
        m[N]=0;
        g[N]=0;
        Pave=0;
        Pave2=0;
      }
      cout<< "here" <<endl;
    }
    
    int main (int argc, char *argv[])
    {  
      double beta=1./2;
      double T=1./beta;
      int part_run;		
      int meas=1000000;
      int Eg = 0, M=0, N;	
      double Eave = 0.0, Emuca=0, E_norm=0, Eave_t;		  
      int pos, pos2,flag=0,run=0;
    
      T=1.5;
      l=10;
      part_run=10000;
      run= 10;
    
      beta=1./T;
    
      N=l*l;
    
      vector <double> g(N+1,0);
      W.resize(N+1,0);
      vector <int> m(N+1,0);
    
      grid_init(1./T);
    
      MUCA_iteration(N, part_run, beta, flag,run);
      cout<< "not here..." << endl;
    
    }
    

    Eine mögliche Fehlerquelle: Bei Updatestep wird eine Referenz angenommen, kann es da Probleme geben?

    Noch was anderes: Ich lerne so ins Blaue hinein und im Vergleich sieht das bei mir noch ganz ordentlich aus.. Was ist denn 'ordentlich' programmieren? Gibt es da einen standard?

    Grüße!



  • Habe eine Vermutung.
    Der Fehler taucht in diesem Codeblock auf:

    for (int ittherm=0; ittherm<part_run; ittherm++)
        {
          for (int n=0; n<N; n++)
          {
            Eg+=update_step(Eg, M, beta, flag);
          }
          pos=(M+N)/2;
          pos2=(Eg+2*N)/4;
          m[pos]+=1;
          g[pos2]+=1;
        }
    

    Dort berechnest du zwei Positionen, mit diesen greift du dann auf die Elemente im Vektor zu. Aaaaber N+1 ist nur 101. Du hast für pos2 aber manchmal sogar 104 als Index. Keine Ahnung, ob der []-Operator Out-of-Boundary abfängt. Aber hier würde ich mal ansetzen.

    Tipps für's ordentliche programmieren:
    - keine einbuchstabigen Variablennamen.
    - ordentliche Einrückung.
    - ein paar Kommentare hinterlassen. Kommentare mit so einer Beschreibung, was du dir als Programmierer dabei gedacht hast. Damit der Typ (oder auch du in 6 Monaten) nicht den "wtf per minute"-Index anwenden muss.
    - als Faustregel gilt: 4 oder 8 Zeichen (Linux-Codingguidelines geben 8 vor) Leerzeichen pro Tabstopp/Einrückungsebene.
    - globale Variablen vermeiden.



  • Dieser Thread wurde von Moderator/in SeppJ aus dem Forum C (alle ISO-Standards) in das Forum C++ (alle ISO-Standards) verschoben.

    Im Zweifelsfall bitte auch folgende Hinweise beachten:
    C/C++ Forum :: FAQ - Sonstiges :: Wohin mit meiner Frage?

    Dieses Posting wurde automatisch erzeugt.



  • Wunderbar, Danke!

    Noch zwei Fragen:

    Leidet nicht die Effektivität wenn ich zB beim Aufruf von update_step (passiert einige Millionen mal) ständig alle Variablen übergeben muss?

    Wie kann man das Problem lokalisieren? Benutzt du ein besonderes Programm?


  • Mod

    Kinderschokolade schrieb:

    Leidet nicht die Effektivität wenn ich zB beim Aufruf von update_step (passiert einige Millionen mal) ständig alle Variablen übergeben muss?

    Nein, die Effektivität wird sich sogar erhöhen. Globale Variablen verhindern Optimierungen. Außerdem machen sie auch sonst nur Probleme. Um jeden Preis vermeiden und Rache verüben an demjenigen, der sie einem empfohlen hat.

    Wie kann man das Problem lokalisieren? Benutzt du ein besonderes Programm?

    valgrind ist ein gutes Programm dafür unter Linux. Gibt bestimmt auch was für andere Systeme. Allgemein helfen natürlich auch Debugger, damit ist es aber etwas mehr Handarbeit. Auch gut: Debugmodus der STL aktivieren. Die meisten Implementierungen haben Schalter dafür.



  • Naja, was passiert schon beim Funktionsaufruf groß? Da du nur primitive Datentypen übergibst, müssen keine komplexeren Kopien deiner Objekte erstellt werden. Du hast also den Overhead der Sicherung deines gegenwärtigen Stackframes plus die Erstellung deines neuen Frames und das Kopieren.

    Alternativ kannst du dir mal dieses kleine Programm anschauen (Powered by Volkard):

    #include <stdint.h>
    #include <stdlib.h>
    #include <stdio.h>
    
    static __inline__ unsigned long long rdtsc(void)
    {
            unsigned hi, lo;
            __asm__ __volatile__ ("rdtsc" : "=a"(lo), "=d"(hi));
            return ( (unsigned long long)lo)|( ((unsigned long long)hi)<<32 );
    }
    
    size_t buffer;
    
    inline __attribute__((always_inline)) size_t bla_inline(double x,double y)
    {
            return ((((rand()/40)*y)+0xffff)/x)/13;
    }
    
    size_t bla(double x,double y)
    {
            return ((((rand()/40)*y)+0xffff)/x)/13;
    }
    
    uint64_t measure(char const* fName,size_t (*f)(double,double))
    {
            int const maxLeft=1000000;
            int left=maxLeft;
            uint64_t minTime=(uint64_t)-1;
            while(left!=0)
            {
                    uint64_t time=-rdtsc();
                    buffer=f(123456789.0,987654321.1);
                    time+=rdtsc();
                    if(time<minTime)
                    {
                            minTime=time;
                            left=maxLeft;
                    }
                    --left;
            }
            printf("function %s needs %lu ticks\n",fName,minTime);
            return minTime;
    }
    
    #define MEASURE(f) measure(#f,f)
    
    int main(void)
    {
            MEASURE(bla);
            MEASURE(bla_inline);
    
            return 0;
    }
    

    bla_inline zwingen wir, inlined zu werden. Heißt, hier wird dann direkt vor Ort Code erzeugt, anstatt in die Funktion zu gehen.

    Bei mir brauchen beide Funktionen gleich lang.
    Makros oder dergleichen sollte man eh nur dann verwenden, wenn es wirklich keinen anderen Sinn ergibt.

    Zur zweiten Frage: Programmiererintuition. 😃
    Ein alter Lehrer von mir meinte mal, wir Informatiker wären praktische Wissenschaftler: Knopf drücken und sehen, was passiert/ob's explodiert.

    Ich habe einfach einige Teile deines Programms auskommentiert, geprüft, ob's noch funktioniert, dann geschaut, was der Unterschied ist. Dann habe ich mir die Längen- und die Positionsvariablen ausgeben lassen, sichergestellt, dass die Vektoren lokal sind und dass diese auch nicht einer andern Funktion zum Erweitern übergeben werden. Der Rest ist Schach.



  • Kleiner Nachtrag, der mir gerade noch eingefallen ist:

    Ich habe mein und dein Beispiel auf primitive Datentypen reduziert. Aber in C - und vor allem in C++ - kann man eigene sehr komplexe Datentypen erstellen. Ach was, man braucht sie nicht mal erstellen, es reicht schon, wenn man so Container aus der STL verwendet. Jedes Mal, wenn du ein komplexes Objekt erstellt wird, wird ein Konstruktor aufgerufen. Und der macht halt mit new/delete rum.

    Ich habe noch gelehrt bekommen, dass man die Anzahl der dynamischen Speicherreservierungen reduzieren soll, aber mein Hauptgebiet ist auch C, da kann man das, ohne die Philosophie hinter der Sprache zu brechen. C++ bringt einen Haufen eigener Typen und Programme mit, die mit Fehlern von C aufräumen sollen, allerdings zu dem Preis einer enorm erhöhten Komplexität (was übrigens auch der Grund war, warum die Leute dann auf den Java-Zug gesprungen sind. Wenn ich mir die Sprachdefinitionen zwischen C++ und Java anschaue, ist Java in der Komplexität aber deutlich weiter). Dynamische Speicherreservierungen sind teuer. Selbst, wenn du keinen Kernel-Call (Speicherreservierung vom Betriebssystem - in der Regel holt sich new/malloc Speicher in einem großen Block und geht dann im Userspace nur interne Listen durch, ob ein angeforderter Chunk noch in einen Hohlraum passt) hast, muss immer noch die interne Liste durchgegangen werden. Und das gleiche bei delete/free . Sowas ist teuer.

    Worauf ich hinauswill, ist: wenn du komplexere Typen verwendest und Call-By-Value machst, dann hast du immer den Aufruf des Konstruktors oder Kopierkonstruktors über dir hängen. Der kann dann, um tiefe Kopien deiner Objekte zu erstellen (oder generell ein Objekt) die dynamische Speicherverwaltung anhauen. Und dann wird das teuer.
    Und dann wird das noch teurer, weil du dann bei Funktionsrückkehr noch Destruktorenaufrufe hast, die dann eventuell wieder delete/free machen. Bei solchen Szenarien kannst du dir ernsthaft Sorgen um die Performance machen. Auch, wenn du immer so tolle Aussagen wie "get what you pay for" zu hören bekommst.

    [Offtopic]Ich habe dein Programm ja kompiliert, und das Binary (auf Linux) ist so 100 KB Groß. Ungestriped (gestriped sind's dann noch 55 KB). Und braucht drei Sekunden zum Kompilieren.
    Als Vergleich: wir hacken hier an einer plattformunabhängigen Bibliothek in C. Quellcode so derzeit 15.000 Zeilen, Kommentare abgezogen, und die Header sind da noch nicht mit eingerechnet. Die braucht ebenfalls 3 Sekunden zum Kompileren, ist aber nur 80 KB groß. Ungestriped. Gestriped dann nur noch 68 KB. Aber das sind auch +15K Zeilen Code.
    Ich erwähne das, weil ich immer "get what you pay for" von den C++-Leuten höre. Aber genug offtopic.[/Offtopic]

    Vermeiden kannst du das übrigens, indem du Referenzen/Adressen (na gut, Adressen/Zeiger sind C, und in C++ soll man sie nicht verwenden) an die Funktionen übergibst, die du const deklarierst. Also, die Parameter jetzt. Damit sagst du, dass du gerne das Originalobjekt hättest, aber versprichst, es nicht zu ändern. Funktioniert natürlich nur dann, wenn du's dann auch nicht änderst. Wenn doch, dann hilft dir allerdings nicht mal der Wechsel zu C. 🙂

    Also: bei kleinen/primitiven Typen ist das kein Problem. Bei komplexeren Call-By-Value-Situationen schon.

    Als Beispiel auch noch mal dieses Programm:

    #include <cstdint>
    #include <iostream>
    #include <string>
    
    static __inline__ unsigned long long rdtsc(void)
    {
            unsigned hi, lo;
            __asm__ __volatile__ ("rdtsc" : "=a"(lo), "=d"(hi));
            return ( (unsigned long long)lo)|( ((unsigned long long)hi)<<32 );
    }
    
    void*buffer;
    
    void* bla_ref(const std::string&xxx)
    {
            return const_cast<char*>(xxx.c_str());
    }
    
    void* bla_value(const std::string xxx)
    {
            return const_cast<char*>(xxx.c_str());
    }
    
    uint64_t measure1(char const* fName,void* (*f)(const std::string))
    {
            int const maxLeft=1000000;
            int left=maxLeft;
            uint64_t minTime=(uint64_t)-1;
            while(left!=0)
            {
                    uint64_t time=-rdtsc();
                    buffer=f("Bla");
                    time+=rdtsc();
                    if(time<minTime)
                    {
                            minTime=time;
                            left=maxLeft;
                    }
                    --left;
            }
            std::cout<<"function "<<fName<<" needs "<<minTime<<" ticks\n";
            return minTime;
    }
    
    uint64_t measure2(char const* fName,void* (*f)(const std::string&))
    {
            std::string xxx="bla";
            int const maxLeft=1000000;
            int left=maxLeft;
            uint64_t minTime=(uint64_t)-1;
            while(left!=0)
            {
                    uint64_t time=-rdtsc();
                    buffer=f(xxx);
                    time+=rdtsc();
                    if(time<minTime)
                    {
                            minTime=time;
                            left=maxLeft;
                    }
                    --left;
            }
            std::cout<<"function "<<fName<<" needs "<<minTime<<" ticks\n";
            return minTime;
    }
    
    #define MEASURE1(f) measure1(#f,f)
    #define MEASURE2(f) measure2(#f,f)
    
    int main(void)
    {
            MEASURE1(bla_value);
            MEASURE2(bla_ref);
    
            return 0;
    }
    

    (Das schlechte C++ bitte ich zu entschuldigen, ich bin wie gesagt eher in C bewandet).

    Hier braucht bla_value deutlich länger als bla_ref . Bei mir sind's auf -O0:

    function bla_value needs 98 ticks
    function bla_ref needs 12 ticks
    

    Auf -O3:

    function bla_value needs 56 ticks
    function bla_ref needs 12 ticks
    

    Und das sind die schnellsten Läufe. Nicht der Durchschnitt.

    Hm, war dann doch nicht so klein. 🙂


Log in to reply