Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Automation for adding ICEs #1529

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

langston-barrett
Copy link
Contributor

@langston-barrett langston-barrett commented Mar 15, 2023

The rustc Github repo has a standard form for ICE bug reports. This automation tries to add recent MREs of open ICE issues to the repo.

It's not finished yet, just wanted to check - is there interest in this?

This is how I did #1528, by the way.

@JohnTitor
Copy link
Member

JohnTitor commented Mar 15, 2023

Thanks for the PR! But this is potentially vulnerable, an attacker could do spamming or execute any code via issues. Moreover, some snippets need an external crate or setup, which means we have to create a shell script instead or just skip that issue.
So, human handling would be better here as we did in the past, I think.

@langston-barrett
Copy link
Contributor Author

Thanks for the review!

But this is potentially vulnerable, an attacker could do spamming or execute any code via issues.

Is that true? I assumed that glacier::test_all only compiled the code, and didn't run it. In that case, for an attacker to compromise this project they'd have to find an abitrary code execution bug in rustc, and then post a public issue on the issue tracker that exploits it.

In any case, what do you think about manual use?

Moreover, some snippets need an external crate or setup, which means we have to create a shell script instead or just skip that issue.

Certainly, this automation can't be comprehensive. Right now, it tests which code snippets do in fact lead to ICEs and removes the ones that don't. Manual effort will generally still be required.

@JohnTitor
Copy link
Member

Ah, I misunderstood this PR's purpose, sorry! I cannot unfortunately review this immediately, maybe Alex can do instead :)

@langston-barrett
Copy link
Contributor Author

langston-barrett commented Mar 17, 2023

Ah, I misunderstood this PR's purpose, sorry! I cannot unfortunately review this immediately, maybe Alex can do instead :)

No worries, I think I didn't make it super clear! But yeah, I think this would be helpful for people running it locally, manually. And it doesn't need a thorough review right now - as long as there's some interest in including something like this, I'm happy to keep pushing on it until it's more polished.

@JohnTitor
Copy link
Member

Okay, I've reviewed roughly and I tend to accept it. Indeed my all previous concerns are covered/resolved and yeah, this could help glacier have more ICEs largely, thank you for writing up!

As a bonus point, I guess it'd be great if grabber outputs failed issues so that we can easily find issues that need manual effort.

@langston-barrett
Copy link
Contributor Author

Great, thanks @JohnTitor! Things I want to get done before merge:

  • Add support for pagination
  • Output a list of issues which couldn't be handled automatically; these can be skipped in future runs
  • Add a second pass that runs rustfmt on everything
  • Documentation (Do we want this in grabber/README.md? The top-level README?)

@Alexendoo
Copy link
Member

Assuming #1562 was created with this a nice addition would be to add a final newline to avoid that 🚫

You may want to avoid running rustfmt as it could also ICE on some parser issues, or potentially cause them to no longer ICE

maybe Alex can do instead :)

Whoops 😅

Thank you for looking at it @JohnTitor

@langston-barrett
Copy link
Contributor Author

You may want to avoid running rustfmt as it could also ICE on some parser issues, or potentially cause them to no longer ICE

Indeed, the idea would be to only save formatted versions when formatting succeeds and doesn't de-ICE the test case, and otherwise to keep the original.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants