When you upload a new review, be sure to update the old one. That way you can see what changed between revisions.

src/cpu/base_dyn_inst.hh (Diff revision 1)

Like I said before, you shouldn't use this instResult mechanism. It's broken and obsolete and needs to be removed all together.

1. Geoffrey Blake Nov. 28, 2011, 2:23 a.m. (Nov. 28, 2011, 2:23 a.m.)

   The CheckerCPU and CPU model being checked are not required to always be in lock step. So the dyn_inst object has to save its results internally using this mechanism so the Checker can sync back up.
   
   The cases where this happens are detailed in a comment at src/cpu/checker/cpu_impl.hh:124, and it involves the interaction between the checker and the ld/st queue.

src/cpu/o3/thread_context.hh (Diff revision 1)

These shouldn't be squished all onto one line.

src/cpu/thread_context.hh (Diff revision 1)

I think all of these new functions could be replaced with a getCheckerCPU call on the base CPU class. Then you can get the TLB pointers from it and even factor some of the code that's duplicated for the checker into functions that take a CPU pointer.

1. Geoffrey Blake Nov. 28, 2011, 2:28 a.m. (Nov. 28, 2011, 2:28 a.m.)

   This keeps it rather clear what is getting accessed by these functions instead of adding another level of indirection, unless its desired to refactor parts of the thread_context code as a whole (getting rid of getDTBPtr for the regular CPUs as well).  The way the Checker, O3 and thread_contexts interact is fairly difficult to understand as it is currently, but I'm not sure how to fix this anyways.

src/mem/packet.hh (Diff revision 1)

I still don't see the point of this. The memory system should be taking care of this, and it shouldn't have anything to do with the checker.

The changes to the packet class fix a long standing limitation where finding a packet that partially satisfied a request would cause a panic. This code remedies that limitation, although it should be in it's own changeset. 

src/mem/packet.cc (Diff revision 1)

This should be a separate change and doesn't need to be protected by a checker ifdef. The code is useful in all cases. 

src/mem/packet.cc (Diff revision 1)

Same as before. The comment should be updated to note that the partial read satisfy issue no longer exists. 

src/mem/bus.cc (Diff revision 2)

Why? This seems odd to me. If it is a response then someone responded during the snooping. If the destination port is the same as the source we have a loop. Why should it not continue if it is the default port?

Is this not working around an issue located somewhere else?

1. Geoffrey Blake Nov. 28, 2011, 4:40 a.m. (Nov. 28, 2011, 4:40 a.m.)

   This workaround is due to the recursive nature of a functional access, and the presence of coherent I/O in FS mode.  If a functional access goes all the way out to the membus, it will first access the snooping devices hanging off the membus, which is the iocache. It has an active address range for the ARM realview test setups I'm using as 0x0-256MB.  If the iocache does not satisfy the request it will send the functional access onto the iobus and probably not find a device to satisfy the access thereby sending it to the default port which is effectively a null pointer, crashing the simulation when accessed.  The Checker setup does exercise the functional memory access path heavily though. Thinking about it now, I'm surprised this hasn't been hit before in normal operation.  
   
   Ali is probably better suited to offer an opinion on a better solution to this as he's pretty familiar with the I/O setup in M5.

2. Andreas Hansson Nov. 28, 2011, 7:05 p.m. (Nov. 28, 2011, 7:05 p.m.)

   Sorry, but I am still not seeing the bigger picture here. A CPU (in this case checker?) makes an access to an address that does not exist in the system? Why would that ever be right? The only reason it would end up at an unconnected default port is if the address is used incorrectly as far as I can tell. Am I missing something?
   

3. Geoffrey Blake Nov. 29, 2011, 1:21 a.m. (Nov. 29, 2011, 1:21 a.m.)

   The Checker makes functional accesses only into the memory system to see if it is seeing the same state as the O3.  These functional accesses visit every memory object that could potentially hold the value the access is looking for by doing a breadth first search of the memory hierarchy tree as defined by the busses and stops the search once the request is satisfied.  The fix in question is just telling the functional access to stop looking along a search path when no memory object attached to a bus can satisfy its request.  So it is not making an access to a non-existent address, but the order it searched the tree of memory objects had the request searching the io devices off the iobus first instead of searching the physmem object which would have satisfied the request in this particular case.  
   
   If an actual access, such as a timing access ends up in this state, it will cause the simulator to panic as it is an actual error.
   
   Does this clarify what this particular piece of code is doing?
   
   

Nice to see this getting resurrected... thanks, Geoff.

build_opts/ARM_wCHECKER_FS (Diff revision 2)

I'm not too excited about exploding the number of built-in configs further by adding these two files.  Just saying USE_CHECKER=1 on the scons command line once (assuming it's sticky) doesn't seem that hard to me.

1. Geoffrey Blake Dec. 5, 2011, 11:36 p.m. (Dec. 5, 2011, 11:36 p.m.)

   I'll check to make sure doing USE_CHECKER=1 on the build command line is sticky and remove the extra configs.  I was not aware of this functionality in scons.

src/cpu/DummyChecker.py (Diff revision 2)

What's this DummyChecker object for?

1. Geoffrey Blake Dec. 5, 2011, 11:39 p.m. (Dec. 5, 2011, 11:39 p.m.)

   In short, the DummyChecker allows checkpoints and fast forwarding to work by setting up the ports the real checker will need when it is switched in with the detailed model.

2. Steve Reinhardt Dec. 6, 2011, 12:48 a.m. (Dec. 6, 2011, 12:48 a.m.)

   Can you explain in a little more detail why this is needed?  What happens when you try and restore a checkpoint to a system with a checker when you don't have a DummyChecker in there to begin with?

3. Geoffrey Blake Dec. 6, 2011, 1:51 a.m. (Dec. 6, 2011, 1:51 a.m.)

   There needs to be two checker objects (a dummy and the O3Checker) for the ports to be constructed and connected properly. If the dummy is not there the python code errors out stating there are ports that do not exist.  On the call to takeOverFrom(), the dummy is then used to transfer its ports to the actual Checker.  
   
   I just checked if I could use the O3Checker object in place of the dummy and it does work (eliminating the extra files and code).  But I do not know how desirable it is for O3 cpu code to be present in the simple cpu code tree.

4. Steve Reinhardt Dec. 19, 2011, 11:16 p.m. (Dec. 19, 2011, 11:16 p.m.)

   This seems pretty hokey to me... I'd think the answer would be to fix the switchover process to make it more robust rather than to add this mechanism with the DummyChecker or wedge O3Checker into the SimpleCPU code.  In addition to the general undesirability of having this new object, I'd think that from a practical perspective you'd like to allow people to use the checker on checkpointed runs even when they didn't plan ahead and have a DummyChecker inserted into the original config.
   
   Is there a way we can create these ports on the fly when they don't exist?  How does this relate to Andreas's single system-wide ProxyPort change; would that make this problem go away?
   
   Sorry I forgot to send this comment sooner... I've been meaning to look into this myself but haven't had the time.

5. Andreas Hansson Dec. 19, 2011, 11:44 p.m. (Dec. 19, 2011, 11:44 p.m.)

   I believe the port proxy patch will indeed make this issue go away as the checker does not have ports on its own (it merely uses existing ports through proxies).

6. Geoffrey Blake Dec. 20, 2011, 7:40 a.m. (Dec. 20, 2011, 7:40 a.m.)

   If ports could be created on the fly, that would eliminate the need for this object.
   
   I guess I wasn't totally clear on the DummyChecker.  It does not need to be present when taking a checkpoint.  It only needs to be present when loading from a checkpoint so all the ports get created correctly at the start of simulation.
   
   The checker does have some ports of its own, like the ports to its own TLBs that shadow the CPU object being Checked.  But it does piggy back on the CPU's icache and dcache ports.  Not sure how that will all fit into the upcoming changes.

src/cpu/thread_context.hh (Diff revision 2)

I agree with Gabe here... you're always doing things like:

if (cpu->getCheckerCpuPtr()) {
  cpu->getCheckerITBPtr()->foo();
  cpu->getCheckerDTBPtr()->foo();
}

so why don't you just do:

CPU *checker = cpu->getCheckerCpuPtr();
if (checker) {
  checker->getITBPtr()->foo;
  checker->getDTBPtr()->foo;
}

which seems just as clear (if not clearer) and avoids the need for these extra methods.

1. Geoffrey Blake Dec. 5, 2011, 11:55 p.m. (Dec. 5, 2011, 11:55 p.m.)

   This wouldn't be too painful of a fix just for the Checker.  The checker is a special case.  
   
   But as to which is clearer, its a wash in my opinion as everywhere in the code base the ITB/DTB pointers are accessed via the ThreadContext object and not a Cpu object so the Checker will always stand out as being special.  

2. Steve Reinhardt Dec. 6, 2011, 12:49 a.m. (Dec. 6, 2011, 12:49 a.m.)

   I'm not clear on your comment... I used CPU in the example, but I think you can do exactly the same thing with ThreadContext, right?

src/mem/bus.cc (Diff revision 2)

I agree with Andreas here... from Geoff's description, I think the real bug he's facing is that the ARM config he's using doesn't have a default responder on the I/O bus.  There should be something out there that if nothing else sends an error response packet back to the requester.  The requester can then decide if it wants to panic and terminate the simulation or not.

If blindly accessing the default port when it's not set is causing us to segfault on a null pointer, then we should require that every bus has a default responder at config time.  We have at least one dummy/bad address device just for this purpose.

1. Geoffrey Blake Dec. 6, 2011, 12:15 a.m. (Dec. 6, 2011, 12:15 a.m.)

   The error is not that the request is bad. Its the order in which the memory objects are searched and the end conditions for a functional access.  Putting a default responder on the IOBus and having it respond to a functional access will just cause the bug to occur from a different place.  Would a better solution be putting a default responder that ignores functional accesses on the IOBus so they can pass through to the memory bus?  That way if the address is truly bad, the badaddr_responder on the membus will cause the simulation to panic and stop.

2. Steve Reinhardt Dec. 6, 2011, 1:08 a.m. (Dec. 6, 2011, 1:08 a.m.)

   OK, I think I see the problem... I think the issue is that the IOCache snoops on the membus, and can contain any address, so it always has to be probed; and caches by default forward functional requests they don't satisfy to the other side, so that (for example) a check in an L2 will cause the request to get forwarded up to its corresponding L1(s) if necessary.  The trick is that normally in the latter case the L2 is the default responder; the IOCache/IOBus case is special because the IOCache is not the default responder.
   
   If this is the problem, then what you need is to put a default responder on the IOBus, but set its address range to restrict it to the I/O address range, and then set the use_default_range flag on the IOBus.  That should prevent the default responder from responding to functional requests that don't map to the I/O bus.

src/cpu/base_dyn_inst.hh (Diff revision 2)

Can we find a way to replace all this stuff with a single templated method?  I think if you add some helper methods to Result like this:
union Result {
    [...]
    void set(uint64_t i) { integer = i; }
    void set(double d) { dbl = d; }
    void get(uint64_t &i) { i = integer; }
    void get(double &d) { d = dbl; }
}

then you could replace these with

template <class T>
T popResult() {
   [blah blah]
}

and maybe clean up some of the other existing code as well.

1. Geoffrey Blake Dec. 6, 2011, 12:15 a.m. (Dec. 6, 2011, 12:15 a.m.)

   I'll see what I can do with this part of the code to make it cleaner if I can.

Cleaned up the code in base_dyn_inst.hh to use templates.  Cleaned up the code in arch/arm/isa.cc to get the ITB/DTB pointers from the CheckerCPU object.  

I have implemented a solution for functional accesses that go to the IOBus by adding a "reflect_functional_access" parameter to isa_fake.cc.  This parameter allows accesses to pass through if they are detected to be functional via checking if the Packet object can be safely cast to a FunctionalPacket object to identify just functional accesses.  I then added this io_device with the reflect param set to the IOBus as a default responder for the ARM setup in configs/common/FSConfig.py.  Not sure if this is what is ultimately desired, but it fixes the problem with the Checker's functional accesses without the hack in the bus.cc code.  I looked at using the default_range variable as Steve suggested, but saw in bus.cc line 359 that the code would panic if the access did not fit in the default range.  Also would like some suggestions as to how to warn on detecting no default responder attached to the IOBus (any bus), should it be in the Python or C++ code?  

This is converging to where it needs to be. There are style problems thinly scattered throughout so you'll need to fix those, and there are some smaller tweaks I'd like to see. I'm surprised to see indentation issues here and there. You should talk to Ali about setting up vim/emacs to do gem5 style automatically and then you won't have to worry about that. Other more knowledgeable people are already discussing the devices/functional access thing you're doing, so I'll just ignore that.

src/arch/arm/isa/insts/misc.isa (Diff revision 3)

This line is too long. I won't point out the others that are as well, but please fix throughout.

src/arch/arm/tlb.cc (Diff revision 3)

What's this for? I don't see any new code in this file that would need this.

src/arch/arm/tlb.cc (Diff revision 3)

Bad indentation, you don't need the {}s, and you could move it all into the assert.

assert(!(timing && functional));

At least fix the indentation, preferably just replace it with this new version.

src/cpu/BaseCPU.py (Diff revision 3)

Yeah, this assert always seemed a bit hacky to me.

src/cpu/CheckerCPU.py (Diff revision 3)

What's your reasoning for changing this? Are you worried that this is likely to happen but not signify a real bug? Have you actually seen that happen?

1. Geoffrey Blake Dec. 20, 2011, 7:29 a.m. (Dec. 20, 2011, 7:29 a.m.)

   Yes.  I see loads from the Checker complain that the values don't match often enough due to no LSQ snooping. I think setting warnOnlyOnLoadError to False should be the default.  Functional LSQ snooping from the Checker should be added as a future task to figure out a way around this.  In FS mode, it is also possible for loads from the Checker to see different values than the O3 because of coherent IO as well.

src/cpu/DummyChecker.py (Diff revision 3)

So much copyright for so little code :-)

src/cpu/base.cc (Diff revision 3)

It would be better to move these declarations down to where the variables are used. That way you can also merge the #if blocks.

src/cpu/base.cc (Diff revision 3)

Have you tested how the checker acts when you switch CPUs? It wouldn't be criminal if that isn't working perfectly at this stage since this change already fixing alot, but it would be nice if that worked.

1. Geoffrey Blake Dec. 20, 2011, 7:30 a.m. (Dec. 20, 2011, 7:30 a.m.)

   The Checker works with fast-forwarding and checkpoints and has been tested.

src/cpu/base_dyn_inst.hh (Diff revision 3)

Bad indentation.

src/cpu/checker/cpu.cc (Diff revision 3)

A lot of this could be moved to the initializer list. It's ok to leave it here, though, since this isn't performance sensitive.

src/cpu/checker/cpu.cc (Diff revision 3)

Drop the {}s.

src/cpu/checker/cpu.cc (Diff revision 3)

I think you're only indented 3 spaces here instead of 4.

src/cpu/checker/cpu.cc (Diff revision 3)

It's really only going to rot, and it'll be in version control anyway. I'd leave a comment saying you deleted it and just get rid of it.

src/cpu/checker/cpu.cc (Diff revision 3)

Bracket should be on the preceding line.

src/cpu/checker/cpu.cc (Diff revision 3)

No {}s, indentation.

1. Steve Reinhardt Dec. 19, 2011, 11:10 p.m. (Dec. 19, 2011, 11:10 p.m.)

   I know we just had this discussion recently... as I said then, I distinctly recall a period in time where we decided that we should *always* have braces even around single-statement blocks, for consistency and to make it easier to add new lines to the block (even temporarily, e.g. for debugging) without having to add the braces.  Obviously that policy didn't make it into the official style guide, and I was always somewhat ambivalent about it anyway, so I'm not trying to revive it.  However, it's equally true that the converse of that policy is not in the style guide, i.e., there's no requirement to *not* have braces around single-statement blocks either.  So IMO Geoff should be free to disregard those specific comments of yours.  (Though there are a few places where the brace does need to be moved to the preceding line if it's not deleted.)
   
   If we want to have a discussion about whether or not we should amend the style guide to specifically cover single-statement blocks, we should do that in a separate thread.  I'm fine with leaving it at the coder's discretion though.

2. Gabe Black Dec. 20, 2011, 5:31 a.m. (Dec. 20, 2011, 5:31 a.m.)

   We did, and then too I said that's not how I remember it. I can't find those emails, but we were talking about insisting the brackets were *not* used except when the code wouldn't fit on a single line. My position was that it shouldn't be mandatory to drop the brackets, and there didn't seem to be opposition to that.
   
   As far as what we do now, I'm still fine with leaving it at the coders discretion and it isn't in the style guide. Geoff wouldn't *have* to remove the {}s, but I think it looks better.

3. Steve Reinhardt Dec. 20, 2011, 6:41 a.m. (Dec. 20, 2011, 6:41 a.m.)

   I'm not surprised you don't remember the original policy, since I believe it was before your time.  Through the magic of gmail I found the thread where we originally decided to require braces on single-line blocks: Nate and Steve Raasch were opposed, Erik and Lisa were in favor, and I was in the middle, but leaned enough toward Erik & Lisa to be the deciding factor.  This was September 12, 2003.  Apparently none of our on-line list archives go back that far anymore.
   
   Apparently Nate eventually forgot that he was on the losing side of that one, and by that point Erik wasn't around to hound people (and Lisa's not the hounding type), so it fell by the wayside.  There are a couple of threads from late 2008 where we basically had the same discussion we're having now:
   http://www.mail-archive.com/gem5-dev@m5sim.org/msg01365.html
   http://www.mail-archive.com/gem5-dev@m5sim.org/msg02026.html
   
   I've updated the style guide to reflect what I believe are the current rules:
   http://gem5.org/Coding_Style#Braces
   If we want to discuss further, we should do it on the list (or break new ground and use the wiki talk page).  I probably should have put this on the list instead of typing it into reviewboard as well, but now that I've done it I'm not sure it's worth switching.

src/cpu/checker/cpu.cc (Diff revision 3)

spaces after ,

src/cpu/checker/cpu.cc (Diff revision 3)

Using ?: here makes this if harder to understand. You should declare a temporary right above this if, maybe extraDataOk. Using the ?: there would be ok since that would be simpler to start with.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

No offset involved...

src/cpu/checker/cpu_impl.hh (Diff revision 3)

It would be better to call this handlePendingInt since it's a more ISA neutral term and is more consistent with the rest of gem5.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Drop the {}s.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

You should declare a temporary which has the inst to check and then have one if and one panic which checks it. That gets rid of the redundancy here.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

This line is gigantic. Wrap it, and/or use a temporary.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Drop the {}s, result.pop(); on the next line.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

ckFault => checkFault. It's not very long and there's no reason to be cryptic.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Not here you aren't...

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Delete instead of commenting it out. I know the code you copied it from had this commented out and eventually I want to fix this, but there's no reason to propagate the problem.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

If the checker CPU is going to be able to trace things, it should have a tracer parameter. If not, pass NULL in as the tracer argument to execute. I'm 95% sure that will work, and if not you should be able to tell pretty quickly.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

I'm not sure it makes sense for the checker CPU to recognize PC based events since the main CPU is going to do that. It may, though.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Bad indentation.

src/cpu/checker/cpu_impl.hh (Diff revision 3)

Could you clarify what you mean here? The PC mapping to an integer register should be handled by the instructions and the decoder, although I can believe there may be subtle side effects.

Also, vagarities => vagaries.

src/cpu/o3/O3Checker.py (Diff revision 3)

I remember seeing this line before so I may have already asked, but why are you changing this?

src/cpu/o3/commit_impl.hh (Diff revision 3)

Drop the {}s, and I already mentioned this function's name earlier.

src/cpu/o3/dyn_inst_impl.hh (Diff revision 3)

{}s

src/cpu/o3/fetch_impl.hh (Diff revision 3)

What are these for? If you added these for a reason please say so, but otherwise be careful not to add dead includes.

src/cpu/o3/thread_context_impl.hh (Diff revision 3)

Drop the {}s. I'll stop pointing these out (and I may have missed some), but please fix throughout.

1. Geoffrey Blake Dec. 20, 2011, 7:31 a.m. (Dec. 20, 2011, 7:31 a.m.)

   I'll go through and correct the style in the upcoming days. Thanks for the review comments.

Made requested fixes to style.  Some small changes to fix a memory leak induced by adding an extra request object in src/cpu/base_dyn_inst.hh.  

Implemented a solution for the bus/functional accesses problem by treating accesses that come in from the mem-side port to be "expressSnoops" and have the bus treat these accordingly if the IOCache does happen to forward snoops to a device with coherent memory.  

Re-posted the diff. 

src/cpu/checker/cpu.cc (Diff revision 5)

Related to the question below. Any ideas how to clean this up? This limits an SE run to only 1 core effectively right now.

src/cpu/o3/O3CPU.py (Diff revision 5)

I have a question for Nate or someone familiar with the guts of the python setup code for a simulation. I had to put this array de-reference to get the python to work properly.  Without it, the python code complains about an unknown type, even if the workload array is 1 element, as python will try to pass this: [[workload1]]. An array of arrays to the C++ code.  How should I fix this?

1. Steve Reinhardt Jan. 23, 2012, 1:23 p.m. (Jan. 23, 2012, 1:23 p.m.)

   Good question... this may be a bug in the internal param code.  When you assign a scalar value to a VectorParam like "workload" it will automatically convert the scalar value to a one-element array.  It almost looks like this is happening twice instead of once.  You could possibly be the first person to ever use the Parent. proxy mechanism with a VectorParam.
   
   You can try putting some print statements in VectorParamDesc.convert() in puthon/m5/params.py to see if you can track down what's going on... unfortunately I don't have time to look at it myself.  Let me know what you see and I can try and help figure it out.

src/mem/bus.cc (Diff revision 5)

This should no longer be needed with the changes done to the functionalAccess in the cache now being able to respect forwardSnoops.

Show all issues

1. Geoffrey Blake Jan. 24, 2012, 1:30 a.m. (Jan. 24, 2012, 1:30 a.m.)

   This is needed in the case there is a coherent device that can be snooped on the bus (cache has forwardSnoops=true). The bus needs to identify if the packet is a forwarded snoop to prevent it from sending it erroneously to a destination port.

src/mem/cache/cache_impl.hh (Diff revision 5)

This should not be needed surely? If it should not forward snoops then don't forward it? 

Show all issues

1. Geoffrey Blake Jan. 24, 2012, 1:30 a.m. (Jan. 24, 2012, 1:30 a.m.)

   Also needed here because the functionalAccess() routine has no knowledge of which port the packet came in on, so this ensures forwardSnoops is respected (requests coming upstream) but also keeps regular accesses (downstream requests) working. 

src/mem/cache/cache_impl.hh (Diff revision 5)

I don't understand why these changes are here. They make me even more confused about what an express snoop is and what it is not. What problem is this aiming to solve?

Show all issues

1. Geoffrey Blake Jan. 24, 2012, 1:30 a.m. (Jan. 24, 2012, 1:30 a.m.)

   This was a design decision on my part.  For the timing path, when a request comes in the MemSidePort, handleSnoop() is called which then creates a new packet with expressSnoop set.  Instead of adding more code with a functionalSnoop() routine and creating additional packets like the timing path, I allow the packet to set/unset itself as a snoop when it comes up the MemSidePort to re-use the functionalAccess() code. 

src/mem/packet.hh (Diff revision 5)

A packet should never be an express snoop and at any point loose that state (at least it messes with my brain and the ability to reason about what type of packets go where).

Show all issues

Re-factored patch to compile and work with Andreas's changes that were recently pushed.

Will fix the python param code issue with the Checker for SE mode in another patch to come soon.

There are only a few small things to fix now, at least as far as I can see.

src/arch/arm/isa.cc (Diff revision 7)

This line is too long.

Show all issues

src/arch/arm/isa.cc (Diff revision 7)

So is this one.

Show all issues

src/arch/arm/isa.cc (Diff revision 7)

And this one.

Show all issues

src/arch/arm/isa.cc (Diff revision 7)

And this one. I'll stop pointing them out, but please fix throughout.

Show all issues

src/cpu/checker/cpu.hh (Diff revision 7)

It's not from you, but there's some whitespace at the end of this line. Since you're getting rid of commented out code, you could get rid of the whitespace too.

Show all issues

src/cpu/checker/cpu.cc (Diff revision 7)

This sentence seems to have dropped off half way there.

Show all issues

Ship It!