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

Add a new mode 2 to the LDC that allows to use the memory as a source for code #849

Merged
merged 27 commits into from
Oct 5, 2024

Conversation

xgreenx
Copy link
Collaborator

@xgreenx xgreenx commented Oct 5, 2024

TODO: Add specification link

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • If performance characteristic of an instruction change, update gas costs as well or make a follow-up PR for that
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@xgreenx xgreenx requested a review from a team October 5, 2024 13:19
@xgreenx xgreenx self-assigned this Oct 5, 2024
fuel-asm/src/lib.rs Outdated Show resolved Hide resolved
Base automatically changed from feature/predicate-ldc to master October 5, 2024 19:15
let len = len.to_addr()?;
owner.verify_ownership(&dst_range)?;

if src_range.end() <= self.stack.len() {
Copy link
Member

Choose a reason for hiding this comment

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

Is all the following validation logic new?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the logic from self.read(src, len) and self.write_noownerchecks(dst, len)? here to avoid to_vec for tmp slice

Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

I'm not enough familiar with the VM to review the logic of the memcpy but the code that has been moved to this function has been well moved :)

fuel-vm/src/predicate.rs Outdated Show resolved Hide resolved
op::ret(0x0),
// Good return opcode that we want to use for the `LDC`.
op::ret(0x1),
// This will be our zeroed blob id
Copy link
Member

Choose a reason for hiding this comment

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

blob id?

op::move_(0x10, IS),
// We don't need to copy `jmpf` and bad `ret` opcodes via `LDC`.
op::addi(0x10, 0x10, 2 * Instruction::SIZE as u16),
// Store start of the code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Store start of the code
// Store end of the code

@xgreenx xgreenx requested a review from AurelienFT October 5, 2024 20:19
@xgreenx xgreenx added this pull request to the merge queue Oct 5, 2024
Merged via the queue into master with commit 0adb5f8 Oct 5, 2024
40 checks passed
@xgreenx xgreenx deleted the feature/ldc-mode-2 branch October 5, 2024 20:27
Voxelot added a commit to FuelLabs/fuel-core that referenced this pull request Oct 5, 2024
## Linked Issues/PRs
FuelLabs/fuel-vm#848
FuelLabs/fuel-vm#849

## Description
<!-- List of detailed changes -->

## Checklist
- [x] Breaking changes are clearly marked as such in the PR description
and changelog
- [x] New behavior is reflected in tests
- [ ] [The specification](https://github.com/FuelLabs/fuel-specs/)
matches the implemented behavior (link update PR if changes are needed)

### Before requesting review
- [x] I have reviewed the code myself

---------

Co-authored-by: AurelienFT <aurelien.foucault@epitech.eu>
Co-authored-by: green <xgreenx9999@gmail.com>
Co-authored-by: Ahmed Sagdati <37515857+segfault-magnet@users.noreply.github.com>
Dentosal added a commit to FuelLabs/fuel-specs that referenced this pull request Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants