Category Archives: C Programming

Make C Pointers Great Again

Recently at my day job we stumbled upon a nasty compiler bug dealing with arrays of structures. On the PIC24 compiler we use, something as simple as this produced problematic code:

typedef struct
{
   char        name[100];
   int         value;
} ObjectStruct;

#define NUM_OBJECTS 2

// Globals
ObjectStruct objects[NUM_OBJECTS];

// Prototypes
void ShowObject (ObjectStruct object);

void main()
{
   int idx;
   
   for (idx = 0; idx < NUM_OBJECTS; idx++)
   {
      objects[idx].value = (100 + idx);
   }
   
   ShowObject (objects[0]);
   
   int n = 0;
   ShowObject (objects[n]);
}

void ShowObject (ObjectStruct object)
{
   printf ("object.value = %d\r\n", object.value);
}

The compiler appeared to generate reasonable code when accessing the structure element by constant “[0]”:

……………….. ShowObject (objects[0]);
*
0044C: MOV #800,W0
0044E: MOV #8D2,W1
00450: REPEAT #65
00452: MOV [W0++],[W1++]
00454: CALL 39A
………………..

…but when accessing it by variable “[n]” the code got ludicrous (prepare to scroll):

………………..
……………….. int n = 0;
*
00458: CLR 8CE
……………….. ShowObject (objects[n]);
0045A: MOV 8CE,W4
0045C: MOV #66,W3
0045E: MUL.SS W4,W3,W0
00460: MOV #800,W4
00462: ADD W0,W4,W0
00464: MOV #A,W4
00466: REPEAT #32
00468: MOV [W0++],[W4++]
0046A: MOV W5,8D2
0046C: MOV W6,8D4
0046E: MOV W7,8D6
00470: MOV W8,8D8
00472: MOV W9,8DA
00474: MOV W10,8DC
00476: MOV W11,8DE
00478: MOV W12,8E0
0047A: MOV W13,8E2
0047C: MOV W14,8E4
0047E: MOV W15,8E6
00480: MOV W0,8E8
00482: MOV W1,8EA
00484: MOV W2,8EC
00486: MOV W3,8EE
00488: MOV W4,8F0
0048A: MOV W5,8F2
0048C: MOV W6,8F4
0048E: MOV W7,8F6
00490: MOV W8,8F8
00492: MOV W9,8FA
00494: MOV W10,8FC
00496: MOV W11,8FE
00498: MOV W12,900
0049A: MOV W13,902
0049C: MOV W14,904
0049E: MOV W15,906
004A0: MOV W0,908
004A2: MOV W1,90A
004A4: MOV W2,90C
004A6: MOV W3,90E
004A8: MOV W4,910
004AA: MOV W5,912
004AC: MOV W6,914
004AE: MOV W7,916
004B0: MOV W8,918
004B2: MOV W9,91A
004B4: MOV W10,91C
004B6: MOV W11,91E
004B8: MOV W12,920
004BA: MOV W13,922
004BC: MOV W14,924
004BE: MOV W15,926
004C0: MOV W0,928
004C2: MOV W1,92A
004C4: MOV W2,92C
004C6: MOV W3,92E
004C8: MOV W4,930
004CA: MOV W5,932
004CC: MOV W6,934
004CE: MOV W7,936
004D0: MOV W6,8D4
004D2: MOV W7,8D6
004D4: MOV W8,8D8
004D6: MOV W9,8DA
004D8: MOV W10,8DC
004DA: MOV W11,8DE
004DC: MOV W12,8E0
004DE: MOV W13,8E2
004E0: MOV W14,8E4
004E2: MOV W15,8E6
004E4: MOV W0,8E8
004E6: MOV W1,8EA
004E8: MOV W2,8EC
004EA: MOV W3,8EE
004EC: MOV W4,8F0
004EE: MOV W5,8F2
004F0: MOV W6,8F4
004F2: MOV W7,8F6
004F4: MOV W8,8F8
004F6: MOV W9,8FA
004F8: MOV W10,8FC
004FA: MOV W11,8FE
004FC: MOV W12,900
004FE: MOV W13,902
00500: MOV W14,904
00502: MOV W15,906
00504: MOV W0,908
00506: MOV W1,90A
00508: MOV W2,90C
0050A: MOV W3,90E
0050C: MOV W4,910
0050E: MOV W5,912
00510: MOV W6,914
00512: MOV W7,916
00514: MOV W8,918
00516: MOV W9,91A
00518: MOV W10,91C
0051A: MOV W11,91E
0051C: MOV W12,920
0051E: MOV W13,922
00520: MOV W14,924
00522: MOV W15,926
00524: MOV W0,928
00526: MOV W1,92A
00528: MOV W2,92C
0052A: MOV W3,92E
0052C: MOV W4,930
0052E: MOV W5,932
00530: MOV W6,934
00532: MOV W7,936
00534: MOV W8,938
00536: MOV W7,8D6
00538: MOV W8,8D8
0053A: MOV W9,8DA
0053C: MOV W10,8DC
0053E: MOV W11,8DE
00540: MOV W12,8E0
00542: MOV W13,8E2
00544: MOV W14,8E4
00546: MOV W15,8E6
00548: MOV W0,8E8
0054A: MOV W1,8EA
0054C: MOV W2,8EC
0054E: MOV W3,8EE
00550: MOV W4,8F0
00552: MOV W5,8F2
00554: MOV W6,8F4
00556: MOV W7,8F6
00558: MOV W8,8F8
0055A: MOV W9,8FA
0055C: MOV W10,8FC
0055E: MOV W11,8FE
00560: MOV W12,900
00562: MOV W13,902
00564: MOV W14,904
00566: MOV W15,906
00568: MOV W0,908
0056A: MOV W1,90A
0056C: MOV W2,90C
0056E: MOV W3,90E
00570: MOV W4,910
00572: MOV W5,912
00574: MOV W6,914
00576: MOV W7,916
00578: MOV W8,918
0057A: MOV W9,91A
0057C: MOV W10,91C
0057E: MOV W11,91E
00580: MOV W12,920
00582: MOV W13,922
00584: MOV W14,924
00586: MOV W15,926
00588: MOV W0,928
0058A: MOV W1,92A
0058C: MOV W2,92C
0058E: MOV W3,92E
00590: MOV W4,930
00592: MOV W5,932
00594: MOV W6,934
00596: MOV W7,936
00598: MOV W8,938
0059A: MOV W9,93A
0059C: MOV W8,8D8
0059E: MOV W9,8DA
005A0: MOV W10,8DC
005A2: MOV W11,8DE
005A4: MOV W12,8E0
005A6: MOV W13,8E2
005A8: MOV W14,8E4
005AA: MOV W15,8E6
005AC: MOV W0,8E8
005AE: MOV W1,8EA
005B0: MOV W2,8EC
005B2: MOV W3,8EE
005B4: MOV W4,8F0
005B6: MOV W5,8F2
005B8: MOV W6,8F4
005BA: MOV W7,8F6
005BC: MOV W8,8F8
005BE: MOV W9,8FA
005C0: MOV W10,8FC
005C2: MOV W11,8FE
005C4: MOV W12,900
005C6: MOV W13,902
005C8: MOV W14,904
005CA: MOV W15,906
005CC: MOV W0,908
005CE: MOV W1,90A
005D0: MOV W2,90C
005D2: MOV W3,90E
005D4: MOV W4,910
005D6: MOV W5,912
005D8: MOV W6,914
005DA: MOV W7,916
005DC: MOV W8,918
005DE: MOV W9,91A
005E0: MOV W10,91C
005E2: MOV W11,91E
005E4: MOV W12,920
005E6: MOV W13,922
005E8: MOV W14,924
005EA: MOV W15,926
005EC: MOV W0,928
005EE: MOV W1,92A
005F0: MOV W2,92C
005F2: MOV W3,92E
005F4: MOV W4,930
005F6: MOV W5,932
005F8: MOV W6,934
005FA: MOV W7,936
005FC: MOV W8,938
005FE: MOV W9,93A
00600: MOV W10,93C
00602: MOV W9,8DA
00604: MOV W10,8DC
00606: MOV W11,8DE
00608: MOV W12,8E0
0060A: MOV W13,8E2
0060C: MOV W14,8E4
0060E: MOV W15,8E6
00610: MOV W0,8E8
00612: MOV W1,8EA
00614: MOV W2,8EC
00616: MOV W3,8EE
00618: MOV W4,8F0
0061A: MOV W5,8F2
0061C: MOV W6,8F4
0061E: MOV W7,8F6
00620: MOV W8,8F8
00622: MOV W9,8FA
00624: MOV W10,8FC
00626: MOV W11,8FE
00628: MOV W12,900
0062A: MOV W13,902
0062C: MOV W14,904
0062E: MOV W15,906
00630: MOV W0,908
00632: MOV W1,90A
00634: MOV W2,90C
00636: MOV W3,90E
00638: MOV W4,910
0063A: MOV W5,912
0063C: MOV W6,914
0063E: MOV W7,916
00640: MOV W8,918
00642: MOV W9,91A
00644: MOV W10,91C
00646: MOV W11,91E
00648: MOV W12,920
0064A: MOV W13,922
0064C: MOV W14,924
0064E: MOV W15,926
00650: MOV W0,928
00652: MOV W1,92A
00654: MOV W2,92C
00656: MOV W3,92E
00658: MOV W4,930
0065A: MOV W5,932
0065C: MOV W6,934
0065E: MOV W7,936
00660: MOV W8,938
00662: MOV W9,93A
00664: MOV W10,93C
00666: MOV W11,93E
00668: MOV W10,8DC
0066A: MOV W11,8DE
0066C: MOV W12,8E0
0066E: MOV W13,8E2
00670: MOV W14,8E4
00672: MOV W15,8E6
00674: MOV W0,8E8
00676: MOV W1,8EA
00678: MOV W2,8EC
0067A: MOV W3,8EE
0067C: MOV W4,8F0
0067E: MOV W5,8F2
00680: MOV W6,8F4
00682: MOV W7,8F6
00684: MOV W8,8F8
00686: MOV W9,8FA
00688: MOV W10,8FC
0068A: MOV W11,8FE
0068C: MOV W12,900
0068E: MOV W13,902
00690: MOV W14,904
00692: MOV W15,906
00694: MOV W0,908
00696: MOV W1,90A
00698: MOV W2,90C
0069A: MOV W3,90E
0069C: MOV W4,910
0069E: MOV W5,912
006A0: MOV W6,914
006A2: MOV W7,916
006A4: MOV W8,918
006A6: MOV W9,91A
006A8: MOV W10,91C
006AA: MOV W11,91E
006AC: MOV W12,920
006AE: MOV W13,922
006B0: MOV W14,924
006B2: MOV W15,926
006B4: MOV W0,928
006B6: MOV W1,92A
006B8: MOV W2,92C
006BA: MOV W3,92E
006BC: MOV W4,930
006BE: MOV W5,932
006C0: MOV W6,934
006C2: MOV W7,936
006C4: MOV W8,938
006C6: MOV W9,93A
006C8: MOV W10,93C
006CA: MOV W11,93E
006CC: MOV W12,940
006CE: MOV W11,8DE
006D0: MOV W12,8E0
006D2: MOV W13,8E2
006D4: MOV W14,8E4
006D6: MOV W15,8E6
006D8: MOV W0,8E8
006DA: MOV W1,8EA
006DC: MOV W2,8EC
006DE: MOV W3,8EE
006E0: MOV W4,8F0
006E2: MOV W5,8F2
006E4: MOV W6,8F4
006E6: MOV W7,8F6
006E8: MOV W8,8F8
006EA: MOV W9,8FA
006EC: MOV W10,8FC
006EE: MOV W11,8FE
006F0: MOV W12,900
006F2: MOV W13,902
006F4: MOV W14,904
006F6: MOV W15,906
006F8: MOV W0,908
006FA: MOV W1,90A
006FC: MOV W2,90C
006FE: MOV W3,90E
00700: MOV W4,910
00702: MOV W5,912
00704: MOV W6,914
00706: MOV W7,916
00708: MOV W8,918
0070A: MOV W9,91A
0070C: MOV W10,91C
0070E: MOV W11,91E
00710: MOV W12,920
00712: MOV W13,922
00714: MOV W14,924
00716: MOV W15,926
00718: MOV W0,928
0071A: MOV W1,92A
0071C: MOV W2,92C
0071E: MOV W3,92E
00720: MOV W4,930
00722: MOV W5,932
00724: MOV W6,934
00726: MOV W7,936
00728: MOV W8,938
0072A: MOV W9,93A
0072C: MOV W10,93C
0072E: MOV W11,93E
00730: MOV W12,940
00732: MOV W13,942
00734: MOV W12,8E0
00736: MOV W13,8E2
00738: MOV W14,8E4
0073A: MOV W15,8E6
0073C: MOV W0,8E8
0073E: MOV W1,8EA
00740: MOV W2,8EC
00742: MOV W3,8EE
00744: MOV W4,8F0
00746: MOV W5,8F2
00748: MOV W6,8F4
0074A: MOV W7,8F6
0074C: MOV W8,8F8
0074E: MOV W9,8FA
00750: MOV W10,8FC
00752: MOV W11,8FE
00754: MOV W12,900
00756: MOV W13,902
00758: MOV W14,904
0075A: MOV W15,906
0075C: MOV W0,908
0075E: MOV W1,90A
00760: MOV W2,90C
00762: MOV W3,90E
00764: MOV W4,910
00766: MOV W5,912
00768: MOV W6,914
0076A: MOV W7,916
0076C: MOV W8,918
0076E: MOV W9,91A
00770: MOV W10,91C
00772: MOV W11,91E
00774: MOV W12,920
00776: MOV W13,922
00778: MOV W14,924
0077A: MOV W15,926
0077C: MOV W0,928
0077E: MOV W1,92A
00780: MOV W2,92C
00782: MOV W3,92E
00784: MOV W4,930
00786: MOV W5,932
00788: MOV W6,934
0078A: MOV W7,936
0078C: MOV W8,938
0078E: MOV W9,93A
00790: MOV W10,93C
00792: MOV W11,93E
00794: MOV W12,940
00796: MOV W13,942
00798: MOV W14,944
0079A: MOV W13,8E2
0079C: MOV W14,8E4
0079E: MOV W15,8E6
007A0: MOV W0,8E8
007A2: MOV W1,8EA
007A4: MOV W2,8EC
007A6: MOV W3,8EE
007A8: MOV W4,8F0
007AA: MOV W5,8F2
007AC: MOV W6,8F4
007AE: MOV W7,8F6
007B0: MOV W8,8F8
007B2: MOV W9,8FA
007B4: MOV W10,8FC
007B6: MOV W11,8FE
007B8: MOV W12,900
007BA: MOV W13,902
007BC: MOV W14,904
007BE: MOV W15,906
007C0: MOV W0,908
007C2: MOV W1,90A
007C4: MOV W2,90C
007C6: MOV W3,90E
007C8: MOV W4,910
007CA: MOV W5,912
007CC: MOV W6,914
007CE: MOV W7,916
007D0: MOV W8,918
007D2: MOV W9,91A
007D4: MOV W10,91C
007D6: MOV W11,91E
007D8: MOV W12,920
007DA: MOV W13,922
007DC: MOV W14,924
007DE: MOV W15,926
007E0: MOV W0,928
007E2: MOV W1,92A
007E4: MOV W2,92C
007E6: MOV W3,92E
007E8: MOV W4,930
007EA: MOV W5,932
007EC: MOV W6,934
007EE: MOV W7,936
007F0: MOV W8,938
007F2: MOV W9,93A
007F4: MOV W10,93C
007F6: MOV W11,93E
007F8: MOV W12,940
007FA: MOV W13,942
007FC: MOV W14,944
007FE: MOV W15,946
00800: MOV W14,8E4
00802: MOV W15,8E6
00804: MOV W0,8E8
00806: MOV W1,8EA
00808: MOV W2,8EC
0080A: MOV W3,8EE
0080C: MOV W4,8F0
0080E: MOV W5,8F2
00810: MOV W6,8F4
00812: MOV W7,8F6
00814: MOV W8,8F8
00816: MOV W9,8FA
00818: MOV W10,8FC
0081A: MOV W11,8FE
0081C: MOV W12,900
0081E: MOV W13,902
00820: MOV W14,904
00822: MOV W15,906
00824: MOV W0,908
00826: MOV W1,90A
00828: MOV W2,90C
0082A: MOV W3,90E
0082C: MOV W4,910
0082E: MOV W5,912
00830: MOV W6,914
00832: MOV W7,916
00834: MOV W8,918
00836: MOV W9,91A
00838: MOV W10,91C
0083A: MOV W11,91E
0083C: MOV W12,920
0083E: MOV W13,922
00840: MOV W14,924
00842: MOV W15,926
00844: MOV W0,928
00846: MOV W1,92A
00848: MOV W2,92C
0084A: MOV W3,92E
0084C: MOV W4,930
0084E: MOV W5,932
00850: MOV W6,934
00852: MOV W7,936
00854: MOV W8,938
00856: MOV W9,93A
00858: MOV W10,93C
0085A: MOV W11,93E
0085C: MOV W12,940
0085E: MOV W13,942
00860: MOV W14,944
00862: MOV W15,946
00864: MOV W0,948
00866: MOV W15,8E6
00868: MOV W0,8E8
0086A: MOV W1,8EA
0086C: MOV W2,8EC
0086E: MOV W3,8EE
00870: MOV W4,8F0
00872: MOV W5,8F2
00874: MOV W6,8F4
00876: MOV W7,8F6
00878: MOV W8,8F8
0087A: MOV W9,8FA
0087C: MOV W10,8FC
0087E: MOV W11,8FE
00880: MOV W12,900
00882: MOV W13,902
00884: MOV W14,904
00886: MOV W15,906
00888: MOV W0,908
0088A: MOV W1,90A
0088C: MOV W2,90C
0088E: MOV W3,90E
00890: MOV W4,910
00892: MOV W5,912
00894: MOV W6,914
00896: MOV W7,916
00898: MOV W8,918
0089A: MOV W9,91A
0089C: MOV W10,91C
0089E: MOV W11,91E
008A0: MOV W12,920
008A2: MOV W13,922
008A4: MOV W14,924
008A6: MOV W15,926
008A8: MOV W0,928
008AA: MOV W1,92A
008AC: MOV W2,92C
008AE: MOV W3,92E
008B0: MOV W4,930
008B2: MOV W5,932
008B4: MOV W6,934
008B6: MOV W7,936
008B8: MOV W8,938
008BA: MOV W9,93A
008BC: MOV W10,93C
008BE: MOV W11,93E
008C0: MOV W12,940
008C2: MOV W13,942
008C4: MOV W14,944
008C6: MOV W15,946
008C8: MOV W0,948
008CA: MOV W1,94A
008CC: SUB #66,W0
008CE: MOV #8E8,W4
008D0: REPEAT #32
008D2: MOV [W0++],[W4++]
008D4: MOV #8EA,W4
008D6: REPEAT #32
008D8: MOV [W1++],[W4++]
008DA: MOV #8EC,W4
008DC: REPEAT #32
008DE: MOV [W2++],[W4++]
008E0: MOV #8EE,W4
008E2: REPEAT #32
008E4: MOV [W3++],[W4++]
008E6: MOV #8F0,W0
008E8: REPEAT #32
008EA: MOV [W4++],[W0++]
008EC: MOV #8F2,W0
008EE: REPEAT #32
008F0: MOV [W5++],[W0++]
008F2: MOV #8F4,W0
008F4: REPEAT #32
008F6: MOV [W6++],[W0++]
008F8: MOV #8F6,W0
008FA: REPEAT #32
008FC: MOV [W7++],[W0++]
008FE: MOV #8F8,W0
00900: REPEAT #32
00902: MOV [W8++],[W0++]
00904: MOV #8FA,W0
00906: REPEAT #32
00908: MOV [W9++],[W0++]
0090A: MOV #8FC,W0
0090C: REPEAT #32
0090E: MOV [W10++],[W0++]
00910: MOV #8FE,W0
00912: REPEAT #32
00914: MOV [W11++],[W0++]
00916: MOV #900,W0
00918: REPEAT #32
0091A: MOV [W12++],[W0++]
0091C: MOV #902,W0
0091E: REPEAT #32
00920: MOV [W13++],[W0++]
00922: SUB #CC,W14
00924: MOV #904,W0
00926: REPEAT #32
00928: MOV [W14++],[W0++]
0092A: SUB #CC,W15
0092C: MOV #906,W0
0092E: REPEAT #32
00930: MOV [W15++],[W0++]
00932: MOV #908,W0
00934: REPEAT #32
00936: MOV [W0++],[W0++]
00938: MOV #90A,W0
0093A: REPEAT #32
0093C: MOV [W1++],[W0++]
0093E: MOV #90C,W0
00940: REPEAT #32
00942: MOV [W2++],[W0++]
00944: MOV #90E,W0
00946: REPEAT #32
00948: MOV [W3++],[W0++]
0094A: MOV #910,W0
0094C: REPEAT #32
0094E: MOV [W4++],[W0++]
00950: MOV #912,W0
00952: REPEAT #32
00954: MOV [W5++],[W0++]
00956: MOV #914,W0
00958: REPEAT #32
0095A: MOV [W6++],[W0++]
0095C: MOV #916,W0
0095E: REPEAT #32
00960: MOV [W7++],[W0++]
00962: MOV #918,W0
00964: REPEAT #32
00966: MOV [W8++],[W0++]
00968: MOV #91A,W0
0096A: REPEAT #32
0096C: MOV [W9++],[W0++]
0096E: MOV #91C,W0
00970: REPEAT #32
00972: MOV [W10++],[W0++]
00974: MOV #91E,W0
00976: REPEAT #32
00978: MOV [W11++],[W0++]
0097A: MOV #920,W0
0097C: REPEAT #32
0097E: MOV [W12++],[W0++]
00980: MOV #922,W0
00982: REPEAT #32
00984: MOV [W13++],[W0++]
00986: MOV #924,W0
00988: REPEAT #32
0098A: MOV [W14++],[W0++]
0098C: MOV #926,W0
0098E: REPEAT #32
00990: MOV [W15++],[W0++]
00992: MOV #928,W0
00994: REPEAT #32
00996: MOV [W0++],[W0++]
00998: MOV #92A,W0
0099A: REPEAT #32
0099C: MOV [W1++],[W0++]
0099E: MOV #92C,W0
009A0: REPEAT #32
009A2: MOV [W2++],[W0++]
009A4: MOV #92E,W0
009A6: REPEAT #32
009A8: MOV [W3++],[W0++]
009AA: MOV #930,W0
009AC: REPEAT #32
009AE: MOV [W4++],[W0++]
009B0: MOV #932,W0
009B2: REPEAT #32
009B4: MOV [W5++],[W0++]
009B6: MOV #934,W0
009B8: REPEAT #32
009BA: MOV [W6++],[W0++]
009BC: MOV #936,W0
009BE: REPEAT #32
009C0: MOV [W7++],[W0++]
009C2: CALL 39A
………………..

UPDATE: A week after reporting this issue (and a related one), the tool provider sent us an updated .DLL file that seems to resolve it. Code generation is now much nicer!

Sadly, this isn’t the only such behavior we have found with this compiler in regards to structures.

But why are we trying to pass a structure in to a function? Wouldn’t it be much more efficient to pass it in by reference, as a pointer?

Yes.

Yes it would.

But pointer problems are one of the leading causes of system crashes. Checking that a pointer is not NULL only means it points somewhere, and does not ensure it points to the expected data.

If the code is mission critical, that pointer check needs to do much more. Perhaps have some ID values in the structure that have to be verified, and add a checksum/CRC to the data:

typedef struct
{
   uint16_2 id; // will be set to some value like 0x1234
   // your data
   // more of your data
   uint16_t checksum; // will be a checksum of the id and data bytes.
} SafeStruct;

Then, when that structure is initialized/created, the ID would get set, and any time values were updated, the checksum would be updated. Any function that gets passed a pointer to a SafeStruct would validate the id and then checksum before proceeding.

If the system happens to have memory protection, and the pointer is in some off-limits memory, that attempt to access the id could cause a BUS TRAP or memory access exception. So, other checks would be needed to see if the pointer is even in the expected range of application memory.

This can get tricky, but if we all coded like this, we’d have far less crashes. I believe, anyway.

But, C allows you to pass in a variable and a copy gets made for use in the the function. This generates extra code and is slower, but it’s safer since the function only works on a COPY of the data and not a pointer that could be wrong.

MyStruct foo;

function (foo); // function() gets a copy of foo to work with

To modify this and return a value, you could do something like this:

foo = function (foo);

But, that means you are no longer able to use the return value for error checking. To work around that, you might have an error field in the structure that is set before it returns.

Yep, extra steps. Larger code. Slower code. But if you are coding as if lives depend on it working, and don’t need every last clock cycle, it’s a good habit to get into.

Unless you are using this PIC24 compiler, in which case, it causes other problems.

Until next time…

C and numeric constants.

Recently, I came across a #define in my day job that was something like:

#define HEX_255 0xff

It made me think about all the various ways a number can be represented in C. Three of these are standard, but the binary representation is not (unless it’s been added to the standard since I last looked). I’ve found the binary version available in GCC and at least one embedded compiler I use.

// There are exactly the same.
unsigned int value1 = 255; // 255 in decimal
unsigned int value2 = 0xff; // 255 in hexadecimal
unsigned int value3 = 0377; // 255 in octal
unsigned int value4 = 0b11111111; // 255 in binary

printf ("%u %u %u %u\n", value1, value2, value3, value4);

It made me wonder … what does one use octal for? I remember being excited when I realized an old synthesizer I had was using octal for it’s sound selections (8 banks of 8 sounds). But beyond that, I don’t know if I’ve ever found octal being used anywhere. (I think this may have been on a Kawai K1/K4 synthesizer).

Anyone have any other examples?

Code as if someone’s life depended on it.

Message for the day…

A useful technique I use to reduce the number of bugs in my buggy programs is to code as if someone’s life depended on them. When “it doesn’t matter,” it’s easy to avoid adding all the error checking.

But once you start looking at code as if it was mission critical, you start to see things differently and begin adding “useless” error checking. As long as you can afford the extra few moments to add the “useless” error checking, and as long as you have code space to hold the “useless” error checking, and as long as you have enough CPU cycles to run the “useless” error checking, you end up with a program that should be better,

Share and enjoy.

Braces! Foiled again!

Here’s another quick C programming thing.

Consider the following code, which nest two for/next loops together. Notice the variable it uses for each one.

for (int i=0 ; i < 10; i++) // using i
{
    printf("%d\n", i);

    for (int i=0 ; i < 3 ; i++) // using i
    {
        printf ("   %d\n", i);
    }
}

What do you think it will print? Will this even work? Running it produces…

0
   0
   1
   2
1
   0
   1
   2
2
   0
   1
   2
3
   0
   1
   2
4
   0
   1
   2
5
   0
   1
   2
6
   0
   1
   2
7
   0
   1
   2
8
   0
   1
   2
9
   0
   1
   2

In C, you create a new instance (scope) of a variable “i” within the braces. The second for/next declares its own local i, and any references to i within those braces will be that second instance of i.

This is confusing, of course, but it works.

A “bug” could exist if the second scope forgot the “int”:

for (int i=0 ; i < 10; i++) // using i
{
    printf ("%d\n", i);

    for (i=0 ; i < 3 ; i++) // where's the int?
    {
        printf ("   %d\n", i);
    }
}

By forgetting that “int”, it re-uses the first instance of i, and does not do what was expected.

0
   0
   1
   2
4
   0
   1
   2
4
   0
   1
   2
4
   0
   1
   2
4
   0
   1
   2
4
...repeat...

This was based on something recently fixed in some production code at my day job.

So, yeah. Use different variable names and avoid this from ever happening.

Next, revisiting a recent post, also dealing with what braces do, I stumbled upon a much better reason to always put braces around things, even if it’s just one statement.

Suppose you had a simple DEBUG macro (this is not a good one, but pretend it is):

#define DEBUG(x) printf(x);

if (x==1) DEBUG("hello");

Looks fine! But if you decide to make the macro contain multiple statements, using the backslash character like this:

#define DEBUG(x) printf("ERROR!\n"); \ printf(X);

It now breaks. That macro changes the resulting code into something like this:

if (x==1) printf("ERROR!\n");
printf("hello");

Not what was expected. To solve this, the macro creator should have put the statements in braces like:

#define DEBUG(x) { printf("ERROR!\n"); \ printf(X); }

Problem solved. Assuming the macro creator did it that way.

So, to prevent things like this that look fine and work fine from doing neither, just get in the habbit of using braces even for one statement:

if (x==1) { DEBUG("hello"); }

Or just always do them in multiple lines:

if (x==1)
{
   DEBUG("hello");
}

…and you don’t ever have to worry about that again.

Until next time…

Function names and “Clean Code”, part 2

See also: Part 1

Oh, the frustrations of obvious function names that don’t actually help.

LED1_On();
LED1_Off();
...
LED9_On();
LED9_Off();

First, it’s quite obvious that LED1_On() probably turns LED #1 on, and LED1_Off() turns it off. Simple.

But what is LED #1?

For most of my programming career, I have coded this way. You can always tell folks what that LED is in the commands… But then, of course, you don’t always do that everywhere it is used, and then if it is ever changed, all the old comments lie. (Thank you, Clean Code book, for making me fear comments.)

Today, I treat this kind of like how the OS-9 operating system treated devices… There is a device description (the thing that you are trying to access by name) and a device driver (the thing that controls the thing you are actually trying to access). Under OS-9, a hard disk might have been called “/h0” or “/h1”. A disk drive might have been “/d0” or “/d1”. The device drivers may have been some cryptic name with a chip number in it, such as “fd507” or “rb1003”. But that didn’t matter, because the used didn’t have to know and didn’t have to care. “/d0” was probably the main disk drive on any OS-9 system with a disk drive, regardless of what the actual underlying hardware was.

So for coding these days, I tend to write “low level” driver type functions like LED1_On() and LED2_Off(), and then wrap them in some clean code like:

void StatusLEDOn()
{
   LED1_On();
}

void StatusLEDOff()
{
   LED1_Off();
}

That adds a “useless” layer of code, but it keeps things nice and separate. If I see “StatusLEDOn()” in code, I can assume it probably turns on whatever the Status LED is. And if I see that function, I can determine that the Status LED must be LED #1.

And a good optimizer will likely just optimize these “useless” functions away, anyway.

Thanks to that stupid book, now when I see “ReadAI(2)” or “ToggleDO(5)” I just want to dive in and write wrapper functions to make that code look like “GetCurrentTemperature()” or “ToggleStatusLight()”.

I really need to go back and update all my code on GitHub some day…

Function names and “Clean Code”

At a previous job, I was introduced to the concept of “clean code.” As an embedded programmer, where we are often trying to squeeze extra bytes out of already optimized code, many of the principals of clean code do not apply.

But, in general, I like what I have read so far in this recommended book on the subject:

https://amzn.to/2KCtFy1 (affiliate link)

With this in mind, I wanted to ask a question about function names (whether they be in C, Perl, Java, or whatever).

In the past, I would write my functions using verb/noun, like this:

InitUserlog();
InitDisplay();
InitNetworking();

But after my exposure to object oriented programming, I learned about how classes and methods are used:

Userlog.init();
Display.init();
Networking.init();

This approach makes it very clean which class a function (method?) belongs to.

I started doing my C code similarly, letting the noun of the code lead:

UserlogInit();
DisplayInit();
NetworkingInit();

With all the Userlog functions starting with Userlog, I think it makes it clearer where things come from, and helps avoid collisions with outside code (or just code from another engineer on the same team).

With clean code, code should read more like English. Thus, using “InitializeUserlog()” is probably cleaner than “UserlogInit()”. But since you can’t do that with object oriented languages, perhaps writing backwards (Userlog.init(), UserlogInit(), etc.) is accepted due to the limitation of the language.

Indeed, “Userlog.lookupName()” seems less clean than “LookupNameInUserlog()”.

Perhaps I shouldn’t be doing this in C, since I *can* write the functions out more like proper English.

What do you think? Comments appreciated.

C and the dangers of memcpy()

Updates:

  • 2019-08-12 – Added note about using unions.

In the C programming language, memcpy (memory copy) is a function used to copy a range of bytes from one location in memory to another. I have used it often. Today, my boss mentioned something about not liking memcpy() because of all the dangers of using it. I understand that incorrect addresses, bad calculations, and simple omissions from a copy/paste can cause a memcpy() to have bad results. But, what’s the alternative?

He mentioned that he prefers to use C structures to copy data around, as opposed to using memcpy() to copy blocks of data.

Instead of doing this…

char Buffer1[64];
char Buffer2[64];

// Load Buffer1 with something.
strncpy (Buffer1, "Copy this string over", 64);

// Copy Buffer1 into Buffer2.
memcpy (Buffer2, Buffer1, 64);

That’s a simple, straight-forward example that seems hard to mess up. But doing something like this would be impossible to mess up:

typedef struct
{
    char Buffer[64];
} CopyStruct64;

CopyStruct64 Buffer1;
CopyStruct64 Buffer2;

// Load Buffer1 structure with something.
strncpy (Buffer1.Buffer, "Copy this string over", 64);

// Copy Buffer1 structure into Buffer2 structure.
Buffer2 = Buffer1;

Interesting. With properly defined structures, copying a block of data from one place to the other is done using code generated by the compiler.

(Update) And it seems I misspoke. He was actually talking about using unions, not structures. I’ve used unions as unions, but never for something like this. I did a similar test. (I realize there are several ways to declare a union, but I am using typedef so it looks similar to the above structure example.)

typedef union
{
    char Buffer[64];
} CopyUnion64;

CopyUnion64 Buffer1;
CopyUnion64 Buffer2;

// Load Buffer1 structure with something.
strncpy (Buffer1.Buffer, "Copy this string over", 64);

// Copy Buffer1 structure into Buffer2 structure.
Buffer2 = Buffer1;

Using union appears to work the same way, and since my example was not using structure elements, it seems to make more sense to do it that way. (End of Update)

I’m just now wrapping my brain around this, trying to figure out how this approach could be used to avoid using memcpy() and pointers.

What do you think? Am I on the right track to what he was talking about?

Until next time…

Braces! Foiled again!

Just a quick rant, based on how example code from a new compiler I am using is presented.

In C (and similar languages), it is very common to see simple logic presented in one line, such as:

if (AlertLevel == RED) TurnOnSiren();

That is simple to understand, and only takes up one line on the screen or a printout. However, at a previous job, our coding standard forbid that, and always required the use of braces around any actions of an “if”.

if (AlertLevel == RED)
{
   TurnOnSiren();
}

Now, ignoring style preferences of where the braces should go, the use of braces for something like this has some advantages when it comes to preventing potential bugs if this code is changed in the future.

In the case of the compiler examples I have been seeing, they show them without braces, such as:

if (AlertLevel == RED)
   TurnOnSiren();

For simple and short logic, this is no different than my first example — it just places the “what to do” stuff on an indented line after the “if”.

However, a common bug I see occurs when someone expands upon that like this:

if (AlertLevel == RED)
   TurnOnSiren();
   FlashRedLight();

In some languages, where program flow is determined by indentation, this would work. But in C, indentation does not matter. The above code would actually be processed as:

if (AlertLevel == RED) TurnOnSiren();

FlashRedLight();

The red lights would flash every time through, regardless of the status of AlertLevel.

Unintended consequences of not using braces, and using indents to format each statement on its own line.

By always using braces (even on simple one-statement things like this example), you avoid that:

if (AlertLevel == RED)
{
   TurnOnSiren();
   FlashRedLight();
}

Now we have the desired result, as adding new code or function calls inside the braces will work as we intended.

It’s 2019. The C Programming Language came out 47 years ago, and I still occasionally run into bugs caused by the lack of braces.

Thanks to a former job, I no longer make this mistake. I always use braces.

This has been a public service announcement of Sub-Etha Software.

C musing of the day: signed ints

I ran across some code today that puzzled me. It was an infinite loop that used a counter to determine if things took too long. Something like this:

int main()
{
  int count;
  int status;

  count = 0;

  do
  {
    status = GetStatus();

    count++;

  } while( status == 0 );

  if (count < 0)
  {
    printf("Time out! count = %d\n", count);

    return EXIT_FAILURE;
  }

  printf("Done. count = %d\n", count);

  return EXIT_SUCCESS;
}

The code would read some hardware status (“GetStatus() in this example)  and drop out of the do/while loop once it had a value. Inside that loop, it increments a count variable. After done, it would check for “count < 0” and exit with a timeout if that were true.

Count is only incremented, so the only way count could ever be less than zero is if it incremented so high it rolled over. With a signed 8-bit value (int8_t), you count from 0 to 127, then it rolls over to -128 and counts back up to 0.

So with an “int” being a 32-bit value (on the system I am using), the rollover happens at 2147483647.

And that seems like quite a weird value.

But I suppose it it took that long, it certainly timed out.

I think if was going to do it, I would have probably used an unsigned integer, and just specified an appropriate value:

unsigned int count;

...

if (count > 10000)
{
  printf("Time out! count = %u\n", count);
  return EXIT_FAILURE;
}

What says you?