LLDB API c++ exports

In trying to clean up the warnings in the Windows build, I found the
most prominent warning to be C4251 ('identifier' : class 'type' needs
to have dll-interface to be used by clients of class 'type2'). This
is because almost all of the exported classes (e.g. SBAddress, etc)
contain an stl object as part of their interface, usually a smart
pointer. This is a bad idea, as described in the following post:
http://stackoverflow.com/questions/5661738/how-can-i-use-standard-library-stl-classes-in-my-dll-interface-or-abi

The warning can usually be ignored if none of the non-exported classes
are made accessible through the public interface of the class, and it
seems that at least some effort has been made to see that this is the
case, but it does not appear to always be true. I've gone through
each of the exported classes, and found the following instances where
non-exported classes are exposed through the public interface:

    SBDebugger(const lldb::DebuggerSP &debugger_sp);
    SBFrame (const lldb::StackFrameSP &lldb_object_sp);
    SBProcess (const lldb::ProcessSP &process_sp);
    SBQueueItem (const lldb::QueueItemSP& queue_item_sp);
    void SBQueueItem::SetQueueItem (const lldb::QueueItemSP& queue_item_sp);
    SBTarget (const lldb::TargetSP& target_sp);
    SBThread (const lldb::ThreadSP& lldb_object_sp);
    SBValue (const lldb::ValueObjectSP &value_sp);
    lldb::ValueObjectSP SBValue::GetSP () const;
    SBWatchpoint (const lldb::WatchpointSP &wp_sp);
    lldb::WatchpointSP SBWatchpoint::GetSP () const;
    void SBWatchpoint::SetSP (const lldb::WatchpointSP &sp);

I guess there are a couple of approaches to fixing this:

1) Add a comment to each function explaining that it should be used
only internally, and suppress the warning.

2) Make the opaque pointer a raw pointer that is manually allocated in
the constructor and de-allocated in the destructor.

#2 seems like the "best" approach, but it's also more work. Does
anyone have strong feelings one way or the other?

You can't do #2 because all objects are tracked internally using shared pointers and sometimes the only thing keeping the backing object alive is the shared pointer itself. If you switch to normal pointers the objects could disappear and you could end up using the memory pointed to by the raw pointer as something that is no longer the type it used to be.

All of the above mentioned APIs above are not needed from a public API perspective, they are used by the other lldb::SB class implementation files to create objects and use the pointers inside lldb::SB objects. So nothing needs to be changed. If you want you can #ifdef them out using something that would be defined for the "lldb" shared library build so the LLDB.SB*.cpp classes can use them, but would not be defined for anyone linking against the library.

Maybe we can use

#if defined(LLDB_PRIVATE)
#endif

Around each function mentioned above. Then we would need the Xcode project and the make files to always define "LLDB_PRIVATE" when building LLDB.framework (macosx), lldb.so (linux) or lldb.dll (windows).

This should fix the warnings. The STL shared pointer classes are thread safe (atomic) and I really don't want to use anything else, so please don't change how the objects are owned within any lldb::SB* classes.

Greg Clayton

You can't use or make lldb::TargetSP's with the header files that exist on the SB side of the LLDB interfaces. They are all shared pointers to opaque classes that are only defined in private headers. Nor do any of the SB API's return lldb::*SP's, so you couldn't get one that way to pass to a call that takes one. It is pretty clear that these are not meant for public consumption.

The SetSP/GetSP API's are all private or protected, and the SB classes are NOT meant to be derived from by the outside world, so these are explicitly not for public consumption.

As for the others, if you want to put a comment, feel free, though it would be kind of annoying to have this boilerplate comment repeated everywhere. Might be better to put some policy rules in a common place (maybe LLDB.h or SBDefines.h) that says how the SB API's are to be used.

Jim

You can't use or make lldb::TargetSP's with the header files that exist on the SB side of the LLDB interfaces. They are all shared pointers to opaque classes that are only defined in private headers. Nor do any of the SB API's return lldb::*SP's, so you couldn't get one that way to pass to a call that takes one. It is pretty clear that these are not meant for public consumption.

The SetSP/GetSP API's are all private or protected, and the SB classes are NOT meant to be derived from by the outside world, so these are explicitly not for public consumption.

There are a few cases where they are not private or protected, see the
above list. All of the functions in that list are public (including
at least 3 versions of GetSP / SetSP).

You can't do #2 because all objects are tracked internally using shared pointers and sometimes the only thing keeping the backing object alive is the shared pointer itself. If you switch to normal pointers the objects could disappear and you could end up using the memory pointed to by the raw pointer as something that is no longer the type it used to be.

What I meant is that you change this (for example):

std::shared_ptr<ValueImpl> m_opaque_sp;

to this:

std::shared_ptr<ValueImpl>* m_opaque_sp;

And allocate in the constructor / copy constructor, delete in the
destructor. Then you change every occurrence of m_opaque_sp-> to
(*m_opaque_sp)->

You can't use or make lldb::TargetSP's with the header files that exist on the SB side of the LLDB interfaces. They are all shared pointers to opaque classes that are only defined in private headers. Nor do any of the SB API's return lldb::*SP's, so you couldn't get one that way to pass to a call that takes one. It is pretty clear that these are not meant for public consumption.

The SetSP/GetSP API's are all private or protected, and the SB classes are NOT meant to be derived from by the outside world, so these are explicitly not for public consumption.

There are a few cases where they are not private or protected, see the
above list. All of the functions in that list are public (including
at least 3 versions of GetSP / SetSP).

The GetSP/SetSP should probably all be private and friended to whoever needs them, that would be a nice cleanup.

But the point is that if you only have the SB API headers, not the _private ones, you can't actually make a lldb::TargetSP. So you can't use an interface that takes one, whether it is private or public. And if you are including the _private headers you are on the inside and we don't guarantee anything about binary compatibility for the lldb_private headers. We might want to document this, or as Greg says include some define to define these out when the SB API's are used naively (though you could always work around this...)

You can't do #2 because all objects are tracked internally using shared pointers and sometimes the only thing keeping the backing object alive is the shared pointer itself. If you switch to normal pointers the objects could disappear and you could end up using the memory pointed to by the raw pointer as something that is no longer the type it used to be.

What I meant is that you change this (for example):

std::shared_ptr<ValueImpl> m_opaque_sp;

to this:

std::shared_ptr<ValueImpl>* m_opaque_sp;

And allocate in the constructor / copy constructor, delete in the
destructor. Then you change every occurrence of m_opaque_sp-> to
(*m_opaque_sp)->

No, I don't think we should do that either. It just adds ugly boiler plate and the chance for error, and doesn't solve any actual problems.

Jim

Greg's suggestion will still generate the warnings (at least in MSVC),
for the simple fact that the shared_ptr<> etc is declared as a
stack-based class member. But if it's guaranteed that these methods
aren't publicly accessible, then suppressing the warning is probably
fine.

I'm a Windows person, so I don't have the means to easily test this
kind of change on Mac OSX. Does Mac OSX use the cmake build? If so,
does this mean I only need to define LLDB_PRIVATE in the cmake file
and can assume it will work on the Mac build? Or is there more
involved?

Does Mac OSX use the cmake build?

Not really. The Xcode lldb-tool build is the canonical MacOSX build. It builds with cmake but IIRC many (most?) of the tests fail with that build.

Hmm, I see. Complete Mac noobie here, is it possible to make these changes without Xcode? I looked at lldb.xcodeproj, and it’s harder to read than a Visual Studio .vcxproj file, so I assume this isn’t intended to be hand-edited.

Would it be sufficient to make my condition something like

#if defined(LLDB_API_INTERNAL) || defined(MAC_OSX)

submit it this way, and then let a mac person make the necessary changes to the Xcode build?

Disregard the above message, I got it done with no private define as discussed, so hopefully the patch I’ve uploaded is a better solution. If the methods and classes aren’t intended to be used externally anyway, then there’s no reason to even have them be part of the public interface to begin with, so I just made them all private.