Restricting `willThrow` hooks to `throw` statements in source; customizing its calling convention to minimize code size impact

Hi lldb folks, I wanted to run this patch by you all before merging:

I noticed that in code generation we were generating `swift_willThrow` calls, which are used by the "Swift Error" breakpoint feature, not only where the user wrote `throw` in their Swift source, but also along many (but not all) implicit error propagation edges. It's my recollection that we only wanted the breakpoint to fire when the user wrote `throw` in source code, so this seems like an oversight. Does this sound good to you all? I could see some utility in also catching errors when they propagate out of ObjC calls, or out of Swift code that's been built without willThrow hooks, but we could address those in more targeted ways without the code bloat and false positives from hooking every single propagation edge.

I'd also like to experiment with customizing the calling convention for the hook so that it uses the error register feature of the Swift calling convention to reduce register shuffling around the call. This would make the error value still available to the hook, but it'd be in the error register instead of in the normal first argument register. Another thing I'd like to try is giving the hook the `preserveall` calling convention, so it doesn't clobber any registers (since it doesn't do anything normally), but this would require us to emit the hook as a local symbol in each image instead of in the dylib to avoid register clobbering by the lazy dynamic linker trampoline. Does lldb simply install a symbolic breakpoint on `swift_willThrow` anywhere, or only as a public symbol in the libswiftCore.dylib image? How disruptive would these changes be to lldb's Swift error handling support?

-Joe

Hi lldb folks, I wanted to run this patch by you all before merging:

[WIP] SILGen: Don't inject "willThrow" hooks before rethrow propagation branches. by jckarter · Pull Request #13585 · apple/swift · GitHub

I noticed that in code generation we were generating `swift_willThrow` calls, which are used by the "Swift Error" breakpoint feature, not only where the user wrote `throw` in their Swift source, but also along many (but not all) implicit error propagation edges. It's my recollection that we only wanted the breakpoint to fire when the user wrote `throw` in source code, so this seems like an oversight. Does this sound good to you all? I could see some utility in also catching errors when they propagate out of ObjC calls, or out of Swift code that's been built without willThrow hooks, but we could address those in more targeted ways without the code bloat and false positives from hooking every single propagation edge.

I don't think the intention was that we would only have this breakpoint fire when the user wrote an explicit throw. The point of the trap errors feature is to help identify the source of a thrown error. So while debugging, if I stop in a catch block I didn't expect to stop in, I can turn on error catching, and run again, and this time I will stop when the error gets thrown, and can presumably figure out why it was happening. If this hook caused you to stop sometimes but not always, I think that would be pretty confusing. It would appear as if the error came out of nowhere. Anyway, so the use for this feature is not to catch user throws, but to explain why the user ended up in their "catch" block somewhere.

So I don't think it is as simple as only notating explicit user throws. I don't know enough about the guts of the compiler's error propagation to know if there's a class of places that aren't going to result in the user ending up in their catch statement. I'm happy for you to leave the hook out of any such bits of business. But if you leave it out of a place that would then end the user up in their catch block, that's going against the design.

I'd also like to experiment with customizing the calling convention for the hook so that it uses the error register feature of the Swift calling convention to reduce register shuffling around the call. This would make the error value still available to the hook, but it'd be in the error register instead of in the normal first argument register. Another thing I'd like to try is giving the hook the `preserveall` calling convention, so it doesn't clobber any registers (since it doesn't do anything normally), but this would require us to emit the hook as a local symbol in each image instead of in the dylib to avoid register clobbering by the lazy dynamic linker trampoline. Does lldb simply install a symbolic breakpoint on `swift_willThrow` anywhere, or only as a public symbol in the libswiftCore.dylib image? How disruptive would these changes be to lldb's Swift error handling support?

I already have to know about the error register for other purposes, so I don't mind using it rather than the first argument register to fetch the error from the hook.

It looks like I neglected to make the original swift_willThrow breakpoint module specific - though that was plainly an oversight. I'd rather not have us look in every loaded module for this symbol. We've been doing that so far, but then swift debugging isn't as speedy as we want it to be... So that change won't make things slower than now. The breakpoint system already handles multiple hits for one specification, so it there happened to be more than one location to the error throw breakpoint, it won't cause any problems.

Jim

···

On Jan 2, 2018, at 10:30 AM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

-Joe
_______________________________________________
swift-lldb-dev mailing list
swift-lldb-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-lldb-dev

Hi lldb folks, I wanted to run this patch by you all before merging:

[WIP] SILGen: Don't inject "willThrow" hooks before rethrow propagation branches. by jckarter · Pull Request #13585 · apple/swift · GitHub

I noticed that in code generation we were generating `swift_willThrow` calls, which are used by the "Swift Error" breakpoint feature, not only where the user wrote `throw` in their Swift source, but also along many (but not all) implicit error propagation edges. It's my recollection that we only wanted the breakpoint to fire when the user wrote `throw` in source code, so this seems like an oversight. Does this sound good to you all? I could see some utility in also catching errors when they propagate out of ObjC calls, or out of Swift code that's been built without willThrow hooks, but we could address those in more targeted ways without the code bloat and false positives from hooking every single propagation edge.

I don't think the intention was that we would only have this breakpoint fire when the user wrote an explicit throw. The point of the trap errors feature is to help identify the source of a thrown error. So while debugging, if I stop in a catch block I didn't expect to stop in, I can turn on error catching, and run again, and this time I will stop when the error gets thrown, and can presumably figure out why it was happening. If this hook caused you to stop sometimes but not always, I think that would be pretty confusing. It would appear as if the error came out of nowhere. Anyway, so the use for this feature is not to catch user throws, but to explain why the user ended up in their "catch" block somewhere.

So I don't think it is as simple as only notating explicit user throws. I don't know enough about the guts of the compiler's error propagation to know if there's a class of places that aren't going to result in the user ending up in their catch statement. I'm happy for you to leave the hook out of any such bits of business. But if you leave it out of a place that would then end the user up in their catch block, that's going against the design.

The main places where this would be an issue would be with calls into ObjC methods that get implicitly turned into throwing calls in Swift, and in places where we called external Swift code that for whatever reason had willThrow calls stripped. (It doesn't appear that we ever do that currently.) It would be straightforward to keep emitting swift_willThrow calls after ObjC calls.

I'd also like to experiment with customizing the calling convention for the hook so that it uses the error register feature of the Swift calling convention to reduce register shuffling around the call. This would make the error value still available to the hook, but it'd be in the error register instead of in the normal first argument register. Another thing I'd like to try is giving the hook the `preserveall` calling convention, so it doesn't clobber any registers (since it doesn't do anything normally), but this would require us to emit the hook as a local symbol in each image instead of in the dylib to avoid register clobbering by the lazy dynamic linker trampoline. Does lldb simply install a symbolic breakpoint on `swift_willThrow` anywhere, or only as a public symbol in the libswiftCore.dylib image? How disruptive would these changes be to lldb's Swift error handling support?

I already have to know about the error register for other purposes, so I don't mind using it rather than the first argument register to fetch the error from the hook.

Cool.

It looks like I neglected to make the original swift_willThrow breakpoint module specific - though that was plainly an oversight. I'd rather not have us look in every loaded module for this symbol. We've been doing that so far, but then swift debugging isn't as speedy as we want it to be... So that change won't make things slower than now. The breakpoint system already handles multiple hits for one specification, so it there happened to be more than one location to the error throw breakpoint, it won't cause any problems.

If you're OK with us saying we only ever emit willThrow when we see a `throw` or ObjC call in Swift code, then I'd say that pushes the calling convention change into pointless microoptimization territory. I'm happy to keep the single symbol in the runtime.

-Joe

···

On Jan 2, 2018, at 2:50 PM, Jim Ingham <jingham@apple.com> wrote:

On Jan 2, 2018, at 10:30 AM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

Hi lldb folks, I wanted to run this patch by you all before merging:

[WIP] SILGen: Don't inject "willThrow" hooks before rethrow propagation branches. by jckarter · Pull Request #13585 · apple/swift · GitHub

I noticed that in code generation we were generating `swift_willThrow` calls, which are used by the "Swift Error" breakpoint feature, not only where the user wrote `throw` in their Swift source, but also along many (but not all) implicit error propagation edges. It's my recollection that we only wanted the breakpoint to fire when the user wrote `throw` in source code, so this seems like an oversight. Does this sound good to you all? I could see some utility in also catching errors when they propagate out of ObjC calls, or out of Swift code that's been built without willThrow hooks, but we could address those in more targeted ways without the code bloat and false positives from hooking every single propagation edge.

I don't think the intention was that we would only have this breakpoint fire when the user wrote an explicit throw. The point of the trap errors feature is to help identify the source of a thrown error. So while debugging, if I stop in a catch block I didn't expect to stop in, I can turn on error catching, and run again, and this time I will stop when the error gets thrown, and can presumably figure out why it was happening. If this hook caused you to stop sometimes but not always, I think that would be pretty confusing. It would appear as if the error came out of nowhere. Anyway, so the use for this feature is not to catch user throws, but to explain why the user ended up in their "catch" block somewhere.

So I don't think it is as simple as only notating explicit user throws. I don't know enough about the guts of the compiler's error propagation to know if there's a class of places that aren't going to result in the user ending up in their catch statement. I'm happy for you to leave the hook out of any such bits of business. But if you leave it out of a place that would then end the user up in their catch block, that's going against the design.

The main places where this would be an issue would be with calls into ObjC methods that get implicitly turned into throwing calls in Swift, and in places where we called external Swift code that for whatever reason had willThrow calls stripped. (It doesn't appear that we ever do that currently.) It would be straightforward to keep emitting swift_willThrow calls after ObjC calls.

It would be nice if it wasn't possible to strip willThrow calls, otherwise somebody is going to think that's a smart idea for their library w/o realizing that it will make it very hard for clients to debug the library's throw behavior. 'Course if it really is a smart idea, then we should more think about making it not so (which was presumably the point of your ABI changes.)

Other than that, if you can keep the willThrow after ObjC calls, this sounds fine.

I'd also like to experiment with customizing the calling convention for the hook so that it uses the error register feature of the Swift calling convention to reduce register shuffling around the call. This would make the error value still available to the hook, but it'd be in the error register instead of in the normal first argument register. Another thing I'd like to try is giving the hook the `preserveall` calling convention, so it doesn't clobber any registers (since it doesn't do anything normally), but this would require us to emit the hook as a local symbol in each image instead of in the dylib to avoid register clobbering by the lazy dynamic linker trampoline. Does lldb simply install a symbolic breakpoint on `swift_willThrow` anywhere, or only as a public symbol in the libswiftCore.dylib image? How disruptive would these changes be to lldb's Swift error handling support?

I already have to know about the error register for other purposes, so I don't mind using it rather than the first argument register to fetch the error from the hook.

Cool.

It looks like I neglected to make the original swift_willThrow breakpoint module specific - though that was plainly an oversight. I'd rather not have us look in every loaded module for this symbol. We've been doing that so far, but then swift debugging isn't as speedy as we want it to be... So that change won't make things slower than now. The breakpoint system already handles multiple hits for one specification, so it there happened to be more than one location to the error throw breakpoint, it won't cause any problems.

If you're OK with us saying we only ever emit willThrow when we see a `throw` or ObjC call in Swift code, then I'd say that pushes the calling convention change into pointless microoptimization territory. I'm happy to keep the single symbol in the runtime.

That would make my life easier. I filed:

<rdar://problem/36266718> Swift exception breakpoint for swift_willThrow is not limited to libswiftCore.dylib

to limit the search to libswiftCore, which I'll do if you are going to keep this to one symbol.

Jim

···

On Jan 2, 2018, at 3:11 PM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

On Jan 2, 2018, at 2:50 PM, Jim Ingham <jingham@apple.com> wrote:

On Jan 2, 2018, at 10:30 AM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

-Joe
_______________________________________________
swift-lldb-dev mailing list
swift-lldb-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-lldb-dev

Hi lldb folks, I wanted to run this patch by you all before merging:

[WIP] SILGen: Don't inject "willThrow" hooks before rethrow propagation branches. by jckarter · Pull Request #13585 · apple/swift · GitHub

I noticed that in code generation we were generating `swift_willThrow` calls, which are used by the "Swift Error" breakpoint feature, not only where the user wrote `throw` in their Swift source, but also along many (but not all) implicit error propagation edges. It's my recollection that we only wanted the breakpoint to fire when the user wrote `throw` in source code, so this seems like an oversight. Does this sound good to you all? I could see some utility in also catching errors when they propagate out of ObjC calls, or out of Swift code that's been built without willThrow hooks, but we could address those in more targeted ways without the code bloat and false positives from hooking every single propagation edge.

I don't think the intention was that we would only have this breakpoint fire when the user wrote an explicit throw. The point of the trap errors feature is to help identify the source of a thrown error. So while debugging, if I stop in a catch block I didn't expect to stop in, I can turn on error catching, and run again, and this time I will stop when the error gets thrown, and can presumably figure out why it was happening. If this hook caused you to stop sometimes but not always, I think that would be pretty confusing. It would appear as if the error came out of nowhere. Anyway, so the use for this feature is not to catch user throws, but to explain why the user ended up in their "catch" block somewhere.

So I don't think it is as simple as only notating explicit user throws. I don't know enough about the guts of the compiler's error propagation to know if there's a class of places that aren't going to result in the user ending up in their catch statement. I'm happy for you to leave the hook out of any such bits of business. But if you leave it out of a place that would then end the user up in their catch block, that's going against the design.

The main places where this would be an issue would be with calls into ObjC methods that get implicitly turned into throwing calls in Swift, and in places where we called external Swift code that for whatever reason had willThrow calls stripped. (It doesn't appear that we ever do that currently.) It would be straightforward to keep emitting swift_willThrow calls after ObjC calls.

It would be nice if it wasn't possible to strip willThrow calls, otherwise somebody is going to think that's a smart idea for their library w/o realizing that it will make it very hard for clients to debug the library's throw behavior. 'Course if it really is a smart idea, then we should more think about making it not so (which was presumably the point of your ABI changes.)

Other than that, if you can keep the willThrow after ObjC calls, this sounds fine.

Yeah, I agree. If we can change willThrow to use the error register, then the code size impact of calling the hook should be minimal, removing most of the motivation to strip the hook.

-Joe

···

On Jan 2, 2018, at 5:09 PM, Jim Ingham <jingham@apple.com> wrote:

On Jan 2, 2018, at 3:11 PM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

On Jan 2, 2018, at 2:50 PM, Jim Ingham <jingham@apple.com> wrote:

On Jan 2, 2018, at 10:30 AM, Joe Groff via swift-lldb-dev <swift-lldb-dev@swift.org> wrote:

I'd also like to experiment with customizing the calling convention for the hook so that it uses the error register feature of the Swift calling convention to reduce register shuffling around the call. This would make the error value still available to the hook, but it'd be in the error register instead of in the normal first argument register. Another thing I'd like to try is giving the hook the `preserveall` calling convention, so it doesn't clobber any registers (since it doesn't do anything normally), but this would require us to emit the hook as a local symbol in each image instead of in the dylib to avoid register clobbering by the lazy dynamic linker trampoline. Does lldb simply install a symbolic breakpoint on `swift_willThrow` anywhere, or only as a public symbol in the libswiftCore.dylib image? How disruptive would these changes be to lldb's Swift error handling support?

I already have to know about the error register for other purposes, so I don't mind using it rather than the first argument register to fetch the error from the hook.

Cool.

It looks like I neglected to make the original swift_willThrow breakpoint module specific - though that was plainly an oversight. I'd rather not have us look in every loaded module for this symbol. We've been doing that so far, but then swift debugging isn't as speedy as we want it to be... So that change won't make things slower than now. The breakpoint system already handles multiple hits for one specification, so it there happened to be more than one location to the error throw breakpoint, it won't cause any problems.

If you're OK with us saying we only ever emit willThrow when we see a `throw` or ObjC call in Swift code, then I'd say that pushes the calling convention change into pointless microoptimization territory. I'm happy to keep the single symbol in the runtime.

That would make my life easier. I filed:

<rdar://problem/36266718> Swift exception breakpoint for swift_willThrow is not limited to libswiftCore.dylib

to limit the search to libswiftCore, which I'll do if you are going to keep this to one symbol.

Jim

-Joe
_______________________________________________
swift-lldb-dev mailing list
swift-lldb-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-lldb-dev