[lldb] Restore Swift IsSymbolARuntimeThunk override

[kastiglione]

The swift override of IsSymbolARuntimeThunk was unintentionally removed in #536. This restores it.

rdar://65681037

[kastiglione]

TODO:

• Add tests to prevent regression (see [lldb] Restore Swift IsSymbolARuntimeThunk override #2917 (comment))

[kastiglione]

@swift-ci test

[jimingham]

Looks right to me.

[kastiglione]

I added a test by exposing IsSwiftThunk in python. I didn't want to have to do that, but it's the most direct way to test this.

[jimingham]

I would rather avoid adding swift specific API's to SBFrame. The notion of a "language runtime thunk" is not swift specific. For instance C++ often uses a thunk to go from the allocating to the non-allocating constructor (or vice versa???). So this should just be IsRuntimeThunk. Or maybe IsLanguageThunk? You don't need to supply the language, since you can get that from the frame.

[kastiglione]

@jimingham I agree. In this change I am exposing IsSwiftThunk in Python. The actual SBFrame::IsSwiftThunk already exists, and is used by clients via the C++ API. I think this should be made more generic, but for this change I think exposing via python for testing purpose is reasonable. It sounds like you're objecting to its very existence, but given that it already exists, are you ok with exposing it via Python for the purpose of test?

[kastiglione]

@swift-ci test

[jimingham]

@jimingham I agree. In this change I am exposing IsSwiftThunk in Python. The actual SBFrame::IsSwiftThunk already exists, and is used by clients via the C++ API. I think this should be made more generic, but for this change I think exposing via python for testing purpose is reasonable. It sounds like you're objecting to its very existence, but given that it already exists, are you ok with exposing it via Python for the purpose of test?

Oh, that's so sad. But I still think there's no reason to expose the wrong API to Python. IsLanguageThunk is pretty trivial, since you just get the language runtime from the frame and call IsThunk on that. We should add that to the SB API's if it isn't there already, and expose that one to Python.

[kastiglione]

I will add the new API, but won't be able to remove the old API that's still in use by clients, so the new API will currently be used only by the tests. But that has an issue I don't like much – we could unknowingly break the old API and wouldn't have tests to catch it. I feel like that defeats the primary purpose of adding these tests originally.

I have a client question that I'll ask out of band. If we can migrate clients to a new API in the near term, then I'm not worried about covering the old API with a test.

[kastiglione]

I have reverted the commit that added the test. What remains is the restoration of IsSymbolARuntimeThunk.

In a follow up PR I'll add IsLanguageThunk, add tests for it, and work with clients to migrate to it.

[kastiglione]

@swift-ci test

[adrian-prantl]

Thanks!!

[adrian-prantl]

I will add the new API, but won't be able to remove the old API that's still in use by clients, so the new API will currently be used only by the tests. But that has an issue I don't like much – we could unknowingly break the old API and wouldn't have tests to catch it. I feel like that defeats the primary purpose of adding these tests originally.

I have a client question that I'll ask out of band. If we can migrate clients to a new API in the near term, then I'm not worried about covering the old API with a test.

Since we established that the old API didn't actually work, it may not be too bad to remove it and maybe force people to upgrade to the new one? It's not something we usually do and it might cause link errors, but maybe it's worth it?

[kastiglione]

@adrian-prantl Yes I think we can remove this one after migrating. I think there's only one client.