[utils] Re-enable -Wunsafe-buffer-usage, add tests
Accidentally disabled when setting up -Weverything, because it triggered
on Wasm tests.
- Fix the Wasm tests.
- Note the unsafe buffers plugin isn't enabled for Wasm builds, so the
`-third_party/` suppression in unsafe_buffers_paths.txt doesn't
apply, but we still get the warnings via -Weverything.
- DAWN_UNSAFE_TODO is used instead of DAWN_UNSAFE_BUFFERS because we
want to switch these to spanified APIs soon.
- Re-enable the warning.
- Add nocompile tests for basic builtin stuff, and for Dawn types that
use DAWN_UNSAFE_BUFFER_USAGE.
Fixes: 523128530
Bug: 523358231
Change-Id: Ice88e4a9ed63e952656829f96e71138a386cce88
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/321976
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Auto-Submit: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/emdawnwebgpu/tests/SpotTests.cpp b/src/emdawnwebgpu/tests/SpotTests.cpp
index d54a42b..a40f8e0 100644
--- a/src/emdawnwebgpu/tests/SpotTests.cpp
+++ b/src/emdawnwebgpu/tests/SpotTests.cpp
@@ -126,8 +126,9 @@
wgpu::SupportedFeatures deviceFeatures;
device.GetFeatures(&deviceFeatures);
mGotCompatibilityMode = true;
- for (uint32_t i = 0; i < deviceFeatures.featureCount; ++i) {
- if (deviceFeatures.features[i] == wgpu::FeatureName::CoreFeaturesAndLimits) {
+ for (auto feature :
+ DAWN_UNSAFE_TODO(std::span(deviceFeatures.features, deviceFeatures.featureCount))) {
+ if (feature == wgpu::FeatureName::CoreFeaturesAndLimits) {
mGotCompatibilityMode = false;
}
}
@@ -218,7 +219,7 @@
TEST_P(SpotTests, GetWGSLLanguageFeatures) {
wgpu::SupportedWGSLLanguageFeatures f;
mInstance.GetWGSLLanguageFeatures(&f);
- auto features = std::span(f.features, f.featureCount);
+ auto features = DAWN_UNSAFE_TODO(std::span(f.features, f.featureCount));
for (auto feature : features) {
// GetWGSLLanguageFeatures should filter out any unknown features.
EXPECT_NE(feature, wgpu::WGSLLanguageFeatureName{0});
@@ -241,7 +242,7 @@
void TestGetFeatures(AdapterOrDevice o, bool shouldHaveCompressedTexture) {
wgpu::SupportedFeatures f;
o.GetFeatures(&f);
- auto features = std::span(f.features, f.featureCount);
+ auto features = DAWN_UNSAFE_TODO(std::span(f.features, f.featureCount));
for (auto feature : features) {
// GetFeatures should filter out any unknown features.
EXPECT_NE(feature, wgpu::FeatureName{0});
diff --git a/src/utils/BUILD.gn b/src/utils/BUILD.gn
index 3ab151f..18319dc 100644
--- a/src/utils/BUILD.gn
+++ b/src/utils/BUILD.gn
@@ -56,6 +56,14 @@
if (dawn_weverything) {
assert(is_clang)
+ if (!is_wasm) {
+ # One of the warnings in -Weverything is Clang's -Wunsafe-buffer-usage,
+ # but we don't want to run that warning on Dawn without the corresponding
+ # Chromium plugin, which implements support for unsafe_buffers_paths.txt
+ # and probably other important things.
+ assert(clang_use_unsafe_buffers_plugin)
+ }
+
cflags = [ "-Weverything" ]
# Suppressions
@@ -118,7 +126,6 @@
"-Wno-reserved-identifier",
"-Wno-unused-member-function",
"-Wno-tautological-type-limit-compare",
- "-Wno-unsafe-buffer-usage",
"-Wno-nonportable-system-include-path",
]
@@ -423,7 +430,7 @@
}
###############################################################################
-# Additional clang plugin for raw_ptr and unsafe-buffers
+# Additional clang plugins.
###############################################################################
# Helper template to define clang plugins.
diff --git a/src/utils/span_nocompile.nc b/src/utils/span_nocompile.nc
index 6a30c7e..c7e7842 100644
--- a/src/utils/span_nocompile.nc
+++ b/src/utils/span_nocompile.nc
@@ -25,7 +25,11 @@
// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-// Test the type checks of dawn::Span
+// Test the type checks of dawn::Span.
+//
+// Note: All unsafe buffer warning tests are in warning_nocompile.nc.
+// Warnings don't trigger in this file because it contains errors. Because of this,
+// DAWN_UNSAFE_BUFFERS() annotations also don't need be to used in this file - they're ignored.
#include <array>
@@ -56,26 +60,16 @@
};
void TestConstPointerToNonConstSpan() {
- DAWN_UNSAFE_BUFFERS(Span<const int>(kSpanData.data(), kSpanData.size())); // Control case.
- DAWN_UNSAFE_BUFFERS(Span<int>(kSpanData.data(), kSpanData.size())); // expected-error {{no matching constructor for initialization}}
+ Span<const int>(kSpanData.data(), kSpanData.size()); // Control case.
+ Span<int>(kSpanData.data(), kSpanData.size()); // expected-error {{no matching constructor for initialization}}
- DAWN_UNSAFE_BUFFERS(Span<const int>(kSpanData.begin(), kSpanData.end())); // Control case.
- DAWN_UNSAFE_BUFFERS(Span<int>(kSpanData.begin(), kSpanData.end())); // expected-error {{no matching constructor for initialization}}
+ Span<const int>(kSpanData.begin(), kSpanData.end()); // Control case.
+ Span<int>(kSpanData.begin(), kSpanData.end()); // expected-error {{no matching constructor for initialization}}
Span<const int>{FakeRange()}; // Control case.
Span<int>{FakeRange()}; // expected-error {{no matching constructor for initialization}}
}
-void TestConstructorsThatRequireDawnUnsafeBuffers() {
- DAWN_UNSAFE_BUFFERS(Span<const int>(kSpanData.data(), kSpanData.size())); // Control case.
- // TODO(https://crbug.com/523128530): DAWN_UNSAFE_BUFFERS doesn't seem required in no-compile tests for some reason. Fix that and add an expected error here.
- Span<const int>(kSpanData.data(), kSpanData.size());
-
- DAWN_UNSAFE_BUFFERS(Span<const int>(kSpanData.begin(), kSpanData.end())); // Control case.
- // TODO(https://crbug.com/523128530): DAWN_UNSAFE_BUFFERS doesn't seem required in no-compile tests for some reason. Fix that and add an expected error here.
- Span<const int>(kSpanData.begin(), kSpanData.end());
-}
-
void TestConstructorWithRangeRequirements() {
Span<const int>{FakeRange()}; // Control case.
@@ -107,7 +101,7 @@
void TestTypedIntegerArguments() {
ityp::span<Index, const int> sp{FakeTypedRange()};
- DAWN_UNSAFE_BUFFERS(ityp::span<Index, const int>(kSpanData.data(), kSpanData.size())); // expected-error {{no matching constructor for initialization}}
+ ityp::span<Index, const int>(kSpanData.data(), kSpanData.size()); // expected-error {{no matching constructor for initialization}}
(void) sp.at(2); // expected-error {{no viable conversion from}}
(void) sp[2]; // expected-error {{no viable overloaded operator[]}}
(void) sp.first(2); // expected-error {{no viable conversion from}}
diff --git a/src/utils/span_tests.cc b/src/utils/span_tests.cc
index 9225f53..054c7d3 100644
--- a/src/utils/span_tests.cc
+++ b/src/utils/span_tests.cc
@@ -652,7 +652,7 @@
std::array<int, 3> ints{};
// SAFETY: This is viewing ints, just with typed indices.
- ityp::span<Index, int> sp{ints.data(), Index(3u)};
+ ityp::span<Index, int> DAWN_UNSAFE_BUFFERS(sp{ints.data(), Index(3u)});
auto bsp = SpanAsBytes(sp);
static_assert(std::is_same_v<Span<const std::byte>, decltype(bsp)>);
EXPECT_EQ(bsp.size(), sp.size_bytes());
@@ -663,7 +663,7 @@
EXPECT_EQ(wbsp.data(), reinterpret_cast<std::byte*>(sp.data()));
// SAFETY: This is viewing ints, just with typed indices.
- ityp::span<Index, volatile int> vsp{ints.data(), Index(3u)};
+ ityp::span<Index, volatile int> DAWN_UNSAFE_BUFFERS(vsp{ints.data(), Index(3u)});
auto vbsp = SpanAsBytes(vsp);
static_assert(std::is_same_v<Span<const volatile std::byte>, decltype(vbsp)>);
EXPECT_EQ(vbsp.size(), vsp.size_bytes());
diff --git a/src/utils/warning_nocompile.nc b/src/utils/warning_nocompile.nc
index 229cb87..03a45ca 100644
--- a/src/utils/warning_nocompile.nc
+++ b/src/utils/warning_nocompile.nc
@@ -25,17 +25,66 @@
// OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+// Tests for warnings. Note this code should ONLY trigger warnings (no other hard errors),
+// otherwise the compiler suppresses the warnings in favor of the errors.
+
#include <stdint.h>
+#include <array>
+#include <span>
+
+#include "src/utils/span.h"
+
namespace dawn {
-// This test verifies that warnings that are part of -Weverything but not
-// -Wall -Wextra are enabled when This test runs only when `dawn_weverything = true`.
-
-// -Wcast-align is one such warning.
-void TestCastAlign() {
+// Verify that warnings that are part of -Weverything but not -Wall -Wextra
+// are enabled. This test runs only when `dawn_weverything = true`.
+void TestWeverything() {
+ // -Wcast-align is one such warning.
char* p = nullptr;
[[maybe_unused]] int* q = (int*)p; // expected-error {{increases required alignment}}
}
+// -Wunsafe-buffer-usage: operator[] on T*
+void TestUnsafeBuffersRawPointer() {
+ constexpr std::array<int, 4> arr{};
+
+ // Safe to get a pointer to the first element.
+ const int* arrPtr = arr.data();
+
+ // But unsafe to use it like an array.
+ { [[maybe_unused]] int x = arr[1]; }
+ { [[maybe_unused]] int x = std::span(arr)[1]; }
+ { [[maybe_unused]] int x = arrPtr[1]; } // expected-error {{unsafe buffer access}}
+}
+
+// -Wunsafe-buffer-usage-in-libc-call: memcpy()
+void TestUnsafeBuffersMemcpy() {
+ int x = 1;
+ int y = 2;
+ memcpy(&y, &x, sizeof(int)); // expected-error {{function 'memcpy' is unsafe}}
+}
+
+// -Wunsafe-buffer-usage: std::span() constructors
+void TestUnsafeBuffersStdSpanConstructors() {
+ std::array<int, 4> arr{};
+ // std::span is NOT tagged as being unsafe when using the unsafe buffers plugin.
+ // (If we use -Wunsafe-buffer-usage without the plugin, it would be. This may be fixable.)
+ { [[maybe_unused]] auto s = std::span(arr.data(), arr.size()); }
+ { [[maybe_unused]] std::span<int> s(arr.data(), arr.size()); }
+}
+
+// -Wunsafe-buffer-usage: dawn::Span() constructors
+void TestUnsafeBuffersDawnSpanConstructors() {
+ {
+ constexpr std::array<int, 5> kArr{};
+
+ DAWN_UNSAFE_BUFFERS(Span<const int>(kArr.data(), kArr.size())); // Control case.
+ Span<const int>(kArr.data(), kArr.size()); // expected-error {{introduces unsafe buffer manipulation}}
+
+ DAWN_UNSAFE_BUFFERS(Span<const int>(kArr.begin(), kArr.end())); // Control case.
+ Span<const int>(kArr.begin(), kArr.end()); // expected-error {{introduces unsafe buffer manipulation}}
+ }
+}
+
}
diff --git a/unsafe_buffers_paths.txt b/unsafe_buffers_paths.txt
index b8c7f20..dd1107b 100644
--- a/unsafe_buffers_paths.txt
+++ b/unsafe_buffers_paths.txt
@@ -25,9 +25,16 @@
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-# See https://chromium.googlesource.com/chromium/src/+/main/docs/unsafe_buffers.md
+# Config used by Chromium's unsafe buffers plugin to control Clang's builtin
+# -Wunsafe-buffer-usage* warnings.
+# Docs: http://crsrc.org/v/docs/unsafe_buffers.md
+#
+# Note the plugin is not used in Wasm builds, so this config is ignored.
-# Checks to enforce
+# Checks to enforce.
+# Docs: http://crsrc.org/v/docs/unsafe_buffers.md#syntax-of-unsafe-buffer-paths-file
+#
+# NOTE: If adding a check here, add a test to warning_nocompile.nc too.
.buffers,libc
# These directories are excluded because they come from outside Dawn and Tint and
