> On Oct. 17, 2016, 10:25 p.m., Jason Lowe-Power wrote: > > src/sim/process.cc, line 593 > > <http://reviews.gem5.org/r/3671/diff/1/?file=61630#file61630line593> > > > > Any reason not to move this code into ProcessParams::create(), and > > eliminate this static function? > > Brandon Potter wrote: > Jason, I'm not sure what you want me to do here. > > I put some gdb breaks on the create function to see how this is all > invoked. I get a backtrace where _wrap_ProcessParams::create > (build/X86/python/m5/internal/param_Process_wrap.cc\:5201) calls the static > function. It looks like Python is wrapped around the C++ create function and > is calling it. > > The C++ method is static because the Process object (which ends up being > a derived type object who's type depends on the ISA) has not been created > yet; you can't call methods on an object that aren't statically declared. So, > the static function in the base class acts as a generator for the derived > Process class. Also, the ProcessParams must have already been initialized > because they're used within the Process::create function (so any new code > that was added would have to be put into the end of ProcessParams > initialization to make these variables available). > > Regarding moving it into the Python code for params, it may be possible, > but I'm not sure how to do it. The macro "THE_ISA" is used to help figure out > what type of derived Process class to return and that's obvious in the code > how it's done in C++. I don't know of any examples for Python or how to go > about doing it. > > Is there a particular reason that you want to remove the static function. > Have you run into problems with similar code in the past? > > Jason Lowe-Power wrote: > I certainly could be missing something here, but it seems like you could > take all of the code in `Process::create(ProcessParams * params)` and put it > in the body of `ProcessParams::create()`. You would need to replace the > `params` variable with `this`, but other than that, I don't see any changes. > Am I missing something? > > It wasn't this that before, which I assumed was because LiveProcess was > one of multiple types of Process classes, but there could have been a good > reason. I haven't run into any problems with this code before. I was just > suggesting a little more housekeeping while you were here. > > Brandon Potter wrote: > Really, I'd rather not alter how this is currently set up. Have a hard > time seeing how I'd implement this and I don't believe that this will be a > simple copy-paste and make alterations job. It will probably involve > sleuthing around the Python code while straddling the SWIG boundary. I prefer > to stay away from that __black magic__ when possible. > > Regarding housekeeping, I don't know why it's better to move this from > C++ into Python. The same code needs to exist in one form or another in > either C++ or Python. > > If you feel strongly about it, I'd be happy to review an implementation > of it. However, I have no desire to actually go and do it myself. > > Jason Lowe-Power wrote: > This isn't a big deal, sorry to have belabored it. No worries if you > don't want to do it. I really just meant to move the code from lines 578 to > 720 and replace line 726. But don't worry about it! It's orthogonal to this > patch. > > Steve Reinhardt wrote: > Brandon, why do you think this would be difficult? I agree with Jason > that it should just be a matter of replacing > Process::create(ProcessParams * params) > with > ProcessParams::create() > and replacing 'params' with 'this' (actually replacing 'params->' with > ''), then getting rid of the current ProcessParams::create() definition. All > we're talking about is collapsing a trivial function call on the C++ side, > not touching anything at all in Python.
I misunderstood what was being asked; apologies for being dense. Updated the code as requested. - Brandon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3671/#review8885 ----------------------------------------------------------- On Oct. 19, 2016, 7:27 p.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3671/ > ----------------------------------------------------------- > > (Updated Oct. 19, 2016, 7:27 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11694:c915cc7cff42 > --------------------------- > syscall_emul: [patch 5/22] remove LiveProcess class and use Process instead > > The EIOProcess class was removed recently and it was the only other class > which derived from Process. Since every Process invocation is also a > LiveProcess invocation, it makes sense to simplify the who organization > by combining the fields from LiveProcess into Process. > > > Diffs > ----- > > configs/common/cpu2000.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/example/apu_se.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/example/se.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/learning_gem5/part1/simple.py > 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/learning_gem5/part1/two_level.py > 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/splash2/cluster.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/splash2/run.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/tru64/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/tru64/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/freebsd/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/freebsd/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/faults.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/linux/syscalls.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/solaris/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/solaris/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/gpu-compute/cl_driver.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/gpu-compute/cl_driver.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/freebsd/freebsd.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/linux/linux.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/linux/linux.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/operatingsystem.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/operatingsystem.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/tru64/tru64.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/Process.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/emul_driver.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_desc.hh PRE-CREATION > src/sim/syscall_desc.cc PRE-CREATION > src/sim/syscall_emul.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > > Diff: http://reviews.gem5.org/r/3671/diff/ > > > Testing > ------- > > > Thanks, > > Brandon Potter > > _______________________________________________ gem5-dev mailing list gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
