[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb2c3ae0b6f05: [Sema] Don't check bounds for function 
pointer (authored by ArcsinX).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15495,6 +15495,8 @@
 ND = ME->getMemberDecl();
 
   if (IsUnboundedArray) {
+if (EffectiveType->isFunctionType())
+  return;
 if (index.isUnsigned() || !index.isNegative()) {
   const auto &ASTC = getASTContext();
   unsigned AddrBits =


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15495,6 +15495,8 @@
 ND = ME->getMemberDecl();
 
   if (IsUnboundedArray) {
+if (EffectiveType->isFunctionType())
+  return;
 if (index.isUnsigned() || !index.isNegative()) {
   const auto &ASTC = getASTContext();
   unsigned AddrBits =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Erich Keane via Phabricator via cfe-commits]

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

>> Friendly ping

Unfriendly LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

Friendly ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

Friendly ping.

In D122748#3417500 , @erichkeane 
wrote:

> would still like to see the feedback from the original submitter here to see 
> if there is more work to do in here that would be valuable.

Unsure, maybe we can process without him? It seems his last activity here was 
more than six months ago.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think I'm generally in favor, would still like to see the feedback from the 
original submitter here to see if there is more work to do in here that would 
be valuable.




Comment at: clang/lib/Sema/SemaChecking.cpp:15471
   if (IsUnboundedArray) {
+if (EffectiveType->isFunctionType())
+  return;

Ah, I see...  I originally made the comment thinking that we wanted to be able 
to get to ~15540 (which this 'return' would seem to undo), but I see this block 
ends in 'return' anyway.  I'm happy with this change the way it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX added a comment.

In D122748#3417240 , @erichkeane 
wrote:

> That said, we might not want to early-exist here, I think we can just skip 
> the `IsUnboundedArray` branch?

It seems you are right, thanks, fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 419242.
ArcsinX added a comment.

Check for function type only if IsUnboundedArray is true


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15468,6 +15468,8 @@
 ND = ME->getMemberDecl();
 
   if (IsUnboundedArray) {
+if (EffectiveType->isFunctionType())
+  return;
 if (index.isUnsigned() || !index.isNegative()) {
   const auto &ASTC = getASTContext();
   unsigned AddrBits =


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15468,6 +15468,8 @@
 ND = ME->getMemberDecl();
 
   if (IsUnboundedArray) {
+if (EffectiveType->isFunctionType())
+  return;
 if (index.isUnsigned() || !index.isNegative()) {
   const auto &ASTC = getASTContext();
   unsigned AddrBits =
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

It seems like this entire section of code was added here: 
https://github.com/llvm/llvm-project/commit/ce44fe199bbfd4b5a44764b678c431fdc117116a

@chrish_ericsson_atx should probably take a look at this.  That said, we might 
not want to early-exist here, I think we can just skip the `IsUnboundedArray` 
branch?  This example worked correctly in the 12.0 branch, so I think it was 
fine before then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122748

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

[PATCH] D122748: [Sema] Don't check bounds for function pointer

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX created this revision.
ArcsinX added reviewers: aaron.ballman, erichkeane, abhinavgaba, 
chrish_ericsson_atx.
Herald added a project: All.
ArcsinX requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Currently, clang crashes with i386 target on the following code:

  void f() {
f + 0xdeadUL;
  }

This problem is similar to the problem fixed in D104424 
, but that fix can't handle function pointer 
case, because `getTypeSizeInCharsIfKnown()` says that size is known and equal 
to 0 for function type.

This patch prevents bounds checking for function pointer, thus fixes the crash.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122748

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15447,7 +15447,7 @@
   const Type *BaseType =
   ArrayTy == nullptr ? nullptr : ArrayTy->getElementType().getTypePtr();
   bool IsUnboundedArray = (BaseType == nullptr);
-  if (EffectiveType->isDependentType() ||
+  if (EffectiveType->isDependentType() || EffectiveType->isFunctionType() ||
   (!IsUnboundedArray && BaseType->isDependentType()))
 return;
 


Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -80,3 +80,7 @@
   (void *)0 + 0xdeadUL;
   // no array-bounds warning, and no crash
 }
+
+void func() {
+  func + 0xdeadUL; // no crash
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -15447,7 +15447,7 @@
   const Type *BaseType =
   ArrayTy == nullptr ? nullptr : ArrayTy->getElementType().getTypePtr();
   bool IsUnboundedArray = (BaseType == nullptr);
-  if (EffectiveType->isDependentType() ||
+  if (EffectiveType->isDependentType() || EffectiveType->isFunctionType() ||
   (!IsUnboundedArray && BaseType->isDependentType()))
 return;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits