Re: [bootstrappable] Re: wip-full-source-bootstrap: from a 357-byte `hex0' to 'hello'

[Jan Nieuwenhuizen]

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 <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com