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

Casting a seq to another seq generates invalid code with --newruntime #11018

Closed
Clyybber opened this issue Apr 12, 2019 · 4 comments

Comments

Projects
None yet
2 participants
@Clyybber
Copy link
Contributor

commented Apr 12, 2019

Casting a seq or a string to another seq generates invalid C code with --newruntime

A couple of examples:

seq -> seq

discard cast[seq[uint8]](@[1])

string -> seq

discard cast[seq[uint8]]("test")

Current Output

Error: execution of an external compiler program 'gcc -c  -w  -I/home/clyybber/builds/nim/lib -I/home/clyybber/projects/tests -o /home/clyybber/.cache/nim/convrt_d/convrt.c.o /home/clyybber/.cache/nim/convrt_d/convrt.c' failed with exit code: 1

/home/clyybber/.cache/nim/convrt_d/convrt.c: In function ‘NimMainModule’:
/home/clyybber/.cache/nim/convrt_d/convrt.c:157:2: error: conversion to non-scalar type requested
  (void)(((tySequence_6H5Oh5UUvVCLiakt9aTwtUQ) (TM_KUSKOxFeGyNT8JCQmxu3cw_2)));
  ^

Expected Output

Compile successfully

Additional Information

Casting an array to a seq does not generate invalid code:

discard cast[seq[uint8]]([1])
$ nim -v
Nim Compiler Version 0.19.9

Clyybber added a commit to Clyybber/Nim that referenced this issue Apr 12, 2019

@Clyybber Clyybber referenced this issue Apr 12, 2019

Merged

Fix #11018 #11019

@krux02

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

I just tried to reproduce the bug, these are my results:

echo cast[seq[uint8]](@[1,2,3,4,5,6,7,8,9])   # @[1, 0, 0, 0, 0, 0, 0, 0, 2]
echo cast[seq[uint8]]("test")                 # @[116, 101, 115, 116]
echo cast[seq[uint8]]([1])                    # segfault (as expected)

What you are doing is very risky, it exploits similarities in the implementation of seq and string, even though the implementation of seq and string is not specified. It is allowed to be changed in future versions of Nim, and code like this will be broken.

@Clyybber

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@krux02 I'm aware that this is missing setLen and is risky, but this is not about running the program successfully, but compiling it.
You probably missed the --newruntime switch when you tried to reproduce this.

@krux02

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

yes you are right, I did miss the newruntime switch. And no it is not about the setLen proc. Casting basically tells the nim compiler to interpret some memory in a different way. In my example above the length (9) from the seq[int] (98 bytes = 72 bytes of data) is interpreted as the length of a seq[uint8] (91 bytes = 9 bytes of data). Doing it this way doesn't cause much harm, you just don't see most of the data in the seq. Doing it the other way around will give you most likely a seqfault as iterating the seq will try to access memory that it does not own.

@Clyybber

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@krux02 Yeah, I'm aware. When doing it the other way around,

someSeq.setLen((someSeq.len * sizeof(originalType)/sizeof(TypeWeCastedTo))).ceil.int)

or

someSeq.setLen((originalSeq.len / sizeof(TypeWeCastedTo)/sizeof(originalType))).ceil.int)

is required.
Interestingly your previous code sample using echo instead of discard revealed a big problem in my attempt to fix this issue in #11019 , that is, it's missing the len of the seq as it only casts the contents of the seq, so the first snippet would no longer work.
Also this makes $ no longer work on the casted seq, as it has no length :P

Clyybber added a commit to Clyybber/Nim that referenced this issue Apr 14, 2019

Clyybber added a commit to Clyybber/Nim that referenced this issue Apr 14, 2019

@Araq Araq closed this in 499fa3f Apr 14, 2019

Clyybber added a commit to Clyybber/Nim that referenced this issue Apr 15, 2019

Araq added a commit that referenced this issue Apr 15, 2019

Extend the fix for #11018 to strings (#11031)
* Extend the fix for #11018 to strings

* Fix testcase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.