[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet marked 2 inline comments as done.
Violet added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2895
   // 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);

lebedev.ri wrote:
> Have you analyzed how we get here?
> Maybe the caller code is faulty?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet updated this revision to Diff 194152.
Violet added a comment.

Address reviewer comment


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/PR41139.cpp
  clang/test/SemaCXX/cxx1y-generic-lambdas.cpp


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -944,6 +944,15 @@
   }(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
 auto f(T x) {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+// This test should not crash.
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -944,6 +944,15 @@
   }(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
 auto f(T x) {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,15 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+// This test should not crash.
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet added a comment.

PING ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet updated this revision to Diff 193045.
Violet added a comment.

a more straightforward testcase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/PR41139.cpp
  clang/test/SemaCXX/cxx1y-generic-lambdas.cpp


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -942,6 +942,13 @@
   return 0;
 };
   }(0)(0);
+
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
 }
 
 namespace PR23716 {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -942,6 +942,13 @@
   return 0;
 };
   }(0)(0);
+
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
 }
 
 namespace PR23716 {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet updated this revision to Diff 193041.
Violet added a comment.

Wrong bug number


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60055/new/

https://reviews.llvm.org/D60055

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/PR41139.cpp


Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }


Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60055

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/PR38077.cpp


Index: clang/test/SemaCXX/PR38077.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38077.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }


Index: clang/test/SemaCXX/PR38077.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR38077.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   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);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits