Warnung (=Fehler) bei -O3



  • 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.



  • 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?



  • 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



  • 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.



  • 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.



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


  • Mod

    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.


Log in to reply