SDCC function parameter problem

[Arnaud]

Hello,

i have a problem with a specific argument of a function when i use a variable.
I don't even see my traces that indicate i enter the function.
I have problem only for the second argument.

I guessed a mix of unsigned with signed in arithmetic operation but i'm not sure.

Thanks.

Ps : i use the last version of SDCC 3.5.0
Ps : i have solve the problem with using structure pointer as argument but i'd like to understand my mistake. 

Some examples :

Code Select Expand


DisplayObject(object->_objectId,
objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X, // Result is 13                                                                   
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y) // Result is 113;
-> Don't work

Code Select Expand


UINT x = objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X;
DisplayObject(object->_objectId,
x,
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);
-> Don't work

Code Select Expand


UINT x = (UINT)objectInRoom->_coord.x *  (UINT)SQUARE_WIDTH ;
x += (UINT)OBJECT_ORIGIN_X;
DisplayObject(object->_objectId,
x,
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);
-> Don't work

Code Select Expand


DisplayObject(object->_objectId,
objectInRoom->_coord.x * SQUARE_WIDTH, // Result is 0
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);
-> Work and see my traces

Code Select Expand


DisplayObject(
object->_objectId,
13,
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);
-> Work  and see my traces

Code Select Expand


UINT x = 13;
DisplayObject(
object->_objectId,
x,
objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);
-> Work  and see my traces

Extract of my code, the entrance point is DrawRoomContent.

Code Select Expand


#define SCENE_ORIGIN_X    12
#define SCENE_ORIGIN_Y    42

#define OBJECT_ORIGIN_X    (SCENE_ORIGIN_X + 1)
#define OBJECT_ORIGIN_Y    (SCENE_ORIGIN_Y + 11)

#define SQUARE_WIDTH    8
#define SQUARE_HEIGHT   32

typedef struct{
    UCHAR x, y;
} SCoord

typedef struct{
     ...
    SCoord _coord;
} SObjectInRoom;

typedef struct
{
    UINT _objectId;
    ...
} SObject;

void DisplayObject(UCHAR pObjectId, UINT x, UINT y)
{
    SObject* object = NULL;
    u8* video;

   MY_TRACES

    video = cpct_getScreenPtr((u8*)_memVideo, x, y);
...
     UCHAR* objects = GetObjectsBuffer() + (SQUARE_WIDTH * MASKED * SQUARE_HEIGHT * object->_type);
     cpct_drawSpriteMasked(objects, video, SQUARE_WIDTH, SQUARE_HEIGHT);
...
}

void DrawRoomContent()
{
...
    DisplayObject(object->_objectId,    objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X,
                                        objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y);                           
...                                   
}

[FloppySoftware]

I can't see where you initialize the struct pointer 'object' to something different to NULL, in the function 'DisplayObject'.

And in the function 'DrawRoomContent', where is the declaration / initialization of the struct pointer 'object'?

It's a global? Or...?

If you use '->', you are accessing the members of a struct pointer, but if you are accessing a struct (not a struct pointer), you must use '.' instead,

I think you know this, but better to be sure

[ronaldo]

There is no apparent flaw in your code. However, I'd like to see it complete, not only some snippets.

When you mark a line with "//Result is 13", you mean that result is actually 13 or that it should be 13 but it is not?

Sometimes, SDCC has a hard time calculating complex parameters. I have seen it even crash when compiling. It might be a problem with the compiler (or not), but you should use the debugger to see it. With indirect traces this may not be analized deeply enough.

[Arnaud]

Hello and thanks for your answers.

Here my full cpctelera project. The problem is in code.c

When you mark a line with "//Result is 13", you mean that result is actually 13 or that it should be 13 but it is not?

It's 13 and it's what i want, i can see the value with a printf

It might be a problem with the compiler (or not), but you should use the debugger to see it. With indirect traces this may not be analized deeply enough.

I saw the --debug option and sdcdb and but it seems complicated to use 

[Alcoholics Anonymous]

I tried to reproduce a problem but couldn't using 3.5.4 (#9311).  Some bugfixes have occurred since 3.5.0.  We'd also need to see the compile line you're using since the code generated will depend on optimization level and other flags.

It could very well be a peephole optimizer issue so try compiling with "--no-peep" and see if that works.

btw:  sdcc inlines a small number of common functions in string.h:

   #define memcpy(dst, src, n)          __builtin_memcpy(dst, src, n)
   #define strcpy(dst, src)             __builtin_strcpy(dst, src)
   #define strncpy(dst, src, n)         __builtin_strncpy(dst, src, n)
   #define strchr(s, c)                 __builtin_strchr(s, c)
   #define memset(dst, c, n)            __builtin_memset(dst, c, n)

Those functions may be inlined with ldir if that would be shorter than a function call --- sdcc function calls are very expensive so that will be the case if most parameters are constants.  Your C versions of these functions will most certainly be slower

[Arnaud]

It could very well be a peephole optimizer issue so try compiling with "--no-peep" and see if that works.

I try it, and the problem is always here.

Now i think it's a SDCC problem rather a lack of knowledge of myself about SDCC and CPC programming.

btw:  sdcc inlines a small number of common functions in string.h:

   #define memcpy(dst, src, n)          __builtin_memcpy(dst, src, n)
   #define strcpy(dst, src)             __builtin_strcpy(dst, src)
   #define strncpy(dst, src, n)         __builtin_strncpy(dst, src, n)
   #define strchr(s, c)                 __builtin_strchr(s, c)
   #define memset(dst, c, n)            __builtin_memset(dst, c, n)

Those functions may be inlined with ldir if that would be shorter than a function call --- sdcc function calls are very expensive so that will be the case if most parameters are constants.  Your C versions of these functions will most certainly be slower

You are right, l'll use it.


Thanks for your help 

[ronaldo]

The version you have attached to the post has this code:

Code Select Expand


                x = objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X;
                y = objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y;
               
                DisplayObject(object->_objectId, x, y);

Is it working this way?

I'm trying to debug it, but I don't now what to do to make your game enter that piece of code. At least at the start, it does not enter there.

[ronaldo]

@Alcoholics Anonymous: I found this in the generated code

Code Select Expand


  0647 E1            [10] 1405     pop    hl
   0648 E5            [11] 1406     push    hl

Should it be a peephole for eliminating this 2 instructions? Do they have any real utility?

[Alcoholics Anonymous]

Code Select Expand


  0647 E1            [10] 1405     pop    hl
   0648 E5            [11] 1406     push    hl

Should it be a peephole for eliminating this 2 instructions? Do they have any real utility?

You're probably reading push/pop there... pop/push is still valid.

I think this may be a compiler issue too so I'll try to take a closer look as well.

[ronaldo]

Sorry, you are right. I was doing this very fast because I had to go and I missread this part. However, I also think it might be a compiler problem related to --fomit-frame-pointer. The piece of code from which I copied this part is full of pushes and pops. I assume this is code complexification due to not being authorized to use IX. That may ultimately cause the error when calculations are done directly in the parameters.

[ronaldo]

Up to know, I don't know what do I have to do in your game to make it enter the portion of code we are analyzing. It enters DisplayRoomContent function after execution, but it does not call DisplayObject. I assume that it I need to have an object in the screen for the function to be called, but I don't know how to do it.

So, right now, I have been looking at the generated code and it seems to be right. I tried these two cases.

Case 1:

Code Select Expand


void DrawRoomContent()
{   
    ...
            if (object != NULL)
            {               
                x = objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X;
                y = objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y;
               
                DisplayObject(object->_objectId, x, y);
            }
   ...
}

Case 2:

Code Select Expand


void DrawRoomContent()
{   
   ...
            if (object != NULL)
            {               
            DisplayObject(object->_objectId,
               objectInRoom->_coord.x *  SQUARE_WIDTH + OBJECT_ORIGIN_X,
               objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y );
            }
   ...
}


In both cases, code generated looks good, seems equivalente, and doesn't seem to contain any flaw. I would need to trace it with real values to check it better. Here you are both generated codes side by side:
[attachimg=1]

I need to experiment a little more, but both codes seem to do the job.

Regarding push & pops, @Alcoholics Anonymous, I think this part is more like what I was referring to previously:

Code Select Expand


   0619 E5            [11] 1369     push    hl
                           1370 ;src/code.c:368: if (objectInRoom->_objectId != NOTHING)
   061A E1            [10] 1371     pop    hl
   061B E5            [11] 1372     push    hl

This part is previous to both cases, but I think it could be summed up as push hl.

[ronaldo]

I found something pretty odd (at least for me), that I think could be a problem.  This function starts with this code:

Code Select Expand


void DrawRoomContent()
{   
    UCHAR obj;
    SObjectInRoom* objectInRoom;
    SObject* object;
    SRoom* room = GetActiveRoom();
    //SPoint pt;
    UINT x, y;
   
    for (obj = 0; obj < MAX_OBJ_IN_ROOM; obj++)
    {
        objectInRoom = &room->_objectsInRoom[obj];
...


This code aims to get a pointer to a room (SRoom*) and then use it to retrieve its objects. However, something odd happens after the call to GetActiveRoom is executed:

Code Select Expand


                        1327 ;src/code.c:356: void DrawRoomContent()
                           1328 ;   ---------------------------------
                           1329 ; Function DrawRoomContent
                           1330 ; ---------------------------------
   05E6                    1331 _DrawRoomContent::
   05E6 F5            [11] 1332  push  af
   05E7 F5            [11] 1333  push  af
   05E8 F5            [11] 1334  push  af
   05E9 3B            [ 6] 1335  dec   sp
                           1336 ;src/code.c:361: SRoom* room = GetActiveRoom();
   05EA CDr00r00      [17] 1337  call  _GetActiveRoom
;; *******************************************************************************************************************
;; ************ HL holds a valid pointer to the room, but next lines overwrite it without previously saving it! *********
;; *******************************************************************************************************************
                           1338 ;src/code.c:365: for (obj = 0; obj < MAX_OBJ_IN_ROOM; obj++)
   05ED 7D            [ 4] 1339  ld a,l
   05EE 21 05 00      [10] 1340  ld hl,#5
   05F1 39            [11] 1341  add   hl,sp
   05F2 C6 09         [ 7] 1342  add   a, #0x09
   05F4 77            [ 7] 1343  ld (hl),a
   05F5 7C            [ 4] 1344  ld a,h
   05F6 CE 00         [ 7] 1345  adc   a, #0x00
   05F8 23            [ 6] 1346  inc   hl
   05F9 77            [ 7] 1347  ld (hl),a
   05FA FD 21 02 00   [14] 1348  ld iy,#2
   05FE FD 39         [15] 1349  add   iy,sp
   0600 FD 36 00 00   [19] 1350  ld 0 (iy),#0x00
   0604                    1351 00106$:
                           1352 ;src/code.c:367: objectInRoom = &room->_objectsInRoom[obj];
   0604 21 02 00      [10] 1353  ld hl, #2+0
   0607 39            [11] 1354  add   hl, sp
   0608 7E            [ 7] 1355  ld a, (hl)
   0609 87            [ 4] 1356  add   a, a
   060A 87            [ 4] 1357  add   a, a
   060B 5F            [ 4] 1358  ld e,a
   060C 21 05 00      [10] 1359  ld hl, #5
   060F 39            [11] 1360  add   hl, sp
   0610 7E            [ 7] 1361  ld a, (hl)
   0611 23            [ 6] 1362  inc   hl
   0612 66            [ 7] 1363  ld h, (hl)
   0613 6F            [ 4] 1364  ld l, a
   0614 16 00         [ 7] 1365  ld d,#0x00
   0616 19            [11] 1366  add   hl, de
   0617 33            [ 6] 1367  inc   sp
   0618 33            [ 6] 1368  inc   sp
   0619 E5            [11] 1369  push  hl


As you can see here, there is a "call  _GetActiveRoom" which I have traced and correctly returns a pointer to the room in HL. However, just after calling the function, HL is immediately destroyed, losing the pointer to the room, which hasn't been stored anywhere. In fact, when code arrives to the point of using room pointer, it tries to get it from SP+5 (where room variable should be stored). Of course, the value it gets from there has nothing to do with the previously lost value from HL.

I have compiled the code with different options (--no-peep, --fomit-frame-pointer, nothing...) and the result is always the same. I have also tried to reproduce this behaviour creating a program with similar structure, but I cannot reproduce it. With a similar program, I get something like this:

Code Select Expand


                            82 ;src/main.c:42: void main(void) {
                             83 ;    ---------------------------------
                             84 ; Function main
                             85 ; ---------------------------------
   0023                      86 _main::
   0023 DD E5         [15]   87     push    ix
   0025 DD 21 00 00   [14]   88     ld    ix,#0
   0029 DD 39         [15]   89     add    ix,sp
   002B 21 F9 FF      [10]   90     ld    hl,#-7
   002E 39            [11]   91     add    hl,sp
   002F F9            [ 6]   92     ld    sp,hl
                             93 ;src/main.c:44: MyStruct *st = returnStructPointer();
   0030 CDr1Fr00      [17]   94     call    _returnStructPointer
   0033 EB            [ 4]   95     ex    de,hl
;; ************************************************************************************************
;; ******* This time, HL is saved into DE before overwriting its contents, as expected *********
;; ************************************************************************************************
                             96 ;src/main.c:47: for (i=0; i < 2; i++) {
   0034 DD 36 F9 00   [19]   97     ld    -7 (ix),#0x00
   0038                      98 00105$:
                             99 ;src/main.c:49: printf("(%d, %d, %d)\n\r", a->id, a->x, a->y);
   0038 21 02 00      [10]  100     ld    hl,#0x0002
   003B 19            [11]  101     add    hl,de
   003C DD 75 FA      [19]  102     ld    -6 (ix),l
   003F DD 74 FB      [19]  103     ld    -5 (ix),h


As you see here, just after calling returnStructPointer function, the value is saved to DE (with ex de, hl) before overwritting HL. Then, when the pointer is required again, it is used from de (add hl, de). This works fine. I don't see why this behaviour difference.

@Alcoholics Anonymous, do you think this looks like a bug? I'm I missing something?

[Arnaud]

It enters DisplayRoomContent function after execution, but it does not call DisplayObject. I assume that it I need to have an object in the screen for the function to be called, but I don't know how to do it.

I think it's exactly the problem, the function is not called with these argument i can deduct that because my printf are not displayed.
If i use the "good" parameters, the function is called and my printf are visibled.

[FloppySoftware]

It seems that something is writing into the sp area and destroying that local var.

Maybe the dec sp instruction and a sys.  interrupt?

I had random problems with that in my compiler.

[AMSDOS]

In both cases, code generated looks good, seems equivalente, and doesn't seem to contain any flaw. I would need to trace it with real values to check it better. Here you are both generated codes side by side:

I don't understand why Comments are taking up valuable memory space. Assembly would never be taking up a byte of memory. If it's a start of a new component, it could still be where the comment is.

[FloppySoftware]

Where is the assembler code that takes room in the SP area for the variables x and y?

Code Select Expand


    UCHAR obj;
    SObjectInRoom* objectInRoom;
    SObject* object;
    SRoom* room = GetActiveRoom();
    //SPoint pt;
    UINT x, y;    <-----------THIS

I mean, after calling GetActiveRoom(), I can't see the code that reserves 4 bytes for x and y:

Code Select Expand

   05E6                    1331 _DrawRoomContent::
   05E6 F5            [11] 1332  push  af
   05E7 F5            [11] 1333  push  af
   05E8 F5            [11] 1334  push  af
   05E9 3B            [ 6] 1335  dec   sp
                           1336 ;src/code.c:361: SRoom* room = GetActiveRoom();
   05EA CDr00r00      [17] 1337  call  _GetActiveRoom
;; *******************************************************************************************************************
;; ************ HL holds a valid pointer to the room, but next lines overwrite it without previously saving it! *********
;; *******************************************************************************************************************
------------------------->MISSING????<----------------
                           1338 ;src/code.c:365: for (obj = 0; obj < MAX_OBJ_IN_ROOM; obj++)
   05ED 7D            [ 4] 1339  ld a,l

[Alcoholics Anonymous]

Where is the assembler code that takes room in the SP area for the variables x and y?
I mean, after calling GetActiveRoom(), I can't see the code that reserves 4 bytes for x and y:

It's hard to say without seeing all the code.  sdcc might not bother to create those variables if they are not used or since they are not volatile and are local, they may be kept in registers during their lifetime.

[Alcoholics Anonymous]

Regarding push & pops, @Alcoholics Anonymous, I think this part is more like what I was referring to previously:

Code Select Expand


   0619 E5            [11] 1369     push    hl
                           1370 ;src/code.c:368: if (objectInRoom->_objectId != NOTHING)
   061A E1            [10] 1371     pop    hl
   061B E5            [11] 1372     push    hl

This part is previous to both cases, but I think it could be summed up as push hl.

Yes that combination can be removed.  I have such a rule in the z88dk rulesets

@Alcoholics Anonymous, do you think this looks like a bug? I'm I missing something?

Yes that could be a bug.  What I am wondering about is if that is valid C.

Code Select Expand


void DrawRoomContent()
{   
    UCHAR obj;
    SObjectInRoom* objectInRoom;
    SObject* object;
    SRoom* room = GetActiveRoom();
    //SPoint pt;
    UINT x, y;

...


I'm not sure you can initialize with anything but a constant.  Does it work if you do it 'properly'?

Code Select Expand



void DrawRoomContent()
{   
    UCHAR obj;
    SObjectInRoom* objectInRoom;
    SObject* object;
    SRoom* room;
    //SPoint pt;
    UINT x, y;

    room = GetActiveRoom();
...



The omit-frame-pointer and especially old-ralloc stuff is not supported by sdcc anymore.  There are bugs there that will probably never be fixed.  Maybe if a code snippet can be made to reproduce the omit-frame-pointer problem they'll accept it.  It's better to change the library code to save ix so that omit-frame-pointer doesn't have to be used.

[ronaldo]

I'm not sure you can initialize with anything but a constant.  Does it work if you do it 'properly'?

I tested it already. Same behaviour.

The omit-frame-pointer and especially old-ralloc stuff is not supported by sdcc anymore.  There are bugs there that will probably never be fixed.  Maybe if a code snippet can be made to reproduce the omit-frame-pointer problem they'll accept it.  It's better to change the library code to save ix so that omit-frame-pointer doesn't have to be used.

The thing is that this happens with and without modifiers. Compiling without --fomit-frame-pointer yields same result in the part where HL is overwritten.

However, I have created other simple programs trying to reproduce this problem and I can't. Don't understand why does it happen here.

[Alcoholics Anonymous]

I tested it already. Same behaviour.
The thing is that this happens with and without modifiers. Compiling without --fomit-frame-pointer yields same result in the part where HL is overwritten.

I only get the problem when compiling with "--fomit-frame-pointer".  Without it I get this (sdcc peephole off, max-allocs-per-node 2000):

Code Select Expand


    call    _GetActiveRoom
    ld    a,l
    add    a, #0x09
    ld    c,a
    ld    a,h
    adc    a, #0x00
    ld    b,a
    ;
    ld    -3 (ix),#0x00


which is correct code placing the returned pointer plus 9 into bc and zeroing the loop counter.


If you add "--reserve-regs-iy" to "--fomit-frame-pointer" which maybe the OP can try you also get correct code (max-allocs-per-node 200000 here, sdcc peephole on):

Code Select Expand


    call    _GetActiveRoom
    ld    de, #0x0009
    add    hl, de
    ld    -2 (ix),l
    ld    -1 (ix),h
   ;
    ld    -5 (ix),#0x00


There's something wrong with sdcc's cost function when using iy so quite often the code is better if you tell it not to use it.

[ronaldo]

You are right, @Alcoholics Anonymous. I didn't pay enough attention to the compiler results, or maybe I was not looking at the correct file.

I have compiled again only with --no-peep and with no compiler options at all, and I get the exact same code as you. Code is correct without "--fomit-frame-pointer". I have even tested the game and DrawRoomContent function works and draws objects as expected.

[attach=2]

I think this clearly settles it: it's a bug due to --fomit-frame-pointer. The only problem I have is how to reproduce it. Maybe we should send the file "as is" to SDCC developers. Would it be okay?

[Alcoholics Anonymous]

I think this clearly settles it: it's a bug due to --fomit-frame-pointer. The only problem I have is how to reproduce it. Maybe we should send the file "as is" to SDCC developers. Would it be okay?

That file is too long... no one wants to wade through that amount of code

This snippet will also reproduce the problem.  I think whittle this down and then submit it:

Code Select Expand



#define NULL 0
#define MAX_OBJ_IN_ROOM 10
#define NB_DIR 6
#define NOTHING 0
#define MAX_MONSTER_IN_ROOM 3
#define SCENE_ORIGIN_X    12
#define SCENE_ORIGIN_Y    42
#define OBJECT_ORIGIN_X    (SCENE_ORIGIN_X + 1)
#define OBJECT_ORIGIN_Y    (SCENE_ORIGIN_Y + 11)
#define SQUARE_WIDTH 8
#define SQUARE_HEIGHT 32

typedef struct
{
    unsigned char x, y;
} SCoord;

typedef struct
{
    unsigned int _objectId;
    SCoord _coord;
} SObjectInRoom;

typedef struct
{   
    unsigned char _roomId;
    unsigned char _attrib;
    unsigned char _dir;
   
    unsigned char _nextRoomId[NB_DIR];
   
    SObjectInRoom _objectsInRoom[MAX_OBJ_IN_ROOM];
    unsigned char _monstersId[MAX_MONSTER_IN_ROOM];
} SRoom;

typedef struct
{
    int visible:1;
    int takable:1;
    int activable:1;
    int usable:1;
    int hidden:1;
    int bit5:1;
    int bit6:1;
    int bit7:1;
} SObjAttrib;

typedef struct
{
    unsigned int _objectId;
    unsigned char _type;
    SObjAttrib _attrib;
    unsigned char _data;
    unsigned char _containerId;
} SObject;

extern SRoom* GetActiveRoom(void);
extern SObject* GetObjectFromWorld(unsigned int pObjectId);
extern void DisplayObject(unsigned char pObjectId, unsigned int x, unsigned int y);

void DrawRoomContent()
{   
    unsigned char obj;
    SObjectInRoom* objectInRoom;
    SObject* object;
    SRoom* room;
    unsigned int x, y;
   
    room = GetActiveRoom();
    for (obj = 0; obj < MAX_OBJ_IN_ROOM; obj++)
    {
        objectInRoom = &room->_objectsInRoom[obj];
        if (objectInRoom->_objectId != NOTHING)
        {
            object = GetObjectFromWorld(objectInRoom->_objectId);
            if (object != NULL)
            {               
                x = objectInRoom->_coord.x * SQUARE_WIDTH + OBJECT_ORIGIN_X;
                y = objectInRoom->_coord.y * SQUARE_HEIGHT + OBJECT_ORIGIN_Y;
               
                DisplayObject(object->_objectId, x, y);
            }
        }
    }
}