gfwilliams / EspruinoCompiler

Node.js-based JavaScript->C++ compiler
25 stars 3 forks source link

no space allocated for BSS section - uninitialized global variables corrupt memory #15

Closed fanoush closed 1 year ago

fanoush commented 1 year ago

see also conversation https://forum.espruino.com/conversations/383232/

I also remember hitting this. Workaround is to initialize variable with nonzero value like int a = -1; below so that it goes into .data section, not .bss

var c = E.compiledC(`
// void init()
// int f()
int a = -1;
void init() {
  a = 0;
}
int f() {
  a++;
  return a;
}
`);

Here is the difference not initialized or initialized to zero ends with:

00000018 <_Z4initv>:
  18:   4b02            ldr     r3, [pc, #8]    ; (24 <_Z4initv+0xc>)
  1a:   447b            add     r3, pc
  1c:   2200            movs    r2, #0
  1e:   601a            str     r2, [r3, #0]
  20:   4770            bx      lr
  22:   bf00            nop
  24:   0000000a        andeq   r0, r0, sl

Disassembly of section .bss:

00000028 <a>:
  28:   00000000        andeq   r0, r0, r0

----------------------------------------
(function(){
  var bin=atob("Akt7RBhoATAYYHBHGgAAAAJLe0QAIhpgcEcAvwoAAAA=");
  return {
    init:E.nativeCall(17, "void()", bin),
    f:E.nativeCall(1, "int()", bin),
  };
})()
----------------------------------------

Initialized ends with

00000018 <_Z4initv>:
  18:   4b02            ldr     r3, [pc, #8]    ; (24 <_Z4initv+0xc>)
  1a:   447b            add     r3, pc
  1c:   2200            movs    r2, #0
  1e:   601a            str     r2, [r3, #0]
  20:   4770            bx      lr
  22:   bf00            nop
  24:   0000000a        andeq   r0, r0, sl

Disassembly of section .data:

00000028 <a>:
  28:   ffffffff                        ; <UNDEFINED> instruction: 0xffffffff

----------------------------------------
(function(){
  var bin=atob("Akt7RBhoATAYYHBHGgAAAAJLe0QAIhpgcEcAvwoAAAD/////");
  return {
    init:E.nativeCall(17, "void()", bin),
    f:E.nativeCall(1, "int()", bin),
  };
})()
----------------------------------------

it can be seen that the var bin string is longer in initialized case woAAAA vs woAAAD/////

One possible fix is to put everything into text segment in the linker file as we load to RAM anyway and don't support separate data vs code and relocation of data references. Or at least the .bss can be included into .data.

diff --git a/inc/linker.ld b/inc/linker.ld
index 7ea3647..7d4d609 100644
--- a/inc/linker.ld
+++ b/inc/linker.ld
@@ -22,17 +22,21 @@ SECTIONS
     *(.rodata)
     *(.rodata*)
     . = ALIGN(4);
+/*
   } >RAM

   .data :
   {
     . = ALIGN(4);
+*/
     *(.data)           /* .data sections */
     *(.data*)          /* .data* sections */
+/*
   } >RAM

   .bss :
   {
+*/
     /* This is used by the startup in order to initialize the .bss secion */
     *(.bss)
     *(.bss*)
fanoush commented 1 year ago

then it looks like this

00000000 <entryPoint>:
   0:   00000019        andeq   r0, r0, r9, lsl r0
   4:   00000009        andeq   r0, r0, r9

Disassembly of section .text:

00000008 <_Z1fv>:
   8:   4b02            ldr     r3, [pc, #8]    ; (14 <_Z1fv+0xc>)
   a:   447b            add     r3, pc
   c:   6818            ldr     r0, [r3, #0]
   e:   3001            adds    r0, #1
  10:   6018            str     r0, [r3, #0]
  12:   4770            bx      lr
  14:   0000001a        andeq   r0, r0, sl, lsl r0

00000018 <_Z4initv>:
  18:   4b02            ldr     r3, [pc, #8]    ; (24 <_Z4initv+0xc>)
  1a:   447b            add     r3, pc
  1c:   2200            movs    r2, #0
  1e:   601a            str     r2, [r3, #0]
  20:   4770            bx      lr
  22:   bf00            nop
  24:   0000000a        andeq   r0, r0, sl

00000028 <a>:
  28:   00000000        andeq   r0, r0, r0

----------------------------------------
(function(){
  var bin=atob("Akt7RBhoATAYYHBHGgAAAAJLe0QAIhpgcEcAvwoAAAAAAAAA");
  return {
    init:E.nativeCall(17, "void()", bin),
    f:E.nativeCall(1, "int()", bin),
  };
})()
----------------------------------------
gfwilliams commented 1 year ago

Thanks for spotting this!

Just shoving everything into the same segment feels like the way to go... Do you want to do a PR, or shall I just make those changes you suggest myself?

fanoush commented 1 year ago

I have just commented out the parts and was not sure if just merging .bss into .data or putting everything to .text is preferable. So if you are for putting everything into text I'll just delete commented out stuff and make a PR with everything in .text.

BTW, unfortunately it does not help with producing smaller code as I mentioned in the conversation. For that only attribute helps as it probably knows the location earlier in optimization phase and can generate proper code. When linker reshuffles stuff later into same section the suboptimal code is already generated. One example, I have code like this

uint16_t blit_bpp=0;
uint16_t blit_w=0;
uint16_t blit_h=0;
uint16_t blit_stride=0;
void blit_setup(uint16_t w,uint16_t h,uint16_t bpp, uint16_t stride){
  blit_bpp=bpp;blit_w=w;blit_h=h;blit_stride=stride; //*bpp/8;
}

and when I mark those variables with attribute section .text it generates this

Disassembly of section .text:

00000024 <blit_bpp>:
00000026 <blit_w>:
00000028 <blit_h>:
0000002a <blit_stride>:
...
00000044 <_Z10blit_setuptttt>:
  44:   b510            push    {r4, lr}
  46:   4c03            ldr     r4, [pc, #12]   ; (54 <_Z10blit_setuptttt+0x10>)
  48:   447c            add     r4, pc
  4a:   8022            strh    r2, [r4, #0]
  4c:   8060            strh    r0, [r4, #2]
  4e:   80a1            strh    r1, [r4, #4]
  50:   80e3            strh    r3, [r4, #6]
  52:   bd10            pop     {r4, pc}
  54:   ffffffd8                        ; <UNDEFINED> instruction: 0xffffffd8

By default it generates this an liker script does not help with it

00000024 <_Z10blit_setuptttt>:
  24:   b510            push    {r4, lr}
  26:   4c06            ldr     r4, [pc, #24]   ; (40 <_Z10blit_setuptttt+0x1c>)
  28:   447c            add     r4, pc
  2a:   8022            strh    r2, [r4, #0]
  2c:   4a05            ldr     r2, [pc, #20]   ; (44 <_Z10blit_setuptttt+0x20>)
  2e:   447a            add     r2, pc
  30:   8010            strh    r0, [r2, #0]
  32:   4a05            ldr     r2, [pc, #20]   ; (48 <_Z10blit_setuptttt+0x24>)
  34:   447a            add     r2, pc
  36:   8011            strh    r1, [r2, #0]
  38:   4a04            ldr     r2, [pc, #16]   ; (4c <_Z10blit_setuptttt+0x28>)
  3a:   447a            add     r2, pc
  3c:   8013            strh    r3, [r2, #0]
  3e:   bd10            pop     {r4, pc}
  40:   00000494        muleq   r0, r4, r4
  44:   00000494        muleq   r0, r4, r4
  48:   0000048a        andeq   r0, r0, sl, lsl #9
  4c:   00000486        andeq   r0, r0, r6, lsl #9

so every variable reference is loaded vie separate offset and pc register :-(

Anyway I can delete those .data,.bss sections in linker and make a PR with everything in .text.

gfwilliams commented 1 year ago

I'm just wondering now, do you think there's a way to get a section that contains the pre-initialised variables with their values? I was pretty sure that for ARM we generally had one of those and then effectively did a memcpy in the startup code.

If that's the case then we could ensure evrything goes into the one section, but then load the initial values in compiler.js and manually merge it in before upload?

fanoush commented 1 year ago

Sorry, I'm lost, that is the .data section? That one is normally copied from flash to RAM at program startup.

Not sure what you are aiming at but I was thinking about actually splitting data and code so that code could live in internal flash and only writable data is in ram, but that needs handling of relocations of data references and with storage in SPI flash the code inside storage file would not be executable anyway.

gfwilliams commented 1 year ago

Sorry, I'm lost, that is the .data section? That one is normally copied from flash to RAM at program startup.

Yes. I think I misunderstood the code you posted and thought it was generating instruction code to initialise that data section instead of just having the data there.

It's been a while since I looked into this so I can't 100% remember what went on

was thinking about actually splitting data and code so that code could live in internal flash and only writable data is in ram

Ahh ok, personally I think that's just going to end up being too painful to do nicely. For folks who are going that far, it's really not that hard just to build their own firmwares...

gfwilliams commented 1 year ago

... but am I right in thinking that right now if I did:

int i = 42;

We would end up with 42 in the section and i in there too? Or would there just be one integer for i which was pre-set to 42?

fanoush commented 1 year ago

The value is already there preinitialized (now in .text segment, previously in .data segment). Same location is modified in place once the code runs.

var c = E.compiledC(`
// int f()
int i=42;
int f() {
  i++;
  return i;
}
`);

gives

Disassembly of section .entrypoint:

00000000 <entryPoint>:
   0:   00000005        andeq   r0, r0, r5

Disassembly of section .text:

00000004 <_Z1fv>:
   4:   4b02            ldr     r3, [pc, #8]    ; (10 <_Z1fv+0xc>)
   6:   447b            add     r3, pc
   8:   6818            ldr     r0, [r3, #0]
   a:   3001            adds    r0, #1
   c:   6018            str     r0, [r3, #0]
   e:   4770            bx      lr
  10:   0000000a        andeq   r0, r0, sl

00000014 <i>:
  14:   0000002a        andeq   r0, r0, sl, lsr #32

----------------------------------------
(function(){
  var bin=atob("Akt7RBhoATAYYHBHCgAAACoAAAA=");
  return {
    f:E.nativeCall(1, "int()", bin),
  };
})()
----------------------------------------

it this the 14: 0000002a bit.

fanoush commented 1 year ago

So once you load it an run, the original value in RAM is gone. It is not a problem though, it is still part of the var bin=atob(".."), To get back to original value you need to rerun the original function and recreate the bin string into ram again.

gfwilliams commented 1 year ago

Ahh, ok, perfect - thanks! So looks like all that works as expected already

gfwilliams commented 1 year ago

So I guess this is fixed with https://github.com/gfwilliams/EspruinoCompiler/pull/16 now