950176 - [hazards] caller/callee names do not always match

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I was chasing down a problem where the analysis was finding hazards in eg AddClearDefiniteGetterSetterForPrototypeChain, which should be within an AutoEnterAnalysis.

The problem turned out to be several steps up the call chain, where

uint8 js::types::FinishCompilation(JSContext*, const class JS::Handle<JSScript*>, uint32, js::types::CompilerConstraintList*, js::types::RecompileInfo*)

calls an ancestor function, but is never called itself. Instead,

uint8 js::types::FinishCompilation(JSContext*, const class JS::Handle<JSScript*>, uint32, CompilerConstraintList*, js::types::RecompileInfo*)

is called. Notice the different qualification of CompilerConstraintList.

As a result, FinishCompilation(..., js::types::CompilerConstraintList, ...) is treated as an unsuppressed root caller.

Comment 1

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

I tried scanning for other mismatches. Here's one:

bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor(JSContext*,
      JS::Handle<JSObject*>, JS::Handle<jsid>,
      JS::MutableHandle<JSPropertyDescriptor>, unsigned int) [with Base = js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy = xpc::ExposedPropertiesOnly]

vs

bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor(JSContext*,
      JS::HandleObject, JS::HandleId,
      JS::MutableHandle<JSPropertyDescriptor>, unsigned int) [with Base = js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy = xpc::ComponentsObjectPolicy; JS::HandleObject = JS::Handle<JSObject*>; JS::HandleId = JS::Handle<jsid>]

Which is kind of scary. Come to think of it, I think the callgraph had 73000 functions (out of 4.5 million) with no callers. So this problem may be fairly widespread.

By the time we get to the linker, these clearly must be canonicalized. Can we get sixgill to do that?

Comment 2

[Brian Hackett [Laid off!]]

I tried to use the decorated name for this stuff when I wrote the gcc plugin frontend way back when but couldn't get it to work.  IIRC the problem is that the decorated names aren't available until a later phase of compilation.  Maybe there's an API to compute it earlier?

Comment 3

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

Well, that's something to go on. Or perhaps there's a way to dump out the mapping when the "decorated" name is computed later?

Comment 4

[Nicolas B. Pierron [:nbp — off until 29-09]]

I don't know how you are making the comparisons, but maybe you can filter out the decoration part of the names, by striping any namespace prefixes which are matching any of the prefixes of the function declaration.  This will work except maybe with "using" directives, in which case the symbol can be identical but still have different prefixes.

(In reply to Steve Fink [:sfink] from comment #1)
> I tried scanning for other mismatches. Here's one:
> 
> bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor … [with Base =
> js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy =
> xpc::ExposedPropertiesOnly]
> 
> vs
> 
> bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor … [with Base =
> js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy =
> xpc::ComponentsObjectPolicy; JS::HandleObject = JS::Handle<JSObject*>;
> JS::HandleId = JS::Handle<jsid>]

Note that the Policy parameters are different too.  In which case I don't think this is a good example of this issue.

Comment 5

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

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> I don't know how you are making the comparisons, but maybe you can filter
> out the decoration part of the names, by striping any namespace prefixes
> which are matching any of the prefixes of the function declaration.  This
> will work except maybe with "using" directives, in which case the symbol can
> be identical but still have different prefixes.

Perhaps that's good enough. It won't help with HandleObject vs Handle<JSObject*>, though.

> (In reply to Steve Fink [:sfink] from comment #1)
> > I tried scanning for other mismatches. Here's one:
> > 
> > bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor … [with Base =
> > js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy =
> > xpc::ExposedPropertiesOnly]
> > 
> > vs
> > 
> > bool xpc::FilteringWrapper<Base, Policy>::getPropertyDescriptor … [with Base =
> > js::SecurityWrapper<js::CrossCompartmentWrapper>; Policy =
> > xpc::ComponentsObjectPolicy; JS::HandleObject = JS::Handle<JSObject*>;
> > JS::HandleId = JS::Handle<jsid>]
> 
> Note that the Policy parameters are different too.  In which case I don't
> think this is a good example of this issue.

Ah! Thanks, I didn't notice that. (I was just comparing a list of all known functions with the list of functions that showed up in the call graph and eyeballing the diff. I probably ought to look at the set of functions with no callers instead.)

Comment 6

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

Attachment: Use mangled names to identify nodes in callgraph

Updates to the analysis to take into account the mangled function names. Note that the unmangled names are still used for annotations and for matching up constructors and destructors, as well as generating more human-readable output.

On the bright side, this eliminates the hack where we try both possible names of every destructor.

This change requires an updated sixgill. Patch for that is in bug 941094. I'll need to rebuild RPMs for the tbpl runs before I can push this.

Comment 8

[Brian Hackett [Laid off!]]

Comment on attachment 8349509 [details] [diff] [review]
Use mangled names to identify nodes in callgraph

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

::: js/src/devtools/rootAnalysis/utility.js
@@ +121,5 @@
>  }
>  
> +// Split apart a function from sixgill into its mangled and demangled name. If
> +// no mangled name was given, use the demangled name as its mangled name
> +function splitFunction(func)

Since uses of this function are just looking for either the mangled or unmangled name, could you use two functions here instead, mangledName(func) and unmangledName(func) or something?  That would get rid of the destructuring and introduction of unused variable names in the callers.

Comment 9

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

(In reply to Brian Hackett (:bhackett) from comment #8)
> Comment on attachment 8349509 [details] [diff] [review]
> Use mangled names to identify nodes in callgraph
> 
> Review of attachment 8349509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/devtools/rootAnalysis/utility.js
> @@ +121,5 @@
> >  }
> >  
> > +// Split apart a function from sixgill into its mangled and demangled name. If
> > +// no mangled name was given, use the demangled name as its mangled name
> > +function splitFunction(func)
> 
> Since uses of this function are just looking for either the mangled or
> unmangled name, could you use two functions here instead, mangledName(func)
> and unmangledName(func) or something?  That would get rid of the
> destructuring and introduction of unused variable names in the callers.

Yes, that would be better. In fact, it's what I did in mrgiggles' Python code.

I also notice that I slipped up in this comment and used "demangled" when I should have said "unmangled" or "readable" or "generated" or something. I want to reserve "demangled" to mean a canonical name produce by |demangle|, as opposed to a nonunique sixgill-generated name that was never mangled in the first place.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/be83b982ebd3

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/be83b982ebd3