----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3696/#review9076 -----------------------------------------------------------
In general, I think this patch could be improved by making better use of copy constructors and overloaded assignment operators. I can review again in more detail once that has been addressed. src/arch/x86/process.cc (line 111) <http://reviews.gem5.org/r/3696/#comment7814> Implementing assigment operator would be nice. src/arch/x86/process.cc (line 1100) <http://reviews.gem5.org/r/3696/#comment7812> Same here. src/arch/x86/process.cc (line 1138) <http://reviews.gem5.org/r/3696/#comment7813> Same here. src/cpu/thread_state.hh (line 113) <http://reviews.gem5.org/r/3696/#comment7830> What is this proxy stuff? Some comments would help. src/mem/page_table.cc (line 107) <http://reviews.gem5.org/r/3696/#comment7816> Name is a bit weird IMO. getMappings make it seem like this is a getter, which it isn't. Maybe something like appendMappings? Also should be const. src/sim/Process.py (line 45) <http://reviews.gem5.org/r/3696/#comment7817> Why change this? src/sim/process.hh (line 190) <http://reviews.gem5.org/r/3696/#comment7825> Seems like this should be an assert() instead of a silent return. src/sim/process.hh (line 264) <http://reviews.gem5.org/r/3696/#comment7818> I prefer shared_pointer here. Also could use some comments explaining what these fields are and that they can be shared accross different processes. src/sim/process.cc (line 161) <http://reviews.gem5.org/r/3696/#comment7819> Unnecessary. src/sim/process.cc (line 166) <http://reviews.gem5.org/r/3696/#comment7821> Looking at how you use this, maybe the right thing to do is to just make getMappings an actual getter: MapVec mappings = pTable->getMappings(); src/sim/process.cc (line 212) <http://reviews.gem5.org/r/3696/#comment7823> np->argv.insert(np->argv.end(), argv.begin(), argv.end()); Seems cleaner. Same for envp. src/sim/process.cc (line 259) <http://reviews.gem5.org/r/3696/#comment7831> Should we panic here? src/sim/process.cc (line 338) <http://reviews.gem5.org/r/3696/#comment7832> What about the other variables you introduce, such as exitGroup, sigchld, maxStackSize, etc? Does checkpointing even work? If not, there should be a warning stating what is not supported. src/sim/syscall_desc.hh (line 110) <http://reviews.gem5.org/r/3696/#comment7824> const src/sim/syscall_emul.hh (line 1183) <http://reviews.gem5.org/r/3696/#comment7826> Seems like there should be a cleaner way to do this with a copy constructor. src/sim/syscall_emul.hh (line 1689) <http://reviews.gem5.org/r/3696/#comment7829> In general, there is too much duplication between this function and cloneFunc. For example, some the ProcessParams stuff seems to be copied. This should be factored out a bit better. - Michael LeBeane On Nov. 14, 2016, 8:37 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3696/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2016, 8:37 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11722:6fea3eedd891 > --------------------------- > syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess > simulations > > Modifies the clone system call and adds execve system call. Requires allowing > processes to steal thread contexts from other processes in the same system > object and the ability to detach pieces of process state (such as MemState) > to allow dynamic sharing. > > > Diffs > ----- > > src/sim/syscall_emul.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/sim/syscall_desc.hh PRE-CREATION > src/sim/syscall_emul.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/sim/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/sim/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/mem/page_table.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/mem/se_translating_port_proxy.hh > c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/sim/Process.py c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/cpu/thread_state.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/mem/page_table.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/cpu/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/cpu/o3/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/cpu/simple_thread.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/types.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/cpu/checker/thread_context.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/linux/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/x86/linux/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/sparc/process.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/sparc/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/sparc/linux/syscalls.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/mips/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/power/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/arm/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/generic/types.hh c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > src/arch/arm/linux/process.cc c38fcdaa5fe508dbb18cc084e758ad0ce8e2e2f4 > > Diff: http://reviews.gem5.org/r/3696/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
