Quoting Ali Saidi <sa...@umich.edu>:
On 2011-05-02 16:42:25, Gabe Black wrote:
> util/statetrace/arch/arm/tracechild.cc, line 79
> <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line79>
>
> I don't know how easy this would be to accommodate, but
you're going to be sending a bunch of extra zeros for int regs that
aren't 64 bits wide. Can you make it so you send full 64 bit values
only when the source is actually 64 bits wide?
Ali Saidi wrote:
There is no reason to worry about sending 4 bytes down the
wire. The speed issue isn't about sending 4 bytes, it's all about
having to put a breakpoint after every instruction.
Gabe Black wrote:
The reason I implemented sending diffs of the state instead of
sending the whole state is that what you send over the wire -does-
matter, significantly. Breakpoints are slow too, but that doesn't
mean everything else is irrelevant.
I fully appreciate that it wouldn't be a good idea to send 1KB of
data over the wire, but we're well past bike shedding arguing about
4 bytes. There is no reason to add complexity and control flow to
try and avoid it.
I don't think it's necessary, although I also don't think it would be
that hard.
Also, I don't like our tendency to characterize disagreement as bike
shedding. If I don't agree, I don't agree. I'm not obligated to change
my mind or just stand by while you do whatever you want. This is my
code and by the convention we've stated on several occasions that
means it's my decision. You're bike shedding by continuing to argue
after I've made my decision.
On 2011-05-02 16:42:25, Gabe Black wrote:
> util/statetrace/arch/arm/tracechild.cc, line 104
> <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line104>
>
> The idea is to verify that you're not falling off of uregs.
Maybe you could do something more flexible like
sizeof(myregs.uregs) / sizeof (myregs.uregs[0]).
Ali Saidi wrote:
uregs is never going to get smaller than it is now and I don't
see a reason to come up with a crazy assert to try and prove that
it isn't.
Gabe Black wrote:
uregs changing size is irrelevant. That formula will be exactly
right all the time and doesn't depend on the coincidence that the
CPSR is last.
Only if you assume that the integer registers are sent first, which
it seems like you're unwilling to make any assumptions about the
code, so perhaps we should somehow verify that?
Depending on the constants in an enum or the members of a data
structure being in a particular order for all of time is dangerous and
in this case pointless.
I don't appreciate your sarcasm or you treating my attempts to protect
the quality of my code like some sort of unreasonable hassle. Do you
think I like taking my time to reverse engineer and review all this
code somebody else wrote, and then having to fight over why it's
wrong? It's an enormous waste of energy, and there are so many better
uses for my time. And yet here we are. Again.
On 2011-05-02 16:42:25, Gabe Black wrote:
> util/statetrace/arch/arm/tracechild.cc, line 111
> <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line111>
>
> The same comment applies as in getRegs, except that you have
to deal with an offset. It would probably be a good idea to define
something in the enum to mark the start of the FP regs. You can
move the assert to after the if and subtract out the offset right
before indexing fpregs.
Ali Saidi wrote:
There are clearly 32 float registers defined in the enum and in
the struct. The assert just verifies that we're actually accessing
a floating point register when we should be. We don't need to
verify the structure size it's correct by construction.
Gabe Black wrote:
I wrote the original assert and know what it's for, verifying
the index and not the structure. Again, it assumes F0 is the first
FP reg which is arbitrary.
No it's not! F0 Is the first floating point register. 0 is the first
whole number so it's first. Would you rather some uglyiness of
START_FP, F0 = START_FP? Why are we arguing about code that is
correct by inspection, has been extensively tested, and works?!
Yes, put that in my code. Why are we arguing about what you want to do
to my code? How could anything be wrong with code that works right
this moment? Why shouldn't I just let you do whatever you want because
it's expedient?
On 2011-05-02 16:42:25, Gabe Black wrote:
> util/statetrace/arch/arm/tracechild.cc, line 129
> <http://reviews.m5sim.org/r/669/diff/1/?file=12215#file12215line129>
>
> Just because libc would use a macro doesn't mean we have to.
You should replace this with a constant of the appropriate type.
Ali Saidi wrote:
I disagree... This will transition nicely as soon as libc gets
it's act together.
Gabe Black wrote:
The fact that gcc uses macros is an unfortunate historical
artifact, not a valid design decision. The transition won't be that
nice either since it'll leave macro cruft in the source. We should
either use their macro because we have to, or use our own thing
that doesn't suck because it makes sense. Here we're combining the
downsides of both of those approaches.
Sure, but if for some reason the value changes when the support
actually makes it into libc it will be correct.
Sure it's the worst of both worlds, but if libc unilaterally changes a
value expected by the kernel it will still work? And this is in the
same review where you argue in several places that resiliency in the
face of change isn't important.
Gabe
_______________________________________________
m5-dev mailing list
m5-dev@m5sim.org
http://m5sim.org/mailman/listinfo/m5-dev
