Discussion :

[takout]

Bonjour à tous,

après avoir écrit un programme j'ai voulu à l'aide de valgrind déterminer les fuites de mémoire et faire un profilage.
mon souci c'est que j'ai eu un message d'erreur mais je ne sais pas comment résoudre ce problème.

voici le message d'erreur de valgrind :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
==2879== Invalid read of size 8
==2879==    at 0x400F79: imersion (imersion.c:30)
==2879==    by 0x4029F7: main (main.c:40)
==2879==  Address 0xed31040 is 0 bytes after a block of size 78,643,200 alloc'd
==2879==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2879==    by 0x400BB0: imersion (imersion.c:18)
==2879==    by 0x4029F7: main (main.c:40)

j'ai modifié ce message pour mettre le message ici :

[Neckara]

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
void imersion(unsigned int nrow,unsigned int ncol,double **I,int **lab)
{  
  typedef struct Pixel //il ne faut pas placer ceci à l'extérieur de la fonction ?
  {
     double val;
     int row;
     int col;
   } Pixel;
 
   register  unsigned int k=0;
   unsigned int size_tri=nrow*ncol;
   Pixel *tri=NULL; //il me semble que c'est inutile
Tu peux directement faire Pixel *tri = malloc(...)
 
 
 
  /* initialisation */
  tri=malloc(size_tri*sizeof(Pixel));assert(tri!=NULL);
 
 
   /* trier les pixel par ordre croissant puis enlever les doublons */
   quicksort(tri,0,size_tri-1); //tu trie un tableau qui est vide (= contenu indéterminé) ? Quel intérêt ?
 
 
   /* Innondation */
    for(k=0;k<size_tri; ){
 
 
    	  double grey_value=tri[k].val;
    	  for( ;tri[k].val == grey_value;++k){ //je ne pense pas que ce soit la méthode la plus claire.
    	     .....
             ......
         }
/*  Proposition */
Pixel * fin = tri + size_tri;
Pixel * newTab = tri + 1;
double value = tri->val;
for(Pixel * t = tri + 1;t <= fin;t++)//parcours du tableau dans son intégralité (sauf le premier élément)
       if(t->val != value)
       {
               value = t->val;
               *newTab = *t;
               newTab++;
        }
//taille du tableau = newTab - tri;
/**/
//tu as alloué dynamiquement de la mémoire pour tri, mais quand libères-tu la mémoire avec free ?
}

[takout]

euh oui la structure a été déclaré dans le .h de la fonction. Pour que ce soit plus compréhensible je l'ai mis la dans le topic.

[Neckara]

D'accord, j'ai édité mon précédant post pour te rajouter une proposition de boucle qui me parait plus simple et judicieux.

Sinon, quand effectues-tu le free?

[takout]

Euh je n'ai mis qu'une partie de mon code le free je le fait à la fin de la boucle.
Pour algorithme que je veux implémenter je suis obligé de faire ce style de boucle. C'est un peu difficile d'expliquer mon problème sans que vous ayez la totalité de mon code.

[Neckara]

Est-ce que tu peux nous dire quelle est la ligne 30 du fichier imersion.c ?
Vu que tu n'as mis que des extraits, la ligne 30 indiquée par Valgrind n'est pas la ligne 30 de ce que tu nous montre.

[takout]

J'ai aussi des fuites de mémoires dont je ne comprends pas l'origine.

[Neckara]

Ta fonction reste un petit peu longue.

Je ne comprend pas pourquoi la ligne 30 de immersion.c pose problème, donc je regarde un peu l'ensemble du code :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
register unsigned short int curdist=0;

Une petite chose que tu peux améliorer :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
 
for (i=0;i<nrow;++i){
  for(j=0;j<ncol;++j){ lab[i][j]=INIT;dist[i][j]=0;
                        tri[k].val=I[i][j]; tri[k].row=i;tri[k].col=j;++k;}}

C'est plus lisible comme ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
 
for (i=0;i<nrow;++i)
      for(j=0;j<ncol;++j, k++)
      {
              lab[i][j]=INIT;
              dist[i][j]=0;
              tri[k].val=I[i][j];
              tri[k].row=i;
              tri[k].col=j;
      }

Ensuite, tu n'as pas protégé tes header des inclusions multiples avec :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
 
#ifndef MonHeader_h
#define MonHeader_h
 
//........
 
 
#endif

Je ne suis même pas sûr que faire ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
typedef struct X
{
       struct X * suivant;
}X;

Soit une bonne idée, X est quoi? La structure ou le type ?
Je pense qu'on préfère faire ceci :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
 
typedef struct m_X
{
       struct m_X * suivant;
}X;

Je continu de regarder ton code.

EDIT : je pense qu'il serait plus facile de diviser ta fonction en sous-fonction et de regarder pour chacune de ces sous-fonctions s'il n'y a pas d'erreur.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
 
int (*q)[2]=NULL,(*r)[2]=NULL;
//....
Free(r);

r a été alloué automatiquement, tu ne peux pas le libérer avec un free.
Il se libèrera de lui-même quand le block où il a été défini se fermera :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
 
{
int i
      {
            //....
       }
} //i est libéré ici.

Si tu as fait :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
r[1] = Malloc(..);
r[2] = Malloc(..);

Il faut les libérer avec

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
Free(r[1]);
Free(r[2]);

J'ajouterai aussi que p, p1, p_temp, q et r ne sont pas très explicites comme noms de variables.

[takout]

au début j'ai fauis comme toi Neckara pour le free(r[1]) ... mais ça ne marche pas. C'est du au fait que r est un pointeur sur un tableau de deux dimensions.
si je fais une allocation comme ceci r=malloc(nrow*sizeof(*r) la liberation des tableaux se fait automatiquement comme tu la mentionnée puis il faut libérer la mémoire qu'occupe les objets pointés par r donc free(r).

[Neckara]

Autant pour moi.

Mais ta fonction est un peu trop longue pour que je voie comment tu utilises chacune de tes variables, il faut vraiment que tu pense à découper une de courtes fonctions simple aux noms explicites.

Tu arrives à te relire car c'est toi qui a écris ceci. Mais pour des yeux externes, c'est tout sauf évidant, il faut se rappeler de ce que contient chacune des variables, essayer de voir ce que tu tentes de faire etc...


EDIT :

p est un tableau a deux dimensions contenant les coordonnees du pixel

Pourquoi ne pas faire une structure coordonnée?
Ceci pourrait simplifier ton code.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
 typedef struct Fifo
{
    int *donnee;
    struct Fifo *suivant;
} Fifo;

Très peu intuitif, il vaut mieux nommer cette structure maillon_Fifo et faire :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
typedef (maillon_Fifo *) Fifo;

Et là Fifo sera ta file FIFO (pointeur sur le premier maillon) et non un maillon.


Pense aussi à indenter ton code proprement.

[takout]

ok, en fait j'ai suivi l'algo et comme l'algo est long ça entraîne cela.

[Neckara]

Ce n'est pas une raison, un algo ne signifie pas une seule fonction.

Sinon pour ta file Fifo, il faudrait que tu fasse une vrai file Fifo.

C'est à dire :
- soit une structure comprenant un pointeur au début de la file et un à la fin ;
- soit une structure comprenant un seul pointeur sur le dernier élément mais le dernier élément pointe sur le premier (file circulaire).

Ceci simplifiera encore ton code.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
 
while(1){	
 
    	fifo_remove(&file,p);
 
                      /* on regarde si on est arrivee a la fin de la liste */
   		if(p[0]==-1 && p[1]==-1) /* si p est egale au fictitious pixel*/
    		{
			if(fifo_isempty(file)){ break; }  
     		       else
                       { fifo_add(&file,&fin_file,FICTITIOUS);
                        ++curdist; fifo_remove(&file,p);}
    		 } 
 
    	/* determination des voisins de p  (on cherche les indices des voisins du pixel p)*/         
         n_neighboor=neighborhood8(nrow,ncol,p,&q);
 
    	 /* labelisation de p si ce n'est pas un minimun local */      		                	
    	for(j=0;j<n_neighboor;++j)
    	{   	   	   
      			/* ie q appartient deja a un bassin versant ou a une watershed */
    	   if((dist[q[j][0]][q[j][1]]<curdist) && ((lab[q[j][0]][q[j][1]]>0) || (lab[q[j][0]][q[j][1]]==WSHED))) 
    	   {	
      	   	if(lab[q[j][0]][q[j][1]]>0) /* q appartient seulement a un bassin */
   	   	{  
   	   	 	if((lab[p[0]][p[1]]==MASK)  || (lab[p[0]][p[1]]==WSHED))
   	 		{lab[p[0]][p[1]]=lab[q[j][0]][q[j][1]];} /* p appartient a une watershed*/
 
 			else if(lab[p[0]][p[1]] != lab[q[j][0]][q[j][1]])
   	 			{ lab[p[0]][p[1]]=WSHED;}
 	        }	 	 		   	   
    	        else if (lab[p[0]][p[1]]==MASK) /* si p est deja sur la watershed */
    	   	 { lab[p[0]][p[1]]=WSHED;}
    	   } 
    	   else if ((lab[q[j][0]][q[j][1]]==MASK) && (dist[q[j][0]][q[j][1]]==0))
    	   {dist[q[j][0]][q[j][1]]=++curdist;
    	     p_temp=Malloc(2,int);
    	     p_temp[0]=q[j][0];
    	     p_temp[1]=q[j][1]; fifo_add(&file,&fin_file,p_temp) ; }     	   
          }  Free(q); 	   		
      }
 
      /* verifie si des nouveaux minima locaux sont detectees*/
 
         for(;tri[k].val==grey_value;++k){
 
    	   	 	p[0]=tri[k].row;p[1]=tri[k].col;
    	   	 	dist[p[0]][p[1]]=0; //la distance associee a p est initialisee
 
    	 if(lab[p[0]][p[1]]==MASK)
    	 {   ++curlab;
    	     fifo_add(&file,&fin_file,p);lab[p[0]][p[1]]=curlab;
 
    	    while(!fifo_isempty(file)){
 
    	       fifo_remove(&file,p1);
 
    	      /* determination du voisinage de p1 (on cherche les indices des voisins de p1 ->r)*/
 
               n_neighboor=neighborhood8(nrow,ncol,p1,&r);
 
    		for(j=0;j<n_neighboor;++j){
 
    		  if(lab[r[j][0]][r[j][1]]==MASK){
    		     p_temp=Malloc(2,int);
    	             p_temp[0]=r[j][0];
    	             p_temp[1]=r[j][1];
    	             fifo_add(&file,&fin_file,p_temp);lab[r[j][0]][r[j][1]]=curlab; }   		  	
    		   } 
    		  	Free(r);  }		  			    		 
    	     }	        	       
    	}              
 
   }

Tu as un while(1), je suis incapable de dire à quelle ligne il s'arrête.
C'est trop mal indenté pour être lu, de plus, diviser ceci en fonction ne serait pas du luxe^^
Déjà les while(1) ne sont pas très conseillé car on ne sait pas trop quand on en sort. (un commentaire pour le dire serait pas mal)
Pour des petits while(1) ça peut passé, mais là il semble assez grand. Impossible de dire les conditions d'arrêt, je ne vois ni return ni break;
(merci ctrl+f, j'ai trouvé le break, avec la coloration de notepad, je suis passé à côté).

[takout]

Ces fonctions sont inclus dans une boucle for qui ce situe plus haut dans le programme c'est pur ça que je l'ai pas découpé.

[Neckara]

Essaye de poser les questions sur ton code ici que les autres lecteurs puissent éventuellement t'aider.

Pour ta ligne

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
 
double grey_value=tri[k].val;
for( i=k;tri[i].val == grey_value;++i){

Tu vérifies où que i est inférieur à size_tri ?

[takout]

J'ai testé mais l'erreur ne vient pas de là car i ne peut prendre des valeurs plus grande que k qui est limité à la valeur size_tri.

[Neckara]

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
 
   	  for( i=k;tri[i].val == grey_value;++i)
         {  
    	         p[0]=tri[i].row;
                 p[1]=tri[i].col; /* coordonnee du pixel courant */ 	  
   	         lab[p[0]][p[1]]=MASK;
 
    	         /* determination des voisins de p */
 
    	         n_neighboor=neighborhood8(nrow,ncol,p,&q);
    	        double compte=0; /* pour savoir s'il faut libérer la memoire*/
    	         for(j=0;j<n_neighboor;++j)
                 {  
    	                if((lab[q[j][0]][q[j][1]]>0) || (lab[q[j][0]][q[j][1]]==WSHED) )
                       {
    	                         ++compte;
    	                         dist[p[0]][p[1]]=1;
	      	                 p_temp=Malloc(2,int);
      	    	 	         p_temp[0]=p[0];
                                 p_temp[1]=p[1];
 	 		         fifo_add(&file,&fin_file,p_temp);	 		
 	 		         Free(q);
                                 break;
    	                  }
                   }
                   if(compte==0)     free(q);    	    	  	     	         	    	                   
          }

C'est déjà plus lisible quand c'est bien indenté non?
Je ne vois pas ce qui empêche i d'être plus grand que k.
A moins que la fin de tri soit marqué par une valeur unique et remarquable ?

[takout]

n'oublie pas que "tri[i].val == grey_value;" dans la boucle for est une expression qui lorsqu'elle n'est pas vérifié provoque la fin de la boucle.
Mon tableau est trié et k ne peut prendre des valeurs supérieur à size_tri, de ce fait i aussi.

[Neckara]

Ton tableau est trié mais il y a plusieurs occurrences d'un même nombre non?
Si les deux dernière valeur de tri sont identiques, i devient plus grand que k.

[takout]

j'ai testé mais l'erreur ne vient pas de là, j'ai mis :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
for( i=k;(tri[i].val == grey_value) && (i<size_tri);++i)

mais j'ai toujours le même souci.

[Neckara]

Et si i est égal à size_tri, que ce passe-t-il exactement ?
Réfléchit bien avant de me donner ta réponse.

Un indice : tri a size_tri élément, et le premier élément est tri[0], le dernier est tri[size_tri - 1]