[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:882-887
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }

vitalybuka wrote:
> rsmith wrote:
> > If we're going to further extend what Clang considers to be a flexible 
> > array access, we should do so consistently across our warning machinery and 
> > our sanitizers. Perhaps we could start by unifying this function with 
> > `IsTailPaddedMemberArray` in `SemaChecking`?
> There is one place in external code which is blocking me from enabling this 
> at Google.
> 
> How much work it's going to be? To me these functions looks very different.
If you don't want to do the refactoring, please at least update 
`Sema::CheckArrayAccess` to skip over commas when looking for a member access 
in `BaseExpr`. Testcase:

```
struct X { int a; int b[1]; } *p;
int n = (0, p->b)[3];
```

... currently warns and trips the array-bounds sanitizer; with this change it 
would still warn but not trip the sanitizer, which seems bad. (Though I suppose 
the opposite case is worse.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374



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

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka marked an inline comment as done.
vitalybuka added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:882-887
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }

rsmith wrote:
> If we're going to further extend what Clang considers to be a flexible array 
> access, we should do so consistently across our warning machinery and our 
> sanitizers. Perhaps we could start by unifying this function with 
> `IsTailPaddedMemberArray` in `SemaChecking`?
There is one place in external code which is blocking me from enabling this at 
Google.

How much work it's going to be? To me these functions looks very different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374



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

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:882-887
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }

If we're going to further extend what Clang considers to be a flexible array 
access, we should do so consistently across our warning machinery and our 
sanitizers. Perhaps we could start by unifying this function with 
`IsTailPaddedMemberArray` in `SemaChecking`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374



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

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374



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

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka updated this revision to Diff 255016.
vitalybuka added a comment.

try arc diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  clang/test/CodeGen/bounds-checking.cpp


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka updated this revision to Diff 254728.
vitalybuka added a comment.

remove debug dump


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  clang/test/CodeGen/bounds-checking.cpp


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

vitalybuka created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
vitalybuka added a reviewer: rsmith.
vitalybuka updated this revision to Diff 254728.
vitalybuka added a comment.
vitalybuka marked 2 inline comments as done.

remove debug dump




Comment at: clang/test/CodeGen/bounds-checking.c:116
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }

C version works even without patch


Depends on D77373 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77374

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGen/bounds-checking.c
  clang/test/CodeGen/bounds-checking.cpp


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),


Index: clang/test/CodeGen/bounds-checking.cpp
===
--- clang/test/CodeGen/bounds-checking.cpp
+++ clang/test/CodeGen/bounds-checking.cpp
@@ -98,3 +98,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @_Z10SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @_Z7S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return ((s->t, (1, s->a)))[i];
+  // CHECK: }
+}
Index: clang/test/CodeGen/bounds-checking.c
===
--- clang/test/CodeGen/bounds-checking.c
+++ clang/test/CodeGen/bounds-checking.c
@@ -100,3 +100,19 @@
   return s->a[i];
   // CHECK: }
 }
+
+// CHECK-LABEL: define {{.*}} @SFlexComma
+int SFlexComma(struct SFlex *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
+
+// CHECK-LABEL: define {{.*}} @S1Comma
+int S1Comma(struct S1 *s, int i) {
+  // a and b are treated as flexible array members.
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -879,6 +879,13 @@
 
   E = E->IgnoreParens();
 
+  while (const BinaryOperator *BO = dyn_cast(E)) {
+if (!BO->isCommaOp())
+  break;
+E = BO->getRHS();
+E = E->IgnoreParens();
+  }
+
   // A flexible array member must be the last member in the class.
   if (const auto *ME = dyn_cast(E)) {
 // FIXME: If the base type of the member expr is not FD->getParent(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D77374: Fix -fsanitize=array-bounds with comma operator

[Vitaly Buka via Phabricator via cfe-commits]

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



Comment at: clang/test/CodeGen/bounds-checking.c:116
+  // CHECK-NOT: @llvm.trap
+  return (s->t, s->a)[i];
+  // CHECK: }

C version works even without patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77374



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