|
From: Nuno L. <nun...@sa...> - 2008-03-23 15:32:03
|
Thank you both for your comprehensive answers. Some comments inline:
>> > ------ IMark(0x80483D5, 7) ------
>> > PUT(60) = 0x80483D5:I32
>> > t137 = Left32(t130)
>> > t26 = Add32(t25,0xFFFFFFF8:I32)
>> > t139 = CmpNEZ32(t137)
>> > DIRTY t139 RdFX-gst(16,4) RdFX-gst(60,4) :::
>> > MC_(helperc_value_check4_fail){0x38006920}() DIRTY 1:I1 RdFX-gst(16,4)
>> > RdFX-gst(60,4) :::
>> > MC_(helperc_STOREV32le)[rp=2]{0x38006d40}(t26,0x0:I32)
>> > STle(t26) = 0x4:I32
>
>> > At first glance I would say that the first PUT(60) could be removed,
>> > right? It is basically a dead store (assuming it can only be read/write
>> > with GET/PUT). I didn't check further why the redudant removal
>> > optimization doesn't pick this, so I'm checking here first if this is
>> > sane.
>>
>> It's not removed because there's another statement (the STle) between it
>> and the subsequent PUT(60) that could cause a memory exception. A signal
>> handler for such an exception might inspect the %eip value, so it has to
>> be
>> up-to-date. Annoying, because it's so unlikely, but necessary.
>
> Yes. But not only for the STle, also for the call to
> MC_(helperc_value_check4_fail).
>
> If you look at struct VexGuestX86State in libvex_guest_x86.h, you can
> see that offset 60 is guest_EIP (the program counter). So the PUT is
> updating the simulated program counter, so it is up to date should the
> store generate an exception, but also (importantly) so that if
> MC_(helperc_value_check4_fail) needs to make a stack backtrace, it will
> start from the right point.
That makes sense. So there's nothing to do here :S
>> > Second question is: can the last memcheck check be removed? i.e. the
>> > last
>> > call to helperc_LOADV32le() is redudant, since it has already done a
>> > store to that location and thus it knows that the operation is safe.
>> > Can
>> > this call be removed? And what value do I assign to t144 then?
>> > VA_BITS32_DEFINED? (I'm just considering the simpler case where the
>> > #store bits >= #load bits).
>>
>> I think it is redundant. Since 0x0 (which is VA_BITS32_DEFINED) was the
>> shadow store value, you'd assign 0x0 to t144. And you're right that you
>> could do likewise if t144 was smaller than 32 bits.
>>
>> > Allow me just a last question: is it safe to replace the 't30 =
>> > LDle:I32(t26)' statement with 't30 = 0x4:I32'? Well in general I would
>> > say it is safe, but I dunno about memory-mapped I/O nor if/how valgrind
>> > handles it. Maybe this can be done in only certain architectures?
>>
>> If we're optimising away the shadow load, maybe it's reasonable to
>> optimise
>> away the real load, but I'm really not sure about that one. Julian might
>> have more to add.
>
> Difficult question! I would comment first that it might be worth looking
> at the original code to figure out why the compiler put a store and then
> a load from the same location in the next insn. It might be that the
> second instruction (0x80483DC) is a branch target, maybe the top of a
> loop.
>
> So is it safe to remove the real and shadow load and forward values
> directly to it from the real and shadow store? Well, that would probably
> work _most_ of the time, but sounds dangerous if multiple threads are
> doing spinlocks or something like that - then it might be that changing
> the number of memory references might have some bad effect.
I see that this doesn't work.. If the ptr point to some allocated memory,
other thread may free() it, the ptr will start pointing to freed memory and
thus following read/writes need to be marked as an error.
So, no luck this time. I'll keep searching :)
Thanks,
Nuno
|