Bug in Windows (10) gefunden?

[Win10BugFinderLober]

Der wollte doch nur ein bischen Lob, dass er der erste und einzige ist dem bis jetzt dieser unglaubliche Bug in Win10 aufgefallen ist. Und was macht ihr, anstelle ihn zu huligen, gibts Erklärungen zum Verhalten. Und dafür hat er sich nun gestern angemeldet. Echt 'ne Schande so mit einem neuen Forenmitglied umzuspringen.

Also vom mir bekommst du ein dickes Lob, nicht nur für den tollen Bug, sondern auch für dein vorbildliches Benehmen.

[Bonita.M]

Erstens bin ich kein "der", sondern eine "die".
Und zweitens ging es mir darum, eine Lösung zu finden. Die ist mir aber selbst eingefallen (siehe vorangegangener Artikel mit dem Workaround). Alles weitere war unnützes Rauschen.

[EOP]

Bonita.M schrieb:

Erstens bin ich kein "der", sondern eine "die".

Unverschämt und anmaßend bist du so oder so.

[var]

Wieder der beste Beweis das Frauen dumm sind.

[Bonita.M]

var schrieb:

Wieder der beste Beweis das Frauen dumm sind.

Du Affe kannst doch nichtmal richtig Deutsch.
Wenn, dann hieße das ", da*ss* Frauen dumm sind.".
Also wer ist hier wohl dumm?

[Bon Aqua]

@Martin Richter: Einmal schreibst du "Ein AutoReset Event ist kein Lock!" und einmal "Ein AutoReset event ist ein Lock."

[Bonita.M]

dot schrieb:

Aber ist auch sinnlos, hier weiter zu diskutieren, sowohl Windows als auch Standard C++ an sich kommen mit einer fertig implementierten Condition Variable, die nicht nur viel effizienter, sondern vor allem auch korrekt ist...

Condition-Variablen werden in Producer-Consumer-Verhältnissen eingesetzt. Dabei wied das Lock nur ganz kurz gehalten und die Verarbeitung dessen was man aus der Queue geholt hat ist entweder lang weil man CPU-Zeit eines anderen Kerns nutzen wollte, oder weil man einen blockenden Aufruf tätigt. Letztlich kommt es in der Praxis daher nie zu dem Szenario, dass *immer* irgendein Thread auf das innewohnende Mutex wartet, so dass jemand der auf die Signalisierung der CV wartet aushungert. Daher habe ich den Workaround nicht in meinen Code eingebaut.
Ich habe meine CV in Code eingesetzt wo ein Producer und 10.000 Consumer-Threads parallel arbeiten (wobei Windows dann ziemlich in die Knie geht - Windows ist halt scheiße) - die CV arbeitet einwandfrei.
Und einen Tick effizienter als die CV von Win32 ist meine CV auch, vor allem macht sie keine Spurious Wakesups (Stolen Wakeups können aber weiter passieren, das liegt in der Natur der Sache). Und sie ist sehr viel effizienter als die CV der MS C++ Standard Library (keine Ahnung wieso die Standard Library nicht die Win32 CV einsetzt, die viel effizienter ist als die Eigenimplementation der Standard Library).

[Agent]

Erst ein Langzeit-Test über mehrere Jahre wird zeigen, ob deine Implementierung richtig ist.

[Bonita.M]

Agent schrieb:

Erst ein Langzeit-Test über mehrere Jahre wird zeigen, ob deine Implementierung richtig ist.

Was für ein Unsinn.
Keiner entwickelt sowas und lässt das erst ein paar Jahre laufen.
Sowas guckt man sich im Source an und denkt das durch.

[Agent]

Bonita.M schrieb:

Agent schrieb:

Erst ein Langzeit-Test über mehrere Jahre wird zeigen, ob deine Implementierung richtig ist.

Was für ein Unsinn.
Sowas guckt man sich im Source an und denkt das durch.

LOL bist du ein Genie oder was! Bei einem Sourcecode mit solch einer Komplexität kann niemand eine 100%ige Sicherheit in der Funktionsweise garantieren.

[Bonita.M]

Agent schrieb:

LOL bist du ein Genie oder was! Bei einem Sourcecode mit solch einer Komplexität kann niemand eine 100%ige Sicherheit in der Funktionsweise garantieren.

Die Implementation meiner Condvar hat ohne Header eine Länge von 125 Zeilen. Das kann man 100% korrekt implementieren.

[Martin Richter]

Bonita.M schrieb:

Agent schrieb:

LOL bist du ein Genie oder was! Bei einem Sourcecode mit solch einer Komplexität kann niemand eine 100%ige Sicherheit in der Funktionsweise garantieren.

Die Implementation meiner Condvar hat ohne Header eine Länge von 125 Zeilen. Das kann man 100% korrekt implementieren.

Die Windows API hat 0 Zeilen...

[Bonita.M]

Martin Richter schrieb:

Die Windows API hat 0 Zeilen...

Das was ich mit der Windows-API mache ist trivial.

[hustbaer]

Bonita.M schrieb:

Was für ein Unsinn.
Keiner entwickelt sowas und lässt das erst ein paar Jahre laufen.
Sowas guckt man sich im Source an und denkt das durch.

Ja, richtig.
Und man lässt es andere durchgucken.
Deswegen sollte man es auch dokumentieren so dass es für andere schnell verständlich ist. Der Teil fehlt bei deinem Code komplett. Eine Implementierung "reverse engineeren" zu müssen um draufzukommen wie sie funktionieren soll, um dann letztendlich prüfen zu können ob sie eben wirklich funktioniert, ist uninteressant.

[Bonita.M]

hustbaer schrieb:

Deswegen sollte man es auch dokumentieren so dass es für andere schnell verständlich ist. Der Teil fehlt bei deinem Code komplett. Eine Implementierung "reverse engineeren" zu müssen um draufzukommen wie sie funktionieren soll, um dann letztendlich prüfen zu können ob sie eben wirklich funktioniert, ist uninteressant.

Ich hab den Code einen amerikanischen Informatik-Professor geschickt von dem ich einen interessanten Artikel über die Implementation von POSIX Condition-Variablen für Win32 gelesen habe. Er hat den Code verstanden, hatte den Ansatz bisher noch nicht gesehen, und fand ihn interessant. Also: wenn man in der Thematik drinsteckt, also grundsätzlich weiß wie Synchronisationsprimitive arbeiten (das ist Wissen was man sich in der englischsprachigen Wikipedia anlesen kann), dann versteht man das auch. Das Thema ist einfach zu umfangreich, als dass ich die Basics in den Source reinschreiben wollte bzw. könnte; der Source wäre bestimmt viemal so groß. Kennt man die aber, versteht man auch was der Code macht. Das gleiche gilt für das kommende Transactional Memory (was es seit den Haswell-CPUs gibt und seit Syklake auch funktioniert). Wenn man damit arbeitet, dokumentiert man den Code nicht insofern als man die Basics im Umgang damit in den Code reinschreibt. Das ist eine völlig neue Denke die man erstmal kennenlernen muss.
BTW: Das Wartende die via WaitForMultipleObjects warten gegenüber denen die via WaitForSingleObject warten insofern nachrangig behandelt werden, dass sie ggf. nie drankommen, hielt der Professor auch für einen "conceptual flaw".

[hustbaer]

Ich habe nicht behauptet dass man den Code nicht verstehen kann. Ich habe behauptet dass es viel umständlicher ist wenn nix dabei steht.

Ich verstehe wie Synchronisationsprimitive funktionieren. Ich bin mir auch sicher dass ich deinen Code nachvollziehen könnte wenn ich mir die Zeit dafür nehme. Ich bin nur einfach nicht ausreichend motiviert Detektiv zu spielen. Damit meine ich nicht rauszubekommen wie Synchronisationsprimitive wie Events oder Semaphore funktionieren, sondern wie dein Code funktioniert. Bzw. genauer: funktionieren möchte. Wer sagt denn dass du alles bedacht hast, dass da keine Fehler drinnen sind? Und wenn Fehler drinnen sind, wie soll ein Fremder dann verstehen was du machen wolltest wenn du es nicht mit Kommentaren (oder einer externen Doku) beschreibst. Gerade wenn du schreibst dass es ein neuer Ansatz ist sollte klar sein dass man den nicht kennen wird.

z.B. was bedeutet "Owners()" und was "Waiters()". Dass du hier zwei getrennte Variablen in einem int64 ablegst, damit du sie gleichzeitig atomat ansprechen/updaten kannst, ist klar. Aber die Bedeutung der Variablen ist nicht klar.
Wenn ich mir z.B. das ansehe:

void CondVar::Enter()
{
    if( m_dwOwningThreadId == FastGetCurrentThreadId() )
    {
        m_dwRecursionCount++;
        return;
    }

    LONGLONG llOwnersAndWaiters = ::InterlockedIncrement64( &m_llOwnersAndWaiters  );

    assert(Owners( llOwnersAndWaiters ) > Waiters( llOwnersAndWaiters ));
...

Der "recursive lock" Teil ist klar.
Dann machst du mit dem InterlockedIncrement quasi "owners++". Das muss man sich aber schonmal raussuchen (gucken wie Owners() und Waiters() implementiert sind), weil man sonst nicht wissen kann was das InterlockedIncrement verändert. Das könnte schonmal als Kommentar dabeistehen.
Und dann das assert...

Wenn ich es mal umschreibe steht hier sinngemäss inetwa

/* snip recursive lock part */
    m_owners++;
    assert(m_owners > m_waiters);

Das ist erstmal total WTF???
Klar, wenn man dann etwas nachdenkt kommt man drauf dass es nicht mehr Threads geben darf die in Wait() hängen als die bereits erfolgreich Enter() aufgerufen haben. Das kann man aber auch dazuschreiben.

BTW:
Das...

m_dwOwningThreadId   = 0;
    dwSavedRecusionCount = m_dwRecursionCount;
    m_dwRecursionCount   = 0;

ist einfach nur ein Fehler.
Den Fall dass Wait mit dwSavedRecusionCount > 1 aufgerufen wird solltest du einfach nur mit assert() verbieten bzw. ggf. abort() aufrufen. Die Mutex einfach so aufzumachen ist mMn. einfach nur falsch. Grob falsch. Gefährlich falsch.

Dann der Loop hier

if( Owners( llOwnersAndWaiters ) > Waiters( llOwnersAndWaiters ) )
        for( ; !::SetEvent( m_xhEvtEnter.h ); assert(false) );

ist auch mehr als nur fragwürdig.
Wenn SetEvent fehlschlägt kannst du das hier nicht sinnvoll behandeln. Ein assert() ist OK, aber der folgende Retry ist es NICHT.

Ich bin sicher dass es da noch einiges gibt. Aber da du kein Interesse daran zu haben scheibst deinen Code gut lesbar und/oder verstehbar zu machen, hab ich auch kein grosses Interesse daran deinen Code durchzugucken.

[Bonita.M]

[code="cs"]

hustbaer schrieb:

BTW:
Das...

m_dwOwningThreadId   = 0;
    dwSavedRecusionCount = m_dwRecursionCount;
    m_dwRecursionCount   = 0;

ist einfach nur ein Fehler.

Nein, da ist kein Fehler. Du hast es nur nicht verstanden.
In dem Moment in dem ich das tue bin habe ich noch die Datenstruktur gelockt, und Owners ist > Waiters. Daher darf ich zu diesem Zeitpunkt noch m_dwOwningThread und m_dwRecursionCount verändern. Wenn ich dann aber Waiters inkrementiere, dann ist Owners möglicherweise sogar gleich Waiters, und niemand hat die Struktur gelockt, und alle die auf eine Signalisierung warten warten auch auf den Eintritt in den kritischen Abschnitt (deswegen auch die zwei Strukturen, das Semaphor für die Signalisierung, und das Event für den Eintritt in den kritischen Abschnitt).

hustbaer schrieb:

Wenn SetEvent fehlschlägt kannst du das hier nicht sinnvoll behandeln. Ein assert() ist OK, aber der folgende Retry ist es NICHT.

Das ist konzeptuell nicht anders möglich. Das darf *nie* fehlschlagen, sonst ist eine derart simple Implementation nicht möglich. CRITICAL_SECTION wirft undokumentiererweise wenn das fehlschlägt eine SEH-Exception, und das Programm terminiert. Das ist auch eine saubere Lösung.
Eigentlich kostet die mit SetEvent ausgelöste Wahl eines anderen Threads in die Runnable Liste sicher nur minimal Resourcen. Aber wenn die Resourcen so knapp sind, dass auch das fehlschlägt, dann läuft sicher auch nicht mehr der Debugger. Insofern ist das assert eigentlich Quatsch.

[hustbaer]

Bonita.M schrieb:

hustbaer schrieb:

BTW:
Das...

m_dwOwningThreadId   = 0;
    dwSavedRecusionCount = m_dwRecursionCount;
    m_dwRecursionCount   = 0;

ist einfach nur ein Fehler.

Nein, da ist kein Fehler. Du hast es nur nicht verstanden.

Lol
Nein.
Du hast es nicht verstanden.
Wait aufzurufen während man die Mutex doppelt gelockt hat ist ein Anwendungsfehler.
Deine Implementierung erlaubt das explizit, und gibt in dem Fall sämtliche rekursiven Locks frei. Und genau das ist der Fehler.

Nebenbei bemerkt: Dein Unit-Test testet die Wait-Funktion nicht effektiv. Wenn ich den laufen lasse wird Wait nie aufgerufen. Ich musste erst noch ein Sleep(1) vor dem ::cv.Enter(); in ThreadFunc einfügen damit das if (::dqtr.empty()) jemals true wird.

ps: Natürlich ist das dein Code, und du kannst definieren was du willst. Du kannst also auch definieren dass dein cv::Wait dieses verhalten hat. Dann ist es kein Fehler. Dann ist nur deine cv Klasse ein nutzloses Teil von dem man besser die Finger lassen sollte.

Bonita.M schrieb:

hustbaer schrieb:

Wenn SetEvent fehlschlägt kannst du das hier nicht sinnvoll behandeln. Ein assert() ist OK, aber der folgende Retry ist es NICHT.

Das ist konzeptuell nicht anders möglich. Das darf *nie* fehlschlagen, sonst ist eine derart simple Implementation nicht möglich. CRITICAL_SECTION wirft undokumentiererweise wenn das fehlschlägt eine SEH-Exception, und das Programm terminiert. Das ist auch eine saubere Lösung.
Eigentlich kostet die mit SetEvent ausgelöste Wahl eines anderen Threads in die Runnable Liste sicher nur minimal Resourcen. Aber wenn die Resourcen so knapp sind, dass auch das fehlschlägt, dann läuft sicher auch nicht mehr der Debugger. Insofern ist das assert eigentlich Quatsch.

Nein, das assert ist überhaupt nicht Quatsch.
Quatsch ist mMn. die Annahme dass der häufigste Fehlerfall bei SetEvent "zu wenig Resourcen" wäre. Ich wüsste nicht dass das überhaupt passieren kann. Wieso sollte SetEvent irgendwelche Resourcen benötigen?

Real mögliche Fehlerfälle sind aber 1) das übergebene Handle ist ungültig und 2) das übergebene Handle nat nicht die nötigen Berechtigungen.
Speziell (1) kann passieren wenn der Code der deine Mutex/CV Implementierung verwendet Mist baut (bzw. natürlich auch wenn dein Code Fehler enthält). Handle-Werte werden unter Windows recht schnell recycled. D.h. wenn der Anwendercode einen Fehler dieser Art enthält

HANDLE h = ...
CloseHandle(h);

g_something = new cv();

CloseHandle(h); // <-- Anwenderfehler

Dann kann es sein dass das 2. (falsche) CloseHandle deinen Event freigibt. In dem Fall unendlich zu loopen ist Quatsch. Da gehört ein assert+abort hin.

Bzw. natürlich genau so wenn der User das cv Objekt bereits zerstört hat.

Solche Fälle kann man nicht "korrekt" behandeln. Aber man kann helfen die Fehler schneller zu finden. Das assert macht also durchaus Sinn. Bzw. wenn du sicherstellen willst dass ein Programm welches so einen Fehler enthält nicht unkontrolliert weiterläuft, dann wie gesagt ein abort().

Ohne den Error-Code zu checken einfach zu loopen ist aber haarsträubender Quatsch.

[dot]

@hustbaer: du verschwendest hier deine Zeit...

[Bonita.M]

hustbaer schrieb:

Lol
Nein.
Du hast es nicht verstanden.
Wait aufzurufen während man die Mutex doppelt gelockt hat ist ein Anwendungsfehler.

Erstens ist kein doppeltes Locken möglich; es gibt immer nur einen Owner. Und zweitens hat der Thread bis zum InterlockedAdd den Besitz an dem Mutex, und kann bis dahin m_dwRecursionCount und m_dwOwningId ändern.

... Dann ist nur deine cv Klasse ein nutzloses Teil von dem man besser die Finger lassen sollte.

Der Code is 100% fehlerfrei. Ich habe ihn mit einer Queue die von einem Haupt-Thread 1.000 Worker-Threads füttert und einer Queue die von den 1.000 Worker-Threads wieder mit den Ergebnissen an den Hauptthread am laufen gehabt.

Bonita.M schrieb:

Nein, das assert ist überhaupt nicht Quatsch.
Quatsch ist mMn. die Annahme dass der häufigste Fehlerfall bei SetEvent "zu wenig Resourcen" wäre. Ich wüsste nicht dass das überhaupt passieren kann. Wieso sollte SetEvent irgendwelche Resourcen benötigen?

Du spekulierst hier wild rum. Du weißt aber nicht was SetEvent im Kernel alles lostritt.

Real mögliche Fehlerfälle sind aber 1) das übergebene Handle ist ungültig und 2) das übergebene Handle nat nicht die nötigen Berechtigungen.

Kommt bei mir nicht vor.