[Libreoffice] cppu::OPropertySetHelper ABI backwards compatibility

Mon Aug 22 09:18:01 PDT 2011

On Sun, Aug 21, 2011 at 12:08:23PM +0200, Lionel Elie Mamane wrote:
> On Sun, Aug 21, 2011 at 01:54:17AM +0200, Lionel Elie Mamane wrote:

>> 11207ae93191fb966676423e6d377c8292a8cf0b
>>     Make XPropertSet2 not a child interface of XPropertySet.

>>     This is to preserve ABI backward compatibility with
>>     cppu::OPropertySetHelper.

>> 80b1e662777100a7dfd80176a2b528880a838167

>>     Added XPropertySet2 to allow disabling of change event
>>     notifications.

>> From the second (later) commit log message, you intended to preserve
>> ABI, but I get the impressions ABI backwards compatibility is still
>> broken in a different way by the addition of

>>     virtual void SAL_CALL enableChangeListenerNotification( sal_Bool bEnable )
>>         throw(::com::sun::star::uno::RuntimeException);

>> From what I observe, at least with GNU GCC/g++ on Debian GNU/Linux
>> amd64 (I wouldn't know about MSVC++ and g++ on MS Windows, but
>> possibly it is the same), it seems all the virtual functions that are
>> declared *after* that one in the header are shifted one position in
>> the virtual function table, and that lookups in the virtual function
>> table are by position, not by name/signature.

> Maybe not. It seems my build was hosed, possibly because of wrong
> inter-module dependencies so that a change to
> cppuhelper/propshlp.hxx is not properly propagated everywhere.

> I'm making new tests doing "make clean && make" every time, and I'll
> let you know what I find out.

I confirm that ABI is broken for cppu::OPropertySetHelper with respect
to 3.4 branch.

I observe, additionally to the "wrong function called from new ABI
code to old ABI code that derives (inherits) from
cppu::OPropertySetHelper" that I already mentioned, that the
destructor for cppu::OPropertySetHelper::Impl is called with
non-sensical "this" pointer when (in old ABI code) a reference to a
class that inherits from cppu::OPropertySetHelper goes out of scope
and the object is destroyed. I have the case with postgresql-sdbc
compiled against libreoffice-3-4.

Here's an example trace of cppu::OPropertySetHelper::Impl ctor/dtor
calls:

Impl ctor this='0x2e851a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e97418'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e862e8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e97598'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e8d0a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e89128'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91638'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91fb8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e92288'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e92558'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e93238'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e91518'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e94298'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e94ac8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e95148'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e95248'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9a0a8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9bc48'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8ebd8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8d098'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f8db48'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f95438'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f93ec8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f98118'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f96a98'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9ade8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f99778'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9db28'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2f9c458'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa0108'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2e9a318'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa37f8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa21d8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa6568'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fa4e98'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fae6f8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fc7978'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fc8398'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl ctor this='0x2fbefa8'; m_handles.size()='0', m_newValues.size()='0', m_oldValues.size()='0'
Impl dtor this='0x7ffdb09d57c8'; m_handles.size()='148277658', m_newValues.size()='6148914691236407954', m_oldValues.size()='-6148914691211804262'

The attached patch makes both these symptoms go away, which is why I
conclude that ABI has changed; I'm not saying this patch should be
applied as-is; rather it should go this way, as mentioned in my first
email:

class OPropertySetHelperFireEventOption : public OPropertySetHelper,
                                          public ::com::sun::star::beans::XPropertySetOptions
{
    bool m_bFireEvent;
public:
(...)
    virtual void SAL_CALL enableChangeListenerNotification( sal_Bool bEnable )
        throw(::com::sun::star::uno::RuntimeException);
(...)
}

So that new code that wants to have the ability to enable/disable
change listener notifications can derive from OPropertySetHelper2
instead, but old code still works.

If people agree that this would be the way to go, I'll prepare a patch
that does that; I'm also looking for a better class name than
OPropertySetHelperFireEventOption :)

-- 
Lionel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ABI.patch
Type: text/x-diff
Size: 3452 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20110822/4f23f50b/attachment.patch>