[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[YingChi Long via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG55d3b79d159b: [clang] add APValue type check in 
`TryPrintAsStringLiteral` (authored by inclyc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

Files:
  clang/lib/AST/APValue.cpp
  clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp


Index: clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// expected-no-diagnostics
+
+// Reported by: https://github.com/llvm/llvm-project/issues/57013
+// The following code should not crash clang
+struct X {
+  char arr[2];
+  constexpr X() {}
+  constexpr void modify() {
+arr[0] = 0;
+  }
+};
+constexpr X f(X t) {
+t.modify();
+return t;
+}
+auto x = f(X());
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.


Index: clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// expected-no-diagnostics
+
+// Reported by: https://github.com/llvm/llvm-project/issues/57013
+// The following code should not crash clang
+struct X {
+  char arr[2];
+  constexpr X() {}
+  constexpr void modify() {
+arr[0] = 0;
+  }
+};
+constexpr X f(X t) {
+t.modify();
+return t;
+}
+auto x = f(X());
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM!

I think this change should be backported to Clang 15.x since the breakage 
happened in that release and the fix is pretty straightforward. After this 
lands, I'll file the issue for it to get that process started. Thank you for 
the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[Zhihao Yuan via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[YingChi Long via Phabricator via cfe-commits]

inclyc added inline comments.



Comment at: clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp:7
+struct X {
+  char arr[2];
+  constexpr X() {}

The `ArrayRef Inits` points to here does not ends with `Int`, so 
`!Inits.back().getInt()` gets assertion failed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 451195.
inclyc added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

Files:
  clang/lib/AST/APValue.cpp
  clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp


Index: clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// expected-no-diagnostics
+
+// Reported by: https://github.com/llvm/llvm-project/issues/57013
+// The following code should not crash clang
+struct X {
+  char arr[2];
+  constexpr X() {}
+  constexpr void modify() {
+arr[0] = 0;
+  }
+};
+constexpr X f(X t) {
+t.modify();
+return t;
+}
+auto x = f(X());
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.


Index: clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/try-print-as-string-literal-type-check.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 %s -std=c++20 -fsyntax-only -verify
+// expected-no-diagnostics
+
+// Reported by: https://github.com/llvm/llvm-project/issues/57013
+// The following code should not crash clang
+struct X {
+  char arr[2];
+  constexpr X() {}
+  constexpr void modify() {
+arr[0] = 0;
+  }
+};
+constexpr X f(X t) {
+t.modify();
+return t;
+}
+auto x = f(X());
Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Can you add test coverage for these changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[YingChi Long via Phabricator via cfe-commits]

inclyc updated this revision to Diff 451049.
inclyc edited the summary of this revision.
inclyc added a comment.

Address reviewer's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

Files:
  clang/lib/AST/APValue.cpp


Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.


Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
@@ -655,6 +655,8 @@
   }
 
   for (auto  : Inits) {
+if (!Val.isInt())
+  return false;
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))
   return false; // Bye bye, see you in integers.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added a comment.

I wonder if `APValue` needs a member function to verify the underlying type of 
an `Array`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/AST/APValue.cpp:658
   for (auto  : Inits) {
 int64_t Char64 = Val.getInt().getExtValue();
 if (!isASCII(Char64))

I believe to be fully correct we also need to add:

```
if (!Val.isInt())
  return false;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131466

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

[PATCH] D131466: [clang] add APValue type check in `TryPrintAsStringLiteral`

[YingChi Long via Phabricator via cfe-commits]

inclyc created this revision.
Herald added a project: All.
inclyc added reviewers: rsmith, aaron.ballman, shafik.
inclyc updated this revision to Diff 451047.
inclyc added a comment.
inclyc edited the summary of this revision.
inclyc published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

.


Fixes https://github.com/llvm/llvm-project/issues/57013

https://reviews.llvm.org/D115031 improved printing of non-type template
parameter args. But checking if the end of Inits is 0 without checking
if APValue is an integer, causes clang to segfault. This patch adds
the code to check the type. (May not be a proper bugfix.)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131466

Files:
  clang/lib/AST/APValue.cpp


Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');


Index: clang/lib/AST/APValue.cpp
===
--- clang/lib/AST/APValue.cpp
+++ clang/lib/AST/APValue.cpp
@@ -637,10 +637,10 @@
 return false;
 
   // Nothing we can do about a sequence that is not null-terminated
-  if (!Inits.back().getInt().isZero())
+  if (!Inits.back().isInt() || !Inits.back().getInt().isZero())
 return false;
-  else
-Inits = Inits.drop_back();
+
+  Inits = Inits.drop_back();
 
   llvm::SmallString<40> Buf;
   Buf.push_back('"');
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits