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

[grischka]

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 ;)

-- grischka

>      Herman
>
>
> _______________________________________________
> Tinycc-devel mailing list
> Tinycc-devel@nongnu.org
> https://lists.nongnu.org/mailman/listinfo/tinycc-devel