1531951 - [hazards] Virtual method resolution is incorrect/incomplete

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

Fix the search path for virtual method implementations: for example static types that inherit their implementations from a base class will currently think that "sibling" classes' implementations are possible.

  Base
   | \
   |  \
Class Sibling
   |
   |
Subclass

Class *c = ...;
c->foo();

If Base::foo is declared as pure virtual, and Subclass and Sibling both override foo() but Class does not, then the call of c->foo() can only invoke Subclass::foo. Previously, we would see that c->foo() calls Base.foo (because foo is inherited from Base) and do a descendant search starting from Base rather than Class.

In the same situation except where Base::foo is defined (as a virtual method), then c->foo() might call either Subclass::foo() or Base::foo(), but still not Sibling::foo().

This turns out to be important for handling methods that are defined at the base of a large class hierarchy that overrides the implementation in the leaves but not the middle ("interface-y") classes. With this patch, you'll only consider the subtree rooted at your static type. Without, you'll assume it could call any of the implementations in the tree.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - Minor improvement to type display r=jonco

Comment 3

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - [hazards] Comments and refactoring r=jonco

Depends on D46542

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - [hazards] Rework how virtual methods are handled, r=jonco

Allows future support for attributes on virtual method declarations.

Also fixes the search path for virtual method implementations: for example static types that inherit their implementations from a base class will currently think that "sibling" classes' implementations are possible.

  Base
   | \
   |  \
Class Sibling
   |
   |
Subclass

Class *c = ...;
c->foo();

If Base::foo is declared as pure virtual, and Subclass and Sibling both override foo() but Class does not, then the call of c->foo() can only invoke Subclass::foo. Previously, we would see that c->foo() calls Base.foo (because foo is inherited from Base) and do a descendant search starting from Base rather than Class.

In the same situation except where Base::foo is defined (as a virtual method), then c->foo() might call either Subclass::foo() or Base::foo(), but still not Sibling::foo().

This turns out to be important for handling methods that are defined at the base of a large class hierarchy that overrides the implementation in the leaves but not the middle ("interface-y") classes. With this patch, you'll only consider the subtree rooted at your static type. Without, you'll assume it could call any of the implementations in the tree.

Depends on D46543

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - Clean up some test code warnings and switch to python3 for print(flush=True). r=jonco

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42d646d94f82
Clean up some test code warnings and switch to python3 for print(flush=True). r=jonco

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a250a3f9b5db
Minor improvement to type display r=jonco
https://hg.mozilla.org/integration/autoland/rev/8c6f9730f77d
[hazards] Comments and refactoring r=jonco

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/42d646d94f82
https://hg.mozilla.org/mozilla-central/rev/a250a3f9b5db
https://hg.mozilla.org/mozilla-central/rev/8c6f9730f77d

Comment 9

[Steve Fink [:sfink] [:s:]]

Oops, I should have marked this leave-open. The 4th patch still needs to land, and can't. I landed the others because they are refactoring prerequisites for some other bugs to land. But the final patch here is the only one that really matters for this bug.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sfink, maybe it's time to close this bug?

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #10)

The leave-open keyword is there and there is no activity for 6 months.
:sfink, maybe it's time to close this bug?

I should probably comment on at least one of these.

The patches are still good, still ready to land, but cannot land until I sort out some issues in Gecko code that block this landing. I'd like to get back to it, but it's lower priority behind some other stuff.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sfink, maybe it's time to close this bug?

Comment 13

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - [hazards] Manually mark more things as non-overridable (necessary to fix virtual method resolution without causing hazards, as it will now be able to see paths to these methods.) Temporary measure until this can be automated.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sfink, maybe it's time to close this bug?

Comment 15

[Steve Fink [:sfink] [:s:]]

Still blocked. Working on the blockage, somewhat.

Comment 16

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - Handle virtual destructors

Comment 17

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1531951 - Suppress hazard false alarms coming from std::swap calling refcounted operator=() in a context that will never drop a refcount to zero

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdf6082fd2f7
[hazards] Manually mark more things as non-overridable (necessary to fix virtual method resolution without causing hazards, as it will now be able to see paths to these methods.) Temporary measure until this can be automated. r=jonco
https://hg.mozilla.org/integration/autoland/rev/51209698131d
[hazards] Rework how virtual methods are handled, r=jonco
https://hg.mozilla.org/integration/autoland/rev/6ecec0210bf1
Suppress hazard false alarms coming from std::swap calling refcounted operator=() in a context that will never drop a refcount to zero r=jonco
https://hg.mozilla.org/integration/autoland/rev/30d9c0309fcb
Handle virtual destructors r=jonco

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63468c1ba08f
Fix lint failure. r=fix CLOSED TREE

Comment 20

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/fdf6082fd2f7
https://hg.mozilla.org/mozilla-central/rev/51209698131d
https://hg.mozilla.org/mozilla-central/rev/6ecec0210bf1
https://hg.mozilla.org/mozilla-central/rev/30d9c0309fcb
https://hg.mozilla.org/mozilla-central/rev/63468c1ba08f

Comment 21

[Steve Fink [:sfink] [:s:]]

(removing leave-open)