ruby: Fix double statistic registration in garnet

Information
Submitter:	Matthew Poremba
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11549:983f44e21307
---------------------------
ruby: Fix double statistic registration in garnet

Currently garnet will not run due to double statistic registration of new
stats in ClockedObject. This occurs because a temporary array named 'cls'
is being added as a child to garnet internal and external link SimObjects.
This patch simply renames the temporary array which prevents it from
being added as a child object and avoids the assertion that a statistic
was already registered.

Testing Done

Garnet runs with patch applied.

Hi all. Garnet has been broken for three weeks now, so it would be nice to push this simple change as soon as possible.

I don't understand why adding an 'r' to the variable name fixed this issue.

I think the normal way to deal with local variables that you don't want added as a child to the SimObject is to prepend an underscore to their name (e.g., _cls).

On a side note, we really need better test coverage if errors like this keep getting checked in!

Matthew Poremba June 29, 2016, 11:35 a.m. (June 29, 2016, 11:35 a.m.)

I'm not exactly sure either. My best guess is that 'cls' is (for some reason) being treated specially by swig and/or python, since this is typically used similarly to 'self' but with classes. Even more confusing, the local array 'nls' directly above this code is not being added as a child, but the only major difference is the variable name. Prepending an underscore also does the job if you think this would be a better name to use.

Jason Lowe-Power June 29, 2016, 11:37 a.m. (June 29, 2016, 11:37 a.m.)

I bet it has something to do with cls often used as an abbreviation for class in Python since class is a reserved keyword. I would much prefer to see it with an underscore as it makes it clear that this is a temporary local variable, not a class member.

Andreas Hansson June 30, 2016, 12:19 a.m. (June 30, 2016, 12:19 a.m.)
```
Is there no regression that tests Garnet?
```

Matthew Poremba June 30, 2016, 11:39 a.m. (June 30, 2016, 11:39 a.m.)

Seems there is not. I'm not too familiar with the tester, but I can volunteer to dig in and add a test for garnet.

Change Summary:

Change renamed variable to _cls. Also rename nearby nls -> _nls for consistency in this file.

Description:

~		Changeset 11549:9b80f63dc6b5
	~	Changeset 11549:983f44e21307

		ruby: Fix double statistic registration in garnet

		Currently garnet will not run due to double statistic registration of new
		stats in ClockedObject. This occurs because a temporary array named 'cls'
		is being added as a child to garnet internal and external link SimObjects.
		This patch simply renames the temporary array which prevents it from
		being added as a child object and avoids the assertion that a statistic
		was already registered.

Diff:

Revision 2 (+18 -16)

Show changes

src/mem/ruby/network/garnet/fixed-pipeline/GarnetLink_d.py

Ship It!

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Change Summary:

Description:

Changeset 11549:9b80f63dc6b5

Changeset 11549:983f44e21307

Diff:

Status: Closed (submitted)