fix #13902 distinct uint64 type corruption on 32-bit with borrow

[timotheecour]

• fix "distinct uint64" type corruption on 32-bit, when using {.borrow.} operators #13902 (regression introduced in catchable defects #13626)

• note: also fixes this replated cpp codegen compilation error
  nim cpp -r --skipParentCfg --skipUserCfg --cpu:i386 --passC:"-m32" --passL:-m32 --stacktrace:off --hints:off -f --lib:lib tests/ccgbugs/t13902.nim

nimbase.h:562:67: error: cannot convert ‘NU64* {aka long long unsigned int*}’ to ‘int*’ for argument ‘3’ to ‘bool __builtin_sadd_overflow(int, int, int*)’
     #define nimAddInt(a, b, res) __builtin_sadd_overflow(a, b, res)
                                                                   ^
/home/ubuntu/.cache/nim/t13902_d/@mt13902.nim.cpp:125:7: note: in expansion of macro ‘nimAddInt’
   if (nimAddInt(s__h3z8bpua5apKtCPZQW9cs9ag, 1ULL, &TM__vd9bcdJbL6EBJYiW1bp6KQQ_3)) { raiseOverflow(); };

note: couldnt' figure out how to make cross compile from linux 64 to 32 on work on CI machines, so I'm just testing without cross compilation

TODO after PR

fix other issues I raised in #13626 (comment) + other comments in that PR

[alaviss]

note: couldnt' figure out how to make cross compile from linux 64 to 32 on work on CI machines, so I'm just testing without cross compilation

You don't have to, the CI already does this for 32 bit linux.

[timotheecour]

You don't have to, the CI already does this for 32 bit linux.

How so? azure-pipelines.yml includes CPU:i386 (for which this test will run) and CPU:amd64 but I'm not seeing anything related to cross compilation (ie compile + run on a 64bit host (say CPU:amd64) a 32bit program.

IMO it's an important use case that should have a regression test (it currently works when doing it manually inside a 64bit ubuntu (say a AWS VM), but is not being tested in CI)

[alaviss]

AFAICT, this is not necessary. To the compiler, the native architecture is just what to generate code for by default. So using `--cpu:i386` should yield the same code as a compiler compiled for i386 natively. Also, Azure Pipelines don't have any 32 bit VM, so how do you think we're doing 32 bit CI? :)

[timotheecour]

AFAICT, this is not necessary. To the compiler, the native architecture is just what to generate code for by default

that's assuming no bugs in implementation, and isolation from host machine environment, which, unless there's a regression test for it, is likely to lead to regressions.

Also, Azure Pipelines don't have any 32 bit VM, so how do you think we're doing 32 bit CI? :)

I don't follow. We have this:

      Linux_i386:
        vmImage: 'ubuntu-16.04'
        CPU: i386

and indeed: https://dev.azure.com/nim-lang/Nim/_build/results?buildId=4201&view=logs&jobId=ba7bbfa4-f55c-5c34-6d52-1b6b4edd3f37&j=ba7bbfa4-f55c-5c34-6d52-1b6b4edd3f37&t=9859ff42-7c6c-5e13-a955-d92d36755b5f
shows:

hostOS: linux, hostCPU: i386, int: 4, float: 8, cpuEndian: littleEndian, cwd: /home/vsts/work/1/s

[alaviss]

I'd invite you to study this:

https://github.com/nim-lang/Nim/blob/devel/azure-pipelines.yml#L67-L108

We literally take a multilib compiler then force it to generate 32bit code :) That's how it's done. the CPU variable means nothing in Azure Pipelines. It's just an environment variable that I use :)

Why does koch said hostCPU: i386? Because that's the architecture I configured the compiler to believe as the native.

[mratsim]

Can we have this in 1.2.2 as backport (cc @narimiran)?

At Status, we currently use 1.2.0 with this patched in manually to workaround the breakage introduced in 1.2.0 (status-im/nimbus-eth2#829 (comment)).

This also prevents people for targeting the stable Nim release in all libraries that use distinct types for safety.

[Araq]

@mratsim Definitely, but we wait for a more bugfixes for --gc:arc etc until we release 1.2.2.

[timotheecour]

the standard way to do this is to have 1 branch per major.minor versions:

• https://github.com/nim-lang/Nim/tree/v1.2.x
• https://github.com/nim-lang/Nim/tree/v1.0.x
  these branches can keep getting backports (with zero delay if done automatically, or with a just a bit of delay if done manually) and don't affect stable release tags (eg 1.2.0 and 1.2.2 etc)

That way, ppl who need to stay on a non-devel version of nim (eg 1.2.x) can still benefit from latest bugfixes, with the caveat that only stable release tags (eg 1.2.2) are thoroughly tested (ie 1.2.x HEAD could theoretically introduce a regression in rare cases)

[narimiran]

the standard way to do this is to have 1 branch per major.minor versions

....just like we're already doing it.

[timotheecour]

....just like we're already doing it.

I wasn't aware; where is the branch v1.2.x or v1.0.x or whatever equivalent of it ?
All I'm seeing are git release tags v1.2.0 v1.0.6 etc (which isnt' the same since, as i described above, you can't get latest backported fixes in a major.minor (eg v1.2.x) until v1.2.2 will be released)

EDIT: i think I found it: version-1-2 etc, cool, didn't know it existed (i guess it just needs to be updated on some frequency)

[timotheecour]

@mratsim

Can we have this in 1.2.2 as backport (cc @narimiran)?
@mratsim Definitely, but we wait for a more bugfixes for --gc:arc etc until we release 1.2.2.

if you don't want to wait, simply send a PR like #14024 targeting branch version-1-2 (looks like that was the 1st PR targeting a branch other than devel so maybe discoverability could be improved in PR template), with all the caveats implied by a non-point release (but I guess a patch release could be added)

[narimiran]

if you don't want to wait, simply send a PR like #14024 targeting branch version-1-2

Please, don't make your own rules for the stuff you're not in charge.

Your PR #14024 was an exception, not a rule nor a guide how to do things. PRs should, as always, target devel (and only devel).

P.S. This has already been backported to 1.2.x

[timotheecour]

Your PR #14024 was an exception, not a rule nor a guide how to do things. PRs should, as always, target devel (and only devel).

that PR was a backport (ie, via git cherry-pick) of the original PR (#13876) that targetted devel. I don't see any other way.

see #13876 (comment)

@narimiran thoughts on backport? I don't even know where prefixDir came from; not this PR, I guess? Or maybe it's just broken in 1.0?