Parameter resurrection considered harmful

[pah]

There is currently undefined behavior in the constructor of a handle "before" the parameter exists, see cci_param_untyped_handle.cpp:48

cci_param_untyped_handle::cci_param_untyped_handle(
  const cci_originator & originator, const std::string& name)
: m_originator(originator), m_orig_param(NULL), m_orig_param_name(name.c_str())
{}

Here, the m_orig_param_name pointer is initialized from a (potentially to be destroyed) string object. During resurrection, this pointer is used to find lookup the parameter from some broker again (see check_is_valid), potentially causing invalid memory accesses.

I thought of fixing this by already adding the name to the CCI name registry. But when looking at cci_name_gen.cpp, I realized that there is always a deep, string-based lookup involved when calling cci_get_name.

As a result, even the "fast path" constructor does such a potentially very costly name lookup for every handle creation:

cci_param_untyped_handle::cci_param_untyped_handle(
        cci_param_if & orig_param,
        const cci_originator & originator)
        : m_originator(originator), m_orig_param(&orig_param),
          m_orig_param_name(cci_get_name(orig_param.get_name().c_str())) // <-- HERE
{
    m_orig_param->add_param_handle(this);
}

We potentially create (multiple) handles upon parameter accesses for each registered callback (see e.g. cci_param_typed.h:608`):

    bool
    pre_write_callback(value_type value, const cci_originator &originator) const {
      // ...
      for (unsigned i = 0; i < m_pre_write_callbacks.vec.size(); ++i) {
         // ..
            // Prepare parameter handle for callback event
            cci_param_untyped_handle param_handle =
                    create_param_handle(m_pre_write_callbacks.vec[i].originator);
    }

In models with 1,000s of parameters (and other SystemC objects), such lookups will involve tens of string comparisons in cache-unfriendly locations for each callback upon each parameter access. And hierarchical names can become quite long in big systems, certainly beyond any small-string optimization limits.

This run-time complexity is not a reasonable design to address an exotic corner case. The two virtual function calls (add_param_handle, remove_param_handle) are already quite expensive for the callback case, where the parameter always outlives the handle.

What can we do about this?

I propose to get rid of resurrection (or "re-attaching") in the LRM and the PoC. If this is really a required use case for some scenarios, we can rather easily add a cci_utils::reattaching_param_handle implemented with the help of a create callback later. Such a "reattaching handle" could also take care of re-creating callbacks, which are not kept in the current implementation.

As a side effect, we fix the undefined behavior mentioned at the beginning of this issue.

[markfoodyburton]

correct me if I'm wrong, but that would remove huge amounts of code designed to handle resurrection all over the place? I am in favour.....

[pah]

Whether it's a huge amount is subjective. But it would remove code, yes. Most importantly, it would remove the magic boolean flag from the is_valid function.

And it would fix another "interesting" bug, where you can fool the type system:

cci_param<int> p( "int_param", 10 );
auto h1 = p.create_param_handle( cci_originator() );
auto h2 = cci_param_typed_handle<string>( h1 ); // try type conversion (mismatched type)
sc_assert( !h2.is_valid() );                    // handle is invalid, good!
sc_assert( h2.is_valid(true) );                 // ask for resurrection (optional)
sc_assert( h2.get_type_info() == typeid(int) ); // handle is resurrected, BAD!
h2.get_value();                                 // BOOM!

(Of course, this could be fixed by explicitly clearing the parameter name as well, when invalidating the typed handle.)

[pah]

I have pushed a potential fix, i.e. removal of the parameter reconnection behavior as ada677d (on top of #197). Should we agree on this change, I can include this in #197 easily.

[twieman]

In 11/2 WG meeting, we agrees to proceed; Philipp will add to #197.