Brainer: Mere software

man aug 30, 2010 (Rolf)

Den sidste brainer vi havde med med .NET var nok for svær… Erik Haggström får kradset lidt i lakken, men det er det tætteste i kære læsere kom på et svar.

Så vi kommer igen med en lidt nemmere igen fra software hjørnet her.

Hvad er der galt med følgende C kode:

assert(NewDataContainter = (DataContainter *)
  malloc(sizeof (DataContainter)));

Første med et rigtig svar i kommentarfeltet vinder hele hæderen.

Kategori: Brainer
Meta: ping/trackback | RSS 2.0

Kommentarer (6)

  1. Kristian Vang added on 30. august 2010

    Det er svært at svare på uden at kende definitionen af DataContainter og NewDataContainter.
    hvis de er defineret som følgende:
    typedef int DataContainter;
    DataContainter *NewDataContainter;
    Så er der kun den skønhedsmæssige fejl at NewDataContainter burde have været navngivet pNewDataContainter for at vise at det er en pointer.

    hvis NewDataContainter ikke er en pointer:
    DataContainter NewDataContainter;
    så prøver vi at sætte en variable af typen DataContainter = med en pointer til en DataContainter hvilket selvfølgeligt er forkert.

    Med Venlig hilsen

    Kristian Vang

  2. Jørgen Abrahamsen added on 30. august 2010

    assert er en macro, og ikke en funktion. Der bør være et ekstra sæt paranteser om malloc for at være sikker på at macro substitutionen går godt:

    assert( (NewDataContainter = (DataContainter *) malloc(sizeof (DataContainter))) );

  3. Morten Piil added on 30. august 2010

    Man får ikke kørt sin constructor i klassen hvis man allokerer med malloc, man skal bruge new operatoren. New sørger for at objektet bliver oprettet korrekt – tilsvarende skal man bruge delete og ikke free når det skal frigives igen.

  4. Peter Stuge added on 30. august 2010

    Hvis der stod C++ i teksten ville Morten Piil have en god punkt, men nu står der C, og uanset dette er der noget endnu mere galt med koden:

    Før man bruger assert() skal man have #include med i sit program. Hvis man også har #define NDEBUG *før* #include (eller f.x. -DNDEBUG i Makefile) så oversættes hele assert()-linien til at ikke generere nogen kode. I en debug-version af programmet vil det altså køre fint, men når man slår debug fra for at sende en release ud mangler den så (mindst) et malloc()-opkald. Er man heldig vil man få advarsel fra sin compiler når man så bruger NewDataContainter, fordi den ikke er blevet assignet et værdi – men måske var den allerede det tidligere i programmet, da er der ikke nogen advarsel.

    En regel for at undgå dette problem er at aldrig have kode med en side-effect inden i assert(). Den kode her burde altså skrives sådan her i stedet for:

    NewDataContainter = (DataContainter *) malloc(sizeof (DataContainter));
    assert(NewDataContainter);

    Der kan muligvis også være nogle andre fejl, men det er svært at afgøre uden tilgang til mere kode fra det aktuelle scope. Jeg prøver alligevel at foreslå nogle ting som også kunne undersøges lidt mere indgående:

    1. DataContainter burde nok hedde DataContainer. (uden t)

    2. assert() ved malloc()-fejl er i min mening ikke god stil. Det gør at programmet umiddelbart afbrydes hvis en allokering ikke lykkes. Jeg mener at det tilstand burde håndteres bedre af programmet, før evt. afslut, selv om det kun er at give besked om *hvorfor* malloc() ikke lykkedes.

    3. Det fremgår ikke hvad for type DataContainter er, hvilket kan påvirke sizeof() på flere måder. Med udgangspunkt i følgende har vi måske et problem:

    struct dc {
    unsigned char size;
    unsigned char buffer[0];
    };
    typedef struct dc DataContainter;

    En array med 0 elementer er valid C99 og en måde at sige til compilern “jeg ved ikke lige hvor stor den her kommer til at være ved runtime” – den buffer skal blive dynamisk allokeret til den rigtige størrelse senere i programmet. Sådan en array er altid nødt til at komme sidst i en struct, lige fordi det ikke går at vide hvor stor den skal være; altså hvorhen en efterfølgende medlem i structen kan gemmes. sizeof() på sådan en struct udelukker buffer fra størrelsesberegningen af samme grund. Hvis vi regner med at den her struct er byte aligned vil sizeof(DataContainter) altså være 1, og NewDataContainter->buffer vil være nødt til at allokeres separat.

    4. Hvis DataContainter er en pointer til en array, i stedet som i 3. en typedef til en struct, vil sizeof(DataContainter) igen være 1, uanset hvor stor den udpegede array faktisk er.

    1.-4. er kun potentielle fejl. Fejlen som opstår med #define NDEBUG før assert.h i programmet, eller -DNDEBUG (/DNDEBUG i Windows) ved build, er konkret og rigtig alvorlig.

  5. Rolf added on 1. september 2010

    Det er desværre ikke ligetil at skrive > (større-end-tegn) og < (mindre-end-tegn) i kommentarerne – det må man lige gætte sig til. Tricket er at skrive (uden mellemrum) & l t ; eller & g t ; i stedet for de to tegn.

  6. Morten Kristiansen added on 7. oktober 2010

    Der var lidt flere gode svar denne gang. Jørgen Abrahamsen er
    helt klart på rette spor; men prisen må gå til Peter Stuge for hans
    grundige svar. I forbindelse med den manglende include og definition af struct’en vil
    jeg gøre gældende at de var taget ud for at gøre koden overskuelig.

    Den fejl vi havde til hensigt at demonstrere var netop at assert
    udtryk “forsvinder” når NDEBUG er defineret. De mere urutinerede
    programmører har det med at konstatere at compileren har en
    defekt der gør at optimering ikke virker; hvilket jo absolut ikke er
    tilfældet.

    Til trods for det simple eksempel har Peter Stuge formået at
    vride langt flere detaljer ud af koden end først antaget og
    fortjener derfor et par gyldne laurbær. Dog har jeg følgende
    bemærkninger:

    Mht. til Peters punkt 4 vil sizeof(DataContainter) sjældent (læs aldrig)
    være 1; men i stedet være arkitekturafhægig, dvs. 2, 4 eller 8.

    Man skal i øvrigt være overordentlig påpasselig med at anbringe
    udtryk med sideeffekter i makroer, idet man ikke har kontrol over
    hvor mange gange udtrykkene evalueres. I dette tilfælde vil det
    give anledning til en memory leak. Leaks kan i forvejen være nogle
    svære bæster at have med at gøre; men jeg tør godt påstå at i dette
    tilfælde vil den være ekstra svær at finde – med mindre man kender
    sin C indefra og ud. Viden er alt.

    NB: Det er korrekt at der skulle have stået datacontainer og ikke datacontainter. Det var en stavefejl fra min side.

Hvad mener du?