subject:"\[PATCH\] D92892\: \[clang\] Change builtin object size to be compatible with GCC when sub\-object is invalid"

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-20 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D92892#2510445 , @george.burgess.iv 
wrote:

> reverted in 
> https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea
>  until we can figure out how to address the issues outlined above. thanks!

Thank you for this! I've also pinged @jtmott-intel internally in case he's not 
seen the post-commit feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-20 Thread George Burgess IV via Phabricator via cfe-commits

george.burgess.iv added a comment.

reverted in 
https://github.com/llvm/llvm-project/commit/b270fd59f0a86fe737853abc43e76b9d29a67eea
 until we can figure out how to address the issues outlined above. thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-20 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D92892#2510299 , @manojgupta wrote:

> This CL has caused two issues in Chrome OS :
> Compilation fail with FORTIFY: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1168199
> Runtime failures: 
> https://bugs.chromium.org/p/chromium/issues/detail?id=1167504
>
> I have requested George to take a look but will it be ok to revert this 
> meanwhile?

Yes, it's fine to revert this until we get these issues ironed out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-20 Thread Manoj Gupta via Phabricator via cfe-commits

manojgupta added a comment.

This CL has caused two issues in Chrome OS :
Compilation fail with FORTIFY: 
https://bugs.chromium.org/p/chromium/issues/detail?id=1168199
Runtime failures: https://bugs.chromium.org/p/chromium/issues/detail?id=1167504

I have requested George to take a look but will it be ok to revert this 
meanwhile?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-15 Thread Peter Collingbourne via Phabricator via cfe-commits

pcc added a comment.

This causes us to reject the following (reduced from AOSP):

  int sprintf(char* __s, const char* __fmt, ...)
  __attribute__((__format__(printf, 2, 3))) ;
  int sprintf(char* dest, const char* format)
  __attribute__((overloadable))
  __attribute__((enable_if(((__builtin_object_size(((dest)), (1))) != 
((unsigned long) -1) && (__builtin_object_size(((dest)), (1))) < 
(__builtin_strlen(format))), "format string will always overflow destination 
buffer")))
  
  __attribute__((unavailable("format string will always overflow 
destination buffer")));
  
  void f() {
unsigned char number_buffer[26] = {0};
sprintf((char *)number_buffer, "null");
  }

It doesn't seem like we ought to be rejecting this case. Can you please take a 
look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-07 Thread George Burgess IV via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG275f30df8ad6: [clang] Change builtin object size when 
subobject is invalid (authored by jtmott-intel, committed by george.burgess.iv).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92892

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/CodeGen/object-size.c


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(&ds1[9].snd[0], 1);
 
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&ds0[9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11408,9 +11408,9 @@
   return false;
   }
 
-  // If we point to before the start of the object, there are no accessible
-  // bytes.
-  if (LVal.getLValueOffset().isNegative()) {
+  // If we point outside of the object, there are no accessible bytes.
+  if (LVal.getLValueOffset().isNegative() ||
+  ((Type & 1) && !LVal.Designator.isValidSubobject())) {
 Size = 0;
 return true;
   }


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-07 Thread Mott, Jeffrey T via Phabricator via cfe-commits

jtmott-intel added a comment.

Seems I don't have commit access. I'll look into it. For now, could someone 
push this commit? Thanks.


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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-06 Thread Mott, Jeffrey T via Phabricator via cfe-commits

jtmott-intel updated this revision to Diff 314965.
jtmott-intel added a comment.

Updated comments to reflect "outside of" instead of "before".


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

https://reviews.llvm.org/D92892

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/CodeGen/object-size.c


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
   gi = OBJECT_SIZE_BUILTIN(&ds1[9].snd[0], 1);
 
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&ds0[9].snd[0], 1);
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, 
i1
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -11405,9 +11405,9 @@
   return false;
   }
 
-  // If we point to before the start of the object, there are no accessible
-  // bytes.
-  if (LVal.getLValueOffset().isNegative()) {
+  // If we point outside of the object, there are no accessible bytes.
+  if (LVal.getLValueOffset().isNegative() ||
+  ((Type & 1) && !LVal.Designator.isValidSubobject())) {
 Size = 0;
 return true;
   }


Index: clang/test/CodeGen/object-size.c
===
--- clang/test/CodeGen/object-size.c
+++ clang/test/CodeGen/object-size.c
@@ -310,7 +310,7 @@
 void test25() {
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0x1000, 2);
@@ -321,7 +321,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* {{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN((void*)0 + 0x1000, 2);
@@ -337,7 +337,7 @@
 
   // CHECK: store i32 316
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[11], 0);
-  // CHECK: store i32 312
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[12], 1);
   // CHECK: store i32 308
   gi = OBJECT_SIZE_BUILTIN(&t[1].v[13], 2);
@@ -433,7 +433,7 @@
 
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 0);
-  // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
+  // CHECK: store i32 0
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 1);
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 true, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(d0->snd, 2);
@@ -518,7 +518,7 @@
   // CHECK: call i64 @llvm.objectsize.i64.p0i8(i8* %{{.*}}, i1 false, i1 true, i1
   gi = OBJECT_SIZE_BUILTIN(&ds1[9].snd[0], 1);
 
-  // CHECK: call i64 @l

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-06 Thread George Burgess IV via Phabricator via cfe-commits

george.burgess.iv accepted this revision.
george.burgess.iv added a comment.
This revision is now accepted and ready to land.

thanks for working on this!

just one tiny nit and lgtm




Comment at: clang/lib/AST/ExprConstant.cpp:11408
 
   // If we point to before the start of the object, there are no accessible
   // bytes.

nit: the new part of this condition makes this comment somewhat outdated. 
should it say something like "outside of" instead of "before"?


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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

2021-01-06 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added subscribers: cfe-commits, aaron.ballman.
aaron.ballman added reviewers: rsmith, jyknight.
aaron.ballman added a comment.

Adding some reviewers and subscribing the mailing lists. FWIW, the changes seem 
reasonable to me.


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

https://reviews.llvm.org/D92892

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

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

[PATCH] D92892: [clang] Change builtin object size to be compatible with GCC when sub-object is invalid

10 matches

Site Navigation

Mail list logo

Footer information