Skip to content

Fix ICE for functions with more than 65535 arguments #88733

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 11, 2021

Conversation

Noble-Mushtak
Copy link
Contributor

This pull request fixes #88577 by changing the param_idx field in the Param variant of WellFormedLoc from u16 to u32, thus allowing for more than 65,535 arguments in a function. Note that I also added a regression test, but needed to add // ignore-tidy-filelength because the test is more than 8000 lines long.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2021
@CraftSpider
Copy link
Contributor

While on one hand this fixes the stated issue in sense, it would be nice if the compiler could recognize when a function overflowed the maximum number of parameters, so it would never ICE even on [insert arbitrarily large number here]

@Noble-Mushtak
Copy link
Contributor Author

That makes sense. The error in the issue comes from trying to convert a usize with value 65536 to a u16, so if we wanted to, we could use usize instead of u32 for parameter indices. Then we wouldn't need to do .try_into().unwrap() to convert the usize to a u32, which would get rid of the possibility of this panic entirely. I used u32 instead of usize in my pull request because I was not sure if the memory used by a 32-bit integer vs a 64-bit integer was a concern in this part of the compiler.

@steffahn
Copy link
Member

steffahn commented Sep 7, 2021

See this comment

#88577 (comment)

for a more reasonably sized test case. In order to avoid unnecessary deleted big files in git, I'd suggest you also do a rebase / amend and force-push in order to remove your huge test file.

@klensy
Copy link
Contributor

klensy commented Sep 7, 2021

.. it would be nice if the compiler could recognize when a function overflowed the maximum number of parameters, so it would never ICE even on [insert arbitrarily large number here]

Good idea, but changing parameter index type to type which range will not be used in 99,99..% of real cases isn't looking good.

C++ have limit about ~256 (+-, different values in different compilers).

@leonardo-m
Copy link

What's the point of having so many arguments?

@estebank
Copy link
Contributor

estebank commented Sep 8, 2021

I would prefer to go with @CraftSpider's approach: catch an overflowing number of args earlier in the process, emit an error and recover (by maybe clamping the param_idx to u16::MAX), although in this case I think it is reasonable to use an unrecoverable error (you need to call FatalError.raise(), I think), as this will be an incredibly uncommon case and making people change their approach and not give them later errors seems reasonable (to avoid complicating later stages: you'd have to account for the return type when reducing the number, everywhere that deals with arg counts or arg type checking would need to know about this, etc.).

@Noble-Mushtak
Copy link
Contributor Author

I would prefer to go with @CraftSpider's approach: catch an overflowing number of args earlier in the process, emit an error and recover (by maybe clamping the param_idx to u16::MAX)

If so, should I change param_idx back to u16, and then throw an error in check_fn_or_method if there are more than 65535 arguments? (check_fn_or_method is the place where the ICE from the issue occurred, since that is where the usize is being converted to a u16.) My concern about this approach is it means functions with more than 65535 arguments which compile without error in the current stable version of Rust (1.54.0) will now result in a compiler error. However, since functions with this many arguments are impractical anyways, maybe breaking old Rust code with functions with that many arguments is fine.

@notriddle
Copy link
Contributor

@rustbot label: +T-lang

@rustbot rustbot added the T-lang Relevant to the language team label Sep 8, 2021
@estebank
Copy link
Contributor

estebank commented Sep 8, 2021

I would change it back, yes. We can do a crater run to check if any open source project has code exploiting this and I would expect that the only change would be that what was formerly an ICE would be a proper error and no other difference in user visible behavior for valid code.

@Noble-Mushtak Noble-Mushtak changed the title Use u32 instead of u16 for parameter indices Fix ICE for functions with more than 65535 arguments Sep 9, 2021
@estebank
Copy link
Contributor

estebank commented Sep 9, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 9, 2021

📌 Commit 804ccfa has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2021
…ingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#87904 (Reword description of automatic impls of `Unsize`.)
 - rust-lang#88147 (Fix non-capturing closure return type coercion)
 - rust-lang#88209 (Improve error message when _ is used for in/inout asm operands)
 - rust-lang#88668 (Change more x64 size checks to not apply to x32.)
 - rust-lang#88733 (Fix ICE for functions with more than 65535 arguments)
 - rust-lang#88757 (Suggest wapping expr in parentheses on invalid unary negation)
 - rust-lang#88779 (Use more accurate spans for "unused delimiter" lint)
 - rust-lang#88830 (Add help for E0463)
 - rust-lang#88849 (don't clone types that are Copy (clippy::clone_on_copy))
 - rust-lang#88850 (don't convert types into identical types)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 746eb1d into rust-lang:master Sep 11, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 11, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when creating function with 65536 or more arguments
10 participants