Segmentation fault in clangd after a638648

[Trass3r]

@upsj since a638648 I get a segmentation fault on some code, I reduced it to:

#include <string>
#include <memory>

struct Foo
{
    Foo(std::string s, void* p);
};

void foo()
{
    std::make_unique<Foo>(std::string{}, nullptr);
}

// edit: it's actually in isExpandedFromParameterPack, optimizations...
#0  chooseParameterNames () at ../clang-tools-extra/clangd/InlayHints.cpp:602
#1  processCall () at ../clang-tools-extra/clangd/InlayHints.cpp:440
#2  0x0000555557341289 in VisitCallExpr () at ../clang-tools-extra/clangd/InlayHints.cpp:296
#3  WalkUpFromCallExpr () at tools/clang/include/clang/AST/StmtNodes.inc:931
#4  0x0000555557340ffc in TraverseCallExpr () at ../clang/include/clang/AST/RecursiveASTVisitor.h:2700
#5  0x000055555732e0be in dataTraverseNode () at tools/clang/include/clang/AST/StmtNodes.inc:931
#6  TraverseStmt () at ../clang/include/clang/AST/RecursiveASTVisitor.h:700

Is anyone able to confirm?

[upsj]

I'll take a look at it, thanks for the report!

[upsj]

Looks like we're matching the wrong expression here, probably due to the fact that passing s by value invokes a copy-constructor. But it looks like there are additional incorrect assumptions in ForwardingCallVisitor/resolveForwardingParameters that should be checked. I'll take a look at that later today

[kadircet]

AST looks quite weird in this case to be honest:

CXXConstructorDecl 0x4bc94c8 </usr/local/google/home/kadircet/repos/tmp/a.cc:6:5, col:31> col:5 used Foo 'void (std::string, void *)'
|-ParmVarDecl 0x4bc92c8 <col:9, col:21> col:21 s 'std::string':'std::basic_string<char>' destroyed
`-ParmVarDecl 0x4bc9348 <col:24, col:30> col:30 p 'void *'
CXXBindTemporaryExpr 0x4b64e18 'std::string':'class std::basic_string<char>' (CXXTemporary 0x4b64e18)
`-CXXConstructExpr 0x4b64de0 'std::string':'class std::basic_string<char>' 'void (class std::basic_string<char> &&) noexcept'
  `-CallExpr 0x43f1ed0 'class std::basic_string<char>':'class std::basic_string<char>' xvalue
    |-ImplicitCastExpr 0x43f1eb8 'class std::basic_string<char> &&(*)(typename std::remove_reference<class basic_string<char> >::type &) noexcept' <BuiltinFnToFnPtr>
    | `-DeclRefExpr 0x43f1df8 '<builtin fn type>' Function 0x43f19d0 'forward' 'class std::basic_string<char> &&(typename std::remove_reference<class basic_string<char> >::type &) noexcept' (FunctionTemplate 0x51d1f90 'forward')
    `-DeclRefExpr 0x43f11a8 'class std::basic_string<char>':'class std::basic_string<char>' lvalue ParmVar 0x55f3288 '__args' 'class std::basic_string<char> &&'
ImplicitCastExpr 0x4b64e38 'void *' <NullToPointer>
`-CallExpr 0x43f2d20 'std::nullptr_t':'std::nullptr_t' xvalue
  |-ImplicitCastExpr 0x43f2d08 'std::nullptr_t &&(*)(typename std::remove_reference<std::nullptr_t>::type &) noexcept' <BuiltinFnToFnPtr>
  | `-DeclRefExpr 0x43f2c48 '<builtin fn type>' Function 0x43f2820 'forward' 'std::nullptr_t &&(typename std::remove_reference<std::nullptr_t>::type &) noexcept' (FunctionTemplate 0x51d1f90 'forward')
  `-DeclRefExpr 0x43f2008 'std::nullptr_t':'std::nullptr_t' lvalue ParmVar 0x45acf20 '__args' 'std::nullptr_t &&'

call to std::forward happens inside the copy constructor. I'd expect it to be other way around, as we're wrapping the constructor call inside forward (i.e std::forward(std::string{}, nullptr)). i think that's the reason why arg-pack forwarding detection logic can't find it and moreover picks the wrong function decl (constructor for std::string).

here's a self contained reproducer:

struct Foo {};
void foo(Foo s, int x);

template <typename... _Args>
void bar(_Args ...__args) {
  foo(__args...);
}

void foo() {
  bar(Foo{}, 3);
}

[upsj]

Yes, my assumption in the code was that a pack expansion expression is always expanded completely, i.e. after I found the start of the expanded pack, I know what (and how many) the subsequent arguments are. Adding such a check (checking the last entry of the pack as well as the first entry?) seems like a good approach to prevent this. Any idea how the AST came to be? This actually makes a lot of sense to me, since std::forward's template type is determined by the input, not the location it will be used in. Might make sense to add a special case to deal with single-parameter constructors on top of just implicit conversions here?

[upsj]

Should be fixed by https://reviews.llvm.org/D130259