[Clang] Refactor uses of `Cand->Function` in SemaOverload.cpp #98965

vortex73 · 2024-07-15T21:34:59Z

adds checks to called functions containing Cand->Function as an argument.
Assigned Cand->Function as a FunctionDecl* to enhance readablity.
Solves: valgrind error in function NoteFunctionCandidate, file SemaOverload.cpp line 11961 #98769 and Refactor uses of Cand->Function in SemaOverload.cpp #98942

github-actions · 2024-07-15T21:35:16Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-07-15T21:54:30Z

@llvm/pr-subscribers-clang

Author: Narayan (vortex73)

Changes

adds checks to called functions containing Cand->Function as an argument.
Assigned Cand->Function as a FunctionDecl* to enhance readablity.
Solves: #98769 and #98942

Full diff: https://github.com/llvm/llvm-project/pull/98965.diff

1 Files Affected:

(modified) clang/lib/Sema/SemaOverload.cpp (+23-13)

diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 074062ebbb594..b68e54b9caa5b 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -46,6 +46,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/Casting.h"
 #include <algorithm>
+#include <cassert>
 #include <cstddef>
 #include <cstdlib>
 #include <optional>
@@ -9994,8 +9995,9 @@ Sema::AddArgumentDependentLookupCandidates(DeclarationName Name,
                                    CandEnd = CandidateSet.end();
        Cand != CandEnd; ++Cand)
     if (Cand->Function) {
-      Fns.erase(Cand->Function);
-      if (FunctionTemplateDecl *FunTmpl = Cand->Function->getPrimaryTemplate())
+      FunctionDecl *CandFunc = Cand->Function;
+      Fns.erase(CandFunc);
+      if (FunctionTemplateDecl *FunTmpl = CandFunc->getPrimaryTemplate())
         Fns.erase(FunTmpl);
     }
 
@@ -11349,8 +11351,7 @@ static void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand,
     }
   }
 
-  if (TakingCandidateAddress &&
-      !checkAddressOfCandidateIsAvailable(S, Cand->Function))
+  if (TakingCandidateAddress && !checkAddressOfCandidateIsAvailable(S, Fn))
     return;
 
   // Emit the generic diagnostic and, optionally, add the hints to it.
@@ -11376,6 +11377,7 @@ static void DiagnoseBadConversion(Sema &S, OverloadCandidate *Cand,
 /// over a candidate in any candidate set.
 static bool CheckArityMismatch(Sema &S, OverloadCandidate *Cand,
                                unsigned NumArgs, bool IsAddressOf = false) {
+  assert(Cand->Function && "Candidate is required to be a function.");
   FunctionDecl *Fn = Cand->Function;
   unsigned MinParams = Fn->getMinRequiredExplicitArguments() +
                        ((IsAddressOf && !Fn->isStatic()) ? 1 : 0);
@@ -11466,8 +11468,10 @@ static void DiagnoseArityMismatch(Sema &S, NamedDecl *Found, Decl *D,
 /// Arity mismatch diagnosis specific to a function overload candidate.
 static void DiagnoseArityMismatch(Sema &S, OverloadCandidate *Cand,
                                   unsigned NumFormalArgs) {
+    assert(Cand->Function && "Candidate must be a function");
+    FunctionDecl *Fn = Cand->Function;
   if (!CheckArityMismatch(S, Cand, NumFormalArgs, Cand->TookAddressOfOverload))
-    DiagnoseArityMismatch(S, Cand->FoundDecl, Cand->Function, NumFormalArgs,
+    DiagnoseArityMismatch(S, Cand->FoundDecl, Fn, NumFormalArgs,
                           Cand->TookAddressOfOverload);
 }
 
@@ -11767,19 +11771,22 @@ static void DiagnoseBadDeduction(Sema &S, NamedDecl *Found, Decl *Templated,
 static void DiagnoseBadDeduction(Sema &S, OverloadCandidate *Cand,
                                  unsigned NumArgs,
                                  bool TakingCandidateAddress) {
+    assert(Cand->Function && "Candidate must be a function");
+    FunctionDecl *Fn = Cand->Function;
   TemplateDeductionResult TDK = Cand->DeductionFailure.getResult();
   if (TDK == TemplateDeductionResult::TooFewArguments ||
       TDK == TemplateDeductionResult::TooManyArguments) {
     if (CheckArityMismatch(S, Cand, NumArgs))
       return;
   }
-  DiagnoseBadDeduction(S, Cand->FoundDecl, Cand->Function, // pattern
+  DiagnoseBadDeduction(S, Cand->FoundDecl, Fn, // pattern
                        Cand->DeductionFailure, NumArgs, TakingCandidateAddress);
 }
 
 /// CUDA: diagnose an invalid call across targets.
 static void DiagnoseBadTarget(Sema &S, OverloadCandidate *Cand) {
   FunctionDecl *Caller = S.getCurFunctionDecl(/*AllowLambda=*/true);
+  assert(Cand->Function && "Candidate must be a Function.");
   FunctionDecl *Callee = Cand->Function;
 
   CUDAFunctionTarget CallerTarget = S.CUDA().IdentifyTarget(Caller),
@@ -11837,6 +11844,7 @@ static void DiagnoseBadTarget(Sema &S, OverloadCandidate *Cand) {
 }
 
 static void DiagnoseFailedEnableIfAttr(Sema &S, OverloadCandidate *Cand) {
+  assert(Cand->Function && "Candidate must be a function");
   FunctionDecl *Callee = Cand->Function;
   EnableIfAttr *Attr = static_cast<EnableIfAttr*>(Cand->DeductionFailure.Data);
 
@@ -11846,11 +11854,13 @@ static void DiagnoseFailedEnableIfAttr(Sema &S, OverloadCandidate *Cand) {
 }
 
 static void DiagnoseFailedExplicitSpec(Sema &S, OverloadCandidate *Cand) {
-  ExplicitSpecifier ES = ExplicitSpecifier::getFromDecl(Cand->Function);
+  assert(Cand->Function && "Candidate must be a function");
+  Function *CandFunc = Cand->Function;
+  ExplicitSpecifier ES = ExplicitSpecifier::getFromDecl(CandFunc);
   assert(ES.isExplicit() && "not an explicit candidate");
 
   unsigned Kind;
-  switch (Cand->Function->getDeclKind()) {
+  switch (CandFunc->getDeclKind()) {
   case Decl::Kind::CXXConstructor:
     Kind = 0;
     break;
@@ -11858,7 +11868,7 @@ static void DiagnoseFailedExplicitSpec(Sema &S, OverloadCandidate *Cand) {
     Kind = 1;
     break;
   case Decl::Kind::CXXDeductionGuide:
-    Kind = Cand->Function->isImplicit() ? 0 : 2;
+    Kind = CandFunc->isImplicit() ? 0 : 2;
     break;
   default:
     llvm_unreachable("invalid Decl");
@@ -11868,7 +11878,7 @@ static void DiagnoseFailedExplicitSpec(Sema &S, OverloadCandidate *Cand) {
   // (particularly an out-of-class definition) will typically lack the
   // 'explicit' specifier.
   // FIXME: This is probably a good thing to do for all 'candidate' notes.
-  FunctionDecl *First = Cand->Function->getFirstDecl();
+  FunctionDecl *First = CandFunc->getFirstDecl();
   if (FunctionDecl *Pattern = First->getTemplateInstantiationPattern())
     First = Pattern->getFirstDecl();
 
@@ -11937,6 +11947,7 @@ static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
                                   unsigned NumArgs,
                                   bool TakingCandidateAddress,
                                   LangAS CtorDestAS = LangAS::Default) {
+  assert(Cand->Function && "Candidate must be a function");
   FunctionDecl *Fn = Cand->Function;
   if (shouldSkipNotingLambdaConversionDecl(Fn))
     return;
@@ -11951,8 +11962,7 @@ static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
   // Skip implicit member functions when trying to resolve
   // the address of a an overload set for a function pointer.
   if (Cand->TookAddressOfOverload &&
-      !Cand->Function->hasCXXExplicitFunctionObjectParameter() &&
-      !Cand->Function->isStatic())
+      !Fn->hasCXXExplicitFunctionObjectParameter() && !Fn->isStatic())
     return;
 
   // Note deleted candidates, but only if they're viable.
@@ -12050,7 +12060,7 @@ static void NoteFunctionCandidate(Sema &S, OverloadCandidate *Cand,
     return;
 
   case ovl_fail_addr_not_available: {
-    bool Available = checkAddressOfCandidateIsAvailable(S, Cand->Function);
+    bool Available = checkAddressOfCandidateIsAvailable(S, Fn);
     (void)Available;
     assert(!Available);
     break;

vortex73 · 2024-07-16T19:03:56Z

@haberman Please review.

shafik

Thank you for this refactor, just a couple of minor comments but otherwise looks good. Please fix first and then merge after it passes tests, if you need someone to merge for you please let me know.

shafik · 2024-07-18T02:27:15Z

clang/lib/Sema/SemaOverload.cpp

@@ -11846,19 +11854,21 @@ static void DiagnoseFailedEnableIfAttr(Sema &S, OverloadCandidate *Cand) {
 }

 static void DiagnoseFailedExplicitSpec(Sema &S, OverloadCandidate *Cand) {
-  ExplicitSpecifier ES = ExplicitSpecifier::getFromDecl(Cand->Function);
+  assert(Cand->Function && "Candidate must be a function");
+  Function *CandFunc = Cand->Function;


We use Fn elsewhere, we should be consistent throughout the file.

clang/lib/Sema/SemaOverload.cpp

vortex73 · 2024-07-18T04:59:07Z

@shafik I've made the changes. Please merge as I do not have write access.

github-actions · 2024-07-19T02:49:45Z

✅ With the latest revision this PR passed the C/C++ code formatter.

vortex73 · 2024-07-24T05:38:15Z

@shafik are any changes required for this PR?

shafik

LGTM

github-actions · 2024-07-24T20:42:40Z

@vortex73 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Summary: - [ ] adds checks to called functions containing `Cand->Function` as an argument. - [ ] Assigned `Cand->Function` as a `FunctionDecl*` to enhance readablity. Solves: #98769 and #98942 Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250569

[Clang] Refactor uses of in SemaOverload.cpp

60a51d9

vortex73 added 3 commits July 16, 2024 03:16

[Clang] Refactor uses of Cand->Function in SemaOverload.cpp

8a4cea6

[Clang] Refactor uses of Cand->Function in SemaOverload.cpp

d48e7d3

Merge remote-tracking branch 'origin/sema' into sema

4d211cf

vortex73 marked this pull request as ready for review July 15, 2024 21:54

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 15, 2024

vortex73 changed the title ~~[Clang] Refactor uses of in SemaOverload.cpp~~ [Clang] Refactor uses of Cand->Function in SemaOverload.cpp Jul 15, 2024

shafik approved these changes Jul 18, 2024

View reviewed changes

shafik requested changes Jul 18, 2024

View reviewed changes

clang/lib/Sema/SemaOverload.cpp Outdated Show resolved Hide resolved

vortex73 added 2 commits July 18, 2024 10:09

renamed occurences of CandFunc to Fn

5e0c33c

renamed occurences of CandFunc to Fn

ef007ca

vortex73 requested a review from shafik July 18, 2024 05:10

renamed occurences of CandFunc to Fn

9b1df76

shafik approved these changes Jul 24, 2024

View reviewed changes

shafik merged commit 10ff2bc into llvm:main Jul 24, 2024
7 checks passed

This was referenced Oct 1, 2024

valgrind error in function NoteFunctionCandidate, file SemaOverload.cpp line 11961 #98769

Closed

Refactor uses of Cand->Function in SemaOverload.cpp #98942

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Refactor uses of `Cand->Function` in SemaOverload.cpp #98965

[Clang] Refactor uses of `Cand->Function` in SemaOverload.cpp #98965

Uh oh!

vortex73 commented Jul 15, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jul 15, 2024

Uh oh!

llvmbot commented Jul 15, 2024

Uh oh!

vortex73 commented Jul 16, 2024

Uh oh!

shafik left a comment

Uh oh!

shafik Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

vortex73 commented Jul 18, 2024

Uh oh!

github-actions bot commented Jul 19, 2024 •

edited

Loading

Uh oh!

vortex73 commented Jul 24, 2024

Uh oh!

shafik left a comment

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2024

Uh oh!

Uh oh!

[Clang] Refactor uses of Cand->Function in SemaOverload.cpp #98965

[Clang] Refactor uses of Cand->Function in SemaOverload.cpp #98965

Uh oh!

Conversation

vortex73 commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 15, 2024

Uh oh!

llvmbot commented Jul 15, 2024

Uh oh!

vortex73 commented Jul 16, 2024

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

shafik Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vortex73 commented Jul 18, 2024

Uh oh!

github-actions bot commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vortex73 commented Jul 24, 2024

Uh oh!

shafik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 24, 2024

Uh oh!

Uh oh!

[Clang] Refactor uses of `Cand->Function` in SemaOverload.cpp #98965

[Clang] Refactor uses of `Cand->Function` in SemaOverload.cpp #98965

vortex73 commented Jul 15, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading