1453506 - [templates] Search for references to nsDisplayOwnLayer constructor does not find calls through MakeDisplayItem, similar issue for MakeRefPtr

Status: RESOLVED FIXED

(Webtools :: Searchfox, enhancement)

Description

[Botond Ballo [:botond]]

When I search for references to this nsDisplayOwnLayer constructor [1], it would be nice if it somehow found the calls via MakeDisplayItem, like this one [2] and this one [3].

[1] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/painting/nsDisplayList.h#5494
[2] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/generic/nsGfxScrollFrame.cpp#3059
[3] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/layout/xul/nsSliderFrame.cpp#461

Comment 1

[Botond Ballo [:botond]]

It's not immediately clear how this would work.

The call to the constructor itself is in the function template instance MakeDisplayItem<nsDisplayOwnLayer>. Searchfox doesn't allow you to visualize template instances, so there's no place to navigate to. Navigating to the uninstantiated function MakeDisplayItem is not helpful, because what you're really interested in is the call sites of MakeDisplayItem<nsDisplayOwnLayer>; querying the call sites of the uninstantiated MakeDisplayItem function will turn up calls to all instances.

One idea might be to annotate (or have Searchfox special-case) MakeDisplayItem as a "constructor" function, such that if a caller is in a MakeDisplayItem instance, Searchfox goes one more hop to find the callers of that instance and shows you those.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)

One idea might be to annotate (or have Searchfox special-case)
MakeDisplayItem as a "constructor" function,

To mention some prior art here: CQuery, which has faced a similar problem, has chosen to heuristically consider any function template whose name starts with make or Make as a "constructor" function for such purposes.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

On the weekend I dug into this a bit for MakeRefPtr purposes. Note that everything I'm talking about in this comment is specific to function templates.

The root of the problem is that the unspecialized FunctionTemplateDecl only has a CXXNewExpr, whereas the specialized ones also have a CXXConstructExpr which is what searchfox actually processes to emit the "constructor" "use". The CXXNewExpr has an inherently different Loc than the CXXConstructExpr, so even if the CXXNewExpr were marked as a dependent location (it wasn't), the "constructor" "use" won't be emitted without additional support.

I added additional support logic to that end[1], but having all of the constructor invocations show up inside MakeRefPtr didn't seem useful. So the next step was to try and get all of the dependent records for the template "analysis" path be emitted at the point of invocation of function templates. This part is more involved and educational. Preliminary attempts to accomplish this show that the point of instantiation of the redecls of the specializations still gets attributed to RefPtr.h and that walking up DeclContext hierarchy just provides the lexical scopes from the same location. I stopped work while attempting to use the ASTContext::getParents mechanism and adding JSON-outputted logging[2].

If the AST node mechanism doesn't work (or is freshly inducing computation of the parent map which allegedly can have non-trivial memory/CPU costs), then it looks likely that the glue logic will need to happen while visiting DeclRefExpr nodes in the AST that exist at the call-sites in the source files (ex: TestRefPtr.cpp) that end up referencing the template definition/declarations and their specializations (ex: RefPtr.h). The concern with the latter is that it would mean we either need to save off some of the byproducts of the "gather" traversal of the canonical FunctionTemplateDecl (which is a new thing for the indexer), or re-derive the dependent locations every time we hit a function template call-site.

1: CXXNewExpr will emit a dependent location record to the AutoTemplateContext in "gather" mode and specialize an invocation of VisitCXXConstructExpr using the CXXNewExpr location, which is slightly off but was a good proof-of-concept.

2: I had added some helpers that create "trace" records that serve as records as where templates were processed for gather phases, identifying what dependent locations were record. Also a trace record for where specializations were processed and what their specialized symbol was. These were very valuable so the next step was adding a trace record for printf-style debugging that would perform a limited dump of an AST node and its N generation of ancestors.

Comment 4

[Nicolas Guichard]

Attachment: Draft GitHub Pull Request

Comment 5

[Nicolas Guichard]

Attachment: Bug 1453506 - Forward type-dependent news to template call locations. r=asuth!

This provides a way to make type-dependent statements appear as-if they
were located at the template call site.

During the analysis of the template, this first registers all type-dependent
function call locations in ForwardedTemplateLocations (we don't have more
information at this point).

Then when analyzing the template instantiations, we link the actual
type-resolved statements to the template instantiation.

Then when emitting the analysis data for a template call, we also (recursively)
emit all the analysis data for the forwarded items at the call site.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ae01e5c83cc
Forward type-dependent news to template call locations. r=asuth

Comment 7

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/0ae01e5c83cc

Comment 8

[Nicolas Guichard]

Attachment: Bug 1453506 - Forward constructor calls from template member functions too. r=asuth!

This calls VisitForwardedStatements in VisitCallExpr and makes
VisitForwardedStatements traverse CallExprs when forwarding
type-dependentconstructor calls to the surrounding function template
call location.

I also renamed to MakeUnique.cpp test to ForwardingTemplates.cpp, and
added a test provided by Botond around the emplace template.

Previously reviewed at https://github.com/mozsearch/mozsearch/pull/838

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d53e9b61411b
Forward constructor calls from template member functions too. r=asuth

Comment 10

[pstanciu]

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