C++ Extrem dämliche Fehler - wie vermeiden?

V

VikingGe

Gast
Guten Morgen. Ich muss mal eine Geschichte erzählen.

Wem fällt an diesem Code hier etwas auf:
Code:
      void GUIVertexData::updateDataBuffer(const Vector2D<uint32> blockSize, const Vector2D<uint32> blockCount) {
        
        Array<BlockData> blockData(blockCount.x * blockCount.y);
        
        ...
        
        this->_context->bufferTransferManager()->upload(
          this->_dataBuffer, 0, blockData.size() * sizeof(BlockData),
          Lambda([blockData = std::move(blockData)]
            (void* dst, const size_t size) {
              Functions::copyNT(
                reinterpret_cast<      char*>(dst),
                reinterpret_cast<const char*>(blockData.data()),
                size);
            }));
        
      }
Niemandem? Gut. Ich habe nämlich auch den ganzen Tag dafür gebraucht, herauszufinden, wo der Fehler steckt.


Aber der Reihe nach.

Was soll das Stück Code tun? Nun, es präpariert ein Array und kopiert dessen Inhalt dann asynchron in einen OpenGL-Buffer, um damit ein ansonsten auf der CPU gerendertes Bild auf den Bildschirm zu malen. Das sollte dann ungefähr so aussehen:

Bildschirmfoto397.png

Das tat es bis einschließlich heute morgen auch, aber dann bin ich auf die glorreiche Idee gekommen, das ganze mal mit g++ zu compilieren und zu testen. Normalerweise verwende ich clang zum Entwickeln.
Ergebnis:

Bildschirmfoto396.png

...ups :freak:

Nunja, relativ ratlos habe ich dann erstmal angefangen, alles mit expliziten Memory Barriers vollzuknallen, um festzustellen, dass es wohl nicht daran lag, dass der Compiler irgendwelche Speicherzugriffe durcheinanderoptimiert. Auch eine klassische Race-Condition war schnell ausgeschlossen, die Queue, in die das Funktionsobjekt geschrieben wird, wird ordnungsgemäß gelockt. Und eine Speicherkorruption war es auch nicht, die hätte man spätestens mit valgrind bemerken müssen.

Als nächstes dann probiert, ob es denn mit einer anderen Variante der upload-Methode funktioniert, die zwar im Endeffekt dasselbe macht, vorher aber noch eine Kopie der Daten anlegt. Ja, ging, das Wissen brachte mich aber nicht weiter.

Dann wurde dank printf-Debugging klar, dass die Lambda-Funktion in dem GCC-Build gar nicht ausgeführt wird, die aufrufende Funktion jedoch schon.

Dass die angezeigten Aufrufe allerdings von einer anderen Stelle kamen, habe ich trotz eindeutiger Zahlen auf dem Bildschirm nicht erkannt und und ich dachte, möglicherweise funktioniert meine Klasse für Funktionsobjekte nicht richtig, obwohl diese seit ~9 Monaten existiert und nichts weiter war als ein Mini-Wrapper für std::function. Hab ich also die Klasse neu geschrieben und quasi so eine Art eigenes std::function gebaut - aber auch das löste das Problem nicht. Dafür hat das Projekt jetzt 500 Zeilen Code mehr und einen Todo-Eintrag weniger

Naja. Der upload-Aufruf mündet nach einem kurzen Umweg hier:
Code:
      void BufferTransferManager::upload(const size_t size, UploadFunction&& upload, ExplicitTransferFunction&& execute) {
        
        if (size == 0)
          return;
        
        // bisschen Code
        
      }

Ein Haufen Aufrufe ist tatsächlich an der if-Abfrage gescheitert, obwohl das eigentlich gar nicht hätte passieren dürfen.
Aber immerhin war dadurch sofort die Ursache des Fehlers klar:
Code:
          this->_dataBuffer, 0, blockData.size() * sizeof(BlockData),
          Lambda([blockData = std::move(blockData)]

Joa. Ganz grob das, was passiert:
Code:
clang-Build:
1. blockData.size(), gibt 640 Bytes zurück
2. std::move(blockData)

GCC-Build:
1. std::move(blockData)
2. blockData.size(), ist jetzt natürlich 0

Also klassisches undefiniertes Verhalten, weil der Move Seiteneffekte hat. Warnungen vom Compiler aber in beiden Fällen Fehlanzeige, trotz -Wall.


Ich meine, das hier war längst nicht der erste seltsame und zugleich tierisch nervige Fehler, den ich erlebt habe, aber auf jeden Fall einer der merkwürdigsten. Jetzt ist nur die Frage... wie vermeidet man sowas in Zukunft am besten?
Compiler sagt nichts, cppcheck sagt(e) nichts... irgendwelche Ideen?
 
Zuletzt bearbeitet:
Kann dir zwar Im Moment leider nicht weiterhelfen, aber ich bedanke mich für den wirklich ausführlichen Bericht mit Erklärungen. Das sind Sachen, die mich sicher auch im schlimmsten Fall einige Tage gekostet hätten. Aber gut zu wissen für die Zukunft, wenn ich mal vor einem ähnlichen Problem stehe :)
Wenn ich die Tage mal Zeit haben sollte, kann ich mal versuchen das komplett nachzuvollziehen :D Heut ists zu spät, aber Bookmark ist mal angelegt ^^
 
Aah, es lag also daran, dass du dich auf eine bestimmte Reihenfolge bei der Evaluierung der Ausdrücke in der Argumentenliste eines Funktionsaufrufs verlassen hast. Hmm, ich wüßte aber auch ehrlich nicht, wie ein Compiler dich davor warnen sollte. Da müßte er schon eine verdammt ausführliche Analyse deines Codes durchführen, um zu erkennen, dass das in diesem Fall ein Problem wäre.
 
Eine Methode die du Anwenden kannst ist debug messages in einem terminal ausgeben lassen wenn speicher allokiert wird, freigegeben wird, oder speicher verschoben wird. Dann kann man auch immer mal valgrind oder einen debugger laufen lassen um zu sehen, welche variablen welche Werte haben.
Ich hab auch oft Unit Tests fuer meine Funktionen geschrieben. Die helfen um die Funktion zu testen, da kann man sehen ob es Fehler gibt wie bei dir, oder ob dir dein Programm komplett in die Luft fliegt.
 
P.S. Eins interessiert mich noch. Deine void GUIVertexData::updateDataBuffer-Funktion empfängt ihre Vector2D-Argumente by-const-value statt by-const-reference. Ist das so gewollt? Ich meine, ich würde entweder by-const-reference oder einfach nur by-value, aber eben nicht by-const-value, übergeben. Dem Aufrufer ist es doch eh vollkommen schnuppe, ob eine Funktion ein by-value übergebenes Argument verändert oder nicht.
Ergänzung ()

FreddyMercury schrieb:
Eine Methode die du Anwenden kannst ist debug messages in einem terminal ausgeben lassen wenn speicher allokiert wird, freigegeben wird, oder speicher verschoben wird. Dann kann man auch immer mal valgrind oder einen debugger laufen lassen um zu sehen, welche variablen welche Werte haben.
Ich hab auch oft Unit Tests fuer meine Funktionen geschrieben. Die helfen um die Funktion zu testen, da kann man sehen ob es Fehler gibt wie bei dir, oder ob dir dein Programm komplett in die Luft fliegt.

Debug-Messages hat er doch verwendet, und an Speicherhandling hat es hier nicht gelegen. Und um dieses Problem hier aufzuspüren, hätte ein Unit-Test dermaßen umfangreich sein müssen, dass er an Komplexität das zu testende Modul wahrscheinlich um ein Vielfaches übertreffen würde.
Es gibt eben leider nicht für alle Fälle einen Silberpfeil. Manchmal muß man einfach brutal auf die Nase fallen.
 
Zuletzt bearbeitet:
@antred: Das ist halt immer das Problem. Ein Compiler ist dumm. Er weiß zwar vieles und noch mehr besser, aber er hat keine Intelligenz bzw. kann ein menschliches Hirn nicht verstehen. Egal wie toll man programmiert, im Endeffekt ist es immer im Kopf ein prozeduraler Ablauf den man sich vorstellt und da ist nunmal die Reihenfolge einfach eine logische Vorgehensweise. Wenn der Compiler das dann wegoptimiert ist es in 99,9% der Fälle ja auch richtig so. Aber eigentlich auch falsch, wenn die ursprüngliche Logik dabei verloren geht. Damit mein ich die Logik des Programmierers und der Programmiersprache die es zulässt.
Wieder ein typischer Fall von: Traue keiner Maschine
 
Zuletzt bearbeitet:
rg88 schrieb:
Wieder ein typischer Fall von: Traue keiner Maschine

:) Wobei es in den meisten Fällen eigentlich die Maschine ist, die lieber nicht dem Programmierer vertrauen sollte. ;)

Ich möchte übrigens gar nicht mal ausschließen, dass es Tools gibt, die solche Probleme erkennen können. In meinem letzten Job haben wir Rational Purify (TEUER!) eingesetzt. Das Tool kombiniert statische und dynamische (also zur Laufzeit) Analyseaspekte und kann verdammt viele Fehler aufspüren, die viele andere Tools niemals bemerken, auch wenn es häufig eine Menge false positives (also vermeintliche Problemstellen, die aber gar keine sind) meldet.
 
Ok, bis auf den legendären Intel FDIV-Bug macht die Maschine eigentlich selten echte Fehler :D
Aber der Compiler ist halt auch nur ein Stück Software, das meint ich eigentlich eher damit ;)

und grundsätzlich zu deinem Beitrag antred: FullACK
 
Also wenn ich es richtig verstehe (habe mit dieser Art C++-Code eher selten zu tun):

1) Du übergibst Deiner Upload-Funktion mehrere Parameter.
2) Die Parameter sind in einigen Fällen das Ergebnis weiterer Funktionsaufrufe, wie z.B. Lamba() und blockData.size().
3) Die Parameter hängen voneinander ab.

-> D.h. also ein Teil des Programmsablaufs ist also effektiv im Funktionsaufruf selbst statt in der Abfolge der Codezeilen ?

-> Klar das der Compiler nicht meckert.
Er kann die Absicht des Programmierers niemals kennen, und für ihn ist es eindeutig was der Code alles tun soll.

-> Wenn ja, eine entsprechende CodingRule würde helfen,
z.B. "logischen Ablauf und Übergabe andere Funktionen klar trennen".
Aber sie dann wirklich "durchsetzen" ist schon schwerer,
v.a. weil so etwas wie hier ja auch indirekt passieren kann und nicht nur direkt nebeneinander in einer Codezeile. Das wäre dann schon ein Design-Problem.
Gute Tools sind teuer und werfen ggf. viel mehr FalsePositives als wirkliche Probleme,
was Probleme dann doch wieder verschleiert.


Ich persönlich packe eher ungern Funktionsaufrufe als Parameter in andere Funktionsaufrufe.

- Es erschwert oft die Lesbarkeit (mag jetzt subjektiv sein, es ist halt auch sehr praktisch weil kompakter),
da Parameter zum entscheidenden Zeitpunkt erst noch ermittelt werden.
- Es macht das Debuggen teils etwas nervig:
Reinsteppen in 1. "Parameter", man will ja die Werte Wissen bevor sie übergeben werden;
NextStep;
NextStep...oh, da sind wir jetzt über den eigentlich zu debuggenden Aufruf hinaus.
 
Zuletzt bearbeitet:
antred schrieb:
P.S. Eins interessiert mich noch. Deine void GUIVertexData::updateDataBuffer-Funktion empfängt ihre Vector2D-Argumente by-const-value statt by-const-reference. Ist das so gewollt? [...] Dem Aufrufer ist es doch eh vollkommen schnuppe, ob eine Funktion ein by-value übergebenes Argument verändert oder nicht.
Dem Aufrufer ja, aber der Implementierung selbst nicht. Ich conste bei lokalen Variablen eigentlich alles, was nicht aus gutem Grund verändert werden muss/sollte, was wohl auch schon den ein- oder anderen dämlichen Fehler mit Nervpotential vermieden hat.
Const-Reference ist in diesem Fall unerwünscht, weil die Vector-Typen tatsächlich einfach per XMM-Register übergeben werden.

Ich habs vor ner Weile schon in nem anderen Thread geschrieben: Veränderbarer Zustand ist des Programmierers größter Feind. Der Eingangspost ist das beste Beispiel dafür. :freak:

antred schrieb:
Hmm, ich wüßte aber auch ehrlich nicht, wie ein Compiler dich davor warnen sollte. Da müßte er schon eine verdammt ausführliche Analyse deines Codes durchführen, um zu erkennen, dass das in diesem Fall ein Problem wäre.
Das wäre wohl schon etwas aufwändiger, ja. Allerdings ist dieser Fall ja noch relativ einfach, weil dasselbe Objekt quasi parallel sowohl in const- als auch in non-const-Art und Weise benutzt wird. Das müsste zu erkennen sein und spuckt im schlimmsten Fall mal eine Warnung zu viel aus, aber gewollt kann das eigentlich nie sein.

rg88 schrieb:
und da ist nunmal die Reihenfolge einfach eine logische Vorgehensweise. Wenn der Compiler das dann wegoptimiert ist es in 99,9% der Fälle ja auch richtig so. Aber eigentlich auch falsch, wenn die ursprüngliche Logik dabei verloren geht.
In diesem Fall gibt es ja keine vorgegebene Reihenfolge. Die Parameter-Evaluierung darf der Compiler umordnen wie er will, und nur weil GCC genau das auch tat, habe ich den Fehler überhaupt entdecken können.

FreddyMercury schrieb:
Ich hab auch oft Unit Tests fuer meine Funktionen geschrieben.
Hätte in diesem Fall wahrscheinlich nicht geholfen, weil es mit Clang nunmal zufällig funktioniert hat, aber es dort eben auch immer funktioniert hat.

Andererseits halte ich den Renderer-Code für nicht testbar. Ich will jetzt nicht zu weit ausholen, aber OpenGL selbst ist nicht threadsicher, der Renderer selbst nutzt allerdings einen Thread-Pool, um die wirkliche Arbeit zu erledigen. Sprich: Während der Hauptthread OpenGL-Befehle aus einer Queue liest und ausführt, werden von einem Haufen anderer Threads schonmal Buffer-Updates sowie die Command Queues für den (über)nächsten Frame vorbereitet.

Das ist nebenbei auch der Grund, warum das Array in dem Code oben vor dem upload-Aufruf erzeugt werden muss. Der eigentliche Upload passiert erst später.



Was allerdings zumindest beim Debuggen geholfen hätte, wären strengere Regeln für die upload-Funktion gewesen - statt nem early-out im Falle von size == 0 eine Assertion einbauen und das ganze mit einer Größe von 0 einfach verbieten.
 
Zuletzt bearbeitet:
VikingGe schrieb:
Ich habs vor ner Weile schon in nem anderen Thread geschrieben: Veränderbarer Zustand ist des Programmierers größter Feind. Der Eingangspost ist das beste Beispiel dafür. :freak:

Da bin ich voll bei dir. Eine durchschnittliche Seite Code enthält bei mir meistens ein paar Dutzend consts, aber eine Funktionssignatur mit (für den Aufrufer) überflüssigen Details zu verschmutzen geht mir dann doch zu weit. ;)
 
Zurück
Oben