[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGaaaece65a80f: [clang][ExprConst] Handle 0 type size in
builtin_memcpy etc. (authored by tbaeder).
Changed prior to commit:
https://reviews.llvm.org/D157252?vs=557826=557859#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157252/new/
https://reviews.llvm.org/D157252
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/Sema/builtin-memcpy.c
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify=c
+// RUN: %clang_cc1 -x c++ %s -fsyntax-only -verify=cxx
+
+// cxx-no-diagnostics
+
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // c-warning {{buffer has size 0, but size
argument is 2}}
+ return 0;
+}
+
+#ifdef __cplusplus
+// FIXME: This is UB and GCC correctly diagnoses it. Clang should do the same.
+constexpr int b2() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2);
+ return 0;
+}
+static_assert(b2() == 0, "");
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9545,6 +9545,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify=c
+// RUN: %clang_cc1 -x c++ %s -fsyntax-only -verify=cxx
+
+// cxx-no-diagnostics
+
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // c-warning {{buffer has size 0, but size argument is 2}}
+ return 0;
+}
+
+#ifdef __cplusplus
+// FIXME: This is UB and GCC correctly diagnoses it. Clang should do the same.
+constexpr int b2() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2);
+ return 0;
+}
+static_assert(b2() == 0, "");
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9545,6 +9545,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM with a fixme comment added.
Comment at: clang/test/Sema/builtin-memcpy.c:4-8
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // expected-warning {{buffer has size 0, but
size argument is 2}}
+ return 0;
+}
tbaeder wrote:
> aaron.ballman wrote:
> > The only other test I'd like to see is one like:
> > ```
> > constexpr int b() {
> > struct { } a[10];
> > __builtin_memcpy([2], a, 2); // UB here should be caught, right?
> > return 0;
> > }
> >
> > static_assert(b() == 0, "");
> > ```
> > (if that's follow-up work, that's fine too, just leave a test with a FIXME
> > comment.)
> Nope, clang accepts that and GCC rejects the function when calling.
Amusingly, Clang and EDG (GNU mode) accept and GCC rejects. GCC is correct, so
it's worth adding a FIXME to the case below so we know this isn't intentional
behavior.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157252/new/
https://reviews.llvm.org/D157252
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder updated this revision to Diff 557826.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157252/new/
https://reviews.llvm.org/D157252
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/Sema/builtin-memcpy.c
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify=c
+// RUN: %clang_cc1 -x c++ %s -fsyntax-only -verify=cxx
+
+// cxx-no-diagnostics
+
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // c-warning {{buffer has size 0, but size
argument is 2}}
+ return 0;
+}
+
+#ifdef __cplusplus
+constexpr int b2() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2);
+ return 0;
+}
+static_assert(b2() == 0, "");
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9545,6 +9545,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify=c
+// RUN: %clang_cc1 -x c++ %s -fsyntax-only -verify=cxx
+
+// cxx-no-diagnostics
+
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // c-warning {{buffer has size 0, but size argument is 2}}
+ return 0;
+}
+
+#ifdef __cplusplus
+constexpr int b2() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2);
+ return 0;
+}
+static_assert(b2() == 0, "");
+#endif
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9545,6 +9545,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added inline comments.
Comment at: clang/test/Sema/builtin-memcpy.c:4-8
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // expected-warning {{buffer has size 0, but
size argument is 2}}
+ return 0;
+}
aaron.ballman wrote:
> The only other test I'd like to see is one like:
> ```
> constexpr int b() {
> struct { } a[10];
> __builtin_memcpy([2], a, 2); // UB here should be caught, right?
> return 0;
> }
>
> static_assert(b() == 0, "");
> ```
> (if that's follow-up work, that's fine too, just leave a test with a FIXME
> comment.)
Nope, clang accepts that and GCC rejects the function when calling.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157252/new/
https://reviews.llvm.org/D157252
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
aaron.ballman added inline comments.
Comment at: clang/test/Sema/builtin-memcpy.c:4-8
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // expected-warning {{buffer has size 0, but
size argument is 2}}
+ return 0;
+}
The only other test I'd like to see is one like:
```
constexpr int b() {
struct { } a[10];
__builtin_memcpy([2], a, 2); // UB here should be caught, right?
return 0;
}
static_assert(b() == 0, "");
```
(if that's follow-up work, that's fine too, just leave a test with a FIXME
comment.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157252/new/
https://reviews.llvm.org/D157252
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9512 uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity(); +if (TSize == 0) + return false; shafik wrote: > I think we should issue a diagnostic, we don't have any indication that this > is failing, let alone because the destination object size is zero. You mean in addition to the one we are emitting? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
shafik added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9512 uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity(); +if (TSize == 0) + return false; I think we should issue a diagnostic, we don't have any indication that this is failing, let alone because the destination object size is zero. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157252/new/ https://reviews.llvm.org/D157252 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D157252: [clang][ExprConst] Handle 0 type size in builtin_memcpy etc.
tbaeder created this revision.
tbaeder added a reviewer: clang.
Herald added a project: All.
tbaeder requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Fixes https://github.com/llvm/llvm-project/issues/44176
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157252
Files:
clang/lib/AST/ExprConstant.cpp
clang/test/Sema/builtin-memcpy.c
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // expected-warning {{buffer has size 0, but
size argument is 2}}
+ return 0;
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9509,6 +9509,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
Index: clang/test/Sema/builtin-memcpy.c
===
--- /dev/null
+++ clang/test/Sema/builtin-memcpy.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+
+/// Zero-sized structs should not crash.
+int b() {
+ struct { } a[10];
+ __builtin_memcpy([2], a, 2); // expected-warning {{buffer has size 0, but size argument is 2}}
+ return 0;
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9509,6 +9509,8 @@
// Figure out how many T's we're copying.
uint64_t TSize = Info.Ctx.getTypeSizeInChars(T).getQuantity();
+if (TSize == 0)
+ return false;
if (!WChar) {
uint64_t Remainder;
llvm::APInt OrigN = N;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
