C Speicherzugrifffehler bei Versuch Liste aus Textfile zu lesen

Dome113 · 16. Februar 2017

Guten Abend zusammen,
Im Prinzip versuche ich gerade eine verkettete Liste aus einem Textfile einlesen zulassen. Dafür habe ich die nötigen Daten zeilenweise je Datensatz und mit einem #getrennt je Komponente in ein Textfile geschrieben. Das lesen funktioniert für sich genommen auch gut soweit (Funktion "tokout") allerdings funktioniert die Zuweisung auf die Struktur nicht bzw. habe ich irgendwo was mit malloc verbockt. Da ich noch nicht so erfahren bin bitte ich jede Todsünde dich begangen habe zu entschuldigen.
Hier die Funktionen:

Code:

tName *pNxt=NULL;
tName *pAnfang=NULL;
FILE *data;
tName *daten;

tName *tokout(char *buf, tName *daten)
{
    strcpy(daten->VName,strtok(buf," #"));
    strcpy(daten->NName,strtok(NULL," #"));
    daten->Nr=(atoi(strtok(NULL," #")));
    return daten;
}

void load()
{
    if ((daten=malloc(sizeof(tName)))==NULL)
    {
        printf("Fehler bei malloc");
        exit(-1);
    }
    data=fopen("data.txt","rt");
    tName *pCur=pAnfang;
    while(fgets(tBuf,64,data))
    {
        if ((daten=calloc(1,sizeof(tName)))==0)
        {
            printf("Calloc fehlgeschlagen.\n");
            exit(-1);
        }
        if (pAnfang==NULL)
        {
            if ((pAnfang=malloc (sizeof(tName)))==NULL)
            {
                printf("Malloc fehlgeschlagen.\n");
                exit(-1);
            }
            tokout(tBuf,daten);
            pAnfang=daten;
            pAnfang->pNxt=NULL;
        }
        else
        {
            if ((pCur->pNxt=malloc (sizeof(tName)))==NULL)
            {
                printf("Malloc fehlgeschlagen.\n");
                exit(-1);
            }
            pCur=pCur->pNxt;
            tokout(tBuf,daten);
            pCur=daten;
            pCur->pNxt=NULL;
            
        }
    }
    fclose(data);
    free(pAnfang);
    free(pCur->pNxt);
    free(daten);
    return;
}

Und das Headerfile mit der Struktur:

Code:

char tBuf[64];

typedef struct Name {
    char VName[15];
    char NName[15];
    int Nr;
    struct Name *pNxt;
}tName;

tName *tokout(char *buf, tName *daten);
void load();

So also kurz zur erklärung was ich mir so gedacht habe: "daten" soll meine Zwischenablage für die Inhalte der einzelnen Datensätze sein, "pAnfang" der erste Datensatz der Liste und "pCur" alle Nachfolgenden. Wie gesagt kommt allerdings sobald ich load(); abrufe "Speicherzugriffsfehlers (Speicherabzug geschrieben)". Ich hoffe das mir jemand hier helfen kann und bedanke mich vorab für alle Antworten.

MfG Dome

aroxx · 16. Februar 2017

was genau ist denn die fehlermeldung? malloc failed?

"if ((daten=calloc(1,sizeof(tName)))==0)" da willst du auf NULL vergleichen oder?

wofür das calloc?

Dome113 · 17. Februar 2017

Also compilieren funktioniert fehlerfrei und wenn die Funktions dann innerhalb des Programms abgerufen wird kommt nur Speicherzugriffsfehler (Speicherabzug geschrieben). Wird da irgendwo eine genauere Fehlermeldung hinterlegt?

Ja da sollte NULL stehen aber auch nach der Änderung bleibt alles wie es ist. Nunja das calloc habe ich benutzt um Platz für den einen Datensatz zu schaffen der eingelesen werden soll aber eigentlich sollte dafür ja auch schon das erste malloc reichen oder? (Auch wenn ich das calloc auskommentiere ändert sich nichts)

Finalspace · 17. Februar 2017

Zu aller erst:

- Nehme lange Namen für Variablen und Typen.
- Benenn alles in Deutsch oder English aber nicht mischen!
- Benutze keine Globale Variablen (Man kann alles auch ohne globale Variablen implementieren!) -> Es sollte explizit Sichtbar sein, bzw. vom Namen der eindeutig ob ein Globaler Status verändert wird.

- Erzeuge für jedes Linked List Element einen Eintrag im Speicher oder besser: Erzeuge ein Array mit der max. möglichen Anzahl an Elemente und erhöhe dann schlichtweg immer nur den index -> Vorteil: contiguous memory, besser für Cache/Performance

- Das erste Element ist dein Head und erstes vorheriges
- Next Pointer nicht vergessen zu setzen

Code:

FILE *fileHandle = fopen(filename, "rb");

int maxDataElementCount = 1024; // TODO: Read number of max element count from file
DataElement *dataElements = (DataElement *)malloc(sizeof(DataElement) * maxDataElementCount);

DataElement *firstElement = 0;
DataElement *prevElement = 0;

int dataElementIndex = 0;
char buffer[64];
while (readDataFromFile(&fileHandle, &buffer, 64)) {
  assert(dataElementIndex < maxDataElementCount);
  DataElement *dataElement = dataElements + dataElementIndex;
  ++dataElementIndex;

  // Clear to zero (Malloc does not clear!)
  *dataElement = {};

  // Do something with the new data element

  if (!firstElement) {
    firstElement = prevElement  = dataElement;
  }
  assert(prevElement);
  assert(!prevElement->next);
  prevElement->next = dataElement;
  prevElement = dataElement;
}

fclose(fileHandle);

return firstElement;

Wichtig: Hier musst du noch dafür sorgen, das nicht benötigter Speicher freigegeben wird oder merkst dir wieviel Elemente du tatsächlich immer brauchst pro Datei - dann entfällt dieser Part komplett.

Ich nehme immer ein Statisches Speicher Verfahren (Ein Globaler Block an Speicher in main und diesen schlichtweg selbst Partitionieren, z.b. als Dynamische Arena) -> Vorteil: Einmalig Speicher anlegen und erst am Ende der Anwendung ein einziges free() und done.
Sieht angefähr so aus:

Code:

struct MemoryBlock {
	void *base;
	uint64_t used;
	uint64_t size;
};

inline MemoryBlock MemoryBlockCreate(void *base, uint64_t size) {
	assert(base);
	assert(size > 0);
	MemoryBlock result = {};
	result.base = base;
	result.size = size;
	return(result);
}
inline void *__PushSize(MemoryBlock *block, uint64_t size) {
	assert(block);
	assert(size > 0);
	assert(block->used + size <= block->size);
	void *result = (U8*)block->base + block->used;
	block->used += size;
	return(result);
}

#define PushSize(block, size, ...) \
	__PushSize(block, size, ## __VA_ARGS__)
#define PushStruct(block, type, ...) \
	(type *)__PushSize(block, sizeof(type), ## __VA_ARGS__)
#define PushArray(block, type, count, ...) \
	(type *)__PushSize(block, sizeof(type) * count, ## __VA_ARGS__)

int main() {
  uint64_t appMemorySize = MegaBytes(500ULL);
  void *appMemoryBase = malloc(appMemorySize);
  MemoryBlock appMemory = MemoryBlockCreate(appMemoryBase, appMemorySize);

  while (readDataFromFile()) {
    DatenElement *newElement = PushStruct(&appMemory, DatenElement);
    *newElement = {};

    // Do something with the new element
  }

  free(appMemory);
}

Das kann man übrigens auch in nen Dynamisches Speicherverfahren umstellen -> Anstatt Asserts, erzeugt man nen neuen Block Speicher und hängt den alten als Linked List Element hinten dran. Vorteil: Man hat ein Zentrales System was Speicher erzeugt und muss sich nicht mehr direkt mit malloc oder free auseinander setzen ;-)

Dome113 · 17. Februar 2017

OK vielen Dank erstmal für die ausführliche Antwort, ich werde versuchen mir die Tipps einzuprägen.
Ich muss schon mal sagen das dein Speicherverwaltungssystem (für mich als Neuling) ziemlich komplex aussieht deshalb werde ich wohl erstmal bei meinen simplen Methoden bleiben (und hoffen das alles gut geht).
Jetzt zum 1. Codeabschnitt: Die asserts schließen das Programm wenn die Bedigung nicht erfüllt ist wenn ich das richtig verstanden habe. Sie sind also für meinen Zweck nicht so gut geeignet da diese load() Funktion ja am Anfang meines eigentlichen Programmes steht und danach mit der Liste gearbeitet werden soll. Ich schätze eine if Bedingung mit nem return 0; sollte ja das gleiche bewirken oder? Außerdem verwirrt mich noch folgende Stelle:

Code:

assert(prevElement);
assert(!prevElement->next);
prevElement->next = dataElement;
prevElement = dataElement;

Das es abbrechen soll wenn prevElement NULL ist macht Sinn aber prevElement->next ist doch beim ersten Durchlauf immer NULL und es wird erst danach etwas zugewiesen also ist es doch ein garantierter Abbruch oder?

Balou72 · 18. Februar 2017

Dein Speicherzugriffsfehler tritt in Zeile 43 auf

Code:

if ((pCur->pNxt=malloc (sizeof(tName)))==NULL)

Beim ersten mal, wo das Programm hier vorbeikommt ist pCur nämlich noch NULL und somit gibt es beim Aufruf von pCur->pNxt einen Speicherzugriffsfehler. Denk hier nochmal nach, wo Dein Fehler liegt.

Dein zweiter Fehler ist das Benutzen von "daten" zum einlesen der Daten aus der Datei. Wenn Du diese Variable dann Deiner Listenstruktur zuweist ist die bisher aufgebaute Liste wieder weg. Nutze statt "daten" die Variable, der Du eine Zeile zuvor den Speicher mit malloc zugewiesen hast.

Und zu guter letzt musst Du die Liste, die Du in der while Schleife aufgebaut hast, auch wieder in einer Schleife aufräumen. Einfach nur die wenigen free Aufrufe am Ende werden Dir ansonsten Speicherlecks übrig lassen.

Finalspace · 20. Februar 2017

Dome113 schrieb:
OK vielen Dank erstmal für die ausführliche Antwort, ich werde versuchen mir die Tipps einzuprägen.
Ich muss schon mal sagen das dein Speicherverwaltungssystem (für mich als Neuling) ziemlich komplex aussieht deshalb werde ich wohl erstmal bei meinen simplen Methoden bleiben (und hoffen das alles gut geht).

Kannst du gerne tun, war nur nen Tip wie man es auch anderst machen kann und tatsächlich in der Praxis auch funktioniert

Dome113 schrieb:
Jetzt zum 1. Codeabschnitt: Die asserts schließen das Programm wenn die Bedigung nicht erfüllt ist wenn ich das richtig verstanden habe. Sie sind also für meinen Zweck nicht so gut geeignet da diese load() Funktion ja am Anfang meines eigentlichen Programmes steht und danach mit der Liste gearbeitet werden soll. Ich schätze eine if Bedingung mit nem return 0; sollte ja das gleiche bewirken oder? Außerdem verwirrt mich noch folgende Stelle:

Code:

assert(prevElement); assert(!prevElement->next); prevElement->next = dataElement; prevElement = dataElement;

Das es abbrechen soll wenn prevElement NULL ist macht Sinn aber prevElement->next ist doch beim ersten Durchlauf immer NULL und es wird erst danach etwas zugewiesen also ist es doch ein garantierter Abbruch oder?

Validiert die existenz für das vorherige Element:

Code:

assert(prevElement);

Geht davon aus, das das vorherige Element kein nächstes Element hat - wir fügen ja kein Element in der Mitte hinzu, sondern hängen es immer hinten dran:

Code:

assert(!prevElement->next);

Asserts sind dafür gedacht, den State-Fluß der Anwendung zu validieren. Für eine Verkette Liste oder Baum sehr wichtig, weil man damit sehr leicht die Konsistenz sicherstellen kann. Auch sehr sinnvoll um Pointer zu validieren, bevor man drauf zugreift. Achja und Asserts kosten überhaupt nichts, da diese nur im Debug-Build deiner Anwendung ausgeführt werden!

Asserts feuern sofort und brechen deine Anwendung vollständig ab, damit merkst du gleich dass man an der Stelle Probleme mit dem State-Fluß hat.

Auch würde ich empfehlen sollche asserts niemals durch normale Bedingungen zu setzen, weil damit verschleierst du gegenfalls recht miese Fehler - die du entweder gar nicht oder nur sehr spät bemerkst.

Fazit: Asserts um deine Logik bzw. Programmstatus zu validieren.

Suche

C Speicherzugrifffehler bei Versuch Liste aus Textfile zu lesen

Dome113

Cadet 4th Year

aroxx

Lieutenant

Dome113

Cadet 4th Year

Finalspace

Lt. Junior Grade

Dome113

Cadet 4th Year

Balou72

Cadet 2nd Year

Finalspace

Lt. Junior Grade

Ähnliche Themen