986522 - Jump location for NS_NewHTMLScrollFrame finds proto, not def

Status: RESOLVED FIXED

(Webtools Graveyard :: DXR, defect)

Description

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

Navigate to the NS_NewHTMLScrollFrame function (as of this writing, http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#68) and find the callers. This results in the query:

http://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22NS_NewHTMLScrollFrame%28class+nsIPresShell+*%2C+class+nsStyleContext+*%2C+_Bool%29%22

which shows no results. However doing a substring search for NS_NewHTMLScrollFrame does show a call site at nsCSSFrameConstructor.cpp:4260.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is there anything we can do to get bugs like this fixed? DXR is really nice when it works, but it's pretty common for me that it returns no results. It makes it hard to trust, so I usually just use MXR.

Comment 2

[Botond Ballo [:botond]]

Eclipse CDT finds the NS_NewHTMLScrollFrame call site fine :)

Comment 3

[Erik Rose [:erik][:erikrose]]

That's funny; it doesn't look like it's even recognized as a function at https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#4412. It doesn't get a context menu or anything. I'm going to do a round of these clang analysis bugs one of these days, probably in Q3, after Q1-2's port to elasticsearch (which will send performance through the roof, support multiple languages, let us index many trees (and in parallel) without one build failure scuttling the rest, and do line-based searching rather than the weird file/line hybrid of the current version) lands. I only just recently got paid help on the project, so I apologize for the singlethreadedness up to this point. I will get to it eventually!

The code at fault is almost certainly https://github.com/mozilla/dxr/blob/es/dxr/plugins/clang/dxr-index.cpp. It's...venerable, you might say, and somewhat challenging to debug because it runs inside the clang compiler. We have pretty good dev docs, in case any of you feel like a midnight hack: http://dxr.readthedocs.org/en/es/development.html.

Actually, if anyone can get this to happen in a manageable test case, I can almost certainly fix it fast. Waiting 4 hours for moz-central builds between attempts is what drags these things out; the fixing isn't nearly so bad. You can pretty much follow the setup instructions linked above, duplicate the test_basic folder, and edit it until you get the failure. We're on #static all day and happy to help. Otherwise, I can assure you we'll get to it!

Comment 4

[Erik Rose [:erik][:erikrose]]

For that matter, there's a chance this could even be fixed on the "es" branch; I'll be sure to check next time I do a build. I have done significant cleanups to the clang plugin there.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: dxr-bug

Well, here's a patch with a test that shows the problem.

Do you have any idea when the es branch will be merged? I'd like to contribute to the project, but it makes me leery when there are so many unmerged changes.

Comment 6

[Erik Rose [:erik][:erikrose]]

We're working on staging the es branch on dxr.allizom.org now (though it's not there yet). We want to let that bake in for a week or 2, and fix a few niggling little blockers (see bug 1127125), and then we can put it live. Ordinarily we don't do such long-running branches--on master, we do CD--but this one changed many assumptions throughout the system.

Thanks for the test case. That was right quick! I'll have a look soon.

Comment 7

[Erik Rose [:erik][:erikrose]]

Comment on attachment 8573917 [details] [diff] [review]
dxr-bug

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

Hi, Bill. I ran make and dxr-serve and poked around, but I don't see anything that surprises me. I can click NS_NewHTMLVideoFrame in test2.cpp and search for its callers, and I find the one in f(). It's not great that the NS_NewHTMLVideoFrame prototype in test.cpp isn't context-clickable, but that's not what we were chasing, was it? Can you point out what you expected?

Thanks!

Comment 8

[Erik Rose [:erik][:erikrose]]

Comment on attachment 8573917 [details] [diff] [review]
dxr-bug

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

Wait, did you apply this against master or against the es branch? I did the latter.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I was using master. Maybe the issue is fixed.

Comment 10

[Erik Rose [:erik][:erikrose]]

That's my hope. I did go through the clang plugin and remove/change a lot of things that made no sense. So the behavior I saw sounds good?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Actually, I managed to test on the es branch and it's still not working. If I click on the NS_NewHTMLVideoFrame call in f(), the "Jump to definition" link takes me to the prototype declaration in the same file (test.cpp#6). I want it to go to the actual definition of the function in test2.cpp.

Comment 12

[Erik Rose [:erik][:erikrose]]

Hmm, yes, that's kind of dumb. Thanks for the repro.

Comment 13

[Erik Rose [:erik][:erikrose]]

I spent the day digging into this. These notes are mostly for me, so I can pick it up again, but you might find them entertaining, and any opinion you do have is welcome.

The line number for the Jump To Definition menu item comes out of the defloc item of this CSV line:

ref,defloc,"test.cpp:6:0",loc,"test.cpp:10:4",locend,"test.cpp:10:24",kind,"function",name,"NS_NewHTMLVideoFrame",qualname,"NS_NewHTMLVideoFrame(class nsIPresShell *, class nsStyleContext *)"

That line is emitted by the printReference() call in VisitDeclRefExpr(). I tried to do the efficient thing, finding the definition within the compiler plugin, but, apparently, the compiler isn't aware where the definition is at the time, since the proto isn't pulled into the translation unit by means of any #includes: calls to fd->isDefined() and fs->hasBody() both return false. So we're stuck doing the inefficient thing: we'll need to build a big lookup table of function (and other?) qualnames to definition locations and consult that whenever we find a function ref in a CSV. Building that table will be a new and time-consuming step in the global analysis pass.

Faster, less RAM-hungry, easier, and more durable (less likely to yield broken links, that is) would be to rejigger the Jump To Definition menu item to do a dynamic search, something along the lines of +function:"NS_NewHTMLVideoFrame(class nsIPresShell *, class nsStyleContext *)". Since it would return at most one result, direct search should happily whisk the user directly to the definition. This has basically nothing but advantages and so is the way I'm leaning right now.

Comment 14

[Erik Rose [:erik][:erikrose]]

Did I say the proto isn't pulled into the translation unit? I meant the definition.

Comment 18

[Tom Klein]

Fixed by https://github.com/mozilla/dxr/commit/e54343a1bfb56988657d4a525edc0e2fe402128a