Skip to content

Add BestOverloadFunctionMatch & IsFunction Remove BestTemplateFunctionMatch #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Mar 9, 2025
Merged

Add BestOverloadFunctionMatch & IsFunction Remove BestTemplateFunctionMatch #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
edd1617
Add `BestOverloadFunctionMatch` & `IsFunction` Remove `BestTemplateFu…
Vipul-Cariappa Mar 5, 2025
cc75f31
make cling happy
Vipul-Cariappa Mar 6, 2025
63917bd
make codecov happy
Vipul-Cariappa Mar 6, 2025
9e5f2f7
skip using specilised function for methods
Vipul-Cariappa Mar 6, 2025
d3d913e
adapted from `Sema::BuildCallExpr`
Vipul-Cariappa Mar 7, 2025
097603a
fix Expr memory ownership
Vipul-Cariappa Mar 8, 2025
0a54e71
fix cmake to accept variable length array
Vipul-Cariappa Mar 8, 2025
323b5fc
brief memory leak at clang
Vipul-Cariappa Mar 8, 2025
2a5c211
fix cmake for windows (-Wno-vla)
Vipul-Cariappa Mar 9, 2025
2b81002
avoid variable length array
Vipul-Cariappa Mar 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions include/clang/Interpreter/CppInterOp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ namespace Cpp {
/// Checks if the scope is a class or not.
CPPINTEROP_API bool IsClass(TCppScope_t scope);

/// Checks if the scope is a function.
CPPINTEROP_API bool IsFunction(TCppScope_t scope);

/// Checks if the type is a function pointer.
CPPINTEROP_API bool IsFunctionPointerType(TCppType_t type);

Expand Down Expand Up @@ -706,17 +709,16 @@ namespace Cpp {
CPPINTEROP_API TCppFunction_t
InstantiateTemplateFunctionFromString(const char* function_template);

/// Finds best template match based on explicit template parameters and
/// argument types
/// Finds best overload match based on explicit template parameters (if any)
/// and argument types.
///
///\param[in] candidates - Vector of suitable candidates that come under the
/// parent scope and have the same name (obtained using
/// GetClassTemplatedMethods)
///\param[in] candidates - vector of overloads that come under the
/// parent scope and have the same name
///\param[in] explicit_types - set of expicitly instantiated template types
///\param[in] arg_types - set of argument types
///\returns Instantiated function pointer
CPPINTEROP_API TCppFunction_t
BestTemplateFunctionMatch(const std::vector<TCppFunction_t>& candidates,
BestOverloadFunctionMatch(const std::vector<TCppFunction_t>& candidates,
const std::vector<TemplateArgInfo>& explicit_types,
const std::vector<TemplateArgInfo>& arg_types);

Expand Down
154 changes: 105 additions & 49 deletions lib/Interpreter/CppInterOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,29 @@

#include "clang/AST/CXXInheritance.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclAccessPair.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclarationName.h"
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/GlobalDecl.h"
#include "clang/AST/Mangle.h"
#include "clang/AST/NestedNameSpecifier.h"
#include "clang/AST/QualTypeNames.h"
#include "clang/AST/RecordLayout.h"
#include "clang/AST/Stmt.h"
#include "clang/AST/Type.h"
#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/Linkage.h"
#include "clang/Basic/OperatorKinds.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/Specifiers.h"
#include "clang/Basic/Version.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Sema/Lookup.h"
#include "clang/Sema/Overload.h"
#include "clang/Sema/Ownership.h"
#include "clang/Sema/Sema.h"
#if CLANG_VERSION_MAJOR >= 19
#include "clang/Sema/Redeclaration.h"
Expand Down Expand Up @@ -210,6 +223,11 @@ namespace Cpp {
return isa<CXXRecordDecl>(D);
}

bool IsFunction(TCppScope_t scope) {
Decl* D = static_cast<Decl*>(scope);
return isa<FunctionDecl>(D);
}

bool IsFunctionPointerType(TCppType_t type) {
QualType QT = QualType::getFromOpaquePtr(type);
return QT->isFunctionPointerType();
Expand Down Expand Up @@ -877,8 +895,19 @@ namespace Cpp {
TCppType_t GetFunctionReturnType(TCppFunction_t func)
{
auto *D = (clang::Decl *) func;
if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D))
return FD->getReturnType().getAsOpaquePtr();
if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionDecl>(D)) {
QualType Type = FD->getReturnType();
if (Type->isUndeducedAutoType() && IsTemplatedFunction(FD) &&
!FD->isDefined()) {
#ifdef CPPINTEROP_USE_CLING
cling::Interpreter::PushTransactionRAII RAII(&getInterp());
#endif
getSema().InstantiateFunctionDefinition(SourceLocation(), FD, true,
true);
Type = FD->getReturnType();
}
return Type.getAsOpaquePtr();
}

if (auto* FD = llvm::dyn_cast_or_null<clang::FunctionTemplateDecl>(D))
return (FD->getTemplatedDecl())->getReturnType().getAsOpaquePtr();
Expand Down Expand Up @@ -1026,62 +1055,89 @@ namespace Cpp {
funcs.push_back(Found);
}

// Adapted from inner workings of Sema::BuildCallExpr
TCppFunction_t
BestTemplateFunctionMatch(const std::vector<TCppFunction_t>& candidates,
BestOverloadFunctionMatch(const std::vector<TCppFunction_t>& candidates,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function feels very similar to Sema::BuildCallExpr can we use that instead and unwrap the result from the call expression? I think the cost would be an AST node allocation per call but we currently do that anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will recommend against it. We will need to create a C++ function call expression for it to work.
I did adapt this function from Sema::BuildCallExpr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will but we do that anyway. If you prefer not to do it, then write a comment that this is copied from there. FWIW, that did not seem the case when I looked...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another note is that we should check the memory growth when we call this function N times. Ideally it should be 0 for failed overload resolution candidate set. In reality it should be close to 0...

const std::vector<TemplateArgInfo>& explicit_types,
const std::vector<TemplateArgInfo>& arg_types) {
auto& S = getSema();
auto& C = S.getASTContext();

for (const auto& candidate : candidates) {
auto* TFD = (FunctionTemplateDecl*)candidate;
clang::TemplateParameterList* tpl = TFD->getTemplateParameters();
#ifdef CPPINTEROP_USE_CLING
cling::Interpreter::PushTransactionRAII RAII(&getInterp());
#endif

// template parameter size does not match
if (tpl->size() < explicit_types.size())
continue;
// The overload resolution interfaces in Sema require a list of expressions.
// However, unlike handwritten C++, we do not always have a expression.
// Here we synthesize a placeholder expression to be able to use
// Sema::AddOverloadCandidate. Made up expressions are fine because the
// interface uses the list size and the expression types.
struct WrapperExpr : public OpaqueValueExpr {
WrapperExpr() : OpaqueValueExpr(clang::Stmt::EmptyShell()) {}
};
auto* Exprs = new WrapperExpr[arg_types.size()];
llvm::SmallVector<Expr*> Args;
Args.reserve(arg_types.size());
size_t idx = 0;
for (auto i : arg_types) {
QualType Type = QualType::getFromOpaquePtr(i.m_Type);
ExprValueKind ExprKind = ExprValueKind::VK_PRValue;
if (Type->isReferenceType())
ExprKind = ExprValueKind::VK_LValue;

new (&Exprs[idx]) OpaqueValueExpr(SourceLocation::getFromRawEncoding(1),
Type.getNonReferenceType(), ExprKind);
Args.push_back(&Exprs[idx]);
++idx;
}

// right now uninstantiated functions give template typenames instead of
// actual types. We make this match solely based on count
// Create a list of template arguments.
llvm::SmallVector<TemplateArgument> TemplateArgs;
TemplateArgs.reserve(explicit_types.size());
for (auto explicit_type : explicit_types) {
QualType ArgTy = QualType::getFromOpaquePtr(explicit_type.m_Type);
if (explicit_type.m_IntegralValue) {
// We have a non-type template parameter. Create an integral value from
// the string representation.
auto Res = llvm::APSInt(explicit_type.m_IntegralValue);
Res = Res.extOrTrunc(C.getIntWidth(ArgTy));
TemplateArgs.push_back(TemplateArgument(C, Res, ArgTy));
} else {
TemplateArgs.push_back(ArgTy);
}
}

const FunctionDecl* func = TFD->getTemplatedDecl();
TemplateArgumentListInfo ExplicitTemplateArgs{};
for (auto TA : TemplateArgs)
ExplicitTemplateArgs.addArgument(
S.getTrivialTemplateArgumentLoc(TA, QualType(), SourceLocation()));

OverloadCandidateSet Overloads(
SourceLocation(), OverloadCandidateSet::CandidateSetKind::CSK_Normal);

for (void* i : candidates) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using Sema::AddFunctionCandidates instead of this loop?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will still require a loop to populate UnresolvedSetImpl from candidates. Also, when trying it locally, the following assertion fails https://github.com/llvm/llvm-project/blob/release/19.x/clang/lib/Sema/SemaOverload.cpp#L5692 for one of the test cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that assertion is coming from an interface on our end that needs fixing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will still require a loop to populate UnresolvedSetImpl from candidates.

It seems to have an append operator taking a range...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to have an append operator taking a range...

The iterator is a UnresolvedSetIterator(reference: https://clang.llvm.org/doxygen/classclang_1_1UnresolvedSetImpl.html#ad1e4dde3cd343da5f9918eb0caedd0a7)

Maybe that assertion is coming from an interface on our end that needs fixing?

I have been debugging this. I am not able to detect the cause. Our interfaces are being used correctly, as far as I can see.

Decl* D = static_cast<Decl*>(i);
if (auto* FD = dyn_cast<FunctionDecl>(D)) {
S.AddOverloadCandidate(FD, DeclAccessPair::make(FD, FD->getAccess()),
Args, Overloads);
} else if (auto* FTD = dyn_cast<FunctionTemplateDecl>(D)) {
// AddTemplateOverloadCandidate is causing a memory leak
// It is a known bug at clang
// call stack: AddTemplateOverloadCandidate -> MakeDeductionFailureInfo
// source:
// https://github.com/llvm/llvm-project/blob/release/19.x/clang/lib/Sema/SemaOverload.cpp#L731-L756
S.AddTemplateOverloadCandidate(
FTD, DeclAccessPair::make(FTD, FTD->getAccess()),
&ExplicitTemplateArgs, Args, Overloads);
}
}

#ifdef CPPINTEROP_USE_CLING
if (func->getNumParams() > arg_types.size())
continue;
#else // CLANG_REPL
if (func->getMinRequiredArguments() > arg_types.size())
continue;
#endif
OverloadCandidateSet::iterator Best;
Overloads.BestViableFunction(S, SourceLocation(), Best);

// TODO(aaronj0) : first score based on the type similarity before forcing
// instantiation.

TCppFunction_t instantiated =
InstantiateTemplate(candidate, arg_types.data(), arg_types.size());
if (instantiated)
return instantiated;

// Force the instantiation with template params in case of no args
// maybe steer instantiation better with arg set returned from
// TemplateProxy?
instantiated = InstantiateTemplate(candidate, explicit_types.data(),
explicit_types.size());
if (instantiated)
return instantiated;

// join explicit and arg_types
std::vector<TemplateArgInfo> total_arg_set;
total_arg_set.reserve(explicit_types.size() + arg_types.size());
total_arg_set.insert(total_arg_set.end(), explicit_types.begin(),
explicit_types.end());
total_arg_set.insert(total_arg_set.end(), arg_types.begin(),
arg_types.end());

instantiated = InstantiateTemplate(candidate, total_arg_set.data(),
total_arg_set.size());
if (instantiated)
return instantiated;
}
return nullptr;
FunctionDecl* Result = Best != Overloads.end() ? Best->Function : nullptr;
delete[] Exprs;
return Result;
}

// Gets the AccessSpecifier of the function and checks if it is equal to
Expand Down
Loading
Loading