nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.63k stars 1.47k forks source link

Calling function with static uint64 openArray may convert data to uint32 #19157

Open etan-status opened 3 years ago

etan-status commented 3 years ago

When passing a static openArray[uint64] to another function, in some cases the Nim compiler generates incorrect code if the caller has the same function name. Instead of passing the uint64 array as is, the compiler generates a copy of the array with all data members converted to pointers instead of NU64, and passes that copy instead. The callee still interprets the array as NU64[]. On 32-bit platforms this leads to incorrect followup computations and out-of-bounds memory access (as the 32-bit pointer array is smaller than the 64-bit uint64 array).

Creating a minimal reproducible example showcasing this bug proved difficult.

Example

Logging an uint64 openArray at call site and at callee site, with $:

Caller:

@[54, 26, 12, 7, 2]

Current Output

Callee:

[111669149750, 30064771084, 2, 0, 0]

Notes:

111669149750 and 0xFFFFFFFF == 54
111669149750 shr 0x20 == 26
30064771084 and 0xFFFFFFFF == 12
30064771084 shr 0x20 == 7

Sometimes, 2 is paired with uninitialized memory (and the 0-values can be uninitialized as well).

Expected Output

[54, 26, 12, 7, 2]

Possible Solution

Renaming the called function works around the problem.

Additional Information

Generated C code attached (only Nim code difference is the function re-name).

Bugged:

static NIM_CONST struct {  TGenericSeq Sup;  tySequence__YdLNCDKYeipzJx3I8Xw82Q* data[5];} TM__iI3OAqVC9c0ZHUGXaPrS8zg_1302 = {{5, 5 | NIM_STRLIT_FLAG}, {54ULL,
26ULL,
12ULL,
7ULL,
2ULL}};static NIM_CONST tySequence__KsLe7fDedG9bDQbwtcCLMuQ* TM__iI3OAqVC9c0ZHUGXaPrS8zg_1301 = ((tySequence__KsLe7fDedG9bDQbwtcCLMuQ*)&TM__iI3OAqVC9c0ZHUGXaPrS8zg_1302);

Fixed (renamed function):

static NIM_CONST struct {  TGenericSeq Sup;  NU64 data[5];} TM__iI3OAqVC9c0ZHUGXaPrS8zg_1302 = {{5, 5 | NIM_STRLIT_FLAG}, {54ULL,
26ULL,
12ULL,
7ULL,
2ULL}};static NIM_CONST tySequence__YdLNCDKYeipzJx3I8Xw82Q* TM__iI3OAqVC9c0ZHUGXaPrS8zg_1301 = ((tySequence__YdLNCDKYeipzJx3I8Xw82Q*)&TM__iI3OAqVC9c0ZHUGXaPrS8zg_1302);
$ nim -v
Nim Compiler Version 1.2.14 [Linux: i386]
Compiled at 2021-11-15
Copyright (c) 2006-2020 by Andreas Rumpf

git hash: 236f0d10f6d405a333e4f5cb711efb91c2bfd411
active boot switches: -d:release
Araq commented 3 years ago

Is version 1.6 also affected?

etan-status commented 1 year ago

This issue still exists on version-1-2 as of Nim v1.2.16, but it doesn't seem that version-1-6 is affected.

The oldest version-1-6 commit that compiles the test code is ee876aee28a93d45f95ae605199b19274eee92c2 (6 Dec 2021), earlier than that would require updates to the sample code.

I'm not sure which Nim commit fixed the issue, or if the problem is just hidden on version-1-6 as a side effect of something else.

I did not manage to produce a minimized example of this issue, but the instruction to reproduce the problem on the bigger example on version-1-2 follow below.


To repro, checkout status-im/nimbus-eth2 commit aa1b8e4a17683d35ded3c3e603c7e9ded5e0eda9, then:

  1. ulimit -n 1024 && rm -rf ~/.cache ~/.nimble build nimcache vendor .git/modules && git submodule sync --recursive
  2. docker run --rm -v ~/Documents/Repos:/Repos -it i386/ubuntu:20.04
  3. export ARCH=32 PLATFORM=x86 CFLAGS='-m32 -mno-adx' && apt update && apt full-upgrade -y && apt install -y build-essential curl git && cd /Repos/nimbus-eth2 && make -j16 deps ARCH_OVERRIDE=$PLATFORM LOG_LEVEL=TRACE V=1 && source env.sh
  4. rename hash_tree_root_multi to hash_tree_root across entire codebase, including submodules

Then, for each compiler version:

commit={{ NIM_COMPILER_VERSION }}
make -j16 deps NIM_COMMIT=$commit ARCH_OVERRIDE=$PLATFORM LOG_LEVEL=TRACE V=1
nim c -p:vendor/nim-unittest2 -p:vendor/nim-presto -p:vendor/nim-libp2p -p:vendor/nim-nat-traversal -p:vendor/nim-normalize/src -p:vendor/nim-unicodedb/src -p:vendor/nim-toml-serialization -p:vendor/nim-confutils -p:vendor/nim-secp256k1 -p:vendor/nim-zlib -p:vendor/nim-websock -p:vendor/nim-http-utils -p:vendor/nim-json-rpc -p:vendor/nim-metrics -p:vendor/nim-sqlite3-abi -p:vendor/nim-eth -p:vendor/nim-snappy -p:vendor/nim-testutils -p:vendor/nim-bearssl -p:vendor/nim-taskpools -p:vendor/nim-blscurve -p:vendor/nim-web3 -p:vendor/nimcrypto -p:vendor/nim-stint -p:vendor/nim-ssz-serialization -p:vendor/nim-chronos -p:vendor/nim-serialization -p:vendor/nim-json-serialization -p:vendor/nim-faststreams -p:vendor/nim-stew -p:vendor/nim-chronicles -d:disable_libbacktrace -d:PREFER_BLST_SHA256=false --passC:"-m32 -mno-adx" -r tests/consensus_spec/altair/test_fixture_sync_protocol