667879 - VTable padding off by one

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, enhancement, P3)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

VTable.h had a bool 'pad[3]' element to offset the bool 'linked' member (that padding was added in changeset:622 by Steven as part of a slew of fixes to structure alignment).

In changeset:3568 it was changed to a 'pad[2]' element to accommodate a new bool member, 'basecase', that Lars added to VTable.

Then in changeset:5923 the 'basecase' member was removed, but the padding was not re-adjusted back to 'pad[3]'

If there was ever a reason for the padding in the first place, then presumably the amount of padding should be adjusted back to 'pad[3]'

REFERENCES
[changeset:622]  http://hg.mozilla.org/tamarin-redux/diff/622/core/VTable.h
[changeset:3568] http://hg.mozilla.org/tamarin-redux/diff/3568/core/VTable.h
[changeset:5923] http://hg.mozilla.org/tamarin-redux/diff/5923/core/VTable.h

Comment 1

[Dan Smith]

How did you discover this?  Inspection?  Is this important enough to add an assertion or self-test for this condition?

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #1)
> How did you discover this?  Inspection?  Is this important enough to add an
> assertion or self-test for this condition?

tltRipley asked about why we had the 'pad[2]' member in the #tamarin chat room.

I was curious about it and couldn't explain its presence via inspection of the tip, so I used 'hg blame' [*] to find out where it was injected, and traced back the history to the individual change sets where it was introduced.

The real question is how Steven found the places to add padding to back in changeset:622.  (I suspect he did so via inspection, which would be unfortunate; but maybe I am wrong and there is a tool that can help us here.)

[*] well, really emacs 'M-x annotate', but same difference

Comment 3

[Steven Johnson]

Totally by inspection. The explicit padding is arguably unnecessary.

Comment 4

[Trevor Baker]

From Felix:

It doesn't need to be resolved before Serrano.  Its absolutely deferrable; it is at most a performance issue, not a correctness one, and I suspect the performance difference is insignificant.