Danny Milosavljevic writes:
Hello Danny,
> I just found the bug in tinycc that caused failed our ARM bootstrap to fail.
>
> I use the following reproducer:
>
> int main() {
> double f = 1.0;
> return 0;
> }
Beautiful! Well done! I can confirm that adding this patch
--8<---------------cut here---------------start------------->8---
diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index a50a238ddd..5a3fa694b3 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -1417,8 +1417,8 @@ ac_cv_c_float_format='IEEE (little-endian)'
(substitute* "test/Makefile.in"
(("^bigtest:.*") "bigtest: basic\n")
(("( |\t)(childin|convfmt|fflush|longwrds|math|negexp)" all
sep) sep))
- (substitute* "io.c"
- (("char rs;") "int rs;"))
+ (substitute* '("builtin.c" "eval.c" "io.c")
+ (("1.0") "1"))
#t))
(add-before 'configure 'setenv
(lambda _
--8<---------------cut here---------------end--------------->8---
to the gawk-mesbot0 recipe also fixes "inc.awk". The pre
increment/decrement code looks like this:
--8<---------------cut here---------------start------------->8---
*lhs = make_number(lval +
(tree->type == Node_preincrement ? 1.0 : -1.0));
--8<---------------cut here---------------end--------------->8---
> on ARM (32) using your patched tcc. (but it's also broken in the unpatched
> tcc)
>
> (tcc cross compiler is not enough. tcc has to actually itself be an ARM
> EABI executable)
>
> I get a bus error here:
>
> │ 0x24698 <init_putv+1688> vstr d0, [r0]
>
> │
Ah, so it may result in a wrong assingment, or bus error even. Great!
> Debugging some more, I find:
>
> tccgen.c:
>
> /* store a value or an expression directly in global data or in local array */
> static void init_putv(CType *type, Section *sec, unsigned long c)
> {
> [...]
> size = type_size(type, &align);
> section_reserve(sec, c + size); // c == 0, size == 8
> ptr = sec->data + c; // sec->data == 0x24b01e, c == 0
> switch(bt) {
> /* XXX: when cross-compiling we assume that each type has the
> same representation on host and target, which is likely to
> be wrong in the case of long double */
> case VT_BOOL:
> vtop->c.i = vtop->c.i != 0;
> case VT_BYTE:
> *(char *)ptr = vtop->c.i;
> break;
> case VT_SHORT:
> *(short *)ptr = vtop->c.i;
> break;
> case VT_FLOAT:
> *(float*)ptr = vtop->c.f;
> break;
> case VT_DOUBLE:
> *(double *)ptr = vtop->c.d;
> break;
> [... and so on]
Ah yes, this code has been problematic in the sense that I found bus errors
here and tried workarounds several times (look at the
wip-arm-bootstrap14 branch).
> tccelf.c:
>
> /* reserve at least 'size' bytes from section start */
> ST_FUNC void section_reserve(Section *sec, unsigned long size)
> {
> if (size > sec->data_allocated) // both 8
> section_realloc(sec, size);
> if (size > sec->data_offset) // both 8
> sec->data_offset = size;
> }
>
> Nothing here make sure that the VFP double is aligned to 8 Byte.
>
> And indeed, (0x24b01e % 8) == 6, not 0.
Ah...I wasn't aware of this requirement...
> Alignment could be disabled on the CPU
>
>
> https://developer.arm.com/documentation/ddi0464/f/system-control/register-descriptions/system-control-register
>
> but I don't think EABI wants that.
Hmm, what does this mean? We are not really using EABI, or are we? We
seem to need this terrible hack
--8<---------------cut here---------------start------------->8---
diff --git a/tccelf.c b/tccelf.c
index 2ac7466c..42546f57 100644
--- a/tccelf.c
+++ b/tccelf.c
@@ -1867,7 +1867,7 @@ static void tcc_output_elf(TCCState *s1, FILE *f, int
phnum, ElfW(Phdr) *phdr,
ehdr.e_ident[EI_OSABI] = ELFOSABI_FREEBSD;
#endif
#ifdef TCC_TARGET_ARM
-#ifdef TCC_ARM_EABI
+#if defined (TCC_ARM_EABI) || BOOTSTRAP
ehdr.e_ident[EI_OSABI] = 0;
ehdr.e_flags = EF_ARM_EABI_VER4;
if (file_type == TCC_OUTPUT_EXE || file_type == TCC_OUTPUT_DLL)
--8<---------------cut here---------------end--------------->8---
at least to get tcc's ARM binaries to run on Aarch64-Linux. Is this
related; iow, could it be that this "fix" for Aarch64 break ARM?
> tinycc does have:
>
> /* reserve at least 'size' bytes aligned per 'align' in section
> 'sec' from current offset, and return the aligned offset */
> ST_FUNC size_t section_add(Section *sec, addr_t size, int align)
> {
> size_t offset, offset1;
>
> offset = (sec->data_offset + align - 1) & -align;
> offset1 = offset + size;
> if (sec->sh_type != SHT_NOBITS && offset1 > sec->data_allocated)
> section_realloc(sec, offset1);
> sec->data_offset = offset1;
> if (align > sec->sh_addralign)
> sec->sh_addralign = align;
> return offset;
> }
>
> But that's not used for init_putv.
OK.
> And section_reserve, which is used, doesn't care about alignment at all.
>
> (it seems there's a reserved part and a data part in each section, and
> it holds that the data part elements are aligned--but the reserved part
> elements are NOT aligned. I don't see how sec->data would be aligned
> by the dynamic memory allocator either)
>
> Other notes:
>
> tccgen.c even has this:
>
>> /* XXX: when cross-compiling we assume that each type has the
>> same representation on host and target, which is likely to
>> be wrong in the case of long double */
>
> Yeah, and even when NOT cross-compiling, the alignment is wrong--which means
> it sometimes won't work at all on ARM, depending on luck.
>
> As a workaround, we can patch tcc to instead do the assignments on elements
> on the stack and then copy those over, instead of doing
>
> *(double *)ptr = vtop->c.d
>
> (the latter of which emits VFP instructions that expect double-aligned
> pointers).
So alignment should be fixed, but that's more work and you propose a
workaround, right? I'm struggling to understand the implications of
this last bit...guessing you will be preparing a patch for the mes-0.23
branch of our "bootstrappable tinycc"? Oh, and we need that same patch
for plain tcc-0.9.27, for "tcc-boot" of course!
Greetings,
Janneke
--
Jan Nieuwenhuizen <jann...@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com
