timex_t functions byReference instead of byValue

[cgundogan]

Sometimes I get the warning/error - not sure what it is - from travis/cppcheck saying:

sys/include/timex.h:70: performance (passedByValue): Function parameter 'a' should be passed by reference.
sys/include/timex.h:70: performance (passedByValue): Function parameter 'b' should be passed by reference.
sys/include/timex.h:80: performance (passedByValue): Function parameter 'a' should be passed by reference.
sys/include/timex.h:80: performance (passedByValue): Function parameter 'b' should be passed by reference.
sys/include/timex.h:102: performance (passedByValue): Function parameter 'a' should be passed by reference.
sys/include/timex.h:102: performance (passedByValue): Function parameter 'b' should be passed by reference.
sys/include/timex.h:135: performance (passedByValue): Function parameter 'a' should be passed by reference.

#2155#issuecomment-69809181

Looking at the function definitions:

timex_t timex_add(const timex_t a, const timex_t b);
timex_t timex_sub(const timex_t a, const timex_t b);
...

Wouldn't it make sense to really change these parameters to be pointers instead?
What's your view on this?

[Kijewski]

#include <stdint.h>

typedef struct {
    uint32_t seconds;
    uint32_t microseconds;
} timex_t;

extern timex_t add1(const timex_t a, const timex_t b);
extern timex_t add2(const timex_t *a, const timex_t *b);
extern void add3(const timex_t *a, const timex_t *b, timex_t *r);

extern timex_t a, b, r;

void test_add1(void) {
    r = add1(a, b);
}

void test_add2(void) {
    r = add2(&a, &b);
}

void test_add3(void) {
    add3(&a, &b, &r);
}

Interesting are the push and pop operations:

i386:

00000000 <test_add1>:
   0:   83 ec 1c                sub    $0x1c,%esp
   3:   8d 44 24 08             lea    0x8(%esp),%eax
   7:   83 ec 0c                sub    $0xc,%esp
   a:   ff 35 04 00 00 00       pushl  0x4
  10:   ff 35 00 00 00 00       pushl  0x0
  16:   ff 35 04 00 00 00       pushl  0x4
  1c:   ff 35 00 00 00 00       pushl  0x0
  22:   50                      push   %eax
  23:   e8 fc ff ff ff          call   24 <test_add1+0x24>
  28:   8b 44 24 24             mov    0x24(%esp),%eax
  2c:   8b 54 24 28             mov    0x28(%esp),%edx
  30:   a3 00 00 00 00          mov    %eax,0x0
  35:   89 15 04 00 00 00       mov    %edx,0x4
  3b:   83 c4 38                add    $0x38,%esp
  3e:   c3                      ret    

00000000 <test_add2>:
   0:   83 ec 1c                sub    $0x1c,%esp
   3:   8d 44 24 08             lea    0x8(%esp),%eax
   7:   52                      push   %edx
   8:   68 00 00 00 00          push   $0x0
   d:   68 00 00 00 00          push   $0x0
  12:   50                      push   %eax
  13:   e8 fc ff ff ff          call   14 <test_add2+0x14>
  18:   8b 44 24 14             mov    0x14(%esp),%eax
  1c:   8b 54 24 18             mov    0x18(%esp),%edx
  20:   a3 00 00 00 00          mov    %eax,0x0
  25:   89 15 04 00 00 00       mov    %edx,0x4
  2b:   83 c4 28                add    $0x28,%esp
  2e:   c3                      ret    

00000000 <test_add3>:
   0:   83 ec 10                sub    $0x10,%esp
   3:   68 00 00 00 00          push   $0x0
   8:   68 00 00 00 00          push   $0x0
   d:   68 00 00 00 00          push   $0x0
  12:   e8 fc ff ff ff          call   13 <test_add3+0x13>
  17:   83 c4 1c                add    $0x1c,%esp
  1a:   c3                      ret

arm-none-eabi:

00000000 <test_add1>:
   0:   e92d4030        push    {r4, r5, lr}
   4:   e59f3038        ldr     r3, [pc, #56]   ; 44 <test_add1+0x44>
   8:   e5931004        ldr     r1, [r3, #4]
   c:   e24dd014        sub     sp, sp, #20
  10:   e58d1000        str     r1, [sp]
  14:   e59f202c        ldr     r2, [pc, #44]   ; 48 <test_add1+0x48>
  18:   e28d4008        add     r4, sp, #8
  1c:   e1a00004        mov     r0, r4
  20:   e8920006        ldm     r2, {r1, r2}
  24:   e5933000        ldr     r3, [r3]
  28:   ebfffffe        bl      0 <add1>
  2c:   e59f5018        ldr     r5, [pc, #24]   ; 4c <test_add1+0x4c>
  30:   e8940003        ldm     r4, {r0, r1}
  34:   e8850003        stm     r5, {r0, r1}
  38:   e28dd014        add     sp, sp, #20
  3c:   e8bd4030        pop     {r4, r5, lr}
  40:   e12fff1e        bx      lr

00000000 <test_add2>:
   0:   e92d4037        push    {r0, r1, r2, r4, r5, lr}
   4:   e59f1020        ldr     r1, [pc, #32]   ; 2c <test_add2+0x2c>
   8:   e1a0000d        mov     r0, sp
   c:   e59f201c        ldr     r2, [pc, #28]   ; 30 <test_add2+0x30>
  10:   ebfffffe        bl      0 <add2>
  14:   e59f5018        ldr     r5, [pc, #24]   ; 34 <test_add2+0x34>
  18:   e89d0003        ldm     sp, {r0, r1}
  1c:   e8850003        stm     r5, {r0, r1}
  20:   e28dd00c        add     sp, sp, #12
  24:   e8bd4030        pop     {r4, r5, lr}
  28:   e12fff1e        bx      lr

00000000 <test_add3>:
   0:   e92d4008        push    {r3, lr}
   4:   e59f0010        ldr     r0, [pc, #16]   ; 1c <test_add3+0x1c>
   8:   e59f1010        ldr     r1, [pc, #16]   ; 20 <test_add3+0x20>
   c:   e59f2010        ldr     r2, [pc, #16]   ; 24 <test_add3+0x24>
  10:   ebfffffe        bl      0 <add3>
  14:   e8bd4008        pop     {r3, lr}
  18:   e12fff1e        bx      lr

avr:

00000000 <test_add1>:
   0:   af 92           push    r10
   2:   bf 92           push    r11
   4:   cf 92           push    r12
   6:   df 92           push    r13
   8:   ef 92           push    r14
   a:   ff 92           push    r15
   c:   0f 93           push    r16
   e:   1f 93           push    r17
  10:   a0 90 00 00     lds     r10, 0x0000
  14:   b0 90 00 00     lds     r11, 0x0000
  18:   c0 90 00 00     lds     r12, 0x0000
  1c:   d0 90 00 00     lds     r13, 0x0000
  20:   e0 90 00 00     lds     r14, 0x0000
  24:   f0 90 00 00     lds     r15, 0x0000
  28:   00 91 00 00     lds     r16, 0x0000
  2c:   10 91 00 00     lds     r17, 0x0000
  30:   20 91 00 00     lds     r18, 0x0000
  34:   30 91 00 00     lds     r19, 0x0000
  38:   40 91 00 00     lds     r20, 0x0000
  3c:   50 91 00 00     lds     r21, 0x0000
  40:   60 91 00 00     lds     r22, 0x0000
  44:   70 91 00 00     lds     r23, 0x0000
  48:   80 91 00 00     lds     r24, 0x0000
  4c:   90 91 00 00     lds     r25, 0x0000
  50:   00 d0           rcall   .+0             ; 0x52 <test_add1+0x52>
  52:   e0 e0           ldi     r30, 0x00       ; 0
  54:   f0 e0           ldi     r31, 0x00       ; 0
  56:   20 83           st      Z, r18
  58:   31 83           std     Z+1, r19        ; 0x01
  5a:   42 83           std     Z+2, r20        ; 0x02
  5c:   53 83           std     Z+3, r21        ; 0x03
  5e:   64 83           std     Z+4, r22        ; 0x04
  60:   75 83           std     Z+5, r23        ; 0x05
  62:   86 83           std     Z+6, r24        ; 0x06
  64:   97 83           std     Z+7, r25        ; 0x07
  66:   1f 91           pop     r17
  68:   0f 91           pop     r16
  6a:   ff 90           pop     r15
  6c:   ef 90           pop     r14
  6e:   df 90           pop     r13
  70:   cf 90           pop     r12
  72:   bf 90           pop     r11
  74:   af 90           pop     r10
  76:   08 95           ret

00000000 <test_add2>:
   0:   60 e0           ldi     r22, 0x00       ; 0
   2:   70 e0           ldi     r23, 0x00       ; 0
   4:   80 e0           ldi     r24, 0x00       ; 0
   6:   90 e0           ldi     r25, 0x00       ; 0
   8:   00 d0           rcall   .+0             ; 0xa <test_add2+0xa>
   a:   e0 e0           ldi     r30, 0x00       ; 0
   c:   f0 e0           ldi     r31, 0x00       ; 0
   e:   20 83           st      Z, r18
  10:   31 83           std     Z+1, r19        ; 0x01
  12:   42 83           std     Z+2, r20        ; 0x02
  14:   53 83           std     Z+3, r21        ; 0x03
  16:   64 83           std     Z+4, r22        ; 0x04
  18:   75 83           std     Z+5, r23        ; 0x05
  1a:   86 83           std     Z+6, r24        ; 0x06
  1c:   97 83           std     Z+7, r25        ; 0x07
  1e:   08 95           ret

00000000 <test_add3>:
   0:   40 e0           ldi     r20, 0x00       ; 0
   2:   50 e0           ldi     r21, 0x00       ; 0
   4:   60 e0           ldi     r22, 0x00       ; 0
   6:   70 e0           ldi     r23, 0x00       ; 0
   8:   80 e0           ldi     r24, 0x00       ; 0
   a:   90 e0           ldi     r25, 0x00       ; 0
   c:   00 c0           rjmp    .+0             ; 0xe <__zero_reg__+0xd>

msp430:

00000000 <test_add1>:
   0:   12 12 00 00     push    &0x0000
   4:   12 12 00 00     push    &0x0000
   8:   12 12 00 00     push    &0x0000
   c:   12 12 00 00     push    &0x0000
  10:   1c 42 00 00     mov     &0x0000,r12
  14:   1d 42 00 00     mov     &0x0000,r13
  18:   1e 42 00 00     mov     &0x0000,r14
  1c:   1f 42 00 00     mov     &0x0000,r15
  20:   b0 12 00 00     call    #0x0000
  24:   31 52           add     #8,     r1      ;r2 As==11
  26:   82 4c 00 00     mov     r12,    &0x0000
  2a:   82 4d 00 00     mov     r13,    &0x0000
  2e:   82 4e 00 00     mov     r14,    &0x0000
  32:   82 4f 00 00     mov     r15,    &0x0000
  36:   30 41           ret

00000000 <test_add2>:
   0:   3e 40 00 00     mov     #0,     r14     ;#0x0000
   4:   3f 40 00 00     mov     #0,     r15     ;#0x0000
   8:   b0 12 00 00     call    #0x0000
   c:   82 4c 00 00     mov     r12,    &0x0000
  10:   82 4d 00 00     mov     r13,    &0x0000
  14:   82 4e 00 00     mov     r14,    &0x0000
  18:   82 4f 00 00     mov     r15,    &0x0000
  1c:   30 41           ret

00000000 <test_add3>:
   0:   3d 40 00 00     mov     #0,     r13     ;#0x0000
   4:   3e 40 00 00     mov     #0,     r14     ;#0x0000
   8:   3f 40 00 00     mov     #0,     r15     ;#0x0000
   c:   b0 12 00 00     call    #0x0000
  10:   30 41           ret

In terms of stack space I think we have a winner.

[OlegHahm]

There was some discussion already here: #1843 (comment)
I once started to change all these functions but got tired of the cumbersome work.

[LudwigKnuepfer]

I changed the classification of this issue to reflect the answer to the former question.

[kaspar030]

@Kijewski Nice comparison. Makes me want a script that creates it...

[kaspar030]

@Kijewski Could you compare again compiling with LTO?

edit let me think more about this, if LTO has any effect, it probably won't affect stack usage.

[Kijewski]

This is the line that I used to get the data:

msp430-gcc -fdata-sections -ffunction-sections -fomit-frame-pointer -ggdb3 -Os -c x.c && msp430-objdump -dC x.o

Seeing LTO in action will be more difficult to test.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.