I’d like to bump the minimum required SWIG version from 4.0.0 (released in April 2019) to 4.1.0 (released in October 2022).
Motivation
The motivation is the improved support for move-only types that was added in 4.1 (see RELEASENOTES). More specifically, I need the following change in order to expose SBLock through SWIG:
2022-06-29: wsfulton
#999 #1044 Enhance SWIGTYPE "out" typemaps to use std::move when copying
objects, thereby making use of move semantics when wrapping a function returning
by value if the returned type supports move semantics.
Wrapping functions that return move only types 'by value' now work out the box
without having to provide custom typemaps.
The implementation removed all casts in the "out" typemaps to allow the compiler to
appropriately choose calling a move constructor, where possible, otherwise a copy
constructor. The implementation also required modifying SwigValueWrapper to
change a cast operator from:
SwigValueWrapper::operator T&() const;
to
#if __cplusplus >=201103L
SwigValueWrapper::operator T&&() const;
#else
SwigValueWrapper::operator T&() const;
#endif
This is not backwards compatible for C++11 and later when using the valuewrapper feature
if a cast is explicitly being made in user supplied "out" typemaps. Suggested change
in custom "out" typemaps for C++11 and later code:
1. Try remove the cast altogether to let the compiler use an appropriate implicit cast.
2. Change the cast, for example, from static_cast<X &> to static_cast<X &&>, using the
__cplusplus macro if all versions of C++ need to be supported.
*** POTENTIAL INCOMPATIBILITY ***
Alternatives
I tried working out the limitations with a SWIG type map, but I don’t think there’s much I can do without the SwigValueWrapper overloads that take rvalue references. I think improved move support would be generally beneficial to the SB API, a possible alternative is to not expose SBLock through the API, or delay doing so until we feel comfortable bumping the minimum SWIG version.
Availability
On macOS and Windows, SWIG is an external dependency that needs to be installed manually. I’m assuming developers on those platforms are generally using the latest available version already. Likely the main concern is package availability on Linux. Ubuntu 20.04 LTS and 22.04 LTS ship 4.0.1 and 4.0.2 respectively. Only 24.04 LTS has a new enough SWIG (4.2.0). I didn’t check other distros but I suspect they’ll be in a similar situation.
CI
All buildbots are already running a recent enough version of SWIG, with the exception of the lldb-arm-ubuntu buildbot and the pre-commit Linux bot, both running 4.0.2.
I didn’t expect to find myself in this position, but.. this is going to be a problem for us, because our internal monorepo is currently at 4.0. I’m not sure what it would take to get a new version of swig in there (I’ve never done it), but I expect it to be a fairly large undertaking. So it’s not so much a question of developers themselves (I have swig 4.2 on my machine) as it is about the build infrastructure that build the lldb we release. I suppose others might be in the same situation (mumbles something about servers one doesn’t have access to).
I wouldn’t completely want to hold up progress due to this in general, but in this case, it seems like this is avoidable. Let me give you two counter-proposals:
I think this is workaround with typemaps as the swig commit says. I think I’ve made it work (on a simple example) with the use of the “optimal” code generation option (maybe we should use this more often actually):
What if, instead of SBLock, we had something like an SBMutex instead. This could be copyable as it would represent a reference to the underlying object (in the same way that SBTarget and many other classes do). And it could be locked by calling methods on it. If we make it meet the BasicLockable requirement (i.e., make sure the methods are called lock() and unlock()) then c++ code should be able to do something like std::lock_guard<lldb::SBMutex> guard(sb_target.GetAPIMutex()) and python code could define __enter__ and __exit__ to call lock and unlock, respectively?
This is a bit of an issue for me - I build LLDB on those versions.
But nothing from SWIG is part of the redistributed packages, so I guess it could be doable to do a custom install of a newer version of SWIG for the build process. Probably doable, but it’s an extra hurdle.
On the swift fork we had a bunch of builders that couldn’t install swig, so we added a built version of the swig files to the sources. In normal development, you wouldn’t use the checked in ones, you are expected to rebuild them. But then there’s a CMake flag to say “use the static ones”.
That gives you a way to upgrade the swig output we use but not require it on builders. This is a little error-prone, when you add new API’s you need to update the static version of the bindings. But it’s been fairly workable on the swift side.
There is also the libc++ bootstrapping-build CI job. This job runs on Ubuntu 22.04 Docker build. This image currently uses actions/runner as base image. In the past we used 22.04 as base, I’m not sure why we switched to actions/runner. So I can’t say whether we an easily switch to 24.04.
If you mean, putting those bindings in the llvm repo, then this could technically work, but with my upstream hat, I don’t think I’d like that much as it means checking in some unreviewable blobs (I mean, it’s still text, which better than binary, but I’m pretty sure one could sneak in some xz-like exploit between all the swig-generated goo).
I’m also not sure what would be our internal policy towards that, but if it came to that, I don’t think this is the direction we’d take. I think we’d just bite the bullet and get a new version of swig. There would probably be some red tape involved, for which we’d appreciate some lead time (O(weeks), not years), but it should be doable.
It’s just that I don’t think it has to come to that, as I think either of my proposals are perfectly reasonable compromises. Number one lets you use the existing API with older Swigs. Number two sidesteps the move question by changing the API, but in a way, I think I like that API even more.
Alright, it sounds like bumping the SWIG version is a little trickier than I had hoped. That’s fine, I knew this was a long shot and at least for the issue motivating this, we have a few alternatives we can explore. I particularly like the SBMutex idea.
I definitely don’t want to go the route of checking in SWIG generated files. As the person that often gets to deal with the fallout of these files getting out of sync, I’m going to politely disagree with the following statement:
If we can get an upgraded swig on the bots then that is definitely the better solution. But it does seem a shame that we can’t implement useful features because we can’t update the bots.
Note, I don’t think that this needs to be a security concern, IRL. I think requiring any builder that is actually releasing lldb built products to keep their swig up to date, and build the swig products locally is a very reasonable policy. The static files would only be used on PR bots where the swig version is too old. I guess that could lead to exploits on the bots, but that seems a stretch.
Note, I’m not pushing this solution in particular, getting all the systems we need to build on to have a sufficiently up to date swig is an obviously better solution. I just don’t want us to have to not do reasonable things because one system somewhere has too old a version of swig installed locally.