1483885 - Refactor XPConnect VTables and get rid of nsXPTCStubBase hack

Status: NEW

(Core :: XPConnect, defect, P3)

Description

[Tom Ritter [:tjr]]

Attachment: XPConnect Vtables.txt

It would be good to refactor and get rid of the nsXPTCStubBase hack that prevents compiler-based vtable verification features (CFI vcall, VTV, and others (like Dimo's)) from working.  

Quoting https://bugzilla.mozilla.org/show_bug.cgi?id=1046600#c11:

> The idea here would be to change the build so that, for any IDL
> interface that can be implemented in JS, emit a concrete subclass
> that implements the methods by calling NS_InvokeByIndex. ... The
> linkage name problem can be dealt with by using the GCC "asm labels" feature:
>  https://gcc.gnu.org/onlinedocs/gcc-4.5.3/gcc/Asm-Labels.html#Asm-Labels
> This seems to work for virtual methods for both gcc and clang.

Nathan and Tom have written more about the way to fix this, in detail. A living document with their comments is present here: https://docs.google.com/document/d/1fO3FkQX50Fk1DblztTinCNS71USkTUgfHtADt308VJ8/edit#heading=h.slig0nuzh48

I have uploaded a snapshot of it in txt form.

Comment 1

[Bobby Holley (:bholley)]

Has code size been considered here? One of the key features of the current design is that methods can be made scriptable without much impact on code size (beyond the xpt data). It may be that the stubs are acceptable overhead relative to the benefits we get, but we should take some estimated measurements before we invest too time pursuing this.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #1)
> Has code size been considered here? One of the key features of the current
> design is that methods can be made scriptable without much impact on code
> size (beyond the xpt data). It may be that the stubs are acceptable overhead
> relative to the benefits we get, but we should take some estimated
> measurements before we invest too time pursuing this.

My assumption here was that we'd only enable multiple bases for CFI/VTV/etc. builds, but perhaps that's too much complexity.

If we want to turn this on everywhere, grep says we have ~1300 (!) scriptable interfaces.  Assuming an average of 10 (virtual) methods/class--I have no idea whether that's a reasonable number, just pulling something out of the air--that means we'd have about 8 bytes/method * 10 methods/vtable * 1300 vtables = 104k of overhead--and note that those are vtables, so not shareable between processes.

Yuck.  Maybe it's worth the complexity to have separate paths.  Are we going to want to turn on CFI/VTV/etc. everywhere?

Comment 3

[Tom Ritter [:tjr]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Are we going to want to turn on CFI/VTV/etc. everywhere?

I think our goal with CFI is to get it to a state where it is performant enough to become the default on all platforms we ship with clang (so mac and windows).  If we can't get it there; then no we won't ship it.

Perhaps that's an argument for two codepaths while we figure out the performance situation and improve it as needed. But my preference would be that if/when we make this change we enable it for some T1 build with tests (maybe the ASAN Nightly build?) so it doesn't bitrot.