Fix handling of a dynamically created event with an empty name

[atalii]

6f1c5d9 is the important commit; the message is copied below with some extra info following:

sc_event: support dynamic events with empty names

Per the LRM, an event created during or after initialization with an
empty name must still be initialized. It is implementation defined
whether or not this becomes a hierarchical event or a top-level event,
but in the first case, it must have a name that begins with "event".
Specifically, the GTest in the following module, instantiated in the
natural manner, fails prior to this commit:

struct hierarchical_event_container : public sc_core::sc_module {
    SC_CTOR(hierarchical_event_container)
    {
        SC_THREAD(do_test);
    }

    void do_test()
    {
        // "Calling the constructor sc_event(const char*) with an empty string
        // will have the same effect as calling the default constructor,
        // regardless of when it is called by the application." (§5.10.4)
        auto e = sc_core::sc_event("");

        // "When the default constructor is called by the application from the
        // initialization phase onwards, whether or not a hierarchically named
        // event is created shall be implementation-defined on a per-instance
        // basis" (§5.10.4), so we may expect to see our event either in the
        // top_level_events or as a hierarchically named child of ourself.
        auto evs = sc_core::sc_get_top_level_events();
        auto hierarchical_evs = sc_core::sc_find_object("hec.do_test")->get_child_events();
        for (auto e : hierarchical_evs) {
    	    evs.push_back(e);
        }
        ASSERT_THAT(evs, Contains(&e).Times(1));

        if (std::find(hierarchical_evs.begin(), hierarchical_evs.end(), &e) != hierarchical_evs.end()) {
    	    // Due to the guard, we may assume that the event is hierarchical.
    	    // Since "[w]hen a hierarchically named event is constructed, if a
    	    // non-empty string is passed as a constructor argument, that string
    	    // shall be used to set the string name of the event. Otherwise, the
    	    // string name shall be set to "event". The string name shall be
    	    // used to determine the hierarchical name as described in 5.17," we
    	    // may expect that it begins with "event" (§5.10.4).
    	    EXPECT_THAT(std::string(e.name()), StartsWith("hec.do_test.event"));
        }
    }
};

My fix is to simply remove what I believe to be an errant refusal to
register a no-name event if the simulation is running.

---

Two additional points:

1. I'm suspicious of the fix I propose since it seems too short, and I don't trust that I haven't mistakenly allowed erroneous behavior. I would appreciate a more familiar pair of eyes to look this over! @pah - you seem to be the last person to have touched this code per git blame (though the exact lines go back to the initial check-in). Perhaps you could let me know what the purpose of that check was? No worries if not, I just figured I'd ask.
2. My personal fork has a branch with GTests that should be a little more informative and reliable than the golden-file stuff. I'd like to build that out more and attempt to upstream it soon-ish (within a month or two). If you see any major blockers there already, please let me know.

Thank you!

[pah]

What your patch does, is to make every event hierarchically named, including those created during simulation. This is allowed by the standard, see IEEE 1666-2023, 5.10.4:

When the default constructor is called by the application from the initialization phase onwards,
whether or not a hierarchically named event is created shall be implementation-defined on a per-
instance basis.

So far, the reference implementation decided to NOT create hierarchically named events during simulation, which in turn results in:

• ev.in_hierarchy() == false
• ev.get_parent_object() == nullptr
• ev NOT being in either sc_get_top_level_events() or any get_child_events()
• ev.name() == ev.basename() == "" (⚠️)

According to the standard, this empty name is currently non-compliant:

An event that is not hierarchically named shall have a non-empty implementation-defined name.

To fix this, I would suggest to change sc_event::name to something to the effect of:

inline const char*
sc_event::name() const
{
  return SC_LIKELY_(!m_name.empty()) ? m_name.c_str() : "$$unnamed$$";
}

The registration in the hierarchy still needs to be skipped during simulation to keep the original intent.

[atalii]

Right, thanks a ton @pah - I assumed (wrongfully) that non-hierarchical events would still appear in sc_get_top_level_events(). Retrospectively, I'm unsure where that assumption came from; it seems quite clear that it wouldn't. Happy to see I wasn't mistaken with the empty name being non-compliant, though.

Regarding your suggestion, I would prefer that m_name be set to $$unnamed$$` or similar during construction so that we don't have to deal with the possibility of an invalid member variable, but that's a very small nitpick. I'll update the patch to do something similar.

[pah]

The latest commit counteracts the idea of keeping event generation during simulation lightweight to reduce the performance impact. Especially using sc_gen_unique_name is pretty costly. Secondly, you change the naming of kernel events, which is an unrelated breaking change drop the distinction between simulation and elaboration.

My previous suggestion to return some non-unique string on demand is the minimal change required to make the implementation standards compliant again without any of the negative side effects in your proposal.

Why did you change the tests? Explicit hierarchically named events are mandated by the standard and were supported before.

[atalii]

The latest commit counteracts the idea of keeping event generation during simulation lightweight to reduce the performance impact. Especially using sc_gen_unique_name is pretty costly. Secondly, you change the naming of kernel events, which is an unrelated breaking change drop the distinction between simulation and elaboration.

Understandable - I didn't realize this was an important path for perf, nor that kernel event names were part of the public API.

Why did you change the tests? Explicit hierarchically named events are mandated by the standard and were supported before.

I just misremembered and thought that these would also be implementation-defined when I saw the failing tests. Sorry about that.

What would you think of the following?

@@ -224,7 +224,10 @@ sc_event::register_event( const char* leaf_name, bool is_kernel_event /* = false

     if( !leaf_name || !leaf_name[0] )
     {
-        if ( sc_is_running( m_simc ) ) return;
+        if ( sc_is_running( m_simc ) ) {
+           m_name = "$$unnamed$$";
+           return;
+       }
         leaf_name = sc_gen_unique_name
             ( is_kernel_event ? SC_KERNEL_EVENT_PREFIX : "event" );
     }

I would prefer that m_name have the same meaning regardless of whether the event is hierarchical or not, but there may certainly be side effects to this change that I don't anticipate, as with my previous patch.

Thanks for pointing these things out - I'm obviously not very familiar with all the stuff going on here, and I appreciate your guidance on what works and what doesn't :).

[aut0]

@atalii, could you please rebase on main?