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

conv needs to be picky about aliases and introduces a temp for addr conv #24818

Merged
merged 6 commits into from
Apr 1, 2025

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Mar 26, 2025

ref #24817
ref #24815
ref status-im/nim-eth#784

{.emit:"""
void foo(unsigned long long* x)
{
}
""".}


proc foo(x: var culonglong) {.importc: "foo", nodecl.}

proc main(x: var uint64) =
  # var s: culonglong = u # TODO:
  var m = uint64(12)
  # var s = culonglong(m)
  foo(culonglong m)

var u = uint64(12)
main(u)

Notes that this code gives incompatible errors in 2.0.0, 2.2.0 and the devel branch. With this PR, conv is kept, but it seems to go back to #24807

@arnetheduck
Copy link
Contributor

arnetheduck commented Mar 27, 2025

so, taking strict aliasing into account, the correct code to generate in c is something like:

void main(NU64* v) {
  unsigned long long tmp = (unsigned long long)(*v);
  foo(&tmp);
  *v = tmp
}

is that the outcome after this PR? ie casting the pointer itself will lead to UB.

@ringabout
Copy link
Member Author

ringabout commented Mar 27, 2025

Okay, I was going to generate a temp for conv in addr conv anyway

@ringabout
Copy link
Member Author

ringabout commented Mar 27, 2025

so, taking strict aliasing into account, the correct code to generate in c is something like:

In this PR, It introduces a temp for the case of addr conv

T1_ = ((unsigned long long) (m_1));
foo(((&T1_)));

The rest could be resolved in the future

It requires #24817 to make CI green

@ringabout ringabout changed the title conv needs to be picky about aliases conv needs to be picky about aliases and introduces a temp for addr conv Mar 27, 2025
@@ -962,6 +962,11 @@ proc cowBracket(p: BProc; n: PNode) =
proc cow(p: BProc; n: PNode) {.inline.} =
if n.kind == nkHiddenAddr: cowBracket(p, n[0])

template noSomeCast(e: PNode): bool =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is noSomeCast? Name it ignoreConv or something that describes what this does.

@Araq Araq added the merge_when_passes_CI mergeable once green label Mar 31, 2025
@narimiran narimiran merged commit f9c8775 into devel Apr 1, 2025
21 checks passed
@narimiran narimiran deleted the pr_incompatible_ctypes branch April 1, 2025 07:37
Copy link
Contributor

github-actions bot commented Apr 1, 2025

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from f9c8775

Hint: mm: orc; opt: speed; options: -d:release
179074 lines; 8.984s; 652.078MiB peakmem

narimiran pushed a commit that referenced this pull request Apr 2, 2025
…r conv` (#24818)

ref #24817
ref #24815
ref status-im/nim-eth#784

```nim
{.emit:"""
void foo(unsigned long long* x)
{
}
""".}

proc foo(x: var culonglong) {.importc: "foo", nodecl.}

proc main(x: var uint64) =
  # var s: culonglong = u # TODO:
  var m = uint64(12)
  # var s = culonglong(m)
  foo(culonglong m)

var u = uint64(12)
main(u)
```
Notes that this code gives incompatible errors in 2.0.0, 2.2.0 and the
devel branch. With this PR, `conv` is kept, but it seems to go back to
#24807

(cherry picked from commit f9c8775)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants