> On Oct. 17, 2016, 3:25 p.m., Jason Lowe-Power wrote: > > Seems reasonable to me, but do we have any historical context for > > "LiveProcess"? > > > > Couple of small things below. > > Brandon Potter wrote: > I do not know about the historical context for LiveProcess. This is > speculation on my part, but I expect that the class just held new features > that were added that weren't needed for EIO. In the recent past (not sure > exactly when), Steve removed the EIOProcess. EIOProcess was supposed to allow > reading traces from SimpleScalar to help with bootstrapping SE mode for > Alpha. (At least, that's what I understood from a short converation that I > had with Steve.) As I mentioned in the changeset log, EIOProcess was the only > other class which derived from Process. So, I think that Process held > commonalities between EIO and Live, but it doesn't look like it was > maintained well. It seems like things were added without much thought for the > class organization.
I can confirm that the original (and only) reason for the split between Process and LiveProcess was for the former to factor out the things that were common between LiveProcess and EIOProcess. Since EIOProcess was rarely used and not part of the main code base, the distinction between Process and LiveProcess was not rigorously maintained as the two classes evolved. Now that EIOProcess is gone, there's no need to have two classes, and I think Brandon's patch is a useful simplification. > On Oct. 17, 2016, 3: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. 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. - Steve ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3671/#review8885 ----------------------------------------------------------- On Oct. 17, 2016, 8:17 a.m., Brandon Potter wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3671/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2016, 8:17 a.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11693:05fc28ce62a5 > --------------------------- > 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 > ----- > > src/arch/sparc/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/faults.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/power/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/mips/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/freebsd/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/arm/freebsd/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/tru64/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/tru64/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/alpha/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/splash2/run.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/splash2/cluster.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/learning_gem5/part1/two_level.py > 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/learning_gem5/part1/simple.py > 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/example/se.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/example/apu_se.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > configs/common/cpu2000.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/linux/syscalls.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_emul.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/syscall_desc.cc PRE-CREATION > src/sim/syscall_desc.hh PRE-CREATION > src/sim/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/emul_driver.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/sim/Process.py 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/tru64/tru64.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/operatingsystem.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/operatingsystem.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/linux/linux.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/linux/linux.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/kern/freebsd/freebsd.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/gpu-compute/cl_driver.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/gpu-compute/cl_driver.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/linux/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/x86/linux/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/solaris/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/solaris/process.hh 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/process.cc 4a86763c0b30cccba0f56c7f48637a46a4663b06 > src/arch/sparc/process.hh 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
