This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Mark most SBAPI methods involving private types as protected or private
ClosedPublic

Authored by bulbazord on May 8 2023, 4:42 PM.

Details

Summary

Many SB classes have public constructors or methods involving types that
are private. Some are more obvious (e.g. containing lldb_private in the
name) than others (lldb::FooSP is usually std::shared_pointer<lldb_private::Foo>).

This commit explicitly does not address FileSP, so I'm leaving that one
alone for now.

Some of these were for other SB classes to use and should have been made
protected/private with a friend class entry added. Some of these were
public for some of the swig python helpers to use. I put all of those
functions into a class and made them static methods. The relevant SB
classes mark that class as a friend so they can access those
private/protected members.

I've also removed an outdated SBStructuredData test (can you guess which
constructor it was using?) and updated the other relevant tests.

Diff Detail

Event Timeline

bulbazord created this revision.May 8 2023, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 4:42 PM
bulbazord requested review of this revision.May 8 2023, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2023, 4:42 PM
mib accepted this revision.May 9 2023, 1:16 PM

LGTM with some comments.

lldb/include/lldb/API/SBBreakpoint.h
18–19

We've talked about this offline, but I think we should stay language agnostic in the SBAPI, so exposing a python namespace here is bothering me a little bit.

lldb/source/API/SBCommandInterpreter.cpp
37

I find it a bit odd to have this here ... May be we should move this class out the the API directory ?

lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
64–90

Although this is nested in the python namespace, I think SWIGBridge should be a templated class defined in the ScriptedInterpreter header and this class should be instead renamed as SWIGPythonBridge and be a specialization of the SWIGBridge class. We can do that as a follow-up but a TODO comment would be nice.

248–254

Can we move these in the python namespace as well ?

This revision is now accepted and ready to land.May 9 2023, 1:16 PM
bulbazord added inline comments.May 9 2023, 1:21 PM
lldb/include/lldb/API/SBBreakpoint.h
18–19

I agree, but we can refactor this later. Removing and/or changing forward declarations doesn't break ABI.

lldb/source/API/SBCommandInterpreter.cpp
37

It's a little odd for sure, but it is an implementation detail so having it in the cpp file seems alright. Where would you put it?

lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h
64–90

Sure, I can add the TODO. I'd rather not add the templates or subclass in this patch as it is complicated enough already.

248–254

Sure

bulbazord updated this revision to Diff 520810.May 9 2023, 1:42 PM

Address feedback and add TODOs for follow-up

This seems like a pretty non-intrusive way of protecting the lldb_private side of the SB API construction.

Looking at the patch makes it seem like we've been semi-randomly assorting members of the SB classes to "protected" and "private". We have NO intentions of ever subclassing these classes, so protected vrs. private is a meaningless distinction (thus the seeming randomness of the assignment, maybe?) It would be cleaner to go make them all private, since we don't intend to offer these for subclassing... But this patch is getting big already, probably don't want to fold that into this one.

lldb/unittests/API/SBCommandInterpreterTest.cpp
24–25

It isn't clear to me how the changes in this file fit in with your overall goal?

bulbazord added inline comments.May 9 2023, 2:19 PM
lldb/unittests/API/SBCommandInterpreterTest.cpp
24–25

I'm removing the SBCommandInterpreter member of this class because in order to construct an object of this class, SBCommandInterpreter would need to have an empty default constructor (which we do not have currently). I didn't want to add a constructor for SBCommandInterpreter just to avoid changing the test.

jingham added inline comments.May 9 2023, 2:48 PM
lldb/unittests/API/SBCommandInterpreterTest.cpp
24–25

Sounds reasonable.

This broke Lua support, so I am fixing it in D150624.

lldb/unittests/API/SBCommandInterpreterTest.cpp