C struct array + malloc

Mark.G.

Ensign
Dabei seit
Okt. 2008
Beiträge
249
Hallo,
ich wollte mal fragen, ob der Code so passt. Laut Ausgabe sollte es passen, da ich aber mit malloc immer wieder meine Probleme habe, frag ich mal sicherheitshalber nach.
Es kann sein, dass einige includes unnötig sind (das ist nur ein Teil des Codes).

Aufgabe des Codes:
Der Code soll dynamisch ein Spielfeld erstellen (in diesem Codestück wurde die Größe mal fixiert) und dafür den Speicher anfordern. Das Spielfeld soll über ein 2D Array aufrufbar sein und einen Wert (100) sowie ein Mutex besitzen.

Frage:
Wird der Speicher richtig angefordert?


Code:
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <unistd.h>
#include <ctype.h>

struct feld
{
    pthread_mutex_t mutex;
    int value;
};

int main()
{
    int x=10, y=10, i=0, j=0;
    struct feld** spielfeld;
    spielfeld = (struct feld**)malloc(sizeof(struct feld*)*y);
   if(spielfeld == NULL) // Prüft ob die Reservierung erfolgreich war
    {
         [B]return EXIT_FAILURE;[/B]
    }
    for(i=0; i<y; i++)
    {
        spielfeld[i] = (struct feld*)malloc(sizeof(struct feld)*x);
        if(spielfeld[i] == NULL) // Prüft ob die Reservierung erfolgreich war
        {
            [B] return EXIT_FAILURE;[/B]
        }
    }

    for(i=0; i<y; i++)
    {
        for(j=0;j<x; j++)
        {
            pthread_mutex_init(&(spielfeld[i][j].mutex),0);
            spielfeld[i][j].value = 100;
        }
    }

    spielfeld[5][5].value=2;
    spielfeld[9][9].value=22;

    printf("%d",spielfeld[9][9].value);
    free(spielfeld);//gibt speicher wieder frei

    [B]return EXIT_SUCCESS;[/B]
}
 
Zuletzt bearbeitet:

NullPointer

Lt. Commander
Dabei seit
Okt. 2009
Beiträge
1.570
Meines Erachtens machst du alles richtig.

Edit: Wenn du's ganz richtig machen willst, müßtest du noch prüfen, ob malloc() 0 bzw. NULL zurückliefert (das tut es, wenn nicht genug Speicher verfügbar ist). Darauf verzichten aber die meisten C-Programmierer ;)
 

asdfman

Commander
Dabei seit
März 2008
Beiträge
2.315
Darauf sollte aber kein C-Programmierer verzichten.
Außerdem solltest du noch irgendwann free() aufrufen, um den Speicher wieder freizugeben.
Normalerweise passiert das zwar, wenn das Programm beendet wird, aber man muss sich da
nicht 100% drauf verlassen, dass das immer passiert. Also lieber selbst machen.
 

Mark.G.

Ensign
Ersteller dieses Themas
Dabei seit
Okt. 2008
Beiträge
249
So danke für die Hinweise, ich hab den Code mal upgedated (updates sind fett). Nehme an, dass es so passt.
 

NullPointer

Lt. Commander
Dabei seit
Okt. 2009
Beiträge
1.570
Nicht schlecht, aber ein paar Anmerkungen habe ich noch:

1. Prinzipiell ist an "return NULL;" in der main() nichts auszusetzen, weil NULL = 0 zum Typ int (Rückgabetyp von main()) kompatibel ist. Allerdings gibt es eine ungeschriebene Konvention, daß main() dann 0 zurückgibt, wenn alles geklappt hat, und im Fehlerfall einen von 0 verschiedenen Fehlercode. Man mag mir jetzt Pedanterie vorwerfen, aber der Vollständigkeit halber wollte ich es erwähnt haben :)

2. Wenn in der for-Schleife ein malloc fehlschlägt, dann returnst du, ohne die Teile des Spielfelds, die schon allokiert wurden, wieder freizugeben. Hier gilt sinngemäß das, was asdfman in Post #3 schrieb.

3. Da dein vollständiges Programm wahrscheinlich etwas länger ist als dieser Beispielcode, würde ich dir empfehlen, das Allokieren und Freigeben des Spielfelds jeweils in eine Funktion auszulagern. Auf diese Weise machst du den Programmcode übersichtlicher. (Vor dem Freigeben eines Pointers bitte auf != NULL prüfen ;))
 

refri89

Cadet 4th Year
Dabei seit
Mai 2010
Beiträge
95
Wieso da ganze nicht auf einmal allokieren und so die Speicherfragmentierung vermeiden ?
Hab schon lange nix mehr in C gemacht (aber des kann ich noch erkennen, daß der obige Code was Laufzeitverhalten und Speicherverwaltung (auch Code Wartung) betrifft schon heftig is...)

struct feld* spielfeld_Tmp;
struct feld** spielfeld;

spielfeld_Tmp = (struct feld*)malloc(sizeof(struct feld)*x*y); //also alle felder auf einmal aufm HEAP
spielfeld = (struct feld**)spielfeld_Tmp; //jetzt den Array-Zeiger einfach in einen doppel Array Zeiger casten
 

asdfman

Commander
Dabei seit
März 2008
Beiträge
2.315
1. Prinzipiell ist an "return NULL;" in der main() nichts auszusetzen, weil NULL = 0 zum Typ int (Rückgabetyp von main()) kompatibel ist. Allerdings gibt es eine ungeschriebene Konvention, daß main() dann 0 zurückgibt, wenn alles geklappt hat, und im Fehlerfall einen von 0 verschiedenen Fehlercode. Man mag mir jetzt Pedanterie vorwerfen, aber der Vollständigkeit halber wollte ich es erwähnt haben
NULL zurückzugeben ist grundsätzlich schlecht. Weil NULL ein Zeiger ist und nicht zwangsläufig den Wert 0
hat. Er zeigt nur auf einen garantiert ungültigen Speicherbereich. Also sollte main() einen integer (gemäß
seiner Deklaration) zurückgeben.

0 bei Erfolg oder etwas Anderes bei einem Fehler zurückzugeben ist auch nicht so ganz toll. Was ist, wenn
es eine merkwürdige Plattform gibt, bei der Anderes erwartet wird? Man sollte hier lieber EXIT_SUCCESS
oder EXIT_FAILURE benutzen. Wobei das meiner Meinung nach nicht so dramatisch ist, und ich selbst es
in der Regel auch direkt mit Zahlen mache :/

Vielleicht hat ja jemand Lust, im Standard nachzulesen, ob 0 bei Erfolg und irgendetwas Anderes bei einem
Fehler klar geht, oder nicht. Ich weiß es nicht genau und würde mich wundern, warum es oben genannte
Makros gibt, wenn die Werte eh standardisiert sind.
 
Zuletzt bearbeitet:

NullPointer

Lt. Commander
Dabei seit
Okt. 2009
Beiträge
1.570
Hast recht - in C ist NULL tatsächlich vom Typ (void*). Ich war gedanklich bei C++, wo es einfach nur als 0 #define'd ist. Laufen würde es trotzdem, wegen impliziter Typumwandlung, ist aber nicht "sauber".

Die 0-bei-Erfolg-Konvention ist POSIX-weit üblich, ebenso unter Windows.
 

7H3 N4C3R

Lt. Commander
Dabei seit
Feb. 2002
Beiträge
1.816
Also zur NULL-Diskussion zitiere ich einfach mal den (99er) Standard:

7.17 (3)
The macros are
NULL
which expands to an implementation-defined null pointer constant;
und 6.3.2.3 (3)
An integer constant expression with the value 0, or such an expression cast to type
void *, is called a null pointer constant.55) If a null pointer constant is converted to a
pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal
to a pointer to any object or function.
Davon abgesehen, sollten die Subarrays natürlich auch noch alle ein free spendiert bekommen.

Außerdem frage ich mich nach dem Sinn der Mutexe. Würde mich mal ernsthaft interessieren. :) Weil wenn das in Richtung Multithreading und Performance gehen soll, wäre das absolut kontraproduktiv und die Performance, so wie es ist, mit Sicherheit deutlich geringer als in einem Singlethread-Programm.
 

Mark.G.

Ensign
Ersteller dieses Themas
Dabei seit
Okt. 2008
Beiträge
249
So danke für euer Feedback.
Code wurde wieder etwas angepasst.

@refri89
Dein Code halt leider einen "Segmentation fault" verursacht.

@7H3 N4C3R
Wenn ich "spielfeld" freigebe, werden dann nicht auch alle Subarrays freigegeben oO? Bitte deinen 2 Absatz etwas genauer ausführen.

dankö nochmal an alle für euer Feedback
 

Mark.G.

Ensign
Ersteller dieses Themas
Dabei seit
Okt. 2008
Beiträge
249
hmm ich habs mit

Code:
int freemem(int y, int x)
{
    int i=0, j=0;
    for(i=0; i<y; i++)
    {
        for(j=0;j<x; j++)
        {
            free(spielfeld[i][j]);
        }
    }
    return EXIT_SUCCESS;
}
probiert, da kommt dann immer die Fehlermeldung
error: cannot convert ‘feld’ to ‘void*’ for argument ‘1’ to ‘void free(void*)’
 

7H3 N4C3R

Lt. Commander
Dabei seit
Feb. 2002
Beiträge
1.816
Also zum Punkt mit den Mutexen:

Mein Bauchgefühl sagt mir: Viel zu feingranular. Folgendes ist nur aus Annahmen geraten.

Zuerst scheint mir erstmal das gesamte Spielfeld in einer "Aktion" als schützenswert, nicht nur ein einzelnes Feld. Zweitens würde ich spontan erwarten, dass die Feldinhalte sich oft ändern oder oft gelesen werden. Wenn dafür jedes Mal ein Mutex bemüht ist, sprengt das jede Grenze von gut und böse, da die Synchronisationsoperationen um ein vielfaches teurer sind als der eigentliche Zugriff auf den Wert.

Außerdem hat man so, wie der Speicher allokiert ist, ein False Sharing zwischen Threads. Die Daten liegen so in der selben Cacheline. D.h. wenn ein Prozessor einen Wert ändert, invalidiert das die Caches der anderen Prozessoren - selbst wenn sie sich nicht für die konkrete Änderung interessieren - da jeder das Spielfeld im Cache halten wird und dann extrem teuer (in Relation zum Cache) aus dem Hauptspeicher neu laden muss.

Mit 2 Cores/Prozessoren würde ich aus dem Bauch raus sagen, mindestens 50% langsamer (ich glaube, diese Schätzung ist noch sehr optimistisch). Und um so langsamer, je mehr Cores dazu kommen.
 

Mark.G.

Ensign
Ersteller dieses Themas
Dabei seit
Okt. 2008
Beiträge
249
Also, um Mutex komm ich ned herum, das wird verlangt :D

Beschreibung des Progs:
2-26 Threads fahren am Feld herum und ziehen Wände hoch (tron). Über Mutex soll immer das Feld gesperrt werden, auf welches ein Thread fahren möchte.
 

asdfman

Commander
Dabei seit
März 2008
Beiträge
2.315
Eine Funktion EXIT_SUCCESS zurückgeben lassen ist Quatsch. Das ist nur für den Exitstatus des ganzen
Programms gedacht.

Was das free() angeht, scheinst du nur y mal malloc() aufzurufen, dann aber x*y mal free(). Da kann doch
etwas nicht stimmen. Für jedes malloc() einmal free(). Nicht öfter. Nicht seltener.
 

refri89

Cadet 4th Year
Dabei seit
Mai 2010
Beiträge
95
stimmt, hab Sch... gelabert, dann aber schnell den DEVCPP
geladen und nochmal selbst (Threads auskommentiert, weil grad die Thread Lib einlinken mir zu deppert war, aber des sollt ja nicht der punkt sein)

#include <stdio.h>
#include <stdlib.h>
//#include <pthread.h>
#include <unistd.h>
#include <ctype.h>

struct feld
{
//pthread_mutex_t mutex;
int value;
};

int main()
{
int width=10, height=10, i=0, j=0;
struct feld* spielfeld;

spielfeld = (struct feld*)malloc(sizeof(struct feld)*width*height);
if(spielfeld == NULL)return EXIT_FAILURE;


for(i=0; i<height; i++)
{
for(j=0;j<width; j++)
{
//pthread_mutex_init(&( (spielfeld+i*width+j)->mutex),0);
(spielfeld+i*width+j)->value = 100;
}
}

(spielfeld+5*width+5)->value=2;
(spielfeld+9*width+9)->value=22;

printf("%d",(spielfeld+9*width+9)->value);
free(spielfeld);//gibt speicher wieder frei

return EXIT_SUCCESS;
}


Man kann nicht mehr mit dem [][] für doppelte Arrays zugreifen, das war auch der Fehler in meinem ersten Schnellschuß, denn bei feld spielfeld[][] bzw. feld **spielfeld wird ein Array dessen Einzelelemente wieder Arrays und dann dessen Einzelemente "feld-structs" sind erwartet.

Des geht vermutlich an deinem Gesuch vorbei, da du vmtl. den doppelten Array [][] Operator bevorzugt verwenden möchtest und nicht die Pointer Arithmetik, allerdings tät mich die Unübersichtlichkeit und die Speicherfragmentierung stören...brauchst aber nur ein free() für ein zusammenhängenden Speicherbereich und MemoryLeaks sind so ziemlich das Übelste was es gibt...
 
Zuletzt bearbeitet:

Mark.G.

Ensign
Ersteller dieses Themas
Dabei seit
Okt. 2008
Beiträge
249
So scheint es zu gehen, danke an asdfman

Code:
    for(i=0; i<ydim; i++)//speicher freigeben
    {
        free(spielfeld[i]);
    }
 

refri89

Cadet 4th Year
Dabei seit
Mai 2010
Beiträge
95
um [][] Operator benutzen zu können kommst eben um eine extra schleife nicht rum...


#include <stdio.h>
#include <stdlib.h>
//#include <pthread.h>
#include <unistd.h>
#include <ctype.h>

struct feld
{
//pthread_mutex_t mutex;
int value;
};

int main()
{
int width=10, height=10, i=0, j=0;
struct feld* spielfeld_TMP;
struct feld** spielfeld;

spielfeld_TMP = (struct feld*)malloc(sizeof(struct feld)*width*height);
if(spielfeld_TMP == NULL)return EXIT_FAILURE;
spielfeld = (struct feld**)malloc(sizeof(struct feld *)*height);
if(spielfeld == NULL)return EXIT_FAILURE;

for(i=0; i<height; i++)
spielfeld = spielfeld_TMP+i*width;

for(i=0; i<height; i++)
{
for(j=0;j<width; j++)
{
//pthread_mutex_init(&(spielfeld[j].mutex),0);
spielfeld[j].value = 100;
}
}

spielfeld[5][5].value=2;
spielfeld[9][9].value=22;

printf("%d",spielfeld[9][9].value);
free(spielfeld);//gibt speicher wieder frei
free(spielfeld_TMP);//gibt speicher wieder frei

return EXIT_SUCCESS;
}
 
Top