[Serialization] Don't crash if we can't get the type for an XREF #12914
Conversation
If we can't resolve a cross-reference unambiguously, we're supposed to produce an llvm::Error and let the calling code handle it. However, if we couldn't even resolve the /type/ of the cross-reference, we would just crash. Follow the supported error path in that case too -- in many cases the error can just propagate upwards to something that can handle it. rdar://problem/34821187, plus an extra test case from rdar://problem/35157494. (The latter will be fixed better later, but meanwhile let's not regress on the crashing part.)
|
|
||
| Type filterTy = getType(TID); | ||
| if (!isType) | ||
| Type filterTy; |
There was a problem hiding this comment.
Is there a way to avoid the copy and paste here?
There was a problem hiding this comment.
The top-level part and the non-top-level part are pretty different, but I can see if there's enough in common to factor it out more than it is. It's still going to be ugly because of the need for the early return.
There was a problem hiding this comment.
Yeah, I don't think there's really a good way to avoid this. Only this section is copy/pasted, and because of the way llvm::Error works, you'd still end up with most of the duplication and a less clear building up of pathTrace. Maybe if we had a monadic try in C++.
|
@swift-ci Please test |
DougGregor
left a comment
DougGregor
left a comment
There was a problem hiding this comment.
LGTM. I wonder how many more of these crashes are lurking.
|
Ideally we'd excise all uses of the old APIs, or at least outside of SIL deserialization, but I tried that the first time and it was way too big a change. I did do an audit when the "checked" variants first went in but clearly it wasn't perfect. |
3 participants
If we can't resolve a cross-reference unambiguously, we're supposed to produce an llvm::Error and let the calling code handle it. However, if we couldn't even resolve the type of the cross-reference, we would just crash. Follow the supported error path in that case too – in many cases the error can just propagate upwards to something that can handle it.
rdar://problem/34821187, plus an extra test case from rdar://problem/35157494. (The latter will be fixed better later, but meanwhile let's not regress on the crashing part.)