This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting
ClosedPublic

Authored by nridge on Sep 28 2022, 1:00 PM.

Diff Detail

Event Timeline

nridge created this revision.Sep 28 2022, 1:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 1:00 PM
Herald added a subscriber: arphaman. · View Herald Transcript
nridge requested review of this revision.Sep 28 2022, 1:00 PM
nridge edited the summary of this revision. (Show Details)Sep 28 2022, 1:01 PM

Speculative fix for an infinite recursion with no testcase. I'm happy to circle back to this if/when we get a testcase, but I don't think there's any downside to this patch in the meantime.

sammccall accepted this revision.Sep 28 2022, 7:30 PM

Based on the stacktrace, I came up with this crashing example, maybe it's a usable testcase?

template <int> class X {
template <int> class Y {
    using Y<0>::xxx;
};
};

(not currently on a machine that can build clangd, so I haven't verified it's the same crash)

Another interesting indirect case

template <int> class X {
template <int> class Z;
template <int> class Y {
    using Z<0>::xxx;
};
template <int> class Z {
    using Y<0>::xxx;
};
};
clang-tools-extra/clangd/SemanticHighlighting.cpp
143

This looks good+safe.

I wonder if the check should be moved inside resolveUsingValueDecl - having it resolve to itself seems unhelpful always.

This revision is now accepted and ready to land.Sep 28 2022, 7:30 PM

Thanks for the testcases!

The indirect one is particularly interesting, and suggests that we may want a "seen decls" set (similar to SeenTemplates here) in the kindForDecl() recursion?

(I'd move the check into resolveUsingValueDecl(), but that goes against the grain of putting the more thorough check in the caller -- or should we do both?)

nridge requested review of this revision.Oct 30 2022, 12:56 AM

(I'm going to re-request review since I have an outstanding question about the fix approach.)

nridge updated this revision to Diff 506359.Mar 19 2023, 1:24 AM

add test case

nridge added inline comments.Mar 19 2023, 1:27 AM
clang-tools-extra/clangd/SemanticHighlighting.cpp
143

I wonder if the check should be moved inside resolveUsingValueDecl - having it resolve to itself seems unhelpful always.

I haven't done this because, as mentioned earlier, I think a fix for the mutually-recursive case will need to build on logic here rather than inside resolveUsingValueDecl.

I will go ahead and merge this partial fix because it is an improvement over the current state.

@sammccall I'm still curious to know your thoughts on the approach for fixing the mutually-recursive case. Is there something more general than adding a recursion guard like this to semantic highlighting code that I'm overlooking?

we may want a "seen decls" set (similar to SeenTemplates here) in the kindForDecl() recursion

This revision was not accepted when it landed; it landed in state Needs Review.Mar 19 2023, 1:31 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Mar 19 2023, 1:33 AM

This breaks tests: http://45.33.8.238/linux/102084/step_9.txt

Please take a look and revert for now if it takes a while to fix.

Did you not run tests before commiting?

My apologies. I did run tests locally but on a slightly older tree. I'll post a fix momentarily.