Warnung (=Fehler) bei -O3

Erhard Henkes

uint16_t udptcpCalculateChecksum(void* p, uint16_t length, IP_t srcIP, IP_t destIP, uint16_t protocol)
{
    updtcpPseudoHeader_t pseudo;
    pseudo.src.iIP = srcIP.iIP;
    pseudo.dest.iIP = destIP.iIP;
    pseudo.length = htons(length);
    pseudo.prot = protocol;
    pseudo.res = 0;

    uint32_t pseudoHeaderChecksum = 0;
    uint8_t* data = (uint8_t*)&pseudo;

    for(uint32_t i = 0; i < sizeof(updtcpPseudoHeader_t); i+=2)
    {
        // pseudo header contains 6 WORD
        pseudoHeaderChecksum += (data[i] << 8) | data[i+1]; // Big Endian
    }

    return internetChecksum(p, length, pseudoHeaderChecksum);
}

Dabei tritt in der Zeile

pseudoHeaderChecksum += (data[i] << | data[i+1];

die Warnung: "array subscript is above array bounds" auf.

Was läuft da bis -O2 korrekt, aber ab -O3 nicht mehr? Wie kann ich den Code auf -O3 Niveau bringen.

Falls das noch wichtig ist:

typedef struct
{
    IP_t src;
    IP_t dest;
    uint8_t res;
    uint8_t prot;
    uint16_t length;
} __attribute__((packed)) updtcpPseudoHeader_t;

und

typedef union
{
    uint8_t IP[4];
    uint32_t iIP;
} __attribute__((packed)) IP_t;

Das is kein Sprachfehler sondern nen fehlerhafter Compiler

Als kleine Anmerkung: GIDF

Nehmen wir mal an, das sizeof(updtcpPseudoHeader_t) ergibt 12.
Dann läuft deine Schleife von 0 bis 11.
Soweit in Ordnung.
Dann greifst du aber auf das Element i+1 zu. Das ist bei i==11 dann 12.
Dieses Element existiert aber nicht.

Du greifst auf ein Arrayelement über der Arraygrenze zu.

cooky451

DirkB schrieb:

Dann greifst du aber auf das Element i+1 zu. Das ist bei i==11 dann 12.

Die Schleife kann aber nur mit max i = 10 durchlaufen werden.

cooky451 schrieb:

[Die Schleife kann aber nur mit max i = 10 durchlaufen werden.

Ups.
Vielleicht ist der Compiler an der Stelle auch so kurzsichtig wie ich.

Kannst du an der Stelle mit ntohs() oder htons() arbeiten?

Wutz

Mann mann, so kannst du das nicht machen. Solche brutalen 8/16 Bit Durchläufe und dann auch noch über Strukturinhalte sind schlichtweg schlecht, unportabel, fehleranfällig, schwer wart/debugbar, ...
Sei froh, dass dich der Compiler auf diesen Mist hinweist (auch wenn zufällig und in anderem Zusammenhang).
Du bist doch mit dem Quasi-Standard htons schon auf dem richtigen Weg, außerdem weisen explizite Strukturalignments meist auf suboptimale Designs hin oder anders ausgedrückt: ist ein Programm auf Strukturalignment angewiesen, ist dies schon eine Abhängigkeit zuviel, kümmere dich erstmal um deinen fehlerfreien fachlichen Inhalt, technisch optimieren und irgendwo irgendwelche Bits einsparen kannst du anschließend immer noch.
Aber wenigstens liest du Compilerwarnungen .

updtcpPseudoHeader_t pseudo = { srcIP, destIP, 0, protocol, htons(length) };

sollte wohl auch einfacher als dein Originalcode sein

uint16_t udptcpCalculateChecksum(void* p, uint16_t length, IP_t srcIP, IP_t destIP, uint16_t protocol)
{
    updtcpPseudoHeader_t pseudo = { srcIP, destIP, 0, protocol, htons(length) };
    uint32_t pseudoHeaderChecksum = 0;

    for(uint32_t i = 0; i < sizeof(updtcpPseudoHeader_t); i+=2)
    {
        pseudoHeaderChecksum += htons(((uint16_t*)&pseudo)[i]);
    }

    return internetChecksum(p, length, pseudoHeaderChecksum);
}

So besser? Läuft zumindest durch -O3

Wutz

Bei ungeradem sizeof(updtcpPseudoHeader_t) gibt das auch wieder undefinertes Verhalten.
Lagere die Funktionalität in eine eigene Funktion aus und kümmere dich dort auch um nichtgerade Größen:

uint32_t calcPseudoChecksum(const unsigned char *b,size_t s) {...}

Auch ist die Berechnung einer uint32_t Prüfsumme fragwürdig, da anschließend nur ein uint16_t weiterverarbeitet wird.

Bashar

Wutz schrieb:

außerdem weisen explizite Strukturalignments meist auf suboptimale Designs hin oder anders ausgedrückt: ist ein Programm auf Strukturalignment angewiesen, ist dies schon eine Abhängigkeit zuviel, kümmere dich erstmal um deinen fehlerfreien fachlichen Inhalt, technisch optimieren und irgendwo irgendwelche Bits einsparen kannst du anschließend immer noch.
[...]
Bei ungeradem sizeof(updtcpPseudoHeader_t) gibt das auch wieder undefinertes Verhalten.

Erzähl das doch denen, die die TCP- und UDP-Protokolle erfunden haben.

Auch ist die Berechnung einer uint32_t Prüfsumme fragwürdig, da anschließend nur ein uint16_t weiterverarbeitet wird.

Sorry, aber korrekte Checksum ergibt sich nur mit uint32_t, internetChecksum(...), das beim Protokoll IP verwendet wird, gibt dann die korrekte 16-bit-Checksum zurück.

Der Pseudoheader ist zur Zeit gerade, nämlich 12 Byte, wird auch vermutlich so bleiben.

Wutz

Und beim Umstieg auf IPv6 fängst du dann wieder an, am Code rumzumachen.

camper

Ich würde es mal mit

union
{
    updtcpPseudoHeader_t header;
    uint8_t data[sizeof(updtcpPseudoHeader_t);
} pseudo = { ...

versuchen.

pseudoHeaderChecksum += (data[i] << 8u) | data[i+1]; // Big Endian

damit kein vorzeichenbehaftetes shift ausgeführt wird (wäre allerdings nur bei 16bit-integern ein Problem).

Vielleicht genügt es ja auch die Schleifenbedingung als

i < sizeof(...)-1

zu formulieren um den Compiler zufrieden zustellen.