-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
validate promoteds #80235
validate promoteds #80235
Conversation
2a24467
to
06ca7b7
Compare
Ah, but validation complains about...
Since promoteds are constants and constants may not point to statics. |
Meanwhile, let's see if this affects perf. |
Awaiting bors try build completion. |
⌛ Trying commit 06ca7b7 with merge d24e527f2368bc3de48bd2932d21d8ced60d3318... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
Queued d24e527f2368bc3de48bd2932d21d8ced60d3318 with parent 59aaa2a, future comparison URL. @rustbot label: +S-waiting-on-perf |
ah lol, I guess we need to differentiate between promoteds in statics and promoteds elsewhere? |
I guess promoted in consts should not contain refs to statics, yeah... |
Finished benchmarking try commit (d24e527f2368bc3de48bd2932d21d8ced60d3318): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
Looks like there's a perf hit of up to 4.5% for some examples. |
I fixed the breakage, tests should pass now (the ones I ran locally did). What remains is deciding if we are okay with the perf hit. |
So... I'm ok with this perf hit, though I'm wondering if we should somehow "turn it off" and just keep it as a sort of debug assertion that we aren't doing anything bad with promotion. I mean... this should never actually cause a validation failure afaict. |
This would be true if we did not promote |
Riiight, those. So let's do this now and consider moving to debug assertions once we know the result of the crater run @bors r+ |
📌 Commit 97cae9c has been approved by |
☀️ Test successful - checks-actions |
Turn on const-value validation for promoteds. This is made possible now that #67534 is resolved.
I don't think this is a breaking change. We don't promote any unsafe operation any more (since #77526 landed). We do promote
const fn
calls under some circumstances (inconst
/static
initializers), but union field access and similar operations are not allowed inconst fn
. So now is a perfect time to add this check. :Dr? @oli-obk
Fixes #67465