1479673 - Fix gaps in the callgraph

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

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

I did some cross-checking of the callgraph to identify places where we are calling a function that we don't have a definition for, and it isn't in a known category (eg external glibc calls).

Comment 1

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

Attachment: Synthesize calls between various destructor variants that better match observed behavior and my current understanding

Many of these turned out to be yet more examples of the problems arising from the way gcc sets up its constructors (ctors) and destructors (dtors). For dtors, gcc has 4 different types -- more if you count in-charge vs not-in-charge, but they aren't orthogonal. gcc will emit a body for one of them (either the in-charge or not-in-charge D4 variant, fwiw) and then create either aliases or thunks from the others to that one if valid. And sometimes it will generate full bodies with regular calls.

This most likely does not end up in the best possible place. sixgill is doing some manual adjustment before this code sees any of it, then this code does some fairly conservative stuff to maximize the chance that a caller will see some sort of edge to emitted code, even if it's not the real path it would take.

A later change will switch things much more to using mangled names for (almost) everything, but I haven't redone any of this with that in mind -- in my latest version, I don't think this code changes much at all further.

Comment 2

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

Attachment: Trivial return value change

JS::ubi::Concrete<T> inherits from JS::ubi::Base but not JS::ubi::Node, so the bare 'Size' in the class declaration refers to Base::Size, not Node::Size. (Note that they're the same underlying type.)

This resulted in a break in the callgraph, since the unmangled name used in the body of the caller did not match the unmangled name of the callee that was actually defined. But it's a spot-fix to a problem that shows up in more places, which I fix in a later patch by switching to matching only on the mangled names (where the different names for the types are boiled out.)

Comment 3

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

Attachment: Update to sixgill with less CSU qualification (630e2025191d)

This is the big fix here -- I had many missing edges because somebody called foo(JSContext*), but only foo(struct JSContext*) was defined. I've fixed this before, by making sure more and more places add the struct/class/union keyword -- until I eventually discovered that there were places where gcc was generating unmangled function names that lacked them! So this goes the other way and removes them. That appears to have fixed all callgraph gaps of this type.

I will upload the sixgill changes separately.

Comment 4

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

Attachment: Do not qualify types with struct/class/enum, to match the internal types gcc produces

We were passing TFF_CLASS_KEY_OR_ENUM in some places, but gcc was producing some unmangled names without class/struct/union keywords. Note that this change will become irrelevant with a later change, where I stop using unmangled names for almost everything.

Comment 5

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

Attachment: "Handle" assembly code

This doesn't really belong in this bug, but it fixes a case where encountering inline assembly caused the analysis to crash. Ignore it instead, which isn't strictly correct, but it doesn't seem worth the trouble of figuring out uses and definitions.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8996207 [details] [diff] [review]
Synthesize calls between various destructor variants that better match observed behavior and my current understanding

Review of attachment 8996207 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ddac0d1af4
Trivial return value change, r=jonco

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/12ddac0d1af4

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aedd937e5d6a
Disable the analysis while calling function pointers during tracing, r=me.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7adcbe1979
Synthesize calls between various destructor variants that better match observed behavior and my current understanding, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8e57ba1b07
"Handle" assembly code. r=me

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/aedd937e5d6a
https://hg.mozilla.org/mozilla-central/rev/8b7adcbe1979
https://hg.mozilla.org/mozilla-central/rev/ed8e57ba1b07

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a07eed99ca8
Update to sixgill with less CSU qualification (630e2025191d), r=me

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8a07eed99ca8