[boost] Re: [serialization] Regressions?

From: Vladimir Prus (ghost_at_[hidden])
Date: 2005-05-10 02:36:27

Robert Ramey wrote:

>>> instantiated from this code of mine:
>>>
>>>         ofstream ofs(av[2]);
>>>         boost::archive::binary_oarchive oa(ofs);
>>>         oa << *data_builder.finish();
>>>
> 
> This code should be considered erroneous.  The documentation in 1.32
> addressed this but unfortunately the enforcement was lost in the shuffle.

Excuse me, but I refuse to accept that this code is erroneous. As I've said,
this is my existing design -- the 'finish' function returns non-const
pointer. The fact that somewhere else, I serialize the result of 'finish'
has nothing to do with 'finish' return type, and I should not be forced to
change my existing interface just because I use serialization somewhere.

> The intention is to trap the saving of tracked non-const objects. This is
> to prevent users from doing something like
> For(...
>     A a;
>     ...
>     ar << a; // will save the address of a - which is on the stack
> 
> If a is tracked here, instances of a after the first will be stored only
> as reference ids. When the data is restored, all the as will be the same.
> Not what the programmer intended - and a bear to find.
> This is really the save counterpart to the load situation which required
> the implementation of reset_object_address.

So, you're saing that "const" == "no allocated on stack"? I don't see why
this statement is true. I can just as well do this:

  void foo(const A& a)
  {
         ar << a;
  }

and circumvent your protection. Further, how often is it that non-pointer
object is tracked? I think it's rare case, while saving a pointer is a
common case, and making common case inconvenient for the sake of non-common
case does not seem right.

> Note that the documentation suggests that he above be reformulated as
> For(...
>     ar << a[i]; //
> 
> Enforcing const-ness also has the effect of preventing serialization from
> altering the state of the object being serialized - another almost
> impossible to find bug. 

Can you explain:
1. How this bug can happen
2. Why the "watch" command on gdb is not a reliable way to catch such bugs?

> Remember that when a tracked object is saved more 
> than once, only the first time is the data saved. If the object can be
> changed during serialization, we have a problem.
> 
> Having said that - the & operator doesn't do the const checking. Doing so
> inhibits its usage.
> Also, in spite of much effort, I was unable to make the const checking
> function to my taste when objects are wrapped in an nvp wrapper.
> 
> Also, I had to tweak a number of my tests and demos to make them work with
> this new rule. However, the tweaking was not difficult. If altering one's
> code to conform this rule isn't easy, one should make an effort to
> understand why. Its possible that a very subtle and hard to understand bug
> might be being introduced.

Are you saying that my code is buggy? 

>>> Where 'finish' returns auto_ptr<Data>. It's looks like serialization
>>> checks if the serialized type is 'const' and if not, complains.
>>> Basically, it's some heuritic to prevent saving on-stack object
>>> (though I don't understand why it would work at al).
>>>
>>> I find this a bit too much . I have no reason whatsoever to make
>>> 'finish' return auto_ptr<const Data>. And writing
>>>
>>>         oa << const_cast<const Data&>(*data_builder.finish());
>>>
> 
> This would "fix" it but might not be necessary.  I envisioned that the
> operator << would usually be used in a function templateof the following
> signature
> 
> save(Archive & ar, const T &t ,...
> 
> so that normally the issue wouldn't arise.  

In the above case, I'm saving a specific object with a specific type. Using
any function template is not an option.

> If it does arise, its sort a 
> warning to really think about what the code is doing. If the return value
> of data_builde.finish() can be changed during the serialization process,
> one is
> going to have a problem.  

How can it be changed? Due to bug in 'save' for by class type? How often it
happens?

> That is why the STATIC_ASSERT only trips when 
> tracking is enabled for the corresponding datatype.
> 
>>> looks very strange. And modifying all places where I get this error
>>> is not nice too. So, can this static assert be removed?
> 
> As I said, if its in a lot of places one should think about this.  If this
> is truely intolerable and you don't mind driving without seat belts, use
> the & operator instead.

This is inferiour solution, because operator<< is more logical. 

>>> Then, I get ambiguity in iserializer.hpp, in this code:
>>>
>>> #ifndef BOOST_NO_FUNCTION_TEMPLATE_ORDERING
>>> template<class Archive, class T>
>>> inline void load(Archive &ar, const serialization::nvp<T> &t){
>>>     load(ar, const_cast<serialization::nvp<T> &>(t));
>>> }
>>>
>>> For some reason, both boost::archive::load and some other 'load' in
>>> 'boost' namespace (part of earlier serialization lib) that I still
>>> use are considered overload candidates. Adding explicit
>>> boost::archive:: fixes this. See attached patch.
>>>
>>> Then I get error at this code:
>>>
>>>     ar << boost::lexical_cast<std::string>(*this);
>>>
>>> The error message is:
>>>
>>> error: no match for 'operator<<' in 'ar <<
>>>    boost::lexical_cast(Source) [with Target = std::basic_string<char,
>>>    std::char_traits<char>, std::allocator<char> >, Source =
>>>    lvk::nm_model::BlockFormula]()'
>>> /space/NM/boost/boost/archive/detail/interface_oarchive.hpp:75:
>>> error: candidates
>>>    are: Archive&
>>>
>>>
>>>
>>>
>>> boost::archive::detail::interface_oarchive<Archive>::operator<<(T&)
>>> [with T = std::basic_string<char, std::char_traits<char>,
>>> std::allocator<char> >, Archive = boost::archive::binary_oarchive]
>>> ...unrelated operator<< definitions snipped...
>>>
>>> Apparently, passing rvalue to "T&" does not work. Yes another
>>> attached patch fixes this issue.
> 
> This is the same issue as before.  Passing a non-const tracked type to the
> archive << operator.

Ok,
1. Why std::string is 'tracked type'?
2. How do you suggest me to fix the above?

- Volodya