Sunday, March 15, 2015

Patching a Leaky Boat - Handling UVM Bugs

This week I stumbled on an issue with the UVM base class library (BCL). I was using the register layer to access some memories and some things just didn't add up. I've posted a description in the forums, so let's see what the higher ups say.

I need that functionality now, though. I can't wait for UVM 1.1e (which will never come out) or UVM 1.2a. I also wouldn't want to switch to UVM 1.2 yet, even if the issue were fixed there. This got me thinking what the best way to handle such a situation is.

A bit of background on the UVM standard: just the API document is standardized, not the BCL. The Accellera UVM library is only a proof of concept. A major plus for the EDA vendors in increased UVM adoption is that they can develop debug extensions for it, which should make us more productive. Such a feature needs infrastructure, though. I've seen two implementation models up to now. In the first case, the BCL is left unchanged and vendor extensions are added on top, via a separate package. In the second case, the BCL itself is modified to include vendor extensions. Both approaches are "legal" according to the UVM philosophy, because as long as the API stays the same and stuff behaves as described in the standard, they get the UVM seal of approval.

In the first case, fixing a BCL bug is easy. We can just take the BCL and patch it and we won't get any problems with the vendor extensions. In the second case, things aren't as straightforward. Here, the UVM package comes bundled with the simulator and is installed in some read-only location. Also, because each simulator version comes with a (potentially) different version of UVM, editing the code directly isn't feasible.

In my current project I'm using a vendor of the second persuasion. A key requirement I have is that I want to be able to easily switch between simulator versions. This means I need a non-intrusive fix. This is only possible if I can replace instances of a specific class with my own extended class. This is where the UVM factory comes in, but to be able to use it, the offending objects have to be created using the factory.

In our case, we want to replace all instances of uvm_reg_map with an extended class we'll call vgm_reg_map. Luckily, uvm_reg_map is registered with the factory and is instantiated using create(...). We'll do our fixes in a separate package, vgm_uvm_patches. We'll need to import this package and set a type override inside our verification environment:

import vgm_uvm_fixes::vgm_reg_map;

class some_tb_env extends uvm_env;
  function void build_phase(uvm_phase phase);
    patch();

    // Build env
    // ...
  endfunction


  function void patch();
    uvm_factory factory = uvm_factory::get();
    factory.set_type_override_by_type(uvm_reg_map::get_type(),
      vgm_reg_map::get_type());
  endfunction
endclass

How do we go about fixing uvm_reg_map? We'll need to create an extended class and register it with the factory:

class vgm_reg_map extends uvm_reg_map;
  `uvm_object_utils(vgm_reg_map)

  function new(string name="vgm_reg_map");
    super.new(name);
  endfunction
endclass

Because we care about the quality of our work, we're going to create unit tests that expose the issue. Ideally we'd also create unit tests for the existing behavior, to make sure that we don't break anything else. Since this is going to be a small fix, we won't do it because it's not really worth it. Here's a test that fails due to this bug:

`SVTEST(get_physical_addresses__max_offset__returns_end_addr)
  uvm_reg_addr_t addrs[];
  map.get_physical_addresses(32'h0, 32'hffff, 4, addrs);

  `FAIL_IF(addrs.size() != 1)
  `FAIL_IF(addrs[0] != 32'hffff)
`SVTEST_END

When trying to get the physical address of offset 0xffff, we get 0x3_fffc, causing the test to fail. The only thing we can do in this case is to copy the code for the offending function from uvm_reg_map and paste it into our extension. When trying to compile, we'll get some errors that the method tries to use local fields. The first one occurs at the following line:

int multiplier = m_byte_addressing ? bus_width : 1;

Since m_byte_addressing is declared as local, we can't use it in the extended class. The only way to get it's value is by using the get_addr_unit_bytes(...) function:

function int unsigned uvm_reg_map::get_addr_unit_bytes();
  return (m_byte_addressing) ? 1 : m_n_bytes;
endfunction

What this function returns is suspiciously similar to what the original developer tried to assign to multiplier. Assigning it the return value of the function and fixing the other references to local fields will make our unit test pass.

This method of fixing the issue seems kind of clunky. I'm not very comfortable copy/pasting so much code, but we had to do this because the offending method was so big and poorly encapsulated. It could have been split into sub-methods, which would have made it easier to test and change. We'll talk more about this in a future post after I finish reading Refactoring: Improving the Design of Existing Code.

The fix we made is not-intrusive to the UVM package, but it's intrusive to our own testbench code. We need to compile the package in our run script, but we also have to add the necessary factory override to our environment class. If there were to be an official fix in the future, we'd need to go back and remove them from our code.

I can't resist making a comment as to how aspect oriented programming would have been so much better here. In e we'd just create a file, implement our patch there and import it after importing UVM. All instances of the affected class would get the fix. Since there wouldn't be any changes in our code, such a fix would be truly non-intrusive.

You can find the code for the package on SourceForge. This can be a good starting point for developing a similar package for your company/department/team. If there's interest from the public, I can create an own repository for this package and add to it. Let me know in the comment section.

7 comments:

  1. But, but, but... If there is any existing code that extends the UVM base class that you've patched and factory-overridden, you're opening the gates to a world of hurt with class type incompatibility thanks to your branched class hierarchy. This is the same problem that afflicts any attempt to make site-specific extensions of the normal UVM component or object base classes.

    Of course you're correct to point out that AOP could solve this. But the real blame lies not with lack of AOP, but with your vendor who has allowed a flawed base class to infiltrate their non-patchable BCL distribution.

    I don't recall who it was that first said "prefer composition over inheritance", but they were right.

    ReplyDelete
    Replies
    1. I didn't really think about that (I was too focused on uvm_reg_map and, seriously, who's going to extend that?). In that case, you'd need to extend from the patch class instead of the original class => more intrusiveness.

      The Gang of Four said "prefer composition over inheritance" in their "Design Patterns" book.

      Delete
  2. When the buggy code in the BCL is sealed up in class that hasn't been registered with the factory, with no virtual methods and/or the class properties (variables) you need to reach are local, you can download the reference library then fix it yourself. Then you bypass the simulator's pre-compiled versions of the library and compile your own version instead. I always do this anyway because I have to pass in some defines from the command-line that alters the default behavior of the BCL. The pre-compiled binaries are useless to me.

    Of course, you will lose your fixes with the next release of UVM, but 1.2 is solid and should be around for quite a while. Back when I did this for OVM when it was changing from time to time, I just had to run a diff and patch the new release myself.

    ReplyDelete
  3. hi,
    this is a hot topic. unfortunately the BCL is so monolithic and has too much non-virtuals, visible member fields, static by-type access etc so that non-intrusive changes are almost impossible. Doing more GOF would make sense but likely require a rewrite of UVM and more SW/OO patterns would leave those behind who are already asking for an "easier uvm".

    ReplyDelete
    Replies
    1. I'd be in favor of a gradual rewrite (and also a split, but more on that in some future post). UVM is anyway supposed to be a library, so how it does its stuff should be transparent to the users. It can use as many patterns in the background as it likes.

      Delete
  4. I just wanted to comment to congratulate you on reading Refactoring. Awesome book. I wish more verification engineers would read it.

    ReplyDelete
    Replies
    1. I didn't read it yet. It's in the works. I plan to do a post on it.

      Delete