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

`lent T` can return garbage #10942

Closed
cooldome opened this issue Apr 1, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@cooldome
Copy link
Member

commented Apr 1, 2019

I was able to reproduce this problem only with --cpu:i386 --cc:vcc compiler arguments, but the problem is fundamental. Test case:

type
  MyField = object
    b: seq[string]

  MyObject = object
    f: MyField

proc getX(x: MyObject): lent MyField = 
  x.f

let a = MyObject()
echo a.getX.b.len

# doAssert: a.getX().unsafeAddr == a.f

The generated code for getX proc has fundamental flaw:

N_LIB_PRIVATE N_NIMCALL(tyObject_MyField_TRBCGJu2GrSeYqJblPj9bNA*, 
getX_bA682BkQAkX49bq4vsNGpig)(tyObject_MyObject_J9bt9aFMq9aaD4DaLIDS9cAW1Q x) {
	tyObject_MyField_TRBCGJu2GrSeYqJblPj9bNA* result;
	result = (tyObject_MyField_TRBCGJu2GrSeYqJblPj9bNA*)0;
	result = (&x.f);
	return result;
}

The x argument is passed by copy hence returned pointer is a pointer to a temporary object. I am afraid it is not a valid C code.

If proc returns lent T then all object/tuple arguments needs be passed by reference.
I have tried fixing it by changing codegen, but it wasn't not enough. sempass needs to add hidden addr, derefs nodes to make it work.

@cooldome cooldome changed the title lent T can return garbage `lent T` can return garbage Apr 1, 2019

@cooldome

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

The last commented line in the test would have detected the problem, but unsafeAddr doesn't work for lent T at the moment. Should we add support for lent T in semAddrArg and/or isAssignable?

@Araq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

If proc returns lent T then all object/tuple arguments needs be passed by reference.

Or maybe lent shouldn't be available here? Would it be too bad if we require a var MyObject here?

@cooldome

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

var MyObject would be a problem.
I though about it once again, it should be possible to fix it only in the codegen as {.byref.} pragma on MyObject fixes the problem. I will give it another try.

@Araq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

I don't really like it but it's ok, at least it doesn't require yet another language extension.

@zah

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

Why is this object passed by value in the first place? Is this some kind of optimization based on the object size? My assumption is that all object types are passed by the equivalent of const & by default.

@cooldome

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2019

Yes,
It is an optimization based on the object/ tuple size. Taking this optimization out will solve the problem.
Araq, what do you think? Is it OK to always pass object/tuple by reference?

@Araq

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

No way, it's a very useful optimization. But you can simply look at the return type, if lent then the first parameter needs to be passed as a pointer.

cooldome added a commit to cooldome/Nim that referenced this issue Apr 2, 2019

@Araq Araq closed this in #10946 Apr 3, 2019

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

fixes #10942. Lent T bug (#10946)
* fixes #10942

* add test

* bug build
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.