mallocで色々メモリ確保したので全部freeしたい

いつもお世話になっております。
初心者なので、初歩的な質問ばかりで申し訳ありません。
以下のコードの通り、Aという疎行列とその転置であるBを作成し、それらをsmatrix_free(A)やsmatrix_free(B)でfreeしたいです。

そこでコンパイル後にvalgrind --leak-check=full ./a.outによりメモリを全部解放できているかどうか調べてみたところ、残念ながら解放できていないようです。

単純な書き間違いなのか、根本的にコードがおかしいのかよく分かりません。
どなたか、ご教授いただければ幸いです。
宜しくお願いいたします。

編集：コードが長ったらしい上に、実際valgrindでどういう出力があったのかについては全く言及しておりませんでしたね。大変失礼いたしました。

C
1#include <stdio.h>
2#include <string.h>
3#include <stdlib.h>
4
5#define NEW(p, n){p = malloc((n)*sizeof(p[0]));} //動的メモリ確保の関数
6
7//リストの要素の構造体
8typedef struct slobj_{
9    struct slobj_* next;
10    int j;
11    double v;
12}* slobj;
13
14//リストの構造体
15typedef struct{
16    slobj head;
17    slobj tail;
18}* slist;
19
20//疎行列の構造体(リストの配列ということ)
21typedef struct{
22    int n,m;
23    slist* A;
24} smatrix;　
25
26//新しいリストを作成
27slist slist_new(void){
28    slist L;
29    NEW(L, 1);
30    L -> head = NULL;
31    L -> tail = NULL;
32    return L;
33}
34
35//新しいリストの要素を作成
36slobj slobj_new(int x, double y){
37    slobj p;
38    NEW(p, 1);
39    p -> j = x;
40    p -> v = y;
41    p -> next = NULL;
42    return p;
43}
44
45//リストの先頭に挿入
46void slist_insert_head(slist L, slobj p){
47    p -> next = L -> head;
48    L -> head = p;
49}
50
51//リストの末尾に挿入
52void slist_insert_tail(slist L, slobj p){
53    if(L -> head == NULL){
54        L -> tail = p;
55        L -> head = p;
56    }
57    else if(L -> head == L -> tail){
58        L -> tail = p;
59        L -> head -> next = p;
60    }
61    else{
62        L -> tail -> next = p;
63        L -> tail = p;
64    }
65}
66
67//リストをプリント
68void slist_print(slist L)
69{
70    slobj p;
71    p = L->head;
72    while (p != NULL){
73        printf("%d ", p -> j);
74        printf("%lf ", p -> v);
75        p = p-> next;
76    }
77}
78
79//新しい疎行列を作成
80smatrix smatrix_new(int n, int m){
81    smatrix S;
82    int i;
83    NEW(S.A, n);
84    S.n = n;
85    S.m = m;
86    for(i=0; i<= n-1; i++){
87        S.A[i]=slist_new();
88    }
89    return S;
90}
91
92//標準出力から行列を読み込み、別のメモリに記憶しておく
93smatrix smatrix_read(void){
94    smatrix S;
95    int i, n, m, j;
96    double v;
97    scanf("%d", &n);
98    scanf("%d", &m);
99    S = smatrix_new(n, m);
100    for(i = 0; i <= n-1; i++){
101        while(1){
102            scanf("%d", &j);
103            if (j < 0){
104                break;
105            }
106            scanf("%lf", &v);
107            slist_insert_tail(S.A[i], slobj_new(j ,v));
108//このときslobj_newで確保したメモリがslist_freeで解放できていないようです
109        }
110    }
111    return S;
112}
113
114//疎行列をプリント
115void smatrix_print(smatrix S){
116    int i;
117    printf("%d ", S.n);
118    printf("%d\n", S.m);
119    for(i = 0; i <= S.n-1; i++){
120        slist_print(S.A[i]);
121        printf("%d\n", -1);
122    }
123}
124
125//疎行列の転置を作成・出力
126smatrix smatrix_transpose(smatrix S){
127    int i, k;
128    double x;
129    slobj p;
130    smatrix T;
131    T = smatrix_new(S.m, S.n);
132    for(i = 0; i <= S.n-1; i++){
133        p = S.A[i]->head;
134        while(1){
135           if(p==NULL){
136               break;
137           }
138           k = p->j;
139           x = p->v;
140           slist_insert_tail(T.A[k-1], slobj_new(i+1, x));
141//このときslobj_newで確保したメモリがslist_freeで解放できていないようです
142           p = p->next;
143        }
144    }
145    return T;   
146}
147
148//リストのメモリを解放
149void slist_free(slist L){
150    slobj p, q, r;
151    p = L->head;
152    q = p->next;
153    while (q != NULL){
154        r = q;
155        q = q-> next;
156        free(p);
157        p = r;
158    }
159    free(L);
160}
161
162//疎行列のメモリを解放(リストを順にfreeしているつもりです)
163void smatrix_free(smatrix S){
164    int i;
165    for(i = 0; i <= S.n-1; i++){
166            slist_free(S.A[i]);
167    }
168    free(S.A);
169}
170
171int main(){
172    smatrix A;
173    smatrix B;
174    A = smatrix_read();
175    B = smatrix_transpose(A);
176    smatrix_print(B);
177    smatrix_free(A);
178    smatrix_free(B);    
179    return 0;
180}

valgrindでの出力は以下の通りでした。

valgrind
1valgrind --leak-check=full ./a.out
2==152== Memcheck, a memory error detector
3==152== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
4==152== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
5==152== Command: ./a.out
6==152==
7==152== error calling PR_SET_PTRACER, vgdb might block
82 3
91 1.0 2 2.0 3 3.0 -1
101 4.0 2 5.0 3 6.0 -1
113 2
121 1.000000 2 4.000000 -1
131 2.000000 2 5.000000 -1
141 3.000000 2 6.000000 -1
15==152==
16==152== HEAP SUMMARY:
17==152==     in use at exit: 120 bytes in 5 blocks
18==152==   total heap usage: 21 allocs, 16 frees, 8,600 bytes allocated
19==152==
20==152== 48 bytes in 2 blocks are definitely lost in loss record 1 of 2
21==152==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
22==152==    by 0x1087F6: slobj_new (42.c:33)
23==152==    by 0x108A5A: smatrix_read (42.c:97)
24==152==    by 0x108CED: main (42.c:158)
25==152==
26==152== 72 bytes in 3 blocks are definitely lost in loss record 2 of 2
27==152==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
28==152==    by 0x1087F6: slobj_new (42.c:33)
29==152==    by 0x108BD2: smatrix_transpose (42.c:127)
30==152==    by 0x108D08: main (42.c:159)
31==152==
32==152== LEAK SUMMARY:
33==152==    definitely lost: 120 bytes in 5 blocks
34==152==    indirectly lost: 0 bytes in 0 blocks
35==152==      possibly lost: 0 bytes in 0 blocks
36==152==    still reachable: 0 bytes in 0 blocks
37==152==         suppressed: 0 bytes in 0 blocks
38==152==
39==152== For counts of detected and suppressed errors, rerun with: -v
40==152== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

このように、smatrix_readやsmatrix_transposesで新しい行列を作成し、そこに順次リストの要素を挿入していく際に確保したリスト要素のメモリはsmatrix_freeやその中のslist_freeで全て解放しているはずなのですが、どうやら全部解放しきれていないようです。(問題がありそうな個所はコードの中でその旨をコメントしておきました)

slist_freeの記述がおかしいのでしょうか。

jimbe

2020/04/26 18:59

> 単純な書き間違いなのか、根本的にコードがおかしいのか何処までご自分で調査されたのでしょうか.

Zuishin

2020/04/26 22:02

プロセスが終了すると全て解放されるので別に問題ありませんが、new が散らばりすぎて見通し悪いのが原因です。

Zuishin

2020/04/27 00:53

もう一度書きますが、プロセスが終了すると全て解放されるので別に問題ありませんが、new が散らばりすぎて見通し悪いのが原因です。とりあえず今、さしあたって、場当たり的に、泥縄的に、直すのもいいですが、見通しが悪い(長いというのはオブラートに包んだ表現です)のが諸悪の根源なので、それをどうにかしましょう。

Zuishin

2020/04/27 00:54

つまり、設計し直して、最初から最後まで総書き換えです。

Zuishin

2020/04/27 00:55

あと、テストを作りましょう。

jimbe

2020/04/27 02:40

> slist_freeの記述がおかしいのでしょうか。そう思われましたら, Zuishin さんの言われるように 0件/1件/2件(以上)の3パターンのリストが渡された場合にそれぞれどのように slist_free が動作するか(リスト内の全ての slobj に対し free を実行出来ているか) を, 別の main を作ってテストしてみては如何でしょうか. (ポインタに慣れていれば思考実験も出来るかと思いますが.)

行動規範の内容に同意します

回答5件

根本的に

ということで, マトリクス生成時に大きさが決まっているのでリストにする必要が無いようですし, 実体とポインタが混在して分かり難く感じましたので, リスト関係を消し, SMATRIX の malloc をするようにしてみました.
大きな行列で無ければこのほうが解放抜けはし難いと思います.
テストを簡単にするため, smatrix_read を改造しています.

#修正
リストを使うように戻し, 2次元的に動作するようにしました.
データが無い所を 0 表示するようにしました.

c
1#include <stdio.h>
2#include <stdlib.h>
3
4//要素の構造体
5typedef struct slobj {
6  struct slobj *next; //必ず構造体の先頭にすること
7  int x, y;
8  double v;
9} SLOBJ;
10
11//新しい要素を作成
12SLOBJ *slobj_new(int x, int y, double v){
13  SLOBJ *p;
14
15  p = (SLOBJ *)malloc(sizeof(SLOBJ));
16  p->next = NULL;
17  p->x = x;
18  p->y = y;
19  p->v = v;
20  return p;
21}
22
23//リストの構造体
24typedef struct slist {
25  SLOBJ *next; //必ず構造体の先頭にすること
26  SLOBJ *last;
27} SLIST;
28
29//初期化
30void slist_init(SLIST *slist) {
31  slist->next = NULL;
32  slist->last = (SLOBJ *)slist;
33}
34
35//書き換えor追加
36void slist_put(SLIST *slist, int x, int y, double v) {
37  SLOBJ *p;
38
39  for(p=slist->next; p!=NULL; p=p->next) {
40    if(p->x == x && p->y == y) {
41      p->v = v;
42      return;
43    }
44  }
45  p = slobj_new(x, y, v);
46  slist->last->next = p;
47  slist->last = p;
48}
49
50//取り出し
51double slist_get(SLIST *slist, int x, int y) {
52  SLOBJ *p;
53
54  for(p=slist->next; p!=NULL; p=p->next) {
55    if(p->x == x && p->y == y) return p->v;
56  }
57  return 0.0;
58}
59
60//解放
61void slist_clear(SLIST *slist) {
62  SLOBJ *p, *q;
63  for(p=slist->next; p!=NULL; p=q) {
64    q = p->next;
65    free(p);
66  }
67  slist_init(slist);
68}
69
70//疎行列の構造体
71typedef struct {
72  int n, m;
73  SLIST a;
74} SMATRIX;
75
76//新しい疎行列を作成
77SMATRIX *smatrix_new(int n, int m) {
78  SMATRIX *s;
79
80  s = (SMATRIX *)malloc(sizeof(SMATRIX));
81  s->n = n;
82  s->m = m;
83  slist_init(&s->a);
84  return s;
85}
86
87//標準出力から行列を読み込み、別のメモリに記憶しておく
88SMATRIX *smatrix_read() {
89  int i, n, m, x, y, k=0;
90  double v;
91  SMATRIX *s;
92  int test[] = { 2,3,  1,1,1, /*2,1,2*/ 1,2,4,  2,2,5, 3,2,6, 3,1,3, }; //テストデータ
93
94  n = test[k++]; //scanf("%d",&n);
95  m = test[k++]; //scanf("%d",&m);
96  s = smatrix_new(n, m);
97  for (i=0; i<(sizeof(test)/sizeof(int)-2)/3; i++) {
98    x = test[k++]; //scanf("%d",&s->a[i].j);
99    y = test[k++];
100    v = test[k++]; //scanf("%lf",&s->a[i].v);
101    slist_put(&s->a, x, y, v);
102  }
103  return s;
104}
105
106//疎行列の転置を作成・出力
107SMATRIX *smatrix_transpose(SMATRIX *s) {
108  SLOBJ *p;
109  SMATRIX *t;
110
111  t = smatrix_new(s->m, s->n);
112  for(p=s->a.next; p!=NULL; p=p->next) {
113    slist_put(&t->a, p->y, p->x, p->v); //行列変換
114  }
115  return t;
116}
117
118//疎行列をプリント
119void smatrix_print(char *label, SMATRIX *s) {
120  if(label != NULL) printf("%s\n", label);
121  printf("%d %d\n", s->n, s->m);
122
123  for(int y=1; y<=s->n; y++) {
124    for(int x=1; x<=s->m; x++) {
125      printf("(%d,%d)=%lf%s", x, y, slist_get(&s->a,x,y), x==s->m?"\n":" ");
126    }
127  }
128}
129
130void smatrix_free(SMATRIX *s) {
131  slist_clear(&s->a);
132  free(s);
133}
134
135int main() {
136  SMATRIX *a;
137  SMATRIX *b;
138
139  a = smatrix_read();
140  b = smatrix_transpose(a);
141  smatrix_print("A:", a);
142  smatrix_print("B:", b);
143  smatrix_free(a);
144  smatrix_free(b);
145  return 0;
146}

plain
1A:
22 3
3(1,1)=1.000000 (2,1)=0.000000 (3,1)=3.000000
4(1,2)=4.000000 (2,2)=5.000000 (3,2)=6.000000
5B:
63 2
7(1,1)=1.000000 (2,1)=4.000000
8(1,2)=0.000000 (2,2)=5.000000
9(1,3)=3.000000 (2,3)=6.000000