FastMM Memory Leaks : Lesen und verstehen von Stacktrace

[taveuni]

O.K. Das war ja einfach :roll: Das ist der Verusacher:

Delphi-Quellcode:

procedure TDetectionCore.CopyPictures(PicFrom, PicTo: String);

function CopyFileEx(const ASource, ADest: string; ARenameCheck: boolean = false): boolean;
var
  sh: TSHFileOpStruct;
begin
  // so was hat offensichtlich gefehlt. Das war im Original nicht drin.
{--->} FillChar(sh,SizeOf(sh),0); {<---}
  sh.wFunc := FO_COPY;
  sh.pFrom := PChar(ASource + #0);
  sh.pTo := PChar(ADest + #0);
  sh.fFlags := fof_Silent or fof_MultiDestFiles;
  if ARenameCheck then
    sh.fFlags := FOF_NOCONFIRMATION;
  Result:=ShFileOperation(sh)=0;
end;

begin
  FFileLock.Enter;
  Try
    if ForceDirectories(ExtractFilePath(PicTo)) then
    begin
      if not CopyFileEx(PicFrom, PicTo, True) then
        LogError('could not copy File:%s to:%s',[PicFrom,PicTo]);
    end
    else
      LogError('could not create directory: %s',[ExtractFilePath(PicTo)]);
  Finally
    FFileLock.Release;
  End;
end;

Muss ich das einfach wissen? Vermutlich würde auch ZeroMemory gehen? Diese Struktur ist als Record definiert. Und der Record wird der Funktion ShFileOperation als const übergeben. Üble Sache. Müssen Records grundsätzlich so behandelt werden? Oder nur mit Windows eigenen Funktionen?

Nebenbei: Bevor ich auf das gekommen bin habe ich die Copyfunktion durch das ersetzt:

Delphi-Quellcode:

var
  mem: TMemoryStream;
begin
  FFileLock.Enter;
  Try
    mem:= TMemoryStream.Create;
    if ForceDirectories(ExtractFilePath(PicTo)) then
    begin
      mem.LoadFromFile(PicFrom);
      mem.SaveToFile(PicTo);
    end
  Finally
    mem.Free;
    FFileLock.Release;
  End;

Gibt es da eine Präferenz bezüglich Geschwindigkeit oder anderen Gesichtspunkten das Eine oder das Andere zu nutzen?

Auf jeden Fall: Ich danke Euch vielmals für Eure Hinweise. Besonders nachdem FastMM zufrieden war hätte ich nicht weiter gewusst.
Jetzt kann ich endlich aus dem Memory Loch wieder herauskriechen :-D

Gruss Werner

[Dejan Vu]

Wenn Du einen Record mit Werten belegst, dann musst Du alle Werte explizit belegen, ansonsten steht da doch Müll drin. Sehr unsauber. Das hier ein Speicherleck entsteht, hätte ich nicht gedacht, aber das es überhaupt funktioniert, wundert mich schon.

[DeddyH]

Nebenbei:

Zitat:

Delphi-Quellcode:

sh.fFlags := fof_Silent or fof_MultiDestFiles;
  if ARenameCheck then
    sh.fFlags := FOF_NOCONFIRMATION;

Ist das wirklich so gewollt, dass die gerade gesetzten Flags wieder überschrieben werden? Oder soll FOF_NOCONFIRMATION ggf. hinzugefügt werden? In dem Fall müsste es heißen:

Delphi-Quellcode:

sh.fFlags := sh.fFlags or FOF_NOCONFIRMATION;

[taveuni]

Was ich gerade gesehen habe: Der Record in der ShellApi ist so definiert:

Delphi-Quellcode:

 _SHFILEOPSTRUCTW = record
    Wnd: HWND;
    wFunc: UINT;
    pFrom: PWideChar;
    pTo: PWideChar;
    fFlags: FILEOP_FLAGS;
    fAnyOperationsAborted: BOOL;
    hNameMappings: Pointer;
    lpszProgressTitle: PWideChar;

Also pFrom und pTo sind WideChar. In der Funktion werden die aber als PChar gecastet. Ist das vielleicht nicht so gut?

@DeddyH: Ja logisch - völliger Unsinn.

@Dejan Vu: Keine Ahnung wie Du das meinst. pFrom und pTo sind Variablen eines Records. Was soll man da komplett(er) belegen?

[DeddyH]

In Delphi >= 2009 ist PChar ein PWideChar, davor war es PAnsiChar. In neueren Delphi-Versionen ist es für die Funktionalität also nicht von Bedeutung, allerdings unsauber IMO.

[Sir Rufo]

Es ist völlig ok hier mit

Delphi-Quellcode:

PChar

zu arbeiten, denn es wird ja auch mit

Delphi-Quellcode:

TSHFileOpStruct

und mit

Delphi-Quellcode:

SHFileOperation

gearbeitet und eben nicht mit den expliziten Ansi oder Wide Varianten.

Hier wird je nach Compiler-Version automatisch zwischen der Ansi und Wide Variante gewechselt, genau so, wie auch

Delphi-Quellcode:

PChar

dann in der Bedeutung (

Delphi-Quellcode:

PAnsiChar

vs

Delphi-Quellcode:

PWideChar

) wechselt.

Es ist also nicht unsauber, sondern genau richtig!

[Dejan Vu]

Zitat:

Zitat von taveuni (Beitrag 1272696)

@Dejan Vu: Keine Ahnung wie Du das meinst. pFrom und pTo sind Variablen eines Records. Was soll man da komplett(er) belegen?

Wie wäre es damit, *alle* Felder des Records explizit mit definierten Werten zu belegen? Wo werden denn im Code die Felder Wnd, fAnyOperationsAborted, hNameMappings und lpszProgressTitle gesetzt? Letzteres muss man wohl bei deiner Konstellation nicht belegen, aber unsauber ist es schon, da bei Änderung der Parmeter der Title doch eine Rolle spielen könnte. Nicht in diesem Kontext, aber bei Copy&Paste spielt das schon eine Rolle. Und das das negative Folgen haben kann, einige Werte undefiniert zu lassen, hat man ja wohl gesehen.

Tipp: Da ich eh nicht weiß, wie ein bestimmter Parameterrecord deklariert ist, kopiere ich mir immer die Deklaration in meinen Code und belege dann alle Werte. Das ist viel einfacher, als zu überlegen, was man so alles braucht. Außerdem bin ich so auch gegen Veränderungen gewappnet, da der Programmierer nicht mehr nachschauen muss, welche Felder es denn so gibt.

PS: Mit dem PWideChar wäre ich mir nicht so sicher (ist wohl abhängig -mal wieder- von der Delphiversion). In MSDN sind die Felder als 'LPCTSTR' deklariert, was einem PAnsiChar entspricht, denke ich.

[DeddyH]

@Dejan Vu:

Zitat:

Delphi-Quellcode:

FillChar(sh,SizeOf(sh),0);

Damit wird der komplette Record mit Nullen befüllt, was soll er denn noch machen?

@Sir Rufo: das ist aber nur dann genau richtig, wenn man nicht dynamische Typen/Funktionen mit statischen vermischt, also z.B. ShFileOperation mit dem _SHFILEOPSTRUCTW-Record aufruft.

[taveuni]

Zitat:

Zitat von DeddyH (Beitrag 1272700)

Zitat:

Delphi-Quellcode:

FillChar(sh,SizeOf(sh),0);

Damit wird der komplette Record mit Nullen befüllt, was soll er denn noch machen?

Das habe ich (eigentlich wars nicht ich) aber eben vorher genau nicht gemacht! Und da ist dann das Memory gestiegen. Mit dem FillChar nicht mehr. Meine Frage ist nun: Weshalb muss das gemacht werden? Und: Muss dass in Delphi eigenen Funktionen wo Records (notabene als const) übergeben werden ebenfalls gemacht werden?

[Sir Rufo]

@DeddyH

Ja das sollte schon klar sein und wurde hier ja auch nicht gemacht.

Entweder alles automatisch oder alles fest, dann klappt es auch mit dem Nachbarn.