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.

Sign up for GitHub

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

Jump to bottom

Add raw_ptr type #2828

Merged
merged 36 commits into from
Oct 25, 2022
Merged

Add raw_ptr type #2828

merged 36 commits into from
Oct 25, 2022

Conversation

AlicanC
Copy link
Contributor

@AlicanC AlicanC commented Sep 22, 2022 •
edited
Loading

This PR:

  • Introduces the raw_ptr type.
  • Changes RawVec's ptr field from u64 to raw_ptr.
  • Disallows returning raw_ptrs (and aggregate types that contain it) from being returned from main() fns.

image

Note to @sezna:

On our meeting we discussed checks.rs, which led me to this implementation: 090a4d1

That didn't work because raw_ptrs are converted to sway_ir::irtype::Type::Uint(64)s so there wasn't a way to differentiate them. I've reverted that and went with this instead: fc6bf34

@AlicanC AlicanC self-assigned this Sep 22, 2022
@AlicanC AlicanC linked an issue Sep 22, 2022 that may be closed by this pull request
Introduce a new raw_ptr type a disallow it to be returned from scripts #2551
Closed
@AlicanC AlicanC added compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen lib: std Standard library language feature Core language features visible to end users labels Sep 22, 2022
@AlicanC AlicanC marked this pull request as ready for review October 11, 2022 15:23
@AlicanC AlicanC requested a review from otrho as a code owner October 11, 2022 15:23
@AlicanC AlicanC requested review from sezna and a team October 11, 2022 15:23
mohammadfawaz
mohammadfawaz reviewed Oct 11, 2022
View reviewed changes
sway-core/src/lib.rs Outdated Show resolved Hide resolved
sway-core/src/type_system/type_info.rs Show resolved Hide resolved
sway-core/src/type_system/type_info.rs Outdated Show resolved Hide resolved
sway-error/src/error.rs Outdated Show resolved Hide resolved
test/src/e2e_vm_tests/test_programs/should_fail/raw_ptr/vec_ret/src/main.sw Outdated Show resolved Hide resolved
sway-lib-std/src/vec.sw Outdated Show resolved Hide resolved
sway-lib-std/src/vec.sw Show resolved Hide resolved
test/src/e2e_vm_tests/test_programs/should_fail/raw_ptr/raw_ptr_ret/src/main.sw Show resolved Hide resolved
@mohammadfawaz mohammadfawaz added the breaking May cause existing user code to break. Requires a minor or major release. label Oct 12, 2022
@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Oct 24, 2022 •
edited
Loading

All is reasonable! I resolved all the comments that have been addressed and left a few open that are related to testing (and the API name thing) but I'm okay with those being addressed later.

Once the conflicts are resolved, I will approve ✅

@AlicanC
Copy link
Contributor Author

AlicanC commented Oct 24, 2022

I've addressed all the issues, but it looks like some tests are failing. (Merge issues?) Trying to fix those now.

@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Oct 24, 2022 •
edited
Loading

I think the contract IDs in scripts need to be updated?

@AlicanC
Copy link
Contributor Author

AlicanC commented Oct 24, 2022

Actually added addr() back to get this in quick. Some tests needed refactoring without it.

@AlicanC AlicanC requested review from mohammadfawaz and a team October 24, 2022 22:29
@AlicanC AlicanC enabled auto-merge (squash) October 24, 2022 22:32
mohammadfawaz
mohammadfawaz previously approved these changes Oct 24, 2022
View reviewed changes
@mohammadfawaz mohammadfawaz self-requested a review October 24, 2022 23:29
@mohammadfawaz mohammadfawaz dismissed their stale review October 24, 2022 23:30

send_message is no longer being tested.

@mohammadfawaz
Copy link
Contributor

mohammadfawaz commented Oct 24, 2022 •
edited
Loading

@jc - in this commit, I:

  • Updated the token_ops test to not pass in data from the SDK using a Vec because that's what's causing the issue, as expected (abigen! cannot handle raw_ptr yet).
  • With the above, both disabled tests should pass. However, the changes you had to send_message seemed a bit buggy. send_message is only tested in token_ops currently so no surprise that Ci was passing. Regardless, I reverted the implementation of send_message to how it was, for the most part. We can iterate on that in the future.

This should be good to go. Once the SDK supports raw_ptr, we should add some extensive tests for Vec in the inputs and probably revert token_ops to how it was before.

@mohammadfawaz mohammadfawaz mentioned this pull request Oct 25, 2022
Closed
otrho
otrho approved these changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

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

This is great, thanks!

@AlicanC AlicanC merged commit 13ffbc3 into master Oct 25, 2022
@AlicanC AlicanC deleted the jc/raw-ptr branch October 25, 2022 01:34
@AlicanC AlicanC mentioned this pull request Oct 25, 2022
Merged
AlicanC added a commit that referenced this pull request Oct 26, 2022
@AlicanC @JoshuaBatty
Use raw_ptr methods in message.sw (#3139)
f63973a 
Following up on this:
#2828 (comment)

> However, the changes you had to send_message seemed a bit buggy.
send_message is only tested in token_ops currently so no surprise that
Ci was passing. Regardless, I reverted the implementation of
send_message to how it was, for the most part. We can iterate on that in
the future.

@mohammadfawaz, it seems like `raw_ptr::read()` didn't behave like I
assumed it would. Still made use of `::add()` and `::write()` to pack
`alloc()`s together and get rid of one `__addr_of()`, but the other one
stayed.

Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
@AlicanC AlicanC mentioned this pull request Mar 29, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohammadfawaz mohammadfawaz mohammadfawaz approved these changes

@otrho otrho otrho approved these changes

@sezna sezna sezna left review comments

Assignees

@AlicanC AlicanC

Labels
breaking May cause existing user code to break. Requires a minor or major release. compiler: frontend Everything to do with type checking, control flow analysis, and everything between parsing and IRgen language feature Core language features visible to end users lib: std Standard library
Projects
Fuel Network
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a new raw_ptr type a disallow it to be returned from scripts
5 participants
@AlicanC @mohammadfawaz @sezna @otrho @canndrew