Re: [Tinycc-devel] A double-to-float conversion bug

[Herman ten Brugge]

On 7/31/23 10:08, grischka wrote:
> On 31.07.2023 09:32, Herman ten Brugge via Tinycc-devel wrote:
> > On 7/30/23 18:27, Vincent Lefevre wrote:
> > > On 2023-07-30 16:07:29 +0600, Viktor M wrote:
> > > > Hello everyone. So today I stumbled upon this bug when doing math
> > > > involving conversions between float and double. A minimal example:
> > > [...]
> > >
> > > Not really minimal. I could simplify it even further:
> > I fixed this on mob.
> > The problem was that stack was overwritten because stack was not 
> > aligned correctly.
>
> Hi Herman,
>
> hope you don't mind some critic:  At first,  if we want to reserve
> some space of 'size' on stack, while respecting alignment of 'align',
> then there is nothing wrong with this line:
>
>    loc = (loc - size) & -align;
>
> As such your addition
>
>    loc &= -align;
>    loc = (loc - size) & -align;
>
> does not make sense, even if it may fix the problem.
>
> So, what is the problem about really?  The problem is that
>
>    sizeof (struct V { int x, y, z; })
>
> is 12, but when returned in registers (on x86_64-linux), the size of
> two registers is 2*8 = 16.
>
> Therefor the problem is not wrong alignment, it is wrong size.
>
> Digging further, it turns out that gfunc_sret() on x86_64-linux for
>     struct V { int x, y, z; }
> returns: one register of type VT_QLONG with regsize 8.
>
> That does not look right either.  In fact, tcc handles VT_QLONG
> as sort of pseudo register, using two processor registers (vtop->r/r2)
> so I'd think that for VT_QLONG, it should pass 16 as the 'regsize'.
>
> In the end, it seems that the space to be reserved on stack should be
> calculated like this
>
>    size = ret_nregs * regsize;
>
> rather than with 'size = type_size()'
>
> Btw note that the other part or your patch
>
> -                || (align & (ret_align-1))) {
> +                && (align & (ret_align-1))) {
>
> exactly undoes a previous patch from Yao Zi
>
> -                && (align & (ret_align-1))) {
> +                || (align & (ret_align-1))) {
>
> As I tried to point out in an earlier email, this previous patch was
> not the correct fix for the other problem, either.
>
> That's why I think that our patches must strive for two things always:
> 1) to fix the problem and 2) in a way that logically does make sense ;)

I agree with your comments above. The size is incorrect.
I could change gfunc_sret in x86_64-gen.c and then calculate the size in 
tccgen.c
as you suggested. But I am not sure regsize is set correctly all the time.

I like this better:
--- a/tccgen.c
+++ b/tccgen.c
@@ -6142,7 +6142,7 @@ special_math_val:
                        space.  Assume register size is power of 2. */
                     if (regsize > align)
                       align = regsize;
-                   loc &= -align;
+                   size = (size + regsize - 1) & -regsize;
                     loc = (loc - size) & -align;
                     addr = loc;
                     offset = 0;

The size should be a multiple of regsize.
What is your opinion?

I was working on a better fix for riscv and reverted the patch from Yao 
Zi by accident.
I will reapply that change.

    Herman