This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Replace SB swig interfaces with API headers
ClosedPublic

Authored by bulbazord on Jan 30 2023, 12:55 PM.

Details

Summary

Instead of maintaining separate swig interface files, we can use the API
headers directly. They implement the exact same C++ APIs and we can
conditionally include the python extensions as needed. To remove the
swig extensions from the API headers when building the LLDB
framework, we can use the unifdef tool when it is available. Otherwise
we just copy them as-is.

Diff Detail

Event Timeline

bulbazord created this revision.Jan 30 2023, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 12:55 PM
bulbazord requested review of this revision.Jan 30 2023, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 12:55 PM
mib added a comment.Jan 30 2023, 1:45 PM

I'm very excited about this :) ! We've already discussed that offline but it would be nice if we could use Doxygen comments in the header files to generate the python Docstrings (I'm pretty sure SWIG supports it).

Then, for python specific comments, we would have a separate documentation file. Also, naming-wise, since the file extension doesn't really matter for SWIG, I'd just match the header file name and change the extension (what about .i for the code specific to the target language and .rst for the documentation).

lldb/bindings/macros.swig
50–51

Pretty cool!

lldb/cmake/modules/LLDBFramework.cmake
99–100

May be we should add a cmake check to make sure unidef is available on the system.

clayborg added inline comments.Jan 30 2023, 5:10 PM
lldb/include/lldb/API/SBAddress.h
14–16

Do we want two different .i files? Right now we have both"interface/SB*Docstrings.i" and "interface/SB*Extensions.i". Can those be combined into just one "interface/SBAddress.i" and only included if we have anything in the .i file that isn't just a copy of the API itself? Or does the doc string stuff have to come first?

30

Do we need all assignment operators left out for Swig? If so it might be good to add a comment explaining so it is clear to people. I can't remember if this is true or not in all cases?

128

Do we need all == operators left out of Swig stuff? Comment if so? Can't remember if this applies to all API objects or just to SBAddress?

bulbazord added inline comments.Jan 30 2023, 5:34 PM
lldb/cmake/modules/LLDBFramework.cmake
99–100

Yeah for the purposes of this diff I just used it without checking. If we go ahead with this patch I'll add some safety checks.

lldb/include/lldb/API/SBAddress.h
14–16

What I discovered when working this patch is that the docstring information needs to come before the declaration of what it wants to document. So SB*Docstrings.i should come first. The SB*Extensions.i are there to extend an existing class, so they should come after the class itself. I'm not sure there's a good way to do this.

Alternatively, we could changes the interfaces.swig file to include ./interface/SBAddressDocstrings.i, then lldb/API/SBAddress.h, then ./interface/SBAddressExtensions.i. That way we don't need to have these 2 #ifdef blocks in the API headers themselves.

30

Yeah I can add a comment. When generating python bindings, swig will ignore assignment operators as it doesn't map cleanly into python, which is why I left this out. It may be more accurate to do something like #ifndef SWIGPYTHON but seeing as the SBAddress.i class doesn't have an operator= in it, I decided to leave it out for swig generation entirely.

128

I don't think that all instances of operator== need to be left out. There are some classes that have it in the interface files, e.g. SBBreakpoint. I intentionally left this one out because it wasn't in SBAddress.i and I wanted this to leave the python bindings roughly the same (no new functions/methods, re-ordering them in the lldb.py should be ok though).

clayborg added inline comments.Feb 2 2023, 3:48 PM
lldb/include/lldb/API/SBAddress.h
14–16

Anything we can do to keep the headers clean would be great. I know that was the original reason we started using them.

30

Will it just ignore these if they are left in, or will it end up doing the wrong thing? Anything we can do to keep the header files clean is good. No worries if we have to keep the #ifndef stuff

128

Again, cleaner headers would be nice if we can get away with removing the #ifndef stuff

bulbazord added inline comments.Feb 2 2023, 4:03 PM
lldb/include/lldb/API/SBAddress.h
14–16

I'll try to minimize the noise in the API headers when I do the complete thing.

30

It ignores them and emits a warning. I suppose we don't care about these specific warnings and swig has a way of disabling specified warnings so I'll just do that instead.

128

I'm going to keep this one for now. This change is supposed to be relatively NFC and I don't want to introduce extra functionality in the python bindings at the same time we do this. We can re-evaluate adding this to the python bindings at a later time.

kastiglione added inline comments.
lldb/include/lldb/API/SBAddress.h
14–16

Alternatively, we could changes the interfaces.swig file to include ./interface/SBAddressDocstrings.i, then lldb/API/SBAddress.h, then ./interface/SBAddressExtensions.i. That way we don't need to have these 2 #ifdef blocks in the API headers themselves.

I was going to make this suggestion. I think it's better to do that.

labath added a comment.Feb 3 2023, 4:21 AM

I like this.

I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also overridable by the user).

kastiglione added inline comments.Feb 3 2023, 9:44 AM
lldb/bindings/interface/SBAddressDocstrings.i
2–3

How much of these header docs are specific to the API, but not specific to Swig bindings? Should/can we move these to the SB headers and have Swig make use of those? In other words, can/should files like SBAddressDocstrings.I contain only docs that are specific to bindings?

mib added inline comments.Feb 3 2023, 9:50 AM
lldb/bindings/interface/SBAddressDocstrings.i
2–3

IIUC, @bulbazord feel free to correct me on this, most doc will be generate from doxygen in the header file and only things that are specific to the bindings will be in the interface.

I like this.

I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also overridable by the user).

Yeah we could search for unified earlier in the process and use it when we distribute headers. The de-swig-ification of the headers is primarily for distribution, do any other platforms distribute the headers? I'm sure that happens, but I'm unsure if they do anything other than just copying the headers outright.

lldb/bindings/interface/SBAddressDocstrings.i
2–3

What Ismail is saying is what I'd like us to move to. Some of the docstrings are not specific to python at all and can be moved to the header files and generated from doxygen. I think that would best be done in a follow-up change in order to make this one more manageable as there is quite a lot to change already.

JDevlieghere added inline comments.Feb 3 2023, 12:46 PM
lldb/bindings/interface/SBAddressDocstrings.i
2–3

+1 for breaking this up into separate patches. Does SWIG automatically pick up Doxygen comments? That seems like a good thing to do regardless, so maybe something we can do while we're ironing out some of the other details in this review.

bulbazord updated this revision to Diff 495695.Feb 7 2023, 5:38 PM

First pass at a complete implementation. Actions taken:

  • Added swig flags to ignore swig warnings indicating that swig is ignoring certain operator overloads (e.g. operator=)
  • Broke SB${Class}.i files into SB${Class}Docstrings.i (containing the docstrings) and SB${Class}Extensions.i (containing additional python functionality)
  • Deleted all SB${Class}.i files.
  • Moved necessary typemaps into python-typemaps.swig.
  • Audited all SB class headers and add preprocessor macros to ignore any functions and methods that were not implemented in the SB${Class}.i files.
    • This is just to make this change more "atomic" (i.e. not adding any new functionality)

I can imagine that folks will have some opinions about this patch. It is pretty large, contains a few
"warts" (some of the swig header guards aren't very pretty), and there are probably better ways to do some things.
I'd therefore like to start getting feedback on this patch now that it works on my machine.
I've run the test suite on my macOS machine which I'm sure won't run every single test in every single configuration that
get run on the bots so I expect there to be some amount of churn even if this is approved and lands.

bulbazord retitled this revision from [lldb][RFC] Replace SB swig interfaces with API headers to [lldb] Replace SB swig interfaces with API headers.Feb 7 2023, 5:39 PM
bulbazord edited the summary of this revision. (Show Details)

In addition to what I wrote above, I also fixed several documentation bugs (putting docstrings on the wrong method).

I manually audited the generated code before and after this change. Here are my notes:

Not too interesting:

  • Some parameter names changed
  • Fixed lots of documentation bugs
  • Some documentation changed because of a re-ordering of function declarations

Potentially interesting:

  • SBData::GetDescription base_addr parameter has default value now
  • SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
  • SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

Certainly interesting:

  • SBListener::StopListeningForEventClass return type conflicts (ABI break?)
  • SBProcess::GetQueueAtIndex parameter types conflict (ABI break?)
  • SBTarget::BreakpointCreateByNames first parameter is different: const char **symbol_name vs const char *symbol_name[]. I'm not sure if this is going to be an issue.

I can see how having parity between the signatures that were exposed in the interface files makes the verification easier, but do we actually want to land the patch like that? Does anyone care that we expose more?

In addition to what I wrote above, I also fixed several documentation bugs (putting docstrings on the wrong method).

I manually audited the generated code before and after this change. Here are my notes:

Not too interesting:

  • Some parameter names changed
  • Fixed lots of documentation bugs
  • Some documentation changed because of a re-ordering of function declarations

Potentially interesting:

  • SBData::GetDescription base_addr parameter has default value now
  • SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
  • SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

Certainly interesting:

  • SBListener::StopListeningForEventClass return type conflicts (ABI break?)

No, probably a copy paste error when originally checked into .i file?

  • SBProcess::GetQueueAtIndex parameter types conflict (ABI break?)

No, probably a copy paste error when originally checked into .i file?

  • SBTarget::BreakpointCreateByNames first parameter is different: const char **symbol_name vs const char *symbol_name[]. I'm not sure if this is going to be an issue.

Maybe SWIG didn't like the "const char **" or it didn't do the right thing? Test and verify if we switch to using the header file we have no issues please?

Potentially interesting:

  • SBData::GetDescription base_addr parameter has default value now
  • SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
  • SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

Make sure it does work. I seem to remember people sometimes making two functions in the .i file for default parameters. Can't remember if SWIG does the right thing with default params or not, but please test to ensure both work (with and without).

bulbazord updated this revision to Diff 495978.Feb 8 2023, 3:46 PM
  • Removed a bunch of header guards effectively adding methods to the python bindings.

Certainly interesting:

  • SBListener::StopListeningForEventClass return type conflicts (ABI break?)

No, probably a copy paste error when originally checked into .i file?

Likely. This method is already tested in TestListener.py. Previously it returns an uint32_t (which turns into a python integer), now it returns a bool.

  • SBProcess::GetQueueAtIndex parameter types conflict (ABI break?)

No, probably a copy paste error when originally checked into .i file?

Almost certainly. This method is well tested, I don't have many concerns here.

  • SBTarget::BreakpointCreateByNames first parameter is different: const char **symbol_name vs const char *symbol_name[]. I'm not sure if this is going to be an issue.

Maybe SWIG didn't like the "const char **" or it didn't do the right thing? Test and verify if we switch to using the header file we have no issues please?

I'm not 100% sure why there was some divergence. For now I've done some ifdef/else/endif to separate this. I don't think there's a problem if we pick one over the other but this method is completely untested so I am punting it a bit. :)

Potentially interesting:

  • SBData::GetDescription base_addr parameter has default value now
  • SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
  • SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now

Make sure it does work. I seem to remember people sometimes making two functions in the .i file for default parameters. Can't remember if SWIG does the right thing with default params or not, but please test to ensure both work (with and without).

I went through all 3 of them myself. Their result of having a default argument now is that there is a version of the function exposed that has 1 less argument. For example, SBInstructionList::GetInstructionsCount has 3 arguments where the last one now has a default value. You can still do GetInstructionsCount(arg1, arg2, arg3) where arg3 can be True or False. This is the same behavior as before. The new thing that got added was GetInstructionsCount(arg1, arg2) where arg3 will default to False without you specifying it. This is true for SBData::GetDescription and SBMemoryRegionInfo's 3rd constructor.

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

SBAttachInfo::~SBAttachInfo()
SBBroadcaster::operator<(const lldb::SBBroadcaster &) const;
SBCommandInterpreter::SourceInitFileInHomeDirectory(lldb::SBCommandReturnObject &, bool);
SBCommandInterpreterRunOptions::SBCommandInterpreterRunOptions(const SBCommandInterpreterRunOptions &);
SBCommandInterpreterRunOptions::GetEchoCOmmentCommands() const;
SBCommandInterpreterRunOptions::SetEchoCommentCommands(bool);
SBCommandInterpreterRunOptions::GetAutoHandleEvents() const;
SBCommandInterpreterRunOptions::SetAutoHandleEvents(bool);
SBCommandInterpreterRunOptions::GetSpawnThread() const;
SBCommandInterpreterRunOptions::SetSpawnThread(bool);
SBDebugger::GetBroadcasterClass();
SBDebugger::PrintDiagnosticsOnError();
SBDebugger::SkipAppInitFiles(bool);
SBDebugger::SaveInputTerminalState();
SBDebugger::RestoreInputTerminalState();
SBDebugger::SetUseSourceCache(bool);
SBDebugger::GetUseSourceCache() const;
SBDebugger::GetREPLLanguage() const;
SBDebugger::SetREPLLanguage(lldb::LanguageType);
SBEvent::GetDescription)(lldb::SBStream &);
SBLaunchInfo::~SBLaunchInfo();
SBLaunchInfo::SBLaunchInfo(const SBLaunchInfo &);
SBModule::GetUUIDBytes() const;
SBPlatform::SBPlatform(const SBPlatform &);
SBProcess::Detach(bool);
SBProcess::GetStopEventForStopID(uint32_t);
SBProcess::GetBroadcasterClass();
SBProcess::LoadImage(const lldb::SBFileSpec &, const lldb::SBFileSpec &, lldb::SBError &);
SBQueue::SBQueue(const SBQueue &);
SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();
SBSourceManager::SBSourceManager(const SBDebugger &);
SBSourceManager::SBSourceManager(const SBTarget &);
SBStringList::GetStringAtIndex(size_t) const;
SBTarget::BreakpointCreateByName(const char *, const SBFileSpecList &, const SBFileSpecList &);
SBTarget::BreakpointCreateByRegex(const char *, const SBFileSpecList &, const SBFileSpecList &);
SBThreadPlan::SBThreadPlan(lldb::SBThread &, const char *, lldb::SBStructuredData &);
SBThreadPlan::QueueThreadPlanForStepOverRange(SBAddress &, lldb::addr_t, SBError &);
SBThreadPlan::QueueThreadPlanForStepInRange(SBAddress &, lldb::addr_t, SBError &);
SBThreadPlan::QueueThreadPlanForStepOut(uint32_t, bool, SBError &);
SBThreadPlan::QueueThreadPlanForRunToAddress(SBAddress, SBError &);
SBTrace::LoadTraceFromFile(SBError &, SBDebugger &, const SBFileSpec &);
SBTypeMember::GetDescription(lldb::SBStream &, lldb::DescriptionLevel);
SBType::GetDescription(lldb::SBStream &, lldb::DescriptionLevel);
SBTypeList::SBTypeList(const lldb::SBTypeList &);
SBTypeCategory::operator==(lldb::SBTypeCategory &);
SBTypeCategory::operator!=(lldb::SBTypeCategory &);
SBTypeSummary::DoesPrintValue(lldb::SBValue);
SBTypeSynthetic::IsClassName();
SBValue::Watch(bool, bool, bool);
SBWatchpoint::Clear();

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Are they not functional from native code? or just not functional from Python?

JDevlieghere added a comment.EditedFeb 9 2023, 1:57 PM

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Are they not functional from native code? or just not functional from Python?

The feature was removed but for ABI stability reasons we can't remove them from the SB API. Most/all of them return an error saying the feature has been removed.

Not sure how useful it would be but I recorded the full list of methods get added with this change. Take a look and let me know if there are any that you think shouldn't be added.

Thanks for generating that list. Given that the reproducers are not functional, let's not add these to the Python API:

SBReproducer::Capture();
SBReproducer::Replay(const char *);
SBReproducer::Replay(const char *, bool);
SBReproducer::Finalize(const char *);
SBReproducer::GetPath();
SBReproducer::Generate();

Are they not functional from native code? or just not functional from Python?

The feature was removed but for ABI stability reasons we can't remove them from the SB API. Most/all of them return an error saying the feature has been removed.

Does this mean we could submit a patch that removes all of the "LLDB_INSTRUMENT*" stuff that is in all of the SB*.cpp files? That would be great to clean that up if they are not needed.

Does this mean we could submit a patch that removes all of the "LLDB_INSTRUMENT*" stuff that is in all of the SB*.cpp files? That would be great to clean that up if they are not needed.

No, they are still needed. They still track API boundaries, just like they did for the reproducers. The API boundaries are used for the API log and for timers/os_signposts. What they do is make sure that when you call into the SB API and the implementation of that SB API method calls another SB API, only the outer one will show up in the log. The macros also ensure that every function is correctly logged. Before the macros, half of the methods didn't log to the API log and the ones that did were riddled with inconsistencies (typos, copy-paste errors, etc).

bulbazord updated this revision to Diff 496299.Feb 9 2023, 5:46 PM

Stop exposing unsupported SBReproducer functions

Does anybody have any further concerns or objections? I'd like to get this change in early next week if possible.

I like this.

I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also overridable by the user).

Yeah we could search for unified earlier in the process and use it when we distribute headers. The de-swig-ification of the headers is primarily for distribution, do any other platforms distribute the headers?

Some certainly do. I haven't found them in debian, but they are present on gentoo for instance. The would be necessary if someone wanted to build a tool linking to liblldb, so I would consider the fact that they are missing a bug.

I'm sure that happens, but I'm unsure if they do anything other than just copying the headers outright.

Well... right now, there isn't anything else to do there.

Anyway, I don't think this is particularly important. I don't know even if the distros would like to do this or whether to they'd prefer to ship unchanged headers. However, to me, it seems like the choice of the distribution method (framework vs. "traditional" unix layout) should be orthogonal to the choice of deswig-ifying the headers.

Well... right now, there isn't anything else to do there.

Anyway, I don't think this is particularly important. I don't know even if the distros would like to do this or whether to they'd prefer to ship unchanged headers. However, to me, it seems like the choice of the distribution method (framework vs. "traditional" unix layout) should be orthogonal to the choice of deswig-ifying the headers.

I think I agree with you about the choice of distribution method being orthogonal to choice of removing the swig ifdefs from the headers. That being said, I don't know if any other distributors would care for this and I'm not sure who I would ask. I added support for using unifdef in the LLDB.framework case because I can ensure it is used and tested. I am hesitant to add support without somebody to test/use this.

Sort of tangential to this, the only other target where I think this would be used would be the lldb-headers target. That seems to install all the lldb headers, including all the private lldb headers. I could use unifdef there (assuming it's available). Is it intended that it installs all the private lldb headers though?

bulbazord updated this revision to Diff 497117.Feb 13 2023, 2:40 PM

Rebasing against top of tree

mib accepted this revision.Feb 14 2023, 12:19 PM

LGTM!

This revision is now accepted and ready to land.Feb 14 2023, 12:19 PM
This revision was automatically updated to reflect the committed changes.

I don't think this necessarily matters or is a reason to revert, but we are seeing issues with this patch and swig 3.0.2, that disappear with swig 4.0.

The docs claim that versions past 3.0 are supported (except for some odd corner cases).

The failure is:

Traceback (most recent call last):
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py", line 19, in test_address_breakpoints
    self.address_breakpoints()
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py", line 50, in address_breakpoints
    launch_info = lldb.SBLaunchInfo(None)
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/google3/third_party/llvm/llvm-project/lldb/lib/python/site-packages/lldb/__init__.py", line 6239, in __init__
    this = _lldb.new_SBLaunchInfo(*args)
ValueError: invalid null reference in method 'new_SBLaunchInfo', argument 1 of type 'lldb::SBLaunchInfo const &'
Config=x86_64-/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/devtools/c/lldb/testing/clang

If I modify the line

launch_info = lldb.SBLaunchInfo(None)

to

launch_info = lldb.SBLaunchInfo("")

I get this error, which gives a clue to the fix:

NotImplementedError: Wrong number or type of arguments for overloaded function 'new_SBLaunchInfo'.
  Possible C/C++ prototypes are:
    lldb::SBLaunchInfo::SBLaunchInfo(char const **)
    lldb::SBLaunchInfo::SBLaunchInfo(lldb::SBLaunchInfo const &)

So if I use a vector of strings to match the first overload, the error disappears and the tests pass:

launch_info = lldb.SBLaunchInfo(["", ""])

Not sure the best way forward here. Either to abandon old versions of swig, or to clean up the launch command.

I don't think this necessarily matters or is a reason to revert, but we are seeing issues with this patch and swig 3.0.2, that disappear with swig 4.0.

The docs claim that versions past 3.0 are supported (except for some odd corner cases).

The failure is:

Traceback (most recent call last):
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py", line 19, in test_address_breakpoints
    self.address_breakpoints()
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/llvm/llvm-project/lldb/test/API/functionalities/breakpoint/address_breakpoints/TestAddressBreakpoints.py", line 50, in address_breakpoints
    launch_info = lldb.SBLaunchInfo(None)
  File "/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/third_party/py/lldb/dotest_wrapper.par/google3/third_party/llvm/llvm-project/lldb/lib/python/site-packages/lldb/__init__.py", line 6239, in __init__
    this = _lldb.new_SBLaunchInfo(*args)
ValueError: invalid null reference in method 'new_SBLaunchInfo', argument 1 of type 'lldb::SBLaunchInfo const &'
Config=x86_64-/build/work/37f17038e9cfbfbee7db556850bc7314bdb1/google3/runfiles/google3/devtools/c/lldb/testing/clang

If I modify the line

launch_info = lldb.SBLaunchInfo(None)

to

launch_info = lldb.SBLaunchInfo("")

I get this error, which gives a clue to the fix:

NotImplementedError: Wrong number or type of arguments for overloaded function 'new_SBLaunchInfo'.
  Possible C/C++ prototypes are:
    lldb::SBLaunchInfo::SBLaunchInfo(char const **)
    lldb::SBLaunchInfo::SBLaunchInfo(lldb::SBLaunchInfo const &)

So if I use a vector of strings to match the first overload, the error disappears and the tests pass:

launch_info = lldb.SBLaunchInfo(["", ""])

Not sure the best way forward here. Either to abandon old versions of swig, or to clean up the launch command.

I'm not sure we're going to be able to drop support for older versions of swig at this time. Prior to this change, SBLaunchInfo only had one constructor exposed to python (the one that takes a const char **). This change exposes the SBLaunchInfo copy constructor.
The way I see it there are two options: (1) Stop exposing the copy constructor, or (2) Modify all the uses of SBLaunchInfo to take a list of empty strings.
Considering that this worked fine before, I'm inclined to stop exposing the SBLaunchInfo copy constructor. I'll upload a patch to handle that.

I'll upload a patch to handle that.

D144228

Well... right now, there isn't anything else to do there.

Anyway, I don't think this is particularly important. I don't know even if the distros would like to do this or whether to they'd prefer to ship unchanged headers. However, to me, it seems like the choice of the distribution method (framework vs. "traditional" unix layout) should be orthogonal to the choice of deswig-ifying the headers.

I think I agree with you about the choice of distribution method being orthogonal to choice of removing the swig ifdefs from the headers. That being said, I don't know if any other distributors would care for this and I'm not sure who I would ask. I added support for using unifdef in the LLDB.framework case because I can ensure it is used and tested. I am hesitant to add support without somebody to test/use this.

Sort of tangential to this, the only other target where I think this would be used would be the lldb-headers target. That seems to install all the lldb headers, including all the private lldb headers. I could use unifdef there (assuming it's available). Is it intended that it installs all the private lldb headers though?

I don't know... maybe it isn't? I guess this isn't particularly important and people can and deswigification steps there if they want to.

I really like the rest of the patch though.

lldb/bindings/interface/SBValueListExtensions.i