syscall_emul: file descriptor interface changes

Information
Submitter:	Brandon Potter
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10917:eafe9bd908a7
---------------------------
syscall_emul: file descriptor interface changes

This patch gets rid of unused Process::dup_fd method and does minor
refactoring in the process class files.  The file descriptor max has been
changed to be the number of file descriptors since this clarifies the loop
boundary condition and cleans up the code a bit.  The fd_map field has been
altered to be dynamically allocated as opposed to being an array; the
intention here is to build on this is subsequent patches to allow processes
to share their file descriptors with the clone system call.

Testing Done

Issue Summary

Description	From	Last Updated	Status
I think this will break existing SE mode checkpoints.	Nilay Vaish	July 22, 2015, 3:05 p.m.	Resolved
I think you should say sim fd and not host fd.	Nilay Vaish	July 21, 2015, 1:56 p.m.	Resolved
How does this change help?	Nilay Vaish	July 22, 2015, 3:05 p.m.	Resolved

src/sim/process.hh (Diff revision 1)

I think this will break existing SE mode checkpoints.

Show all issues

Brandon Potter July 21, 2015, 1:59 p.m. (July 21, 2015, 1:59 p.m.)

Is the problem that it's referenced through a pointer now? If not, can you elaborate on why you think it might be a problem. (Sorry, don't have any experience with using checkpoints in gem5 and only starting to need to support the interface in code; we don't make heavy use of them internally.)

Nilay Vaish July 22, 2015, 2:59 p.m. (July 22, 2015, 2:59 p.m.)

I think we write all the fds whether they are valid or not to the checkpoint file.
Since the number is changing from 256 to 1024, I think earlier checkpoints may not load.
I think you should test with hello world application: take a checkpoint after 1000 instructions
and see if the checkpoint loads correctly and the application runs to completion.

Brandon Potter July 23, 2015, 1:16 p.m. (July 23, 2015, 1:16 p.m.)

Hey Nilay, thanks for this review in particular.

You're right in that it did cause a problem. (There should be a way to check whether checkpoints get trampled with the regression system; although maybe that's asking too much. I'll have to be more careful moving forward.)

Anyways, the changes were more applicable to the next review in the patch series: http://reviews.gem5.org/r/2967/.

I have tested that this code works by creating a checkpoint as you suggested and then running it through updates and then util/cpt_upgrader.py

src/sim/process.hh (Diff revision 1)

I think you should say sim fd and not host fd.

Show all issues

src/sim/process.cc (Diff revision 1)

How does this change help?

Show all issues

Brandon Potter July 21, 2015, 1:59 p.m. (July 21, 2015, 1:59 p.m.)

Rather than warn, I think it's better to generate an error (or ignore it entirely). It's either a problem or it's not and I think having an invalid value for the file descriptor is a problem. It might be better to be strictly > rather than >= though, right?

Nilay Vaish July 22, 2015, 2:59 p.m. (July 22, 2015, 2:59 p.m.)

Is fd ever < -1?  If not, then this assert if of no use.  I think we should have assert(fd > -1).

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Status: Closed (submitted)