New issue

Fix ICE for functions with more than 65535 arguments #88733

Conversation

Copy link

Contributor

Noble-Mushtak commented 11 days ago

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.

Copy link

Collaborator

rust-highfive commented 11 days ago

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.

Copy link

Contributor

CraftSpider commented 11 days ago

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]

Copy link

Contributor

Author

Noble-Mushtak commented 11 days ago

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.

Copy link

Contributor

steffahn commented 11 days ago •

edited

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.

Copy link

Contributor

klensy commented 11 days ago •

edited

.. 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).

What's the point of having so many arguments?

Copy link

Contributor

estebank commented 10 days ago

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.).

Copy link

Contributor

Author

Noble-Mushtak commented 10 days ago

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.

Copy link

Contributor

notriddle commented 10 days ago

@rustbot label: +T-lang

Copy link

Contributor

estebank commented 10 days ago

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

changed the title Use u32 instead of u16 for parameter indices

Fix ICE for functions with more than 65535 arguments

9 days ago

Copy link

Contributor