Re: [PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tomaz Canabrava via cfe-commits]

thank you, this is really appreciated.


On Fri, May 20, 2022 at 12:32 PM Aaron Ballman via Phabricator <
revi...@reviews.llvm.org> wrote:

> aaron.ballman added a comment.
>
> In D123667#3524711 ,
> @aaron.ballman wrote:
>
> > In D123667#3524614 ,
> @tcanabrava wrote:
> >
> >> This affects all released versions of llvm14, and potentially llvm13
> too (I got a crash today on llvm13 hitting this exact code).
> >> yet, this is not backported to llvm14 on the current 14.x github
> branch. Please backport so this is released on the next tarball.
> >
> > I filed https://github.com/llvm/llvm-project/issues/55585 to see if we
> can get this into the 14.x branch, thanks for the request!
>
> And it looks like it was backported today.
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D123667/new/
>
> https://reviews.llvm.org/D123667
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123667#3524711 , @aaron.ballman 
wrote:

> In D123667#3524614 , @tcanabrava 
> wrote:
>
>> This affects all released versions of llvm14, and potentially llvm13 too (I 
>> got a crash today on llvm13 hitting this exact code).
>> yet, this is not backported to llvm14 on the current 14.x github branch. 
>> Please backport so this is released on the next tarball.
>
> I filed https://github.com/llvm/llvm-project/issues/55585 to see if we can 
> get this into the 14.x branch, thanks for the request!

And it looks like it was backported today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D123667#3524614 , @tcanabrava 
wrote:

> This affects all released versions of llvm14, and potentially llvm13 too (I 
> got a crash today on llvm13 hitting this exact code).
> yet, this is not backported to llvm14 on the current 14.x github branch. 
> Please backport so this is released on the next tarball.

I filed https://github.com/llvm/llvm-project/issues/55585 to see if we can get 
this into the 14.x branch, thanks for the request!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tomaz Canabrava via Phabricator via cfe-commits]

tcanabrava added a comment.

This affects all released versions of llvm14, and potentially llvm13 too (I got 
a crash today on llvm13 hitting this exact code).
yet, this is not backported to llvm14 on the current 14.x github branch. Please 
backport so this is released on the next tarball.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman closed this revision.
aaron.ballman added a comment.

Thank you for the fix! I've commit on your behalf in 
225b91e6cbba31ff1ce787a152a67977d08fdcab 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tom Eccles via Phabricator via cfe-commits]

tblah added a comment.

Yes please commit on my behalf. My details are

Tom Eccles

tom.ecc...@codethink.co.uk

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, with a commenting nit. The Precommit CI test failures look unrelated to 
me as well.

Do you need me to commit on your behalf? If so, what name and email address 
would you like me to use for patch attribution? (I can fix the commenting issue 
when I land, so don't worry about pushing up a new patch.)




Comment at: clang/unittests/AST/TypePrinterTest.cpp:74-75
+
+  // regression test ensuring we do not segfault getting the qualtype as a
+  // string
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "",




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tom Eccles via Phabricator via cfe-commits]

tblah added a comment.

I'm not able to reproduce the test failure in CI. It looks spurious to me 
because I have not changed anything related to openmp. The only change in 
behaviour after my patch should be to avoid a null pointer dereference. In 
tests which did not hit that null pointer dereference before there should not 
be any change in behaviour.

What I tried:

  - Use a debian 11 x64 base image
- Install a minimum set of required packages from debian repos (including clang 
and lld from debian)
- Run the commands listed under "Reproduce build locally" on the CI failure 
report


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tom Eccles via Phabricator via cfe-commits]

tblah updated this revision to Diff 422906.
tblah added a comment.

Added a unit test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

Files:
  clang/lib/AST/TypePrinter.cpp
  clang/unittests/AST/TypePrinterTest.cpp


Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -64,6 +64,22 @@
   [](PrintingPolicy ) { Policy.FullyQualifiedName = true; }));
 }
 
+TEST(TypePrinter, TemplateId2) {
+  std::string Code = R"cpp(
+  template  class TemplatedType>
+  void func(TemplatedType Param);
+)cpp";
+  auto Matcher = parmVarDecl(hasType(qualType().bind("id")));
+
+  // regression test ensuring we do not segfault getting the qualtype as a
+  // string
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "",
+ [](PrintingPolicy ) {
+   Policy.FullyQualifiedName = true;
+   Policy.PrintCanonicalTypes = true;
+ }));
+}
+
 TEST(TypePrinter, ParamsUglified) {
   llvm::StringLiteral Code = R"cpp(
 template  class __f>
Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1467,8 +1467,7 @@
 if (!Policy.SuppressScope)
   AppendScope(TD->getDeclContext(), OS, TD->getDeclName());
 
-IdentifierInfo *II = TD->getIdentifier();
-OS << II->getName();
+OS << TD->getName();
   } else {
 T->getTemplateName().print(OS, Policy);
   }


Index: clang/unittests/AST/TypePrinterTest.cpp
===
--- clang/unittests/AST/TypePrinterTest.cpp
+++ clang/unittests/AST/TypePrinterTest.cpp
@@ -64,6 +64,22 @@
   [](PrintingPolicy ) { Policy.FullyQualifiedName = true; }));
 }
 
+TEST(TypePrinter, TemplateId2) {
+  std::string Code = R"cpp(
+  template  class TemplatedType>
+  void func(TemplatedType Param);
+)cpp";
+  auto Matcher = parmVarDecl(hasType(qualType().bind("id")));
+
+  // regression test ensuring we do not segfault getting the qualtype as a
+  // string
+  ASSERT_TRUE(PrintedTypeMatches(Code, {}, Matcher, "",
+ [](PrintingPolicy ) {
+   Policy.FullyQualifiedName = true;
+   Policy.PrintCanonicalTypes = true;
+ }));
+}
+
 TEST(TypePrinter, ParamsUglified) {
   llvm::StringLiteral Code = R"cpp(
 template  class __f>
Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1467,8 +1467,7 @@
 if (!Policy.SuppressScope)
   AppendScope(TD->getDeclContext(), OS, TD->getDeclName());
 
-IdentifierInfo *II = TD->getIdentifier();
-OS << II->getName();
+OS << TD->getName();
   } else {
 T->getTemplateName().print(OS, Policy);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman edited reviewers, added: aaron.ballman; removed: lattner.
aaron.ballman added a comment.

This looks good in general, but can you add a test case for the scenario which 
previously crashed and a release note for the fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123667

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

[PATCH] D123667: [clang][AST] Fix crash getting name of a template decl

[Tom Eccles via Phabricator via cfe-commits]

tblah created this revision.
tblah added a reviewer: rsmith.
Herald added a project: All.
tblah requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

NamedDecl::getIdentifier can return a nullptr when 
DeclarationName::isIdentifier is false, which leads to a null pointer 
dereference when TypePrinter::printTemplateId calls ->getName().

NamedDecl::getName does the same thing in the successful case and returns an 
empty string in the failure case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123667

Files:
  clang/lib/AST/TypePrinter.cpp


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1466,8 +1466,7 @@
 if (!Policy.SuppressScope)
   AppendScope(TD->getDeclContext(), OS, TD->getDeclName());
 
-IdentifierInfo *II = TD->getIdentifier();
-OS << II->getName();
+OS << TD->getName();
   } else {
 T->getTemplateName().print(OS, Policy);
   }


Index: clang/lib/AST/TypePrinter.cpp
===
--- clang/lib/AST/TypePrinter.cpp
+++ clang/lib/AST/TypePrinter.cpp
@@ -1466,8 +1466,7 @@
 if (!Policy.SuppressScope)
   AppendScope(TD->getDeclContext(), OS, TD->getDeclName());
 
-IdentifierInfo *II = TD->getIdentifier();
-OS << II->getName();
+OS << TD->getName();
   } else {
 T->getTemplateName().print(OS, Policy);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits