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

cast to non ptr UncheckedArray does not produce an error or warning #9403

Closed
cooldome opened this issue Oct 16, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@cooldome
Copy link
Member

commented Oct 16, 2018

Used to compile:

type
    MyObj = ref object
      len: int
      val: UncheckedArray[float]
  
   
proc spot(x: MyObj): int64 =
  result = cast[UncheckedArray[int64]](x.val)[0]
  
   
proc newMyObj(len: int): MyObj =  
  unsafeNew(result, sizeof(result[]) + len * sizeof(float))
  result.len = len
    
echo spot(newMyObj(2))

Now generated C fails to compile with error:

nimcache\regression1_d\compiler_regression1.c: In function 'spot_a7qWq7kPv9bXGgmHKgB6vbw':
nimcache\regression1_d\compiler_regression1.c:140:12: error: cast specifies array type
  result = ((tyUncheckedArray_jpBG9aCOjd7lf9cMZpCfZm9bw) ((*x).val))[((NI) 0)];

simple workarounds exists

@LemonBoy

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

This line result = cast[UncheckedArray[int64]](x.val)[0] compiled down to this:

	NI64 result;
	union { tyArray_4U49bqmiA9asEIzdSU1qDkBw source; tyArray_x8Nj1Cq8K2eROjl9a5tAFLQ dest; } LOC1;
	nimfr_("spot", "foo.nim");
	result = (NI64)0;
	nimln_(8, "foo.nim");
	nimCopyMem((void*)LOC1.source, (NIM_CONST void*)(*x).val, sizeof(tyArray_4U49bqmiA9asEIzdSU1qDkBw));
	result = LOC1.dest[(((NI) 0))- 0];
	popFrame();
	return result;

And to me this is complete nonsense, you're only copying a single element over (tyArray_4U49bqmiA9asEIzdSU1qDkBw has size 1*sizeof(NF)) and accessing everything but the zeroth element means you're wandering somewhere in the stack frame.
A bare UncheckedArray (not prefixed with ref or ptr) should never be used IMO.

@cooldome

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

I don't mind a solution that will disallow such kind of casts in semantic pass, they are of minor usefulness, but still can be slightly useful.

I do not agree with the statement:

A bare UncheckedArray (not prefixed with ref or ptr) should never be used IMO.

Having unchecked array as a tail in your object is pretty common idiom in C for example. This is also how current GC seq is implement in Nim

@Araq

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Having unchecked array as a tail in your object is pretty common idiom in C for example. This is also how current GC seq is implement in Nim

This is also explicitly mentioned in Nim's spec.

@LemonBoy

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

I do not agree with the statement:

A bare UncheckedArray (not prefixed with ref or ptr) should never be used IMO.

I meant outside of an object declaration, I'm well aware of the extensible-array trick and am also aware that the compiler should be more strict about those (see #9275)

I don't mind a solution that will disallow such kind of casts in semantic pass, they are of minor usefulness, but still can be slightly useful.

Not really, if you want to cast an array/seq/whatever to a UncheckedArray you go trough its address.
If you write something like this you're getting a full-blown copy of the q array in z.

var q = [1,2,3]
var z = cast[array[3,int]](q)
echo z

If you try to apply the same logic to a UncheckedArray you can easily see that we don't know how many elements we have and we'd end up copying a single element instead (but that's an implementation detail).
The bottom line is, I'm gonna add a check in isCastable in order to make this an error.

@krux02 krux02 changed the title Unchecked array regression cast to non ptr UncheckedArray does not produce an error or warning Oct 20, 2018

@Araq Araq closed this in 0ecaaa8 May 5, 2019

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.