tango-controls/cppTango

Attribute classes use incorrect compiler generated special member functions

t-b opened this issue · 1 comments

t-b commented

If you compile cppTango with -Wdeprecated and a recent clang you get the warning that compiler generated copy/move constructors/assigment special member functions are deprecated.

In file included from /home/firma/devel/cppTango/build/cppapi/server/idl/tangoDynSK.cpp:3:
/home/firma/devel/cppTango/build/cppapi/server/idl/tango.h:3515:13: warning: definition of implicit copy constructor for 'AttributeValue_4' is deprecated because it has a user-declared destructor [-Wdeprecated-copy-dtor]
    virtual ~AttributeValue_4() {if (mut_ptr != NULL)mut_ptr->unlock();}
            ^
/home/firma/devel/cppTango/build/cppapi/server/idl/tangoDynSK.cpp:4402:37: note: in implicit copy constructor for 'Tango::AttributeValue_4' first required here
  Tango::AttributeValue_4* _p = new Tango::AttributeValue_4(_s);
                                    ^
In file included from /home/firma/devel/cppTango/build/cppapi/server/idl/tangoDynSK.cpp:3:
/home/firma/devel/cppTango/build/cppapi/server/idl/tango.h:3556:13: warning: definition of implicit copy constructor for 'AttributeValue_5' is deprecated because it has a user-declared destructor [-Wdeprecated-copy-dtor]
    virtual ~AttributeValue_5() {if (mut_ptr != NULL)mut_ptr->unlock();}
            ^
/home/firma/devel/cppTango/build/cppapi/server/idl/tangoDynSK.cpp:4453:37: note: in implicit copy constructor for 'Tango::AttributeValue_5' first required here
  Tango::AttributeValue_5* _p = new Tango::AttributeValue_5(_s);
                                    ^

Now I thought I can just the declare the obvious thing the compiler does, and use = default, but I'm not sure that this is really what we want.

  1. Consider the patched Attribute_4 and Attribute_5 classes from the IDL defintion.

cppTango/cppapi/server/idl/CMakeLists.txt

Lines 33 to 38 in 1945e65

virtual ~AttributeValue_4() {if (mut_ptr != NULL)mut_ptr->unlock();}\n\
AttributeValue_4() {mut_ptr=NULL;}\n\
void set_attr_mutex(omni_mutex *ptr) {mut_ptr=ptr;}\n\
void rel_attr_mutex() {if (mut_ptr != NULL){mut_ptr->unlock();mut_ptr=NULL;}}\n\
omni_mutex *get_attr_mutex() {return mut_ptr;}\n\
omni_mutex *mut_ptr;\n")

Now the rule of three in C++98 says that if you define one of destructor/copy constructor/copy assignment you should define all of them. But that is not what is done here. So we have compiler generated ones.

But the compiler generated ones can't really work as they don't know anything about mut_ptr.

So what is the correct fix here? And why is that currently not a problem?

  1. The Attribute class has the same issue

cppTango/cppapi/server/attribute.cpp

Lines 236 to 244 in 1945e65

Attribute::~Attribute()
{
try
{
delete ext;
delete [] loc_enum_ptr;
}
catch(omni_thread_fatal &){}
}

mliszcz commented

After some analysis of Device_3Impl::read_attributes_no_except this is how I understand the problem:

  1. When we read attribute(s), we lock the mutex which protects the CORBA sequence stored in Attribute class:

    cppTango/cppapi/server/device_3.cpp

    Lines 516 to 522 in 1945e65

    // Take the attribute mutex before calling the user read method
    //
    if ((att.get_attr_serial_model() == ATTR_BY_KERNEL) && (aid.data_4 != nullptr || aid.data_5 != nullptr))
    {
    cout4 << "Locking attribute mutex for attribute " << att.get_name() << std::endl;
    omni_mutex *attr_mut = att.get_attr_mutex();

  2. We store the data in a structure that will be transferred over the network (AttributeValueList_5):

    cppTango/cppapi/server/device_3.cpp

    Lines 905 to 909 in 1945e65

    //
    // Data into the network object
    //
    data_into_net_object(att,aid,index,w_type,true);

  3. We pass the pointer to the mutex into this network object:

    cppTango/cppapi/server/device_3.cpp

    Lines 921 to 928 in 1945e65

    if ((atsm != ATTR_NO_SYNC) && ((att.is_fwd_att() == true) || (w_type != Tango::WRITE)))
    {
    cout4 << "Giving attribute mutex to CORBA structure for attribute " << att.get_name() << std::endl;
    if (atsm == ATTR_BY_KERNEL)
    GIVE_ATT_MUTEX_5(aid.data_5,index,att);
    else
    GIVE_USER_ATT_MUTEX_5(aid.data_5,index,att);
    }

Now my guess is that the AttributeValueList_5 we return from remote call shares the same data buffer as sequence stored in the Attribute. Thus the lock must outlive read_attributes_no_except and can be released only when omniORB is done with handling the call (and calls destructor of AttributeValue_5), because otherwise concurrent call to read_attribute may corrupt the data in the buffer.

But it makes no sense because we always delete the sequence stored in the attribute (delete ptr) immediately after we transfer the data (the_seq.replace()):

cppTango/cppapi/server/device.h

Lines 3664 to 3692 in 1945e65

#define DATA_IN_OBJECT(A,B,C,D) \
do \
{ \
Tango::A *ptr = att.B(); \
if (aid.data_5 != nullptr) \
{ \
(*aid.data_5)[index].value.D(C); \
A &the_seq = (*aid.data_5)[index].value.D(); \
the_seq.replace(ptr->length(),ptr->length(),ptr->get_buffer(),ptr->release()); \
if (ptr->release() == true) \
ptr->get_buffer(true); \
} \
else if (aid.data_4 != nullptr) \
{ \
(*aid.data_4)[index].value.D(C); \
A &the_seq = (*aid.data_4)[index].value.D(); \
the_seq.replace(ptr->length(),ptr->length(),ptr->get_buffer(),ptr->release()); \
if (ptr->release() == true) \
ptr->get_buffer(true); \
} \
else \
{ \
(*aid.data_3)[index].value <<= *ptr; \
} \
\
if (del_seq == true) \
delete ptr; \
} \
while (false)

I think we keep this mutex locked to support cases where the Attribute is not owning (ptr->release() is false) the buffer used by the sequence. This is to prevent one from calling Attribute code which might change the buffer as long as omniORB is 'owning' the data.

To answer your questions:

So what is the correct fix here?

This depends on what AttributeValue_5 copy-constructor does with the sequence. And from the generated tango.h it seems that it just calls sequence's copy-constructor, which in turns copies all the elements into new buffer regardless of whether original sequence owns the data or not. So IMO we should not copy the mut_ptr but set it to nullptr in the new sequence.

And why is that currently not a problem?

Either this is a problem but we just haven't seen it yet or maybe we never do any copies of AttributeValue_5.

The Attribute class has the same issue

ext member should be unique_ptr. As for loc_enum_ptr, I'm not sure, would need more analysis. But in the first place I suggest that Attribute should not be copyable.