I don't have as big objections as you do. Reasons for the comments: It was causing problems and chasing things around is not as fun as reading a comment. When you inexplicably get a page fault, it would be more helpful for users to be able to figure out what is going on than chasing function calls, IMHO. I think the warning is probably more important than the verbose comments though. I find a lot of comments to be too short to be useful, they don't actually save you any time.
I don't have a problem with the braces issue. The conditional lookup just makes more sense to me. Why would you look up something that you clearly know you just failed to allocate? Seems illogical to me, and just made more sense. You're right that there's no functional change, but it somewhat offended me that you'd do that entire function call when the result is a foregone conclusion. I'd rather it get fixed in the other ISAs than changed back here. To me, vaddr is an obvious synonym to virtual address. In short, I don't object to you being way more anal than I am. ;) Lisa On Sun, Sep 4, 2011 at 4:49 PM, Gabe Black <[email protected]> wrote: > I just noticed this change and have a few comments on it. Ideally it > would have been reviewed and we could have talked about it then, but I > can see how it would be considered a minor change that wasn't worth a > review. > > First, while comments are good, these comments are too verbose. If > somebody wants to know how checkAndAllocNextPage works, they can look at > that function. If they want to know how it's being applied in this > particular instance or if there are any special quirks, behaviors, etc., > that somebody should be aware of, that's appropriate to put here. > Otherwise we end up with a lot of comments that are a bit redundant and > obscure the actual code. > > Second, I don't like how the lookup is now conditional. This makes it > unnecessarily inconsistent with the other ISAs. This isn't a huge > problem, but I don't think it actually buys anything. > > Third, even though the comments don't count as "lines" in the C++ sense, > the do in the textual sense. There should be braces around the body of > the if. > > Fourth, in process.cc, vaddr is a variable name, not something that will > make sense to somebody that hasn't looked at the code yet. It should > just say "address" or "virtual address" if there's some reason to be > specific. > > To stress, this isn't the end of the world and these aren't major > problems, but I'm planning to tidy this up as described. Let me know if > you have any objections, Lisa. > > Gabe > > On 09/02/11 17:06, Lisa Hsu wrote: > > changeset 09745e0c3dd9 in /z/repo/gem5 > > details: http://repo.gem5.org/gem5?cmd=changeset;node=09745e0c3dd9 > > description: > > TLB: comments and a helpful warning. > > > > Nothing big here, but when you have an address that is not in the > page table request to be allocated, if it falls outside of the maximum stack > range all you get is a page fault and you don't know why. Add a little > warn() to explain it a bit. Also add some comments and alter logic a little > so that you don't totally ignore the return value of > checkAndAllocNextPage(). > > > > diffstat: > > > > src/arch/x86/tlb.cc | 18 ++++++++++++++++-- > > src/sim/process.cc | 1 + > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diffs (39 lines): > > > > diff -r 8dac0abb7a1b -r 09745e0c3dd9 src/arch/x86/tlb.cc > > --- a/src/arch/x86/tlb.cc Thu Sep 01 15:25:54 2011 -0700 > > +++ b/src/arch/x86/tlb.cc Fri Sep 02 17:04:00 2011 -0700 > > @@ -618,8 +618,22 @@ > > TlbEntry newEntry; > > bool success = p->pTable->lookup(vaddr, newEntry); > > if (!success && mode != Execute) { > > - p->checkAndAllocNextPage(vaddr); > > - success = p->pTable->lookup(vaddr, newEntry); > > + // This may fail because for some reason the > requested > > + // address is not allocatable on the stack. If it's > a stack > > + // address, then it's because the address fell > outside of > > + // max stack range and user should increase max size > of > > + // stack. Otherwise, it could be a random address > that was > > + // not in the page table and not on the stack. > Either way, > > + // you'll end up with a page fault. > > + if (p->checkAndAllocNextPage(vaddr)) > > + // Might as well not check this if you failed to > > + // allocate. Partially nested this just so code > > + // maintainers can understand this is a separate > and > > + // necessary step not sufficient just by reading > return > > + // value of checkAndAlloc call because there is > a side > > + // effect. This call will populate (it's called > by > > + // reference). > > + success = p->pTable->lookup(vaddr, newEntry); > > } > > if (!success) { > > return new PageFault(vaddr, true, mode, true, > false); > > diff -r 8dac0abb7a1b -r 09745e0c3dd9 src/sim/process.cc > > --- a/src/sim/process.cc Thu Sep 01 15:25:54 2011 -0700 > > +++ b/src/sim/process.cc Fri Sep 02 17:04:00 2011 -0700 > > @@ -351,6 +351,7 @@ > > }; > > return true; > > } > > + warn("Not increasing stack: requested vaddr is outside of stack > range."); > > return false; > > } > > > > _______________________________________________ > > gem5-dev mailing list > > [email protected] > > http://m5sim.org/mailman/listinfo/gem5-dev > > _______________________________________________ > gem5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/gem5-dev > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
