----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1317/#review3117 -----------------------------------------------------------
Hi Nilay, This is great, thanks. Do you know what argument we're getting that's undefined? If you don't have time to make or test any bigger changes, I'd say go ahead and commit this to take care of the outstanding bug. However, I think there are a couple of useful changes that could be made at some point (either now if you have the time, or later): 1. Reverse the default behavior so that the function ends with "return false" but has an explicit "return true" after every point where result gets set. That's probably unnecessary now, but it inherently avoids the current problem of missing a code path where result does not get set as opposed to just putting a band-aid over it. 2. Do a warn() whenever we ignore a cpuid instruction. This is probably useful information for when people are running a program and it doesn't behave like they expect (and they may not even realize it's not behaving like they expect without the warning). There may well be other CPUID functions that we should be implementing but aren't only because the code is robust enough to work regardless. Of course, this change is easier if you also do #1, since then you can just put a single warn() at the bottom just before the "return false". - Steve Reinhardt On July 21, 2012, 9:08 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1317/ > ----------------------------------------------------------- > > (Updated July 21, 2012, 9:08 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9123:bf3f6849bae8 > --------------------------- > X86 CPUID: Return false if unknown processor family > > > Diffs > ----- > > src/arch/x86/cpuid.cc 2f6f0631af48 > > Diff: http://reviews.gem5.org/r/1317/diff/ > > > Testing > ------- > > Ran the pc-simple-timing regression test for x86, would > result in a few stat changes. > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
