525562 - Double var assign in QCache.cpp

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Andrey Kopysov]

User-Agent:       Opera/9.80 (Windows NT 5.1; U; ru) Presto/2.2.15 Version/10.00
Build Identifier: 

patch:

diff --git a/core/QCache.cpp b/core/QCache.cpp
--- a/core/QCache.cpp
+++ b/core/QCache.cpp
@@ -99,7 +99,7 @@
 			}
 			if (evicted_prev)
 			{
-				evicted = evicted_prev->next;
+//				evicted = evicted_prev->next;
 				AvmAssert(evicted != NULL && evicted != gen);
 				WB(m_gc, evicted_prev, &evicted_prev->next, evicted->next);
 			}

Reproducible: Always

Comment 1

[Steven Johnson]

Could you explain a bit more? What is the nature of the bug? Inspection doesn't reveal it to me.

Comment 2

[Andrey Kopysov]

In this code one value is twice assigned to a variable "evicted". I have thought that it can influence performance. I am not assured completely, I have not enough experience with C++.

Comment 3

[Lars T Hansen]

There's a flaw in the code but the offered patch isn't the optimal one.  If you look at the eviction code, 'evicted' is set before the break in the scanning loop.  But then both arms of the 'if' following the loop set 'evicted' first thing, so the assignment in the loop is actually dead and can be removed.

(I expect most compilers will remove the redundant assignment, though, it should be easy enough.)

Comment 4

[Steven Johnson]

Ah. Gotcha. Yeah, a minimal inefficiency, but worth correcting next time we touch this code.

Comment 5

[Lars T Hansen]

Attachment: Obvious patch

Comment 6

[Lars T Hansen]

tamarin-redux changeset:   4399:7d02e45ab981