Check i < FD->getNumParams() before querying

[Violet]

Summary

As was already stated in a previous comment, the parameter isn't
necessarily referring to one of the DeclContext's parameter. We
should check the index is within the range to avoid out-of-boundary
access.

[Violet]

Wrong bug number

[Violet]

a more straightforward testcase

[Violet]

PING ...

[lebedev.ri]

clang/test/SemaCXX/PR41139.cpp

4

Add a comment what this test does?
('just checking that the crash does not happen again')

clang/test/SemaCXX/cxx1y-generic-lambdas.cpp

946

Does this have to be in that namespace?
Can you put it into a new namespace referencing this review?

[Violet]

Address reviewer comment

[lebedev.ri]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2895

Have you analyzed how we get here?
Maybe the caller code is faulty?

[Violet]

clang/lib/Sema/SemaTemplateInstantiate.cpp

2895

It's not the caller's fault. The cause is that a function pointer or a function typedef isn't a declcontext, so even if the getDeclContext() of its parameter returns a function, it has nothing to do with its own ParmVarDecl. That's why we check if the corresponding ParmVarDecl is the same one.

[lebedev.ri]

I won't be able to properly review this, sorry.
While i'm sure this fix silences the assert, i don't know what that means for the original caller.
Does it fail? Does it retry in some other scope? Should it be more picky about the original scope in the first place?

[Violet]

In D60055#1459602, @lebedev.ri wrote:

I won't be able to properly review this, sorry.

Can you suggest someone with familiarity for this part of code? The reviewers I randomly added by git blame doesn't seem to be reviewing.

While i'm sure this fix silences the assert, i don't know what that means for the original caller.
Does it fail? Does it retry in some other scope? Should it be more picky about the original scope in the first place?

This small function is just to get the canonical ParmValDecl for a given one. If the check doesn't pass, it simply mean there is no canonical one (it's obvious for a function pointer), so we just use the passed one. There is nothing wrong here.

[gribozavr]

This fix makes sense to me, however, Richard Smith @rsmith is the best person to review this patch.

[Violet]

Thanks. Can you land it for me? I'm a newcommer without landing privilege.

[gribozavr]

I'd be happy to land it, but I do want @rsmith to take a look.

clang/lib/Sema/SemaTemplateInstantiate.cpp

static const Decl *getCanonicalParmVarDecl(const Decl *D) {
   // want to use the ParmVarDecl from the canonical function declaration,
   // since the map is then valid for any redeclaration or definition of that
   // function.
   if (const ParmVarDecl *PV = dyn_cast<ParmVarDecl>(D)) {
     if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
       unsigned i = PV->getFunctionScopeIndex();
       // This parameter might be from a freestanding function type within the
       // function and isn't necessarily referring to one of FD's parameters.
-      if (FD->getParamDecl(i) == PV)
+      if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)
         return FD->getCanonicalDecl()->getParamDecl(i);
     }
   }
   return D;
 }
 
 
 llvm::PointerUnion<Decl *, LocalInstantiationScope::DeclArgumentPack *> *

clang/test/SemaCXX/PR41139.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+// This test should not crash.
+int f1( unsigned ) { return 0; }
+
+template <class R, class... Args>
+struct S1 {
+    S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+    S1 s1( f1 );
+}

clang/test/SemaCXX/cxx1y-generic-lambdas.cpp

 namespace PR22117 {
   int x = [](auto) {
     return [](auto... run_args) {
       using T = int(decltype(run_args)...);
       return 0;
     };
   }(0)(0);
 }
 
+namespace PR41139 {
+  int y = [](auto outer) {
+    return [](auto inner) {
+      using T = int(decltype(outer), decltype(inner));
+      return 0;
+    };
+  }(0)(0);
+}
+
 namespace PR23716 {
 template<typename T>
 auto f(T x) {
   auto g = [](auto&&... args) {
     auto h = [args...]() -> int {
       return 0;
     };
     return h;