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

feature/void #472

Merged
merged 4 commits into from
Dec 4, 2024
Merged

feature/void #472

merged 4 commits into from
Dec 4, 2024

Conversation

Yi2255
Copy link
Contributor

@Yi2255 Yi2255 commented Nov 18, 2024

Add a new UnaryOperator void

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.

I think I would prefer a different option: introducing a dedicated Discard (or so) JSOperation, similar to e.g. TypeOf. You can probably more or less copy the code related to TypeOf to get it to work.

The reason is that I don't think we would want to generate many void operations ourselves in our code generation pipeline since they are pretty uninteresting. So this we, we avoid "diluting" the other unary opertors.

I think this feature would be mostly useful to support compilation of more .js files to FuzzIL. As such, could you also add support for it in our Compiler? You can take a look at issue #437 or some recent PRs to see how those changes work. Also again, you can probably check how we handle TypeOf and deal with void in a similar manner.

Thanks! :)

@@ -664,6 +664,8 @@ public struct JSTyper: Analyzer {
set(instr.output, .boolean)
case .BitwiseNot:
set(instr.output, maybeBigIntOr(.integer))
case .Void:
set(instr.output, .anything)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be .undefined I think (the output of the void operator is always undefined IIRC)

@saelo
Copy link
Collaborator

saelo commented Dec 4, 2024

Nice! This looks good, I'm just wondering what happened to this commit for the compiler support bb872e1 @vi3tL0u1s? We can also just land this CL as it is and add the compiler support in a follow-up PR. WDYT?

@vi3tL0u1s
Copy link
Contributor

Hi @saelo, I’ve added the compiler support as you suggested. Thank you so much for the reminder!

@saelo
Copy link
Collaborator

saelo commented Dec 4, 2024

Ok great, thanks! I think there's a merge conflict though, and also I think there used to be a compiler test as well (bb872e1#diff-e3c74f57e01c1d64375830a153bdede28b838e44615cad0ba7085d5754390c2e), can you add that again, too?

@vi3tL0u1s
Copy link
Contributor

Hi @saelo, I’ve added the compiler test. Regarding the conflict, I believe it might be due to this PR not including the logic for the delete feature, which was merged earlier. I’m not entirely sure how to resolve this. Could you provide some guidance?

@saelo
Copy link
Collaborator

saelo commented Dec 4, 2024

Ah yes I see, let me try to figure out how to resolve that :)

@saelo
Copy link
Collaborator

saelo commented Dec 4, 2024

Ok, that worked. I guess I'll just merge these commits, then locally squash the merge commit, then force push. A bit hacky but oh well :)

@saelo saelo merged commit 2b9366f into googleprojectzero:main Dec 4, 2024
2 of 3 checks passed
@saelo
Copy link
Collaborator

saelo commented Dec 4, 2024

Okay done! I had to change the test a little bit because it ends up redefining the output constant from the prefix:

/// Prefix to execute before every JavaScript testcase. Its main task is to define the `output` function.
but otherwise, this seems to have worked. Thanks! :)

@vi3tL0u1s
Copy link
Contributor

Thank you so much @saelo!

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