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

fuzzgen: Generate Tail Calls #6641

Merged
merged 2 commits into from
Jun 29, 2023

Conversation

afonso360
Copy link
Contributor

@afonso360 afonso360 commented Jun 25, 2023

👋 Hey,

This PR is a follow up to #6635 and it allows fuzzgen to generate return_call and return_indirect_call instructions. These are modeled as any other control flow instruction however they have a bunch of restrictions before we are actually able to insert them.

As with the other control flow instructions, we try to include as much info as possible in the CFG before inserting it. It doesn't seem as relevant with tail calls as it is with blocks, but I figured it is still worth it. (The alternative would be to pick the tail call target at instruction insertion time, but I think that would actually make this slightly more complicated)

I'm opening this as a draft since it has found some issues and I can't run it for too long before it crashes, and I'm not entirely sure it's generating correct code under all inputs. Just leaving this open if anyone wants to test with it (@fitzgen 👀 ).

@afonso360 afonso360 requested review from a team as code owners June 25, 2023 21:05
@afonso360 afonso360 requested review from elliottt and removed request for a team June 25, 2023 21:05
@afonso360 afonso360 marked this pull request as draft June 25, 2023 21:05
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen isle Related to the ISLE domain-specific language labels Jun 25, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

  • cfallin: isle
  • fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@elliottt elliottt requested review from fitzgen and removed request for elliottt June 26, 2023 07:06
@fitzgen
Copy link
Member

fitzgen commented Jun 26, 2023

Nice! The last commit LGTM, thanks for putting this up. Going to look into the issues you found now.

@github-actions github-actions bot added the fuzzing Issues related to our fuzzing infrastructure label Jun 27, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

  • fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@afonso360 afonso360 marked this pull request as ready for review June 27, 2023 22:19
@afonso360
Copy link
Contributor Author

Fuzzed for around 2 hours with fuzzgen and it all seems stable! 🥳

@afonso360
Copy link
Contributor Author

@fitzgen would you be able to look into this again? It should be ready now. I've just rebased the latest commit onto the main branch.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a ton!

let addr_ty = args[0];
let addr = builder.ins().func_addr(addr_ty, func_ref);
builder.ins().call_indirect(sig_ref, addr, &actuals)
let addr_ty = I64;
Copy link
Member

Choose a reason for hiding this comment

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

Can we maybe factor this out to a helper that checks cfg!(target_pointer_width = "64") and then has an else that panics with a helpful message? Just to future proof this a little bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out we can already query the ISA's pointer type directly, so I've done just that.

@fitzgen fitzgen enabled auto-merge June 29, 2023 17:04
@fitzgen fitzgen added this pull request to the merge queue Jun 29, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 29, 2023
@afonso360 afonso360 added this pull request to the merge queue Jun 29, 2023
Merged via the queue into bytecodealliance:main with commit a43d1dc Jun 29, 2023
@afonso360 afonso360 deleted the fuzzgen-tail-calls branch June 29, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift:area:x64 Issues related to x64 codegen cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure isle Related to the ISLE domain-specific language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants