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

Implements compilation of the delete operator #461

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

TobiasWienand
Copy link
Contributor

This branches enables compilation of the delete operator. Some thoughts I think are worth mentioning

1)

With this change, approximately 300 new seeds from test/mjsunit and the SpiderMonkey JIT test files will become compilable. If Fuzzilli treats the delete operation as a standard unary operator, delete might be applied to non-object properties, such as simple variables (var). If I understand correctly, semantic correctness will not be significantly impacted because delete does not cause a crash when called on unintended targets. For example, if applied to var, let, or parameters, it simply returns false and has no effect.

2)

It can happen that an object has a key that is a number. In that case DeleteElement will be called, even though it is not an array.
Example:

const obj = { 1: 'value' };
delete obj[1];

Similarly, we can delete elements from an array based on a computed index - in that case DeleteComputedProperty is called, not DeleteElement.

Example

const arr = [1, 2, 3];
delete arr[Math.floor(Math.random() * arr.length)];

Is that a problem? IIUC, the main difference between DeleteElement and Delete(Computed)Property In FuzzIL is only syntactical, not semantical.

Copy link
Collaborator

@saelo saelo left a comment

Choose a reason for hiding this comment

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

Nice! Overall looks good already, with a few suggestions

@saelo
Copy link
Collaborator

saelo commented Nov 13, 2024

Regarding you comments:

  1. So IIUC we currently cannot compile something like delete a because that's not a MemberExpression? It's probably not super important, but maybe it's easy enough to fix. We could either add a DeleteName instruction, or, probably even simpler, just compile delete a to a delete this.a (which IIUC is effectively what it's doing). WDYT?

  2. I think this all works correctly the way you describe/implemented it. delete x[1] should always be a DeleteElement and delete x[foo] should always be a DeleteComputedProperty, regardless of what the object or the computed property name is.

@TobiasWienand
Copy link
Contributor Author

Regarding 1), I initially had this in the code, but then I found out that an implicit global variable does not become a property of this in all engines. For example, in Node.js, it becomes a property of global. I am not sure how this is handled in other JS engines, so I avoided it. Also, another problem would be to distinguish between implicit global variables and let, var, etc. The data structure that is processed by Compiler.swift only says that it is an identifier and what name it has.

I did a quick check of how many files in test/mjsunit/* have delete but are not compilable. We are talking about 28 files, and therefore they are in position 12 on the priority list. So perhaps it is best not to invest time in this right now and come back to it later.

@saelo
Copy link
Collaborator

saelo commented Nov 15, 2024

Cool, just a question and one more suggestion. And sounds good, let's not bother with delete a stuff for now :)

@saelo saelo merged commit 98357eb into googleprojectzero:main Nov 18, 2024
2 of 3 checks passed
@saelo
Copy link
Collaborator

saelo commented Nov 18, 2024

Thanks a lot!

parthdt pushed a commit to parthdt/fuzzilli that referenced this pull request Dec 14, 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.

2 participants