GCC (STM32) problem mit Optimierer

[Hugo (Gast)]

Hallo miteinander

Bei meinem Projekt bin ich bisher im Debug-Build mit Optimierung -Og 
(Debug) unterwegs und es funktioniert soweit wie erwartet. Kürzlich 
versuchte ich den Release-Build, bei welchem ich -Os also auf Grösse 
optimieren möchte.
Nun, das Programm funktionierte nicht mehr ganz korrekt. Ich konnte das 
Problem auf eine Funktion eingrenzen.

Meine Feststellung ist, das wenn ich die Laufvariable tIdx als volatile 
deklariere, die Funktion sowohl bei -Og als auch bei -Os korrekt 
funktioniert. Ohne volatile funktioniert sie nur bei -Og.

Aus meiner Sicht braucht es hier aber kein volatile. Wo klemmt es da?

Compiler: gcc version 7.3.1 20180622 (release) [ARM/embedded-7-branch 
revision 261907] (GNU Tools for Arm Embedded Processors 
7-2018-q2-update)

uint16_t TestFunction(const float aFlow)
{
  uint16_t  tRet = 1;
//  uint16_t  tIdx = 0;           // das funktioniert nicht mit Optimierung -Os (size)
  volatile uint16_t  tIdx = 0;  // das funktioniert mit Optimierung -Os (size)

  if (aFlow > 0)
  {
    while ((aFlow >= mVxMapUp[tIdx].Flow) && (tIdx < VX_MAP_ELEMENTS ))
    {
      tRet = mVxMapUp[tIdx].VCmd;
      tIdx++;
    }
  }
  else if (aFlow < 0)
  {
    while ((-aFlow >= mVxMapDn[tIdx].Flow) && (tIdx < VX_MAP_ELEMENTS ))
    {
      tRet = mVxMapDn[tIdx].VCmd;
      tIdx++;
    }
  }
  else
  {
    tRet = 0;
  }

  return tRet;
}

[Niklas G. (erlkoenig)]

Wie sind mVxMapUp, Flow, mVxMapDn und VX_MAP_ELEMENTS definiert? Zeige 
auch den disassemblierten Code der Funktion mit und ohne "volatile".

[foobar (Gast)]

> while ((aFlow >= mVxMapUp[tIdx].Flow) && (tIdx < VX_MAP_ELEMENTS ))

Das kann zu einem Out-of-Bounds-Zugriff führen.  Besser die beiden 
Vergleiche vertauschen - erst Index überprüfen, dann aufs Arrray 
zugreifen.

[M.K. B. (mkbit)]

foobar schrieb:
> while ((aFlow >= mVxMapUp[tIdx].Flow) && (tIdx < VX_MAP_ELEMENTS
> ))
>
> Das kann zu einem Out-of-Bounds-Zugriff führen.  Besser die beiden
> Vergleiche vertauschen - erst Index überprüfen, dann aufs Arrray
> zugreifen.

Das wäre dann Undefined Behavior. Dabei können beim Optimieren "Fehler" 
auftreten, wobei bei Undefined Behavior alles erlaubt ist.

Je nach Größe des Arrays könnte z.B. die Schleife aufgelöst und tIdx 
wegoptimiert werden. Mit volatile verbietest du das.

[Markus F. (mfro)]

ich kann da auf Anhieb - bis auf die Tatsache, dass der direkte 
Vergleich von Integer-Literalen mit floats meist nicht genau das ist, 
was man will - keinen Fehler entdecken.

Wie äussert sich denn das Problem?

Können wir mal die Compiler-Optionen sehen?

[S. R. (svenska)]

M.K. B. schrieb:
>> Das kann zu einem Out-of-Bounds-Zugriff führen.  Besser die beiden
>> Vergleiche vertauschen - erst Index überprüfen, dann aufs Arrray
>> zugreifen.
>
> Das wäre dann Undefined Behavior.

Die Auswertungsreihenfolge logischer Operatoren ist von links nach 
rechts definiert. Wenn also der linke Ausdruck garantiert, dass der 
rechte Ausdruck kein UB mehr auslösen kann, dann ist alles in Ordnung.

Hier ist das umgekehrt. Ich würde daher empfehlen, das mal umzudrehen 
und zu schauen, ob es dann funktioniert. Im Zweifelsfall auch mal einen 
Blick ins Assembly werfen.

[M.K. B. (mkbit)]

Ich habe mich unklar ausgedrückt.
Der Out of bound Zugriff wäre Undefined Behavior. Wenn man es 
vertauchscht, dann ist es natürlich nicht mehr Undefined und sollte wie 
erwartet funktionieren.

[S. R. (svenska)]

Dann sind wir uns ja einig. :-)

[Hugo (Gast)]

Vielen Dank für die zahlreichen Antworten.

Der Fehler äussert sich so, dass der Rückgabewert immer 1 ist. Die 
Schleifen werden also gar nicht ausgeführt.

In der Tat funktioniert es, wenn ich die Abfrage umdrehe: Zuerst den 
Index, dann den anderen Wert.
Ich weiss jetzt nicht so recht, ob mich das beruhigt oder nicht, denn 
das Projekt ist sehr gross...

Ich lege die ergänzenden Daten bei: Deklarationen, Disassembly mit und 
ohne volatile, sowie mit umgedrehter Abfrage.

Die Compileroptionen (includes gekürzt):

-mcpu=cortex-m4 -mthumb -mfloat-abi=hard -mfpu=fpv4-sp-d16 -D__weak=__attribute__((weak)) -DDEBUG -D__packed=__attribute__((__packed__)) -DUSE_HAL_DRIVER -DSTM32F446xx -I../Drivers/CMSIS/Include -Os -g3 -Wall -fmessage-length=0 -ffunction-sections -c -fmessage-length=0

[foobar (Gast)]

Ohne volatile hat er das "tIdx < VX_MAP_ELEMENTS" einfach wegoptimiert, 
nach dem Motto: "er greift ja auf das Array zu, dann muß der Index ja 
stimmen".

[foobar (Gast)]

Btw, wenn ich irgendwo "attribute(packed)" sehe, gehen bei mir die 
Alarmglocken an - das ist so gut wie immer unportabler Pfusch-Code ...

[Nop (Gast)]

Hugo schrieb:

> In der Tat funktioniert es, wenn ich die Abfrage umdrehe: Zuerst den
> Index, dann den anderen Wert.
> Ich weiss jetzt nicht so recht, ob mich das beruhigt oder nicht, denn
> das Projekt ist sehr gross...

Im Gegensatz zu GCC-Warnungen findet CppCheck das und sagt (als 
Style-Warnung):
"Array index tIdx is used before limits check."

Da dieses Tool frei, kostenlos und sehr benutzerfreundlich ist, kannst 
Du damit das Projekt mal eben durchprüfen. Unter Linux direkt aus dem 
Paketmanager, für Windows von hier:

http://cppcheck.sourceforge.net

[Niklas G. (erlkoenig)]

Hugo schrieb:
> Ich weiss jetzt nicht so recht, ob mich das beruhigt oder nicht, denn
> das Projekt ist sehr gross...

Wenn du überall im Code über Array-Grenzen hinweg zugreifst, sollte dich 
das in der Tat beunruhigen, denn das wird früher oder später alle 
möglichen Fehler, gern auch Sicherheitslücken, bewirken.

foobar schrieb:
> Ohne volatile hat er das "tIdx < VX_MAP_ELEMENTS" einfach wegoptimiert,
> nach dem Motto: "er greift ja auf das Array zu, dann muß der Index ja
> stimmen".

Absolut korrekt. Der Compiler darf annehmen, dass Undefined Behaviour 
(UB) niemals auftritt, und da der Zugriff über die Array-Grenze hinweg 
UB ist, nimmt der Compiler an, dass dieser Fall nicht eintritt.

foobar schrieb:
> Btw, wenn ich irgendwo "attribute(packed)" sehe, gehen bei mir die
> Alarmglocken an - das ist so gut wie immer unportabler Pfusch-Code ...

Das wird vermutlich von einer Library gebraucht, ich glaube die HAL 
nutzt das.

[Nop (Gast)]

foobar schrieb:
> Btw, wenn ich irgendwo "attribute(packed)" sehe, gehen bei mir die
> Alarmglocken an - das ist so gut wie immer unportabler Pfusch-Code ...

Ich nutze das manchmal auf Cortex-M, aber mit einem Pack-Alignment von 4 
Bytes, da auf einem 32-Bitter Datentypen wie uint64_t keine 8 Bytes 
Alignment brauchen.

[Hugo (Gast)]

Niklas G schrieb:
> Wenn du überall im Code über Array-Grenzen hinweg zugreifst, sollte dich
> das in der Tat beunruhigen, denn das wird früher oder später alle
> möglichen Fehler, gern auch Sicherheitslücken, bewirken.

Ja da hast du absolut recht: Asche auf mein Haupt...