make full field retagging the default #2985
Conversation
|
I have quality-of-implementation sort of concerns, but I don't think anything that would make me object to this? I do not have a concrete sense of the fallout. I suspect this will primarily make self-referential things explode that didn't before because the self-referential struct was too large to be Scalar/ScalarPair. I agree with the zero-cost abstraction argument for why we should have full field retagging. But at the same time I'm not excited about sanitizing against UB that a user can prove we do not optimize on. I'd be a lot happier if we relied on full field retagging for optimization(s) or at least had a concrete plan to do so. It would be a real shame if there is some compiler design reason that this UB may never be useful. Field retagging is pretty subtle and I'm not sure our diagnostics do a good job of explaining it. When the default changes, some people's code will suddenly start reporting UB where it didn't before. It would be really good to add a note in our diagnostics that a retag was caused by field retagging. For Stack Borrows we already have |
We have a ton of UB of that sort. In fact we are actively avoiding exploiting e.g. Stacked Borrows (the parts that go beyond So, I don't think "the UB should be exploited by the compiler" is a standard we can reasonably satisfy. What I am more concerned about is that some situations where this UB arises don't have a good alternative. They would need something like rust-lang/rfcs#3336. I'm not sure how to make progress on that RFC. Maybe we'll get some more motivating examples when Miri starts reporting more UB for self-referential code...
That is a good point. I am less concerned about Tree Borrows since it is opt-in anyway. |
Hm, maybe we should amend our output to encourage people to reach out to us if they find the UB report troubling. Currently, we do not. |
Done.
I added a message along those lines for all SB UB. Or did you mean only for field retagging? Or all UB? I'm a bit worried about being overwhelmed, we can't offer a general "please help my code is UB" hotline.^^ |
At the time I was thinking of field retagging, because that's where you said we'd it would be good to have motivating examples. I think for Stacked Borrows generally, we already have a lot of motivating examples? So outside of field retagging the message might generate more noise than signal. |
|
Okay I think I managed to get that. It's a bit hard to see through the error reporting layers.^^ |
|
@bors r=saethlin |
|
☀️ Test successful - checks-actions |
|
Hi folks, I wanted to reach out here and say thank you for the care that went into this. My weekly Miri CI failed as a result of this change and I was largely able to diagnose and fix the issues the new sanitizer caught on my own. If you're curious: |
The 'scalar' field retagging mode is clearly a hack -- it mirrors details of the codegen backend and how various structs are represented in LLVM. This means whether code has UB or not depends on surprising aspects, such as whether a struct has 2 or 3 (non-zero-sized) fields. Now that both hashbrown and scopeguard have released fixes to be compatible with field retagging, I think it is time to enable full field retagging by default.
@saethlin do you have an idea of how much fallout enabling full field retagging by default will cause? Do you have objections to enabling it by default?
Fixes #2528