Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code generation creates unecessary copy of global static arrays with custom type, breaks PROGMEM #17497

Open
PMunch opened this issue Mar 24, 2021 · 1 comment

Comments

@PMunch
Copy link
Contributor

PMunch commented Mar 24, 2021

While developing my keyboard firmware I noticed that the nice tricks me and @zevv where using to move .data from the data section of the binaries and into .text via the PROGMEM C macro didn't work any longer. Looking at the C sources it was obvious why, in 1.4.0 Nim would take this code:

type Test = array[3, uint8]
let myData {.codegenDecl: "N_LIB_PRIVATE const $# $# PROGMEM".} = Test([10'u8, 20, 30])

and generate this output (shortened identifiers for readability):

static NIM_CONST tyArray_ myData_ PROGMEM = {((NU8) 100), ((NU8) 200), ((NU8) 300)};

But with 1.4.2 the same Nim code would now generate this C code:

static NIM_CONST tyArray_ TM_ = {((NU8) 10), ((NU8) 20), ((NU8) 30)};
N_LIB_PRIVATE const tyArray_ myData_ PROGMEM;

N_LIB_PRIVATE N_NIMCALL(void, NimMainModule)(void) {
{
    nimCopyMem((void*)myData_, (NIM_CONST void*)TM_, sizeof(tyArray_));
    [...]
}

While obviously an unnecessary copy in any scenario it completely breaks the PROGMEM things as it isn't possible on these chips to modify the .text section while running (at least not via whatever mechanism nimCopyMem uses). This leaves myData uninitialised on runtime.

I ran a git bisect with an automated script and found commit 99032ca to be the one introducing this new behaviour. Looking through it I identified 99032ca#diff-ee922073e19918f7b88397ee2092b6f34c21b0e9983cac3c9a5dae1b32ad2273R214 as the breaking change and reverting that on 1.4.2 did in fact fix the issue. This commit seems to be meant to avoid duplicating Nim constants in code, but somehow it also affects the code generation for let assignments. Looking at the code it's not obvious how it would be called for let assignments, that might be a bug in itself.

For reproduceability here are the code snippets and bisect script I used to test this:
In a freshly cloned Nim repo I created a directory progmemtest with this file:
progmemtest.nim

type Test = array[3, uint8]

let colKeys = Test([10'u8, 20, 30])

And in the root directory of the repo I created this bisect script:
bisect.sh

#!/bin/bash
nim c -d:release koch
if [ $? -ne 0 ]; then
  exit 125
fi
./koch temp -d:release
if [ $? -ne 0 ]; then
  exit 125
fi
/tmp/nim/compiler/nim c --nimcache:progmemtest/nimcache -d:danger --opt:size --os:any --gc:arc -d:useMalloc progmemtest/progmemtest.nim
if [ $? -ne 0 ]; then
  exit 125
fi
if grep -q "nimCopyMem" 'progmemtest/nimcache/@mprogmemtest.nim.c'; then
  exit 1
else
  exit 0
fi

Then I checked out the v1.4.0 branch and ran git bisect start; git bisect good; git bisect bad v1.4.2; git bisect run ./bisect.sh

@PMunch
Copy link
Contributor Author

PMunch commented Mar 24, 2021

It seems like this is a rather dumb check. By changing the code to:

type Test = array[3, uint8]

let colKeys: Test = [10'u8, 20, 30]

It will work, but this obviously shouldn't matter, and makes writing a good PROGMEM system a bit difficult..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant