Closed Bug 1479673 Opened Last year Closed Last year

Fix gaps in the callgraph

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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).
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.
Attachment #8996207 - Flags: review?(jcoppeard)
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.)
Attachment #8996208 - Flags: review?(jcoppeard)
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.
Attachment #8996209 - Flags: review?(jcoppeard)
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.
Attachment #8996210 - Flags: review?(bhackett1024)
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.
Attachment #8996211 - Flags: review?(jcoppeard)
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.
Attachment #8996207 - Flags: review?(jcoppeard) → review+
Attachment #8996208 - Flags: review?(jcoppeard) → review+
Attachment #8996209 - Flags: review?(jcoppeard) → review+
Attachment #8996211 - Flags: review?(jcoppeard) → review+
Attachment #8996210 - Flags: review?(bhackett1024) → review+
Blocks: 1480927
https://hg.mozilla.org/mozilla-central/rev/12ddac0d1af4
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Pushed by sfink@mozilla.com:
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
Duplicate of this bug: 1432993
Depends on: 1499177
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a07eed99ca8
Update to sixgill with less CSU qualification (630e2025191d), r=me
You need to log in before you can comment on or make changes to this bug.