Threadpool Speicherallokierung schlägt ab 4 fehl

SeppJ

mirrowwinger schrieb:

wie zerstört man sich den Speicher?

Durch wildes Pointergefrickel. Besonders gefährlich, wenn es auch noch mit exzessiven, unnötigen Casts kombiniert wird, die selbst offensichtliche Fehler für den Compiler unsichtbar machen.

mirrowwinger

gibt es eine Methode dies zu testen?

SeppJ

mirrowwinger schrieb:

gibt es eine Methode dies zu testen?

Viele.

Erste Station: Unnötige Casts entfernen, Compilerwarnungen hochschrauben. Warnungen wie Fehler behandeln. Wenn sich herausstellt, dass ein sprachlich unnötiger Cast doch nötig ist, damit das Programm compiliert (oder keine Warnungen mehr wirft), dann ist da wohl ein Fehler
Zweite Station: Programme wie valgrind benutzen, alle Tests. Alle Fehler wie Fehler behandeln
Dritte Station: Gucken, ob das System/Compiler eine Debugruntimeumgebung bietet (zum Beispiel mit Prüfung der Arraygrenzen oder Stack-/Heapschutz) und diese benutzen.
Vierte Station: Debugger nutzen

mirrowwinger

Also ich stell hier mal den Anfang meines Programmes rein. Ich verwende eigentlich keine Cast (ich verstehe darunter Typumwandlungen). Compilieren tut er komplett ohne jegliche Warnungen, deswegen weiß ich nicht, ob das hochstufen Warnungen -> Error wirklich etwas bringt. Stufe 3 kenne ich leider nicht und beim Debuggen habe ich das Problem, dass ich erwarte, dass eventuell malloc fehlschlägt. Dies würde einen Nullpointer zurückgeben und diesen fange ich ab. Er steigt bei mir aber direkt bei malloc aus. Aber erst wenn 4 Kameras dran sind. Bei 3 Kameras ist alles OK!

Ich hoffe jemand sieht einen Fehler. An den Rechnerressourcen sollte es ja nicht liegen oder?

int main(int argc, char* argv[])
{
	pthread_t* ThreadPool;
	char keypress;
	iWorking = 1;

	tdsCamera* cameraDevice = (tdsCamera*)malloc(sizeof(tdsCamera));
	PictureBuffer = (unsigned char**)malloc(sizeof(unsigned char*) * cameraDevice->iCount);

	Camera_init(cameraDevice);

	printf("Camera found:\t%d\n", Camera_Get_Count(cameraDevice));
	int i;
	for (i = 0; i < Camera_Get_Count(cameraDevice); i++)
	{
		printf("%s\n", Camera_Get_Device(cameraDevice, i));
		PictureBuffer[i] = (unsigned char*)malloc(sizeof(unsigned char) * BUFFERSIZE);
	}

	ThreadPool = (pthread_t*)malloc(sizeof(pthread_t) * Camera_Get_Count(cameraDevice));   //<-- hier FEHLER
if (ThreadPool == NULL)    //<-- wird bei 4 Kameras NICHT mehr ausgeführt
	{
		printf("Malloc for thread failed!\n");
		exit(0);
	}

...

Das fehlende Struct und die Kamera-Initialisierung:

typedef struct
{
	int iCount;
	char** pcaCameraDevice;
}tdsCamera;

void Camera_init(tdsCamera *This)
{
	int i;
	int iMissingCameraDevice = 0;
	This->iCount = 0;

	for (i = 0; i <= MAXDEVICE; i++)
	{
		char caDeviceName[256];
		Camera_Str_DeviceName(caDeviceName, i);

		FILE* fCameraDevice = fopen(caDeviceName, "r");

		if (fCameraDevice == NULL)
		{
			if (iMissingCameraDevice >= 3) break;
			iMissingCameraDevice++;
		}
		else
		{
			iMissingCameraDevice = 0;
			if (This->iCount > 0)
			{
				char** pcaTempCameraDevice;
				pcaTempCameraDevice = (char**)malloc(sizeof(char*) * This->iCount);
				int j;
				for (j = 0; j <= This->iCount -1; j++)
				{
					pcaTempCameraDevice[j] = (char*)malloc(sizeof(char*) * 256);
					strcpy(pcaTempCameraDevice[j], This->pcaCameraDevice[j]);
				}
				free(This->pcaCameraDevice);
				This->iCount++;
				This->pcaCameraDevice = (char**)malloc(sizeof(char*) * This->iCount);
				for (j = 0; j <= This->iCount - 1; j++)
				{
					This->pcaCameraDevice[j] = (char*)malloc(sizeof(char*) * 256);
					if (j <= This->iCount -2) 
					{
						strcpy(This->pcaCameraDevice[j], pcaTempCameraDevice[j]);
						free(pcaTempCameraDevice[j]);
					}
					else strcpy(This->pcaCameraDevice[This->iCount -1], caDeviceName);
				}
				free(pcaTempCameraDevice);
			}
			else
			{
				This->iCount++;
				This->pcaCameraDevice = (char**)malloc(sizeof(char*) * This->iCount);
				This->pcaCameraDevice[0] = (char*)malloc(sizeof(char*) * 256);
				strcpy(This->pcaCameraDevice[This->iCount -1], caDeviceName);
			}
			fclose(fCameraDevice);				
		}
	}
}

Wie man sieht nichts wirklich weltbewegendes. Ich denke allen allokierten Speicher gebe ich auch wieder frei bis zu dem Punkt. Also sollte doch eigentlich speichermäßig alles in Ordnung sein.

SeppJ

Du hast bei jedem malloc einen komplett sinnlosen Cast. Die dadurch vertuschten Fehler passen auch zu den von dir beschriebenen Symptomen. Wenn du nicht weißt, was ein Begriff bedeutet, dann schlag ihn nach!

Mindestens in Zeile 8 greifst du zudem auf uninitialisierten Speicher zu. Bloß weil du eine Funktion Init nennst, wird daraus nicht automatisch ein Konstruktor im C++-Sinne.

mirrowwinger

@SeppJ also erstmal danke. Bisher dachte ich diese vorgestellten Casts wären so richtig. Werde diese jetzt entfernen.
Zu dem Thema Wörter nachschauen, habe ich diesen doch als "Typumwandlung" bezeichnet, was ja auch richtig ist (zumindest gemäß Wiki so ist), deswegen nehme ich dies als Hinweis den ich ja glücklicher Weise beachtet habe!
Mit der Zeile 8 hab ich noch ein paar Verständnisprobleme. Die Funktion wurde natürlich in den nicht dargestellten Header-Dateien vorschriftsmäßig deklariert:

#ifndef CAMERA_HEADER
#define CAMERA_HEADER

#include "linuxHeader.h"
#include "v4lHeader.h"
#include "bmp.h"
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <pthread.h>

#define MAXDEVICE 20
#define BUFFERSIZE 357120		// 744 * 480
#define DEVICEPATH "/dev/video"

extern unsigned char** PictureBuffer;
extern int iWorking;

typedef struct
{
	int iCount;
	char** pcaCameraDevice;
}tdsCamera;

typedef struct
{
	int iNumber;
	char* caDeviceName;
}tdsCameraThreadInfo;

void	Camera_init(tdsCamera* This);
void	Camera_free(tdsCamera* This);

void*	Camera_ThreadFunction(void* argv);
void	Camera_Start_Threads(tdsCamera* This, pthread_t* ptThreadPool);
void	Camera_Stop_Threads(tdsCamera* This, pthread_t* ptThreadPool);
int		Camera_Get_Count(tdsCamera* This);
void	Camera_Str_DeviceName(char* DeviceName, int iNumber);
char*	Camera_Get_Device(tdsCamera* This, int iNumber);

#endif

Dies hätte ich vieleicht erwähnen sollen. Ich versuche etwas in die OOP mit dem C zu gehen aber ich sehe hier keinen Verstoß gegen Compiler-Regeln. Aber wenn ich hier doch einen Fehler gemacht habe (siehe auch die unnötigen Casts), dann bitte ich nochmal um einen Tipp.

[Edit1]Also die erwähnten Casts haben keine Änderung am beschriebenen Problem erwirkt. Das Programm kompiliert weiterhin ohne jegliche Warnung einwandfrei und führt bei der Benutzung von 4 Kameras zu dem oben beshriebenen Speicherzugriffsfehler.[/Edit1]

SeppJ

Welchen Wert hat denn cameraDevice->iCount in Zeile 8?

Hast du mal die vielen anderen Tipps befolgt? Nein.

Und noch ein anderes Thema: Warum eigentlich so viel malloc? Du benutzt das auch für allerlei statische Strukturen mit Scope-Lebensdauer, was völlig sinnlos ist. Beispiele: Zeile 7, Zeilen 35,47,57 (zweite Datei). Zudem riechen die scheinbar chaotisch verteilten frees nach dicken Speicherlöchern. Ich kann mir beispielsweise beim besten Willen nicht vorstellen, wie die Zeilen 30-38 nicht riesige Mengen Speicher unfreigegeben lassen.

mirrowwinger

Also der Wert von cameraDevice->iCount in Zeile 8 kann gar keinen Wert haben, da hast du Recht. Da ist ein Reihenfolge-Fehler von mir gemacht worden. Richtig muss das natürlich so sein:

tdsCamera* cameraDevice = malloc(sizeof(tdsCamera));
	Camera_init(cameraDevice);
	PictureBuffer = malloc(sizeof(unsigned char*) * cameraDevice->iCount;

Jetzt hat iCount einen richtigen Wert, der der Anzahl der Kameras entspricht. Danke.

Kannst du mir eventuell die richtige Allocierung für die Zeilen 30-38 mal zeigen? Es scheint ja einen Weg zu geben, bei dem ich so nach dem Moto:

char** Variable[256] = malloc(sizeof(char*) * i); // i = Anzahl von Strings der Länge 256

Speicher allocieren kann.

Gibt es auch eine bessere Methode für den Datenaustausch in Zeile 30-51 wo ich auf die temporäre Variable verzichten kann (was dem Einsparen von mehreren mallocs entspricht)? z.B. eine reallocierung, bei der die Inhalte der vorherigen Einträge erhalten bleibt? (Ich erwarte keine komplette Lösung, sondern z.B. Name des benötigten Befehls, den Rest schaffe ich sicherlich :D).

Deine Methoden habe ich wie ich im vorherigen 2 Posts erwähnt habe, befolgt, bis auf Schritt 3, da ich das nicht kenne. Das mit den überflüssigen Casts, die du noch gefunden hast, war einfach ein bei mir eingefleischtes Fehldenken (was ich natürlich bis zu deinem Hinweis nicht als solchen empfunden habe). Das Debuggen habe ich auch getan, wobei ich mich auf den Rückgabewert der malloc-Funktion (die aber einfach abstürzt) konzentriert. Die Werte für die Variablen waren erwartungsmäßig. Der Pointer für den Bildspeicher wurde nicht überprüft, weil ich ihn zwar schon erstellt habe, aber noch gar nicht verwende. In diesem soll später mal die Daten aus den Kamera-Threads kopiert werden, damit ich diese anzeigen kann. Soweit bin ich aber noch gar nicht. Das hochfahren der Warnungen zu Errors, bringt deswegen nichts, da ich keine Warnungen beim Kompilieren erzeuge. Falls es eine Möglichkeit gibt mehr Warnungen mit dem gcc zu erzeugen, kenne ich sie noch nicht und würde mich auch hier über ein Stichwort oder den entsprechenden Komandozeilenzusatz freuen.

Schlussendlich möchte ich noch sagen, ich bin Maschinenbauer und kein Profiprogrammierer. Ich lerne das hier autodidaktisch und habe erst ca. 1 Jahr Erfahrung. Wenn ich etwas nicht mache, was ich nicht kenne, bitte ich dafür um Entschuldigung aber ich frage ja hier, um zu lernen! Ich möchte dafür kein Mitleid, Verachtung oder sonst irgendeine Gefühlsneigung erzeugen, sondern ich möchte Hilfe manchmal auch Hilfe zur Selbsthilfe.

SeppJ

Wenn du N Arrays von 256 chars allokieren möchtest:

char (*foo)[256] = malloc(N*256);

Konzentriere dich beim Debuggen erst einmal darauf, heraus zu finden was die Ursache des Fehlers ist, nicht was du glaubst, was die Ursache des Fehlers ist. malloc selber wird nicht klanglos abstürzen, es wird höchstens in der Zeile mit dem malloc abstürzen (falls malloc abbricht, dann mit einem Fehler). Speicherzugriffsfehler bedeutet meistens Zugriff über ungültige Zeiger. Camera_Get_Count ist da ein hervorstechender Kandidat. Guck dir den Callstack beim Absturz an, wo genau der Fehler auftritt. Guck dir die Argumente der Funktion an, in der der Absturz auftritt, ob diese sinnvoll besetzt sind. In jedem Frame! Wenn du so die unmittelbare Absturzursache gefunden hast, dann ist der Moment gekommen, mit dem Denken anzufangen, wie es dazu kommen konnte.

Gibt es auch eine bessere Methode für den Datenaustausch in Zeile 30-51 wo ich auf die temporäre Variable verzichten kann (was dem Einsparen von mehreren mallocs entspricht)? z.B. eine reallocierung, bei der die Inhalte der vorherigen Einträge erhalten bleibt? (Ich erwarte keine komplette Lösung, sondern z.B. Name des benötigten Befehls, den Rest schaffe ich sicherlich :D).

Sehr sicher geht das besser, aber ich verstehe nicht, was der Code überhaupt erreichen soll:
Erst wird der Inhalt der This->pcaCameraDevice in temporäre Variablen kopiert.
Dann wird This->pcaCameraDevice freigegeben (und zwar höchstwahrscheinlich unvollständig).
Dann wird This->pcaCameraDevice neu allokiert.
Dann wird alles zurück kopiert, aber nur wenn j <= This->iCount -2 (was für Werte bleiben da in anderen Fällen stehen? Uninitialisiert!). Ansonsten wird caDeviceName an die Stelle This->pcaCameraDevice[This->iCount -1] (wo ist da der Zusammenhang mit j?) kopiert.

mirrowwinger

Also das mit den Arrays setze ich gleich mal um. Die funktion Camera_Get_Count ist eigentlich sehr harmlos:

typedef struct
{
	int iCount;
	char** pcaCameraDevice;
}tdsCamera;

int Camera_Get_Count(tdsCamera *This)
{
	return This->iCount;
}

Keine weiteren mallocs vorhanden. Der Wert wird in der oben gezeigten Camera_init-Funktion erzeugt, indem er immer inkrementiert wird, wenn die Datei /dev/videoX mit fopen einen gültigen Pointer erzeugt.

Das umspeichern von This->pcaCameraDevice in eine temporäre Variable hat den Sinn, dass ich ja vorher nicht weiß wie oft ich einen gültigen Pointer für /dev/video erhalte. Die temporäre Variable speichert alle gefundenen Werte, damit sie nicht verloren gehen und danach wird This->pcaCameraDevice um einen Speicherplatz erweitert (frei geben und neu allokieren mit +1 Plätzen im Array). Danach werden alle in der temporären Variable gespeicherten Werte wieder in This->pcaCameraDevice übertragen (hier kommt auch das This->iCount -2) wobei 1 abgezogen werden muss wegen nullbasiertem Array und 1 weil This->iCount schon um +1 erweitert wurde. Die temporäre Variable hat also zum Zeitpunkt des Zurückschreibens This->iCount -2 Einträge. Wie gesagt, wenn du eine bessere Variante kennst für den Erweiterungsprozess von This->pcaCameraDevice ohne Verlust der eingetragenen Daten, würde ich mich freuen, wenn du ihn schematisch erklären könntest oder mir zumindest ein Stichwort für die nötigen Befehle nennen könntest.

[Edit1]Wäre zum Beispiel realloc eine Möglichkeit?

void Camera_init(tdsCamera *This)
{
	int i;
	int iMissingCameraDevice = 0;
	This->iCount = 0;

	for (i = 0; i <= MAXDEVICE; i++)
	{
		char caDeviceName[256];
		Camera_Str_DeviceName(caDeviceName, i);

		FILE* fCameraDevice = fopen(caDeviceName, "r");

		if (fCameraDevice == NULL)
		{
			if (iMissingCameraDevice >= 3) break;
			iMissingCameraDevice++;
		}
		else
		{
			iMissingCameraDevice = 0;
			This->iCount++;
			char (*pcaTempCameraDevice)[256] = realloc(This->pcaCameraDevice, Camera_Get_Count(This) * 256 * sizeof(char*));
			if(pcaTempCameraDevice == NULL)
			{
				// realloc failed -> Exit
				printf("Error:\tRealloc in function Camera_init failed!\n");
				exit(1);
			}
			else
			{
				This->pcaCameraDevice = pcaTempCameraDevice;
				strncpy(This->pcaCameraDevice[This->iCount - 1], caDeviceName, 256);
			}
		}
        fclose(fCameraDevice);				
	}
}

Mit dieser Änderung kann ich jetzt bis 6 Kameras betreiben ohne Fehler oder Warnungen zu erzeugen. Mit dem Hinweis zum Debugger (DDD) habe ich einen potentiellen Speicherzugriffsfehler noch entschärft (war vieleicht auch zwischenzeitlich der entsprechende Übeltäter:

if (fCameraDevice != NULL) fclose(fCameraDevice);

Ohne die if-Abfrage wird bei Fehlschlagen des fopen (Pointer = NULL) versucht die Datei wieder zu schließen. Dies führt zu einem Speicherzugriffsfehler.
Falls du noch Hinweise für besser Strukturierung, Programmierung, Verbesserungen an meinem Quellcode hast, würde ich mich natürlich weiterhin über jeden Hinweis freuen.
[/Edit1]
[Edit2]Du kritisierst, dass die free()-Funktion den Speicher nicht zuverlässig in meinem Quellcode frei gibt. Da ich diese Funktion nicht geschrieben habe und somit deren Verhalten nicht kontrolliere, was ist eine bessere Variante den Speicher wieder frei zu geben, wo ich mir sicher bin, dass er auch wirklich frei ist?[/Edit2]