[hazards] caller/callee names do not always match

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

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.
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?
Assignee: nobody → bhackett1024
Blocks: 898606
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?
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?
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.
(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.)
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.
Attachment #8349509 - Flags: review?(bhackett1024)
Assignee: bhackett1024 → sphink
Status: NEW → ASSIGNED
Duplicate of this bug: 949240
Status: ASSIGNED → NEW
Depends on: 952014
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.
Attachment #8349509 - Flags: review?(bhackett1024) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/be83b982ebd3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Duplicate of this bug: 1143765
You need to log in before you can comment on or make changes to this bug.