Review Board 2.0.15

ruby: get rid of RefCnt and Allocator stuff use base/refcnt.hh

Review Request #21 - Created June 2, 2010 and submitted

Nathan Binkert
ruby: get rid of RefCnt and Allocator stuff use base/refcnt.hh

This was somewhat tricky because the RefCnt API was somewhat odd.  The
biggest confusion was that the the RefCnt object's constructor that
took a TYPE& cloned the object.  I created an explicit virtual clone()
function for things that took advantage of this version of the
constructor.  I was conservative and used clone() when I was in doubt
of whether or not it was necessary.  I still think that there are
probably too many instances of clone(), but hopefully not too many.

I converted several instances of const MsgPtr & to a simple MsgPtr.
If the function wants to avoid the overhead of creating another
reference, then it should just use a regular pointer instead of a ref
counting ptr.

There were a couple of instances where refcounted objects were created
on the stack.  This seems pretty dangerous since if you ever
accidentally make a reference to that object with a ref counting
pointer, bad things are bound to happen.
All regressions pass
Posted (June 6, 2010, 5:34 a.m.)
The changes seem 

I have two observations

1. In several places we are using dynamic_cast. The patch under consideration moves some of these to safe_cast and retains dynamic_cast at other places. Should we not use safe_cast in all places? This would avoid the checks that dynamic_cast carries out when not running the debug version.

2. In the file src/mem/ruby/system/, we use msg->getType/LineAddress/PhysicalAddress() = ;
Should we not call the set() function instead? Both we will have the same effect but set() is more readable.
  1. The first should be read as - "The changes seem fine to me."
Ship it!
Posted (June 6, 2010, 8:27 a.m.)
I haven't looked at this in great detail (and don't plan to), but I'm wondering: I don't know what the purpose of Allocator() was, but does it make sense to replace it with FastAlloc rather than just leaving it to the default allocator?  I see where this would be a little more complicated since it involves tracking down the classes that used Allocator and changing their inheritance, but I figured I'd throw it out there.  I'm fine with doing this in a separate patch if it's a good idea.
Ship it!
Posted (June 10, 2010, 8:17 a.m.)

Ship it!
Posted (June 21, 2010, 8:57 a.m.)
Thanks Nate.  This patch catches a few potential bugs and removes the complexity of having two separate allocators in GEM5.