ARM codegen issue, possibly fixed already

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

ARM codegen issue, possibly fixed already

Harald van Dijk
Hi,

On tcc 0.9.27 and month-old git, on ARM, the following program is
miscompiled:

   void f(char *a, long long b, int c) {
     *a = b < 0 && !c;
   }

   int main(void) {
     puts("Hello, world!");
     char a;
     f(&a, 1, 0);
   }

This fails with a segmentation fault. tcc was configured with

   ./configure --prefix=$HOME/tcc --config-arm_eabihf

and built with GCC 8.3.0. The disassembly for f shows the problem:

00000000 <f>:
    0:   e1a0c00d        mov     ip, sp
    4:   e92d000f        push    {r0, r1, r2, r3}
    8:   e92d5800        push    {fp, ip, lr}
    c:   e1a0b00d        mov     fp, sp
   10:   e24bd004        sub     sp, fp, #4
   14:   e59b000c        ldr     r0, [fp, #12]
   18:   e59b1018        ldr     r1, [fp, #24]
   1c:   e3511000        cmp     r1, #0
   20:   ca00000a        bgt     50 <f+0x50>
   24:   1a000002        bne     34 <f+0x34>
   28:   e59b1014        ldr     r1, [fp, #20]
   2c:   e3511000        cmp     r1, #0
   30:   2a000006        bcs     50 <f+0x50>
   34:   e50b0004        str     r0, [fp, #-4]
   38:   e59b001c        ldr     r0, [fp, #28]
   3c:   e3500000        cmp     r0, #0
   40:   0a000000        beq     48 <f+0x48>
   44:   ea000001        b       50 <f+0x50>
   48:   e3a00001        mov     r0, #1
   4c:   ea000000        b       54 <f+0x54>
   50:   e3a00000        mov     r0, #0
   54:   e51b1004        ldr     r1, [fp, #-4]
   58:   e5c10000        strb    r0, [r1]
   5c:   e89ba800        ldm     fp, {fp, sp, pc}

At <14>, the value of <a> is read. At <34>, that value is saved on the
stack. At <54>, that value is loaded again, and at <58>, the memory
referenced by <a> is written to.

The problem with that is that the branches at <20> and <30> go straight
to <50>, skipping the saving of the value, and whatever value happened
to be there on the stack is used instead.

This particular program no longer crashes since recently: the crash no
longer happens after

commit 8227db3a23fd3cf11840eaa25eab5f3f5f813ac7
Author: grischka <grischka>
Date:   Sat Jun 22 11:45:35 2019 +0200

     jump optimizations

     This unifies VT_CMP with VT_JMP(i) by using mostly VT_CMP
     with both a positive and a negative jump target list.

     Such we can delay putting the non-inverted or inverted jump
     until we can see which one is nore suitable (in most cases).

     example:
         if (a && b || c && d)
             e = 0;

As that commit is an optimization, not a codegen fix, I would like to
ask whether the original issue is still present and just requires a more
complex test case to expose, or whether it has accidentally been fixed
by this commit.

Thanks and keep up the good work!

Cheers,
Harald van Dijk

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: ARM codegen issue, possibly fixed already

grischka
Harald van Dijk wrote:
> As that commit is an optimization, not a codegen fix, I would like to
> ask whether the original issue is still present and just requires a more
> complex test case to expose, or whether it has accidentally been fixed
> by this commit.

No, I think this bug must exist since the very early days of
the project, on all 32-bit platforms.  This might fix it:

--- a/tccgen.c
+++ b/tccgen.c
@@ -1990,6 +1990,7 @@ static void gen_opl(int op)
          vtop[-1] = vtop[-2];
          vtop[-2] = tmp;
          /* stack: L1 L2 H1 H2 */
+        save_regs(4);
          /* compare high */
          op1 = op;
          /* when values are equal, we need to compare low words. since

-- gr

>
> Thanks and keep up the good work!
>
> Cheers,
> Harald van Dijk


_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Reply | Threaded
Open this post in threaded view
|

Re: ARM codegen issue, possibly fixed already

Harald van Dijk
On 14/07/2019 21:31, grischka wrote:
> Harald van Dijk wrote:
>> As that commit is an optimization, not a codegen fix, I would like to
>> ask whether the original issue is still present and just requires a more
>> complex test case to expose, or whether it has accidentally been fixed
>> by this commit.
>
> No, I think this bug must exist since the very early days of
> the project, on all 32-bit platforms.  This might fix it:

That's interesting: I had not seen it on i386, so I figured it would be
ARM-specific.

> --- a/tccgen.c
> +++ b/tccgen.c
> @@ -1990,6 +1990,7 @@ static void gen_opl(int op)
>           vtop[-1] = vtop[-2];
>           vtop[-2] = tmp;
>           /* stack: L1 L2 H1 H2 */
> +        save_regs(4);
>           /* compare high */
>           op1 = op;
>           /* when values are equal, we need to compare low words. since

That looks good for the reduced test case; looking at the generated
code, the changes make sense. I'll do some more testing over the course
of the next week, thanks.

>>
>> Thanks and keep up the good work!
>>
>> Cheers,
>> Harald van Dijk

_______________________________________________
Tinycc-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/tinycc-devel