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

workaround codegen error for Base10.decode #111

Merged
merged 3 commits into from
Aug 24, 2023
Merged

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented May 17, 2022

Calling Base10.decode may lead to different structures being generated
for use with uint64.

The one normally generated is:

struct tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg {NIM_BOOL o;
union{
struct {NCSTRING e;
} _o_1;
struct {unsigned long long v;
} _o_2;
};

But sometimes, it may be generated as:

struct tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg {NIM_BOOL o;
union{
struct {NCSTRING e;
} _o_1;
struct {NU64 v;
} _o_2;
};

When the latter is generated, the compiler throws with:

error: passing 'tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg' (aka 'struct tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg') to parameter of incompatible type 'tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg' (aka 'struct tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg')

for

proc getInt*(ht: HttpTables, key: string): uint64 =
  let res = Base10.decode(uint64, ht.getString(key))
  if res.isOk():
    res.get()    # This line may lead to the compiler error above
  else:
    0'u64

By passing the type as a generic param, the unsigned long long version
gets consistently generated / used regardless of include order.

Minimal POC to trigger the bug, from nimbus-eth2 root:

echo 'import beacon_chain/conf, beacon_chain/sync/sync_manager' >x.nim
nim c -d:"libp2p_pki_schemes=secp256k1" -r x

Swapping include order (conf after sync_manager) works.

Calling `Base10.decode` may lead to different structures being generated
for use with `uint64`.

The one normally generated is:

```
struct tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg {NIM_BOOL o;
union{
struct {NCSTRING e;
} _o_1;
struct {unsigned long long v;
} _o_2;
};
```

But sometimes, it may be generated as:

```
struct tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg {NIM_BOOL o;
union{
struct {NCSTRING e;
} _o_1;
struct {NU64 v;
} _o_2;
};
```

When the latter is generated, the compiler throws with:
```
error: passing 'tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg' (aka 'struct tyObject_Result__xZhi1m1g75ioXsKjx9bN5bg') to parameter of incompatible type 'tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg' (aka 'struct tyObject_Result__559ckyoL0ZZBsNFIYXjaoeg')
```
for
```
proc getInt*(ht: HttpTables, key: string): uint64 =
  let res = Base10.decode(uint64, ht.getString(key))
  if res.isOk():
    res.get()    # This line may lead to the compiler error above
  else:
    0'u64
```

By passing the type as a generic param, the `unsigned long long` version
gets consistently generated / used regardless of include order.

Minimal POC to trigger the bug, from `nimbus-eth2` root:
```
echo 'import beacon_chain/conf, beacon_chain/sync/sync_manager' >x.nim
nim c -d:"libp2p_pki_schemes=secp256k1" -r x
```
Swapping include order (`conf` after `sync_manager`) works.
@etan-status etan-status changed the title fix codegen for Base10.decode workaround codegen error for Base10.decode May 18, 2022
@arnetheduck
Copy link
Member

rebasing should help clear the CI failures here

@arnetheduck
Copy link
Member

is there an upstream bug for this?

@etan-status
Copy link
Contributor Author

is there an upstream bug for this?

Not yet, I have not found the time to minimize this to a reproducible example.

@arnetheduck
Copy link
Member

@etan-status is this still relevant after the latest Result fixes?

@etan-status
Copy link
Contributor Author

Yep, just tried it again and it is still broken.

From nimbus-eth2 repo root:

echo 'import beacon_chain/conf, beacon_chain/sync/sync_manager' >x.nim
nim c -d:"libp2p_pki_schemes=secp256k1" -r x
/Users/etan/Documents/Repos/nimbus-eth2/vendor/nim-chronos/chronos/apps/http/httptable.nim:82:73: error: passing 'tyObject_Result__wclb9c9bJbd6rJAwn3uS2nPg' (aka 'struct tyObject_Result__wclb9c9bJbd6rJAwn3uS2nPg') to parameter of incompatible type 'tyObject_Result__v9aWRrNLc0mAO9a315N8TSpA' (aka 'struct tyObject_Result__v9aWRrNLc0mAO9a315N8TSpA')
                result = value__vendorZnim45chronosZchronosZappsZhttpZhttpserver_1477(res);     }
                                                                                      ^~~
/Users/etan/Documents/Repos/nimbus-eth2/vendor/nim-results/results.nim:796:127: note: passing argument to parameter 'self' here
static N_INLINE(NU64, value__vendorZnim45chronosZchronosZappsZhttpZhttpserver_1477)(tyObject_Result__v9aWRrNLc0mAO9a315N8TSpA self) {   
                                                                                                                              ^
1 error generated.

And no, still couldn't find the time to minimize this example for upstream reporting.

@etan-status etan-status merged commit 3159137 into master Aug 24, 2023
12 checks passed
@etan-status etan-status deleted the dev/etan/base10-type branch August 24, 2023 22:04
@tersec
Copy link
Contributor

tersec commented Jun 27, 2024

nim-lang/Nim#23765

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

Successfully merging this pull request may close these issues.

3 participants