arch, x86: Delete packet in IntDevice::recvResponse

Information
Submitter:	Joel Hestness
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11145:b92a7a451890
---------------------------
arch, x86: Delete packet in IntDevice::recvResponse

IntDevice::recvResponse is called from two places in current mainline: (1) the
short circuit path of X86ISA::IntDevice::IntMasterPort::sendMessage for atomic
mode, and (2) the full request->response path to and from the x86 interrupts
device (finally called from MessageMasterPort::recvTimingResp). In the former
case, the packet was deleted correctly, but in the latter case, the packet and
request leak. To fix the leak, move request and packet deletion into IntDevice
inherited class implementations of recvResponse.

Testing Done

Compiled gem5.debug with --without-tcmalloc. Ran x86 FS tests with Valgrind.

Issue Summary

Description	From	Last Updated	Status
Seems dangerous to do this in a virtual method that looks like the whole point is to override it? Who ...	Andreas Hansson	Sept. 27, 2015, 12:52 p.m.	Resolved
override	Andreas Hansson	Sept. 29, 2015, 7:11 a.m.	Resolved
This is fine, but what about just making it purely virtual?	Andreas Hansson	Sept. 28, 2015, 12:53 p.m.	Dropped

Seems fine to me.

src/dev/x86/intdev.hh (Diff revision 1)

Seems dangerous to do this in a virtual method that looks like the whole point is to override it?

Who is actually allocating the request and packet?

Show all issues

Joel Hestness Sept. 22, 2015, 6:57 a.m. (Sept. 22, 2015, 6:57 a.m.)

The one that triggered the leak is X86ISA::I82094AA::signalInterrupt(). I8259 looks like the only other device that descends from IntDevice, but it uses signal lines rather than sending PIO packets to communicate. Neither overload recvMessage. I could overload recvMessage in I82094AA and delete packets there. Would probably also make sense to change the virtual IntDevice::recvMessage to be a panic to force overloading. Thoughts?

Andreas Hansson Sept. 22, 2015, 7:04 a.m. (Sept. 22, 2015, 7:04 a.m.)

I think that's a better option. That way the allocation and deletion are in the same place.

Description:

~		Changeset 11094:bdbe55d7d4e0
	~	Changeset 11145:b92a7a451890

		arch, x86: Delete packet in IntDevice::recvResponse

		IntDevice::recvResponse is called from two places in current mainline: (1) the
		short circuit path of X86ISA::IntDevice::IntMasterPort::sendMessage for atomic
		mode, and (2) the full request->response path to and from the x86 interrupts
		device (finally called from MessageMasterPort::recvTimingResp). In the former
		case, the packet was deleted correctly, but in the latter case, the packet and
~		request leak. To fix the latter leak, move request and packet deletion into
~		IntDevice::recvResponse().
	~	request leak. To fix the leak, move request and packet deletion into IntDevice
	~	inherited class implementations of recvResponse.

Diff:

Revision 2 (+12 -2)

Show changes

	src/dev/x86/i82094aa.hh
	src/dev/x86/i82094aa.cc
	src/dev/x86/intdev.hh
	src/dev/x86/intdev.cc

src/dev/x86/i82094aa.hh (Diff revision 2)

override

Show all issues

Joel Hestness Sept. 28, 2015, 1:02 p.m. (Sept. 28, 2015, 1:02 p.m.)

Can you clarify? This declaration overrides the virtual declaration in IntDevice, and I don't expect anyone will want to inherit from I82094AA. Am I missing something?

Andreas Hansson Sept. 29, 2015, 1:13 a.m. (Sept. 29, 2015, 1:13 a.m.)

If it is overriding the virtual method in the base class we should add the keyword "override" (or M5_ATTR_OVERRIDE, although the latter is no longer needed).
From clang 3.5 the compiler even enforces it when using -Wall.

Joel Hestness Sept. 29, 2015, 7:13 a.m. (Sept. 29, 2015, 7:13 a.m.)

Ok, for consistency, I've added M5_ATTR_OVERRIDE, since there are no uses of 'override'.

Would you mind adding some notes about this to the wiki? Currently, it's not clear from existing code how this needs to be used. There are no uses of 'override', and use of 'M5_ATTR_OVERRIDE' is pretty spotty (only consistent on drain, checkpointing).

Andreas Hansson Sept. 29, 2015, 7:32 a.m. (Sept. 29, 2015, 7:32 a.m.)

Thanks. We'll sort out the consistency in an upcoming patch. The version of clang that ships with XCode is not happy without it.

Joel Hestness Sept. 29, 2015, 7:38 a.m. (Sept. 29, 2015, 7:38 a.m.)
```
Cool. Is this patch alright to ship?
```

src/dev/x86/intdev.hh (Diff revision 2)

This is fine, but what about just making it purely virtual?

Show all issues

Joel Hestness Sept. 28, 2015, 1:02 p.m. (Sept. 28, 2015, 1:02 p.m.)

As proposed in the latest patch, this allows us to inherit in the I8259 device without needing to explicitly override it there. Sure, pure virtual would require overriding the function before a new device will compile rather than hitting the panic during simulation. However, given recvResponse() is currently only used in I82094AA, I don't yet see a need for it to be explicitly defined in other devices.

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Description:

Changeset 11094:bdbe55d7d4e0

Changeset 11145:b92a7a451890

Diff:

Status: Closed (submitted)