1490521 - Duplicate "Search for function..." options for function that has Mac-only call sites

Status: RESOLVED FIXED

(Webtools :: Searchfox, defect)

Description

[Botond Ballo [:botond]]

This is probably related to the recent addition of indexing of Mac-only files.

* Go to https://searchfox.org/mozilla-central/source/gfx/layers/apz/public/APZInputBridge.h#68
* Click on the symbol "ReceiveInputEvent"
* The popup menu has two entries labelled "Search for function mozilla::layers::APZInputBridge::ReceiveInputEVent"

The two entries point to the following links:

1) https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla6layers14APZInputBridge17ReceiveInputEventERNS_9InputDataEPNS0_19ScrollableLayerGuidEPm&redirect=false

2) https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla6layers14APZInputBridge17ReceiveInputEventERNS_9InputDataEPNS0_19ScrollableLayerGuidEPy&redirect=false

The lists of call sites at #1 and #2 are mostly the same, except that #2 also lists Mac-only call sites in nsChildView.mm, while #1 does not.

I'm guessing #1 is the call sites from the Linux indexing, and #2 is the call sites from the Mac indexing. It would be nice if the two were combined together.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

$ echo _ZN7mozilla6layers14APZInputBridge17ReceiveInputEventERNS_9InputDataEPNS0_19ScrollableLayerGuidEPm | c++filt
mozilla::layers::APZInputBridge::ReceiveInputEvent(mozilla::InputData&, mozilla::layers::ScrollableLayerGuid*, unsigned long*)
$ echo _ZN7mozilla6layers14APZInputBridge17ReceiveInputEventERNS_9InputDataEPNS0_19ScrollableLayerGuidEPy | c++filt
mozilla::layers::APZInputBridge::ReceiveInputEvent(mozilla::InputData&, mozilla::layers::ScrollableLayerGuid*, unsigned long long*)

I guess uint64_t is a long on Linux and a long long on Mac, so we get different mangled names for that function on the two platforms.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

So far I don't have any good ideas on how to fix this. Broadly speaking the options are:

1) somehow adjust the macOS build to match what Linux is doing
2) post-process the macOS analysis files after downloading them from taskcluster to match Linux. This can be done by:
2a) De-manngling the symbols in analysis files, substituting "long long" with "long" and re-mangling, or
2b) Walk through the linux analysis files, generate a map of "real signature" -> mangled symbol (where the "real signature" in this case would still have uint64_t) and then walk through the macOS analysis files and replace the mangled symbols with the equivalent Linux symbols

None of these options are particularly appealing to me.

Comment 3

[Andrew McCreight [:mccr8]]

Nathan, do you have any ideas off the top of your head how we could deal with mangled names being different between OSX and Linux?

Comment 4

[Botond Ballo [:botond]]

Could you use the "real signature" in the analysis files themselves?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Nathan, do you have any ideas off the top of your head how we could deal
> with mangled names being different between OSX and Linux?

My opinion of kats's suggestions:

1) No.
2a) You do not want to get involved in writing a mangler.  (I dunno, maybe it wouldn't be that bad, but it'd be Some Work.)
2b) I don't think you can consistently remap things to uint64_t.

One suggestion is to use cpp_demangle:

https://github.com/gimli-rs/cpp_demangle

to parse the names, and try to merge names that "look identical" up to small datatype differences.  Though it looks like the parsed Symbol datatype:

https://github.com/gimli-rs/cpp_demangle/blob/master/src/lib.rs#L98-L107

doesn't have any `pub` fields, so you'd have to either expose an API for Symbol equality in gimli itself (seems unlikely to be accepted upstream), or get the necessary data exposed to enable writing your own.

The other suggestion is to find symbols that "look identical" up to the m/y difference in comment 1, and say those are really the same symbol.  I think this is a riff on 2b, but it sounds easier to me.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> The other suggestion is to find symbols that "look identical" up to the m/y
> difference in comment 1, and say those are really the same symbol.

It's probably worth doing some investigation to see if there are other cases in which this happens, or if it's limited to uint64_t. I'll try and do that first, that will help inform the difficulty of potential solutions. If it's only uint64_t then yeah some sort of fuzzy match on symbols might be doable.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

I did a quick search and I'm seeing quite a number of differences, although most of them seem to be std::* stuffs. e.g.:

constructor std::pair::pair<_T1, _T2>
-> _ZNSt4pairC1EOSt4pairIT_T0_E on Linux
-> _ZNSt3__14pairC1EONS_4pairIT_T0_EE on Mac

However, after poking around some more I think the correct fix here is that when I unify the analysis files for Linux and Mac, if there are matching source lines, I should union the sym fields from those lines into a comma-separated list. According to the analysis.md [1] source lines can take a comma-separated list of symbols to search for, which I didn't realize before. Right now the linux and mac source lines will be separate, resulting in the duplicated context menu items, but if I combine them properly it should produce the desired behaviour.

I'll try to write a patch that does this.

[1] https://github.com/mozsearch/mozsearch/blob/master/docs/analysis.md#symbols

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I wrote the patch, and it seems to work. At least the issue in comment 0 now produces the desired behaviour. It's deployed to dev.searchfox.org - https://dev.searchfox.org/mozilla-central/source/gfx/layers/apz/public/APZInputBridge.h#68 is the link corresponding to comment 0.

Botond, can you try using this for a bit and see if you run into any other issues?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

As an aside, I discovered that this problem with the duplicate context menu items already happened in some situations without the macOS analysis. For example at https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp#891 clicking on the "Nothing()" produces a context menu with a couple of duplicate "search for constructor" entries. In this case the two constructors in the duplicate pair are actually different constructors (and therefore have different symbols) but it's confusing to the user because that's not apparent from the displayed "pretty" signature. My patch also collapses these context menu items such that clicking on the entry produces the combined results which may or may not be desired, but I think should be acceptable as a side-effect of the macOS fix.

Comment 10

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #8)
> Botond, can you try using this for a bit and see if you run into any other
> issues?

Looks pretty good to me!

The only other potentially interesting scenario I came across is the case of multiple definitions.

As an example, the XUL_LIB_FILE macro is defined differently for different platforms [1]. If you click on a use of it in cross-platform code, such as [2], there are two "Go to definition" entries, which take you to the Mac and Linux definitions, respectively.

I think that's pretty reasonable behaviour, since there are in fact two indexed definitions. It would be slightly more user-friendly to annotate the two entries with the platform name, but that's a fairly minor thing and may not be worth spending time on.

I did notice one other issue while playing around with this, but it seems to be unrelated to the Mac indexing, so I filed a separate bug for it (bug 1491719).

Thanks a lot for making Searchfox better! :)

[1] https://dev.searchfox.org/mozilla-central/source/dom/media/gmp/GMPChild.cpp#361
[2] https://dev.searchfox.org/mozilla-central/source/dom/media/gmp/GMPChild.cpp#516

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #9)
> As an aside, I discovered that this problem with the duplicate context menu
> items already happened in some situations without the macOS analysis. For
> example at
> https://searchfox.org/mozilla-central/source/gfx/layers/apz/src/
> APZCTreeManager.cpp#891 clicking on the "Nothing()" produces a context menu
> with a couple of duplicate "search for constructor" entries. In this case
> the two constructors in the duplicate pair are actually different
> constructors (and therefore have different symbols) but it's confusing to
> the user because that's not apparent from the displayed "pretty" signature.
> My patch also collapses these context menu items such that clicking on the
> entry produces the combined results which may or may not be desired, but I
> think should be acceptable as a side-effect of the macOS fix.

I agree that this is an acceptable side-effect of the fix.

A conceivable improvement would be to change the pretty names to include the constructor arguments, and keep them as different entries, but we can probably revisit that if use cases where differentiating the call sites of the two would be useful, actually come up.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #10)
> As an example, the XUL_LIB_FILE macro is defined differently for different
> platforms [1]. If you click on a use of it in cross-platform code, such as
> [2], there are two "Go to definition" entries, which take you to the Mac and
> Linux definitions, respectively.
> 
> I think that's pretty reasonable behaviour, since there are in fact two
> indexed definitions. It would be slightly more user-friendly to annotate the
> two entries with the platform name, but that's a fairly minor thing and may
> not be worth spending time on.

Ah, good to know. Yeah it would be better to annotate the entries with the platform name. I can file a follow-up to do this but it will be low priority.

(In reply to Botond Ballo [:botond] from comment #11)
> A conceivable improvement would be to change the pretty names to include the
> constructor arguments, and keep them as different entries, but we can
> probably revisit that if use cases where differentiating the call sites of
> the two would be useful, actually come up.

Agreed.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://github.com/mozsearch/mozsearch/pull/145
https://github.com/mozsearch/mozsearch-mozilla/pull/20

Filed bug 1491733 for the XUL_LIB_FILE class of problems.

Comment 14

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

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #12)
> (In reply to Botond Ballo [:botond] from comment #11)
> > A conceivable improvement would be to change the pretty names to include the
> > constructor arguments, and keep them as different entries, but we can
> > probably revisit that if use cases where differentiating the call sites of
> > the two would be useful, actually come up.
> 
> Agreed.

I think I might suggest we special-case for "move-constructor" and "copy-constructor", etc.  Because in this case, the Nothing() example is just "mozilla::Nothing::Nothing()" and "mozilla::Nothing::Nothing(mozilla::Nothing&&)", but there are plenty of constructors where the argument list is ridiculously long and then we start getting into HTML layout issues, not to mention the problem of having to manually diff what we're looking at if it doesn't get truncated.

That said, I think eventually it could make sense to do some kind of clever signature diffing so that the pretty signature becomes something like "...unsigned long*" and "...unsigned long long*" for the example from comment 1.  The heuristic would be a greedy approach so that given:
- Blah(uint32, nsIChannel, LoadFlag, uint32)
- Blah(uint32, nsIGlobalObject, LoadFlags, Puppy)
- Blah(uint32, nsIGlobalObject, LoadFlags, Cat)
we'd get signatures of:
- Blah(...nsIChannel...)
- Blah(...nsIGlobalObject...Puppy)
- Blah(...nsIGlobalObject...Cat)

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Andrew Sutherland [:asuth] from comment #14)
> (In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from
> comment #12)
> > (In reply to Botond Ballo [:botond] from comment #11)
> > > A conceivable improvement would be to change the pretty names to include the
> > > constructor arguments, and keep them as different entries, but we can
> > > probably revisit that if use cases where differentiating the call sites of
> > > the two would be useful, actually come up.
> > 
> > Agreed.
> 
> I think I might suggest we special-case for "move-constructor" and
> "copy-constructor", etc.  Because in this case, the Nothing() example is
> just "mozilla::Nothing::Nothing()" and
> "mozilla::Nothing::Nothing(mozilla::Nothing&&)", but there are plenty of
> constructors where the argument list is ridiculously long and then we start
> getting into HTML layout issues, not to mention the problem of having to
> manually diff what we're looking at if it doesn't get truncated.

Special-casing move/copy constructors is not a bad idea, and should be relatively easy to do.

> That said, I think eventually it could make sense to do some kind of clever
> signature diffing so that the pretty signature becomes something like
> "...unsigned long*" and "...unsigned long long*" for the example from
> comment 1.  The heuristic would be a greedy approach so that given:
> - Blah(uint32, nsIChannel, LoadFlag, uint32)
> - Blah(uint32, nsIGlobalObject, LoadFlags, Puppy)
> - Blah(uint32, nsIGlobalObject, LoadFlags, Cat)
> we'd get signatures of:
> - Blah(...nsIChannel...)
> - Blah(...nsIGlobalObject...Puppy)
> - Blah(...nsIGlobalObject...Cat)

I don't know if this is really necessary. In practice a call site resolves to one specific function and clang knows which one it is, so in the case of overloaded functions we should only be getting one context menu entry already. comment 1 is a special cause because of the macOS analysis merging which should be resolved with the patches I have. The other scenario (with the Maybe/Nothing constructors) I honestly don't know why we end up with multiple source symbols, but the better solution might be to figure that out and make sure only the right symbol gets picked.

Comment 16

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #15)
> The other scenario (with
> the Maybe/Nothing constructors) I honestly don't know why we end up with
> multiple source symbols, but the better solution might be to figure that out
> and make sure only the right symbol gets picked.

In the case of Nothing(), it may have to do with the fact that the default constructor and move constructor are both implicitly generated, and so there is no source location to associate with them other than the class definition.

Comment 17

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

(In reply to Kartikaya Gupta (email:[e-mail]) (parental leave) from comment #15)
> I don't know if this is really necessary.

Right, my examples don't matter for this specific bug.  I meant that since the "pretty" values are pre-computed in the indexing analysis path then changing the "pretty" in one place is changing it everywhere and I don't think the full signatures would be an improvement for that purpose.

This would also have interesting side-effects since the `identifiers` table is keyed based on the "pretty" value of the string and that's the key that the search results are binned by in router.py.  Arguably, "pretty" is a misnomer since it's really "normalized/truncated demangled symbol name".

I think my proposal is probably a bit fancy and would work better as a higher-level post-pass for the search results, since that's where I was really thinking of the differentiated pretty names being useful.  And that can largely be accomplished by augmenting the search results to indicate the underlying symbol they're about (and providing the full de-mangled version of the symbol, not the truncated de-mangled version).

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Deployed