61571 - improve speculative RTTI heuristics

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Chris Waterson]

Turns out that we can improve the speculative RTTI heuristics for c86 gcc: each
typeinfo() routine appears to have as its standard prologue either:

    55     push %ebp
    89e5   mov  %esp,%ebp
    53     push %ebx

or:

    55     push %ebp
    89e5   mov  %esp,%ebp
    56     push %esi

Previously, I was just looking for the first two instructions. Adding the third
makes us a bit more robust without having to go add a lot of ``special'' entry
points (some of which ended up being declared ``static'' in NSPR anyway.) Patch
soon...

Comment 1

[Chris Waterson]

Attachment: fix for nsTypeInfo.cpp

Comment 2

[Chris Waterson]

Attachment: fix for mozilla/gc/boehm/typeinfo.cpp

Comment 3

[Brendan Eich [:brendan]]

+    //    by egcs, plus a ``signature'' instruction that appears
+    //    in the ).
+    unsigned** fp1 = reinterpret_cast<unsigned**>(vt) + 1;

Missing comment words after "in the " and before ")." ?

Otherwise looks good.  r=[e-mail].

/be

Comment 4

[Chris Waterson]

Attachment: fix spelling errors; complete that thought

Comment 5

[Chris Waterson]

Cleaned up the comments a bit. beard, jband: could you sr=?

Comment 6

[Patrick C. Beard]

I'd prefer to see this code use byte comparisons, rather than comparing using an
unsigned pointers. This could have alignment implications and the resulting code
would probably be clearer.

Comment 7

[Brendan Eich [:brendan]]

What platforms don't align vtables on int (unsigned) boundaries?

/be

Comment 8

[Patrick C. Beard]

It's not the vtable, it's the code itself. Enclosing a patch that does what I'm
talking about.

Comment 9

[Patrick C. Beard]

Attachment: byte-by-byte comparison of prologue

Comment 10

[Patrick C. Beard]

sr=beard, either way.

Comment 11

[Chris Waterson]

Sure, beard's is prettier. I'll use it.

Comment 12

[Brendan Eich [:brendan]]

beard, don't you want to axe the if-not-0-mod-4-aligned-then-return here?

+    unsigned char* ip = *fp1;
+    if ((unsigned(ip) & 3) != 0)
+        return 0;

/be

Comment 13

[Chris Waterson]

Nah, leaving that in is fine. Saves us from slamming the signal handler if `ip'
points to 0x1, for example.

Comment 14

[Chris Waterson]

Attachment: consolidated patch; uses `unsigned' instead of `int' to suppress warning

Comment 15

[Brendan Eich [:brendan]]

waterson: I'm confused.  That test dates from when ip was unsigned*, which could
be misaligned when reinterpret_cast'd from a bona fide instruction pointer.  If
you care about nearly-null pointers, do a range test, but (ip & 3) != 0 then
return means the code may as well have used an unsigned* rather than unsigned
char* after all.  Right?

/be

Comment 16

[Chris Waterson]

Ah, I see what you're saying: the function's entry point need not be word 
aligned.

As it turns out, on egcs-1.1.2, with ld-2.9.5, on x86, the typeinfo() entry 
points *are* word aligned. Since this code is specific to the above magic 
concoction of tools, the alignment test doesn't trigger any ``false negatives''. 
And there's a small performance benefit: it reduces the probability that we'll 
trip the SIGSEGV signal handler if it turns out we're looking at a random word 
that's not a pointer.

Comment 17

[Brendan Eich [:brendan]]

Ah, good to know there won't be false negatives -- that was my concern, beyond
any generality in using a byte pointer rather than a word pointer.

>And there's a small performance benefit: it reduces the probability that we'll 
>trip the SIGSEGV signal handler if it turns out we're looking at a random word 
>that's not a pointer.

Good one -- "performance benefit"!

Seriously, if you want to sanity-check the pointer, why not bounds-check it in
relation to known text sections?

/be

Comment 18

[Chris Waterson]

Bah. I'm shippin' it.