tymonx

I have added UVM error/warning messages for all .unpack_*(), .is_null() and .un/pack_object() methods.

Thanks @jrefice. Now I see that the .flush() method is called in the packer new() constructor. It was located at the bottom of the uvm_packer.svh header file (line 1188). I have debugged correctly my issue with invalid m_*_iter values. I see that changes were met in another thread because of a singleton default packer nature. In the standard UVM implementation, creating a new packer object each time when packing or unpacking methods are called, it solves the problem In my implementation, the .is_null() method is not used because I never encode object handler. It is based on the user code flow like in the proto buffer framework. Not from encoded bitstream information. That why it always return false. I can add an UVM error/warning message in the .is_null() method and in un/pack_object() methods when the provided value parameter equal to null I don't see why m_packed value will be in bad state? .pack_*() methods are working like vector append methods (moving pointer) and .set_packed_*() methods like vector assignment methods (setting pointer) Calling .unpack_*() methods without calling .set_packed*() methods are safe but it will return zero value, nothing or empty string. It depends on unpacking method. But here I can add an UVM error/warning message to inform user about that But these methods are called correctly in UVM object class. The behavior is the same like in the standard UVM implementation. More like in UVM 1.2 and previous implementation

I have another minor issue but during extending UVM. I thing it will be better for everyone to split interface from implementation. Because currently I have inherited everything, including some unused private variable members. For example uvm_packer class as an empty class and a separate uvm_packer_impl class with implementation details. It will make inheritance more cleaner.

Sure. I will share: https://gitlab.com/tymonx/uvm-extensions Benefits: it does one simple thing but very good. It packs and unpacks data without any additional bits very good for any kind of protocols (sequence items) like networking, encap/decap or streaming interfaces like AXI4-Stream or Avalon-ST new packer class inherited from the UVM packer class and extended. The default UVM packer is replaced with UVM factory and uvm_packer::set_default() method new coreservice class inherited from the default UVM coreservice class and extended. The default UVM coreservice is replaced with uvm_coreservice_t::set() method no bits size limitation for packing data, unlimited using foreach, fixed slice of the array (+:CONSTANT), stream operators ({>>{...}}) and forward ahead memory allocation for the best performance reducing number of required allocation with the forward ahead memory allocation technique removed unnecessary magic 8 bytes with magic values at the beginning of a packed bit stream that stores internal packer state removed unnecessary magic bits during packing UVM objects proper packer initialization in constructor. Default UVM packer was not initialized correctly (m_*_iter not set to 64 value but to 0) adding the .clone() call for .get_default_*() methods in coreservice default packer object in packing and unpacking methods are now thread safe default packer object in packing and unpacking methods don't have any side effects (not shared state as a singleton) code simplicity Usage: /* Place it somewhere in your custom UVM component class like test or env in * build_phase() method or new() constructor */ uvm_extensions_pkg::setup(); It reduces the code complexity of previous example to simple: byte bytes[]; protocol_c.pack_bytes(bytes); Implementation: function void protocol_c::do_pack(uvm_packer packer); uvm_object protocols[] = { protocol_a, protocol_b }; // Packing protocol_c header packer.pack_field_int(...); // Encapsulation foreach (protocols[i]) begin packer.pack_object(protocols[i]); end endfunction

I will show a real life scenario, not some academic use case that you can find in many UVM cookbooks or tutorial for dummies (like Candy Lovers) which are detached from reality. Lets assume that we have 2 two different protocols and we want to create a new sequence item that will represent a new protocol with header + encapsulate these two protocols inside a payload. Simple usage: byte bytes[]; protocol_c.pack_bytes(bytes); First try: function void protocol_c::do_pack(uvm_packer packer); uvm_object protocols[] = { protocol_a, protocol_b }; // Flush is required, because default UVM packer doesn't initialize internal fields properly! packer.flush(); // Packing protocol_c header packer.pack_field_int(...); // Encapsulation foreach (protocols[i]) begin packer.pack_object(protocols[i]); end endfunction Of course is not working. The pack_object() method add additional bits. Next approach that should have similar effect but is written in different way. It only show how UVM packer methods are not symmetric. And this is an anti pattern! Next try: function void protocol_c::do_pack(uvm_packer packer); uvm_object protocols[] = { protocol_a, protocol_b }; byte unsigned bytes[]; // Flush is required, because default UVM packer doesn't initialize internal fields properly! packer.flush(); // Packing protocol_c header packer.pack_field_int(...); // Encapsulation foreach (protocols[i]) begin byte unsigned tmp[]; protocols[i].pack_bytes(tmp); bytes = {bytes, tmp}; end packer.pack_bytes(bytes); endfunction Works? Of course not! Because UVM object pack/unpack methods use a single packer instance and because of that we have a side affects. It will duplicate data from previous packed protocol! Because protocols are using the same global instance of the UVM packer. Shared state. Next anti pattern (not only this is not thread safe). We must fix it: function void protocol_c::do_pack(uvm_packer packer); uvm_object protocols[] = { protocol_a, protocol_b }; byte unsigned bytes[]; // Flush is required, because default UVM packer doesn't initialize internal fields properly! packer.flush(); // Packing protocol_c header packer.pack_field_int(...); // Encapsulation foreach (protocols[i]) begin byte unsigned tmp[]; protocols[i].pack_bytes(tmp, uvm_packer::type_id::create("packer")); bytes = {bytes, tmp}; end packer.pack_bytes(bytes); endfunction Now it works! We have added several extra lines of code to avoid some UVM packer and UVM object pack/unpack gotchas. Engineer has spent almost two working days to simple pack header fields into array of bytes in a presumption UVM way...

After that: I agree with @sas73. We need to open source UVM library using GitHub or even better GitLab with Issue Tracker, proper CI/CD setup and unit testing. Accellera should strongly consider that. After spending some time for internal UVM source code review... Now I have many doubts about quality and architecture design decisions.

Sadly, some of these cons are also present in previous UVM releases (like UVM 1.2 before IEEE standardization).

To summarize my topic: Default pack/unpack methods from UVM object don't pack only user data but also additional magic UVM packer state (8 bytes) at the beginning and extra bits in mid when using packer.pack_object() method The packing API is not self-explained and it is very confusing. API user is expecting simple packing/unpacking solution It is against the "Doing One Thing, Well" philosophy. It requires extra effort to simplify it (extend basic classes). Not opposite! The UVM object pack/unpack methods are not thread-safe on default. They are using a single global packer instance (singleton). Packer state is shared and not immutable Packed bits stream doesn't contain only pure user data bits. Packer is packing unnecessary extra bits during packer.pack_object() method call The internal packer state is not initialized correctly during packer creation (missing initialization for internal packer field members m_pack_iter and m_unpack_iter). Correct values are assigned during a separate packer .flush() method call Packer limitation to 4088 bytes (-8 extra bytes from 4096 bytes from packed packer state). This can be simple re-implement with performance byte m_bytes[] and extra allocation ahead UVM packer set/get_packed_* methods are not properly aligned with the IEEE specification. Typo in method argument types. It leads to compilation errors

Hi Justin, Thanks for your detailed answer. I'm very grateful to read that. But still... I'm not happy with the UVM packer implementation, behavior and pack/unpack methods from UVM object. In my modest opinion, the UVM packer and pack/unpack methods had enormous potential, but it was simply wasted by that. The state retrieval should be moved to separate method and/or encapsulate within class (maybe a separate proxy class). Not exposed to user world with user packed bits. This is against proper software architecture design. Class methods and object itself should do one thing but very good. Like in an UNIX philosophy "Doing One Thing, Well". I don't see any value in that. It makes only using UVM API very confusing and not natural. Self-explained API is a great and valuable thing. Packer or pack/unpack methods should only pack data, not add extra content. Now I have ended with a custom UVM object (pack/unpack methods, not only do_pack/unpack) and UVM packer (re-implemented, without 4096 limitation) inheritance from original UVM classes to make them more natural for others. For basic stuff like packing/unpacking data. Simple scenario. I have planned to use default pack/unpack methods to store header data with different bit width and a separate byte payload[] for layered encapsulation and decapsulation. This lead me to another issue with the UVM packer behavior. Packing extra bits when someone is using packer.pack_object(). I have read an internal comment/reason about that in UVM sources. And this have no sense at all! For serializing/deserializing data you have two options. Pack/unpack data continuously without any extra information and let code decide to properly pack/unpack these bits to fields. Like protocol buffers framework. Or encode EVERY fields with extra bits information like BSON or something similar. Not mix two approaches in one. If someone is interested in more friendly UVM object pack/unpack methods and UVM packer implementation. I can open source that for everyone.

In my opinion, the internal m_pack() method from the UVM object should be changed to: if (packer == null) begin packer = uvm_packer::get_default().clone(); end To preserve the global configuration and be truly thread safe (separate state per method call).

Also this code is not thread safe. When packer is null, it gets the uvm_packer::get_default() that is a singleton instance... And call methods that changes internal packer states... Only solution is to create a packer object for every pack/unpack calls.

I have found another issue with the original UVM packer. It doesn't initialize internal m_pack_iter and m_unpack_iter iterators! They have undefined values. Only the .flush() method initialize them. How this even pass a code review? Do you have any unit tests for that?

Hi Guys, Current implementation of the UVM packer store some additional contexts information at the beginning of the bitstream (m_bits). A copy of the m_pack_iter and m_unpack_iter members. Exactly 64 bits (8 bytes). During packing or unpacking (un/pack, un/pack_bits/bytes/ints/intlongs) using an UVM object these fields are exposed. It makes these operations useless from user perspective and experience. To pack bits (example from header into bytes of stream), user needs to allocate additional extra magic 8 bytes (reverse engineering) and store some magic values (2x64, again...) at the beginning. This is also true for unpack methods. It is a completely different behavior from old UVM 1.2 and UVM-SystemC (stream operators). And not very unintuitive. To fix this irrelevant implementation, user is forced to inheritance from the default UVM packer base class and override all get_packed* and set_packed* methods to discards this context (state) information from packed/unpacked data stream and use the uvm_packer::set_default() method alongside with the uvm_factory::get().set_type_override_by_type(). ill-considered decision alongside with the UVM packer 4096 limitation.

You must create all necessary SystemC signals, SystemC modules and make connection between them before you run any test in gtest. This require to create own gtest_main.cc implementation. Naturally in SystemC you must put everything in sc_main() function. For this, I would use registry design pattern. First create registry class (registry + factory + singleton). This class will be responsible for storing registered constructors using dynamic allocation with new and smart pointer in lambda expression (see factory::add class). Create all objects using factory::create() method before running all tests. Then you can get object using factory::get() method in you test execution. factory.hpp #ifndef FACTORY_HPP #define FACTORY_HPP #include <map> #include <string> #include <memory> #include <functional> class factory { public: static factory& get_instance(); template<typename T, typename ...Args> class add { public: add(Args&&... args); add(const std::string& name, Args&&... args); }; template<typename T> static T* get(const std::string& name = ""); void create(); void destroy(); private: using destructor = std::function<void(void*)>; using object = std::unique_ptr<void, destructor>; using constructor = std::function<object(void)>; factory(); factory(const factory& other) = delete; factory& operator=(const factory& other) = delete; void add_object(const std::string& name, constructor create); void* get_object(const std::string& name); std::map<std::string, constructor> m_constructors; std::map<std::string, object> m_objects; }; template<typename T, typename ...Args> factory::add<T, Args...>::add(Args&&... args) { add("", args...); } template<typename T, typename ...Args> factory::add<T, Args...>::add(const std::string& name, Args&&... args) { factory::get_instance().add_object(name, [args...] () -> object { return object{ new T(std::forward<Args>(args)...), [] (void* obj) { delete static_cast<T*>(obj); } }; } ); } template<typename T> auto factory::get(const std::string& name) -> T* { return static_cast<T*>(factory::get_instance().get_object(name)); } #endif /* FACTORY_HPP */ factory.cpp #include "factory.hpp" #include <stdexcept> auto factory::get_instance() -> factory& { static factory instance{}; return instance; } factory::factory() : m_constructors{}, m_objects{} { } void factory::create() { for (const auto& item : m_constructors) { m_objects[item.first] = item.second(); } } void factory::destroy() { m_objects.clear(); } void factory::add_object(const std::string& name, constructor create) { auto it = m_constructors.find(name); if (it == m_constructors.cend()) { m_constructors[name] = create; } else { throw std::runtime_error("factory::add(): " + name + " object already exist in factory"); } } auto factory::get_object(const std::string& name) -> void* { auto it = m_objects.find(name); if (it == m_objects.cend()) { throw std::runtime_error("factory::get(): " + name + " object doesn't exist in factory"); } return it->second.get(); } Create your own version of gtest_main.cc implementation. Call factory::create() method to create all SystemC signals and SystemC modules before running any tests RUN_ALL_TESTS(). Because factory class is a singleton design pattern, call factory::destroy() method after finishing all tests to destroy all created SystemC objects. main.cpp #include "factory.hpp" #include <systemc> #include <gtest/gtest.h> int sc_main(int argc, char* argv[]) { factory::get_instance().create(); testing::InitGoogleTest(&argc, argv); int status = RUN_ALL_TESTS(); factory::get_instance().destroy(); return status; } Then define dut class in your test than will create SystemC signals and SystemC modules. In constructor do connection between created SystemC signals and modules. Register defined dut class to registry object using global constructor like this factory::add<dut> g. After than you can get your dut object using simple factory::get<dut>() method. test.cpp #include "my_module.h" #include "factory.hpp" #include <gtest/gtest.h> #include <systemc> class dut { public: sc_core::sc_clock aclk{"aclk"}; sc_core::sc_signal<bool> areset_n{"areset_n"}; sc_core::sc_signal<bool> in{"in"}; sc_core::sc_signal<bool> out{"out"}; dut() { m_dut.aclk(aclk); m_dut.areset_n(areset_n); m_dut.in(in); m_dut.out(out); } private: my_module m_dut{"my_module"}; }; static factory::add<dut> g; TEST(my_module, simple) { auto test = factory::get<dut>(); test->areset_n = 0; test->in = 0; sc_start(3, SC_NS); test->areset_n = 1; test->in = 1; sc_start(3, SC_NS); EXPECT_TRUE(test->out.read()); } For more inspiration, you can check my logic library for SystemC verification: https://github.com/tymonx/logic