This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Make forwarding parameter detection logic resilient
ClosedPublic

Authored by kadircet on Jul 21 2022, 5:49 AM.

Details

Summary

This could crash when our heuristic picks the wrong function. Make sure
there is enough parameters in the candidate to prevent those crashes.

Also special case copy/move constructors to make the heuristic work in
presence of those.

Fixes https://github.com/llvm/llvm-project/issues/56620

Diff Detail

Event Timeline

kadircet created this revision.Jul 21 2022, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 5:49 AM
kadircet requested review of this revision.Jul 21 2022, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 5:49 AM
upsj added a subscriber: upsj.Jul 22 2022, 2:22 AM

Your approach to handling implicit conversions is much nicer than mine, so I abandoned my revision. There is still one case (that might even occur in the code base I'm working on?) that this change would lead to incorrect hints on. WDYT about keeping the other changes, but using my pack detection logic?

clang-tools-extra/clangd/AST.cpp
791

This is not a sufficient check, since the other parameters need not be from an expanded pack.
Example:

void foo(int a, int b);
int baz(int x, int y);
template <typename... Args>
void bar(Args... args) {
  foo(baz(args, 1)...);
}

void foo() {
  bar(1, 42);
}

Here we shouldn't print a hint at all, but we print x: and y:
The important distinction is between Expr(args...) and Expr(args)..., which can be decided in the instantiated case by the check I implemented in https://reviews.llvm.org/D130259 for all cases, except when args only consists of a single element.

kadircet updated this revision to Diff 446759.Jul 22 2022, 2:54 AM
  • Check to ensure function call receives all the arguments
kadircet updated this revision to Diff 446763.Jul 22 2022, 2:59 AM
  • Add OOB check as an asssertion.
kadircet added inline comments.Jul 22 2022, 3:03 AM
clang-tools-extra/clangd/AST.cpp
791

yeah that makes sense, added an extra check.

kadircet marked an inline comment as done.Jul 22 2022, 3:11 AM
upsj accepted this revision.Jul 22 2022, 3:11 AM

Not sure I qualify as a suitable reviewer, but this looks really good to me, save for maybe a small safety measure :)

clang-tools-extra/clangd/AST.cpp
836–840

I think it would be safer to check this explicitly, since advancing the iterator past its end might be UB (depending on the implementation).

This revision is now accepted and ready to land.Jul 22 2022, 3:11 AM
kadircet updated this revision to Diff 446769.Jul 22 2022, 3:34 AM
kadircet marked an inline comment as done.
  • Rather than asserting limit the traversal
  • Have more comments
ilya-biryukov added inline comments.Jul 22 2022, 3:37 AM
clang-tools-extra/clangd/AST.cpp
836

This could crash in foo(1, args...), right?

889

NIT: should fit in a single line

kadircet updated this revision to Diff 446770.Jul 22 2022, 3:37 AM
  • Update test case

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

clang-tools-extra/clangd/AST.cpp
835

The essence of this test seems to be "if we can't rule it out by counting parameters, it must be right" which... doesn't seem very robust? Am I missing something?

AIUI the idea here is that
A) foo(pack...) is "expanded all the way" into consecutive arguments
B) foo(pack)... is not considered forwarding for our purposes
C) we want to treat foo(bar(pack)...) specially if bar is "just a cast" like move or forward

The difference between cases A and B aren't about counting arguments though, they're about what operations occur intercede between pack and the ... operator. (i.e. foo((&pack)...) is fine for the same reason forward is.

This seems like something best handled by:

  • finding the PackExpansionExpr controlling pack in the non-instantiated template AST
  • walking down to try to find pack
  • continue as we walk through e.g. parens or std::forward
  • bail out if we walk through something unknown (e.g. another function call)

Does this seem sensible/feasible?
(Walking up from pack is nicer but the AST doesn't support walking up well...)

836–840

Right, this is definitely UB as written.

FWIW I find this easier to understand in classic bounds-check form:

if ((It - Args.begin()) + Parameters.size() >= Args.size())

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
1468

(Just so I understand: these are getting hinted due to the "only instantiation" logic, right?)

1471

could reasonably expect this to be bar(a:1, 2) or bar(a:1, a:2) - leave a comment explaining why?

upsj added inline comments.Jul 22 2022, 4:00 AM
clang-tools-extra/clangd/AST.cpp
835

This goes a bit more generally into the question how to approach parameter packs across clangd. For inlay hints and hover, we are already looking at instantiated templates (is that true in all cases? Haven't dug in yet), so implementing forwarding there was straightforward. For signature help and code completion, we'd probably need to go via the uninstantiated template (and probably lose a bit of type information on the way, e.g. reference vs. value, l-value vs. r-value). If we wanted a generic way to deal with forwarding, the instantiation pattern would be my first choice, but I didn't have the time to go down this rabbit hole yet.
Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.

On the heuristic itself: Is there a way in C++ to "partially" unpack a parameter pack without this showing up in the AST? I am not aware of any, which is why the test "do the first and last parameters appear in a matching range of the arguments" seems sufficient and necessary in all but single-entry parameter packs.

ilya-biryukov added inline comments.Jul 22 2022, 4:18 AM
clang-tools-extra/clangd/AST.cpp
829–832

NIT: you could use Args.end() - Parameters.size() or std::advance equivalent.

sammccall added inline comments.Jul 22 2022, 5:03 AM
clang-tools-extra/clangd/AST.cpp
835

Unless there is a nice way to map between template and instantiation AST, I don't think combining them is feasible.

Fair enough, I don't see one.

On the heuristic itself: Is there a way in C++ to "partially" unpack a parameter pack without this showing up in the AST?

Not AFAIK, the is either expanded at "this level" yielding sibling args, or at "a higher level" yielding a single arg.
So maybe the "first & last" test is actually robust, but it would need some justification in comments.

There's an exception I think, if the pack size is one it falls over:

bar(int bar0);
bar(int bar0, int bar1);
bar(int bar0i, int bar1, int bar2);

template <class...Args>
void foo(Args...args) {
  bar(args...); // should yield labels when expanded
  bar(args)...; // should not yield labels when expanded
}
// but when there's one arg (foo<int>) the two have the same AST

I think it's OK to just get this one wrong

kadircet updated this revision to Diff 446796.Jul 22 2022, 5:19 AM
kadircet marked 5 inline comments as done.
  • Address comments

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

The fix is not an heuristic. We just make sure we never consider a function with less parameters than the arguments we have, that way rest of the assumptions hold (we might still pick a wrong function call, as we could in the past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of copy/move constructors.

clang-tools-extra/clangd/AST.cpp
835

i agree that we can do things differently here, but with this patch i am mostly trying to make sure we address the resiliency so that this doesn't bite us in production (and a couple of easy improvements to the existing cases for common code patterns).

i'd land this patch as-is and discuss doing these changes in a separate issue if possible.

836–840

i didn't do an explicit check, because we already have that explicit check inside the call site and doing a possibly linear operation for each argument in production setup doesn't feel nice

clang-tools-extra/clangd/unittests/InlayHintTests.cpp
1468

not only instantiation but "only overload" logic (i suppose that's what you mean?)

ilya-biryukov accepted this revision.Jul 22 2022, 5:29 AM

LGTM

clang-tools-extra/clangd/AST.cpp
830

NIT: could be shorter with llvm::size(Args)

driveby thoughts, but please don't block on them.

(if this fix is a heuristic that fixes a common crash but isn't completely correct, that may still be worth landing but warrants a fixme)

The fix is not an heuristic. We just make sure we never consider a function with less parameters than the arguments we have, that way rest of the assumptions hold (we might still pick a wrong function call, as we could in the past, but we won't crash).
In addition to that heuristics are improved a little bit to work in presence of copy/move constructors.

OK, but it's missing a comment that says "this is a heuristic and may not be correct, at least it won't crash"!

The original code was intended to be *correct*, not a heuristic. If we're not going to fix it properly (which is reasonable in the circumstances), we need to document the limitations.

nridge added a subscriber: nridge.Jul 22 2022, 4:08 PM