Re: [Tinycc-devel] A double-to-float conversion bug
On 7/31/23 12:43, grischka wrote: On 31.07.2023 13:42, Herman ten Brugge via Tinycc-devel wrote: 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? Hi, yes, that seems to make sense. Then again I think that align should additionally depend on just 'ret_align', as in if (ret_align > align) align = ret_align; I was working on a better fix for riscv and reverted the patch from Yao Zi by accident. I will reapply that change. Well, as I was trying to say, that change wasn't a good one either. Anyway, I just applied some more extensive fix for both. Hope it does work now. Also yet another pragma_once_normalize patch (which you may or may not want to comment ;) I tested all changes and all work on multiple targets. Great work. I have no comments on the pragma once patch. Sorry 😁 Herman ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] A double-to-float conversion bug
On 31.07.2023 13:42, Herman ten Brugge via Tinycc-devel wrote: 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? Hi, yes, that seems to make sense. Then again I think that align should additionally depend on just 'ret_align', as in if (ret_align > align) align = ret_align; I was working on a better fix for riscv and reverted the patch from Yao Zi by accident. I will reapply that change. Well, as I was trying to say, that change wasn't a good one either. Anyway, I just applied some more extensive fix for both. Hope it does work now. Also yet another pragma_once_normalize patch (which you may or may not want to comment ;) -- gr Herman ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] A double-to-float conversion bug
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 ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] A double-to-float conversion bug
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 ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] A double-to-float conversion bug
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. Herman ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel