diff --git a/.github/workflows/ios-ci.yml b/.github/workflows/ios-ci.yml index dc4af40b8..1fcc19e3e 100644 --- a/.github/workflows/ios-ci.yml +++ b/.github/workflows/ios-ci.yml @@ -86,8 +86,12 @@ jobs: - name: Running iOS tests run: bazel test //platform/ios/test:ios_test --test_output=errors --//:renderer=metal - - name: Running iOS UI tests - run: bazel test //platform/ios/iosapp-UITests:uitest --test_output=errors --//:renderer=metal + # tsan run fails with 'Interceptors are not working. This may be because ThreadSanitizer is loaded too late...' + #- name: Running iOS UI tests (Thread Sanitizer) + # run: bazel test //platform/ios/iosapp-UITests:uitest --test_output=errors --//:renderer=metal --features=tsan + + - name: Running iOS UI tests (Address+UB Sanitizer) + run: bazel test //platform/ios/iosapp-UITests:uitest --test_output=errors --//:renderer=metal --features=include_clang_rt --copt=-fsanitize=undefined --linkopt=-fsanitize=undefined --copt=-fsanitize-recover=null --linkopt=-fsanitize-recover=null # render test diff --git a/include/mbgl/mtl/mtl_fwd.hpp b/include/mbgl/mtl/mtl_fwd.hpp index 9a56a34f4..63a8c8f50 100644 --- a/include/mbgl/mtl/mtl_fwd.hpp +++ b/include/mbgl/mtl/mtl_fwd.hpp @@ -3,10 +3,11 @@ // Forward-declarations for metal-cpp types, allowing // references without including any third-party headers -#include +#include namespace CA { class MetalDrawable; +class MetalLayer; } // namespace CA namespace NS { @@ -39,7 +40,9 @@ class VertexDescriptor; namespace mbgl { namespace mtl { + using CAMetalDrawablePtr = NS::SharedPtr; +using CAMetalLayerPtr = NS::SharedPtr; using MTLBlitCommandEncoderPtr = NS::SharedPtr; using MTLBlitPassDescriptorPtr = NS::SharedPtr; diff --git a/include/mbgl/style/layers/mtl/custom_layer_render_parameters.hpp b/include/mbgl/style/layers/mtl/custom_layer_render_parameters.hpp index 1b6e094b7..572f26970 100644 --- a/include/mbgl/style/layers/mtl/custom_layer_render_parameters.hpp +++ b/include/mbgl/style/layers/mtl/custom_layer_render_parameters.hpp @@ -1,8 +1,8 @@ #pragma once #include - #include + #include #include diff --git a/platform/default/src/mbgl/mtl/headless_backend.cpp b/platform/default/src/mbgl/mtl/headless_backend.cpp index 2f83829aa..c056234e0 100644 --- a/platform/default/src/mbgl/mtl/headless_backend.cpp +++ b/platform/default/src/mbgl/mtl/headless_backend.cpp @@ -4,10 +4,11 @@ #include #include +#include + #include #include #include -#include namespace mbgl { namespace mtl { diff --git a/platform/glfw/metal_backend.mm b/platform/glfw/metal_backend.mm index e49771268..8b94f393c 100644 --- a/platform/glfw/metal_backend.mm +++ b/platform/glfw/metal_backend.mm @@ -1,11 +1,15 @@ #include "metal_backend.h" +#include #include + #include #include namespace mbgl { +using namespace mtl; + class MetalRenderableResource final: public mtl::RenderableResource { public: MetalRenderableResource(MetalBackend &backend): @@ -92,17 +96,17 @@ public: return renderPassDescriptor; } - NS::SharedPtr getSwapchain() { + const CAMetalLayerPtr& getSwapchain() const { return swapchain; } private: MetalBackend &rendererBackend; - NS::SharedPtr commandQueue; - NS::SharedPtr commandBuffer; - NS::SharedPtr renderPassDescriptor; - NS::SharedPtr surface; - NS::SharedPtr swapchain; + MTLCommandQueuePtr commandQueue; + MTLCommandBufferPtr commandBuffer; + MTLRenderPassDescriptorPtr renderPassDescriptor; + CAMetalDrawablePtr surface; + CAMetalLayerPtr swapchain; gfx::Texture2DPtr depthTexture; gfx::Texture2DPtr stencilTexture; mbgl::Size size; diff --git a/platform/ios/BUILD.bazel b/platform/ios/BUILD.bazel index 1fd08ccae..d033f6c12 100644 --- a/platform/ios/BUILD.bazel +++ b/platform/ios/BUILD.bazel @@ -123,6 +123,7 @@ apple_xcframework( data = glob([ "resources/*.lproj/**", "resources/*.xcassets/**", + "resources/metal-cpp-ignores.txt", ]), infoplists = ["info_plist"], ios = { @@ -159,6 +160,7 @@ apple_resource_bundle( resources = glob([ "resources/*.lproj/**", "resources/*.xcassets/**", + "resources/metal-cpp-ignores.txt", ]), visibility = ["//visibility:public"], ) diff --git a/platform/ios/app/MBXAppDelegate.m b/platform/ios/app/MBXAppDelegate.m index 549eb5acd..40b083f86 100644 --- a/platform/ios/app/MBXAppDelegate.m +++ b/platform/ios/app/MBXAppDelegate.m @@ -3,14 +3,30 @@ #import "MBXAppDelegate.h" #import "MBXViewController.h" +#if __has_feature(undefined_behavior_sanitizer) || defined(__SANITIZE_UNDEFINED_BEHAVIOR__) +#import "sanitizer/ubsan_interface.h" +const char* __ubsan_default_options(void) { + static char default_opts[2 * PATH_MAX]; + NSString* path = [[NSBundle bundleForClass:[MBXAppDelegate class]] pathForResource:@"metal-cpp-ignores" ofType:@"txt" inDirectory:@"Mapbox.bundle"]; + NSString* opts = @"print_stacktrace=1,halt_on_error=1,report_error_type=1,verbosity=0"; // add ',help=1' to see available options + if (path) { + opts = [opts stringByAppendingFormat:@",suppressions=%@", path]; + } + NSLog(@"Using ubsan options: %@", opts); + + bzero(default_opts, sizeof(default_opts)); + strncpy(default_opts, [opts UTF8String], sizeof(default_opts) - 1); + return default_opts; +} +#endif + @interface MBXAppDelegate() @end @implementation MBXAppDelegate -- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions -{ +- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions { #ifndef MLN_LOGGING_DISABLED [MLNLoggingConfiguration sharedConfiguration].loggingLevel = MLNLoggingLevelFault; #endif diff --git a/platform/ios/resources/metal-cpp-ignores.txt b/platform/ios/resources/metal-cpp-ignores.txt new file mode 100644 index 000000000..f14d8fad7 --- /dev/null +++ b/platform/ios/resources/metal-cpp-ignores.txt @@ -0,0 +1,7 @@ +# metal-cpp uses calls through null pointers +# Note that the code must be compiled with `-fsanitize-recover=null` for this to work. +null:vendor/metal-cpp/* + +# `NS::String*` implicit cast to `NS::Object*` in Context::createProgram +alignment:src/mbgl/mtl/context.cpp + diff --git a/src/mbgl/geometry/feature_index.cpp b/src/mbgl/geometry/feature_index.cpp index c4b731d65..240c61136 100644 --- a/src/mbgl/geometry/feature_index.cpp +++ b/src/mbgl/geometry/feature_index.cpp @@ -69,8 +69,8 @@ void FeatureIndex::query(std::unordered_map>& // Determine query radius const auto pixelsToTileUnits = static_cast(util::EXTENT / tileSize / scale); - const int16_t additionalPadding = std::min( - util::EXTENT, static_cast(additionalQueryPadding * pixelsToTileUnits)); + const int16_t additionalPadding = static_cast( + std::min(static_cast(util::EXTENT), additionalQueryPadding * pixelsToTileUnits)); // Query the grid index mapbox::geometry::box box = mapbox::geometry::envelope(queryGeometry); diff --git a/src/mbgl/mtl/context.cpp b/src/mbgl/mtl/context.cpp index 84ab6ab75..9b26cd404 100644 --- a/src/mbgl/mtl/context.cpp +++ b/src/mbgl/mtl/context.cpp @@ -81,6 +81,11 @@ BufferResource Context::createBuffer( return {const_cast(*this), data, size, MTL::ResourceStorageModeShared, isIndexBuffer, persistent}; } +// If enabled, ubsan flags the implicit cast from assigning `NS::String*` to `NS::Object*` here +// as an alignment error. The ObjC NSString allocations are cast to a C++ object pointer by +// `NS::Object::sendMessage`, and the alignment rules are different. We assume that, because +// `NS::Object` does not use virtual methods and only serves as a way for C++ code to store +// and pass ObjC `id`s to `objc_msgSend`, this won't cause any real problems. UniqueShaderProgram Context::createProgram(std::string name, const std::string_view source, const std::string_view vertexName, @@ -93,11 +98,11 @@ UniqueShaderProgram Context::createProgram(std::string name, const auto& programDefines = programParameters.getDefines(); const auto numDefines = programDefines.size() + additionalDefines.size(); - std::vector rawDefines; + std::vector rawDefines; rawDefines.reserve(2 * numDefines); const auto addDefine = [&rawDefines](const auto& pair) { - auto* nsKey = NS::String::string(pair.first.data(), NS::UTF8StringEncoding); - auto* nsVal = NS::String::string(pair.second.data(), NS::UTF8StringEncoding); + const auto* nsKey = NS::String::string(pair.first.data(), NS::UTF8StringEncoding); + const auto* nsVal = NS::String::string(pair.second.data(), NS::UTF8StringEncoding); rawDefines.insert(std::next(rawDefines.begin(), rawDefines.size() / 2), nsKey); rawDefines.insert(rawDefines.end(), nsVal); }; @@ -139,7 +144,7 @@ UniqueShaderProgram Context::createProgram(std::string name, } const auto nsVertName = NS::String::string(vertexName.data(), NS::UTF8StringEncoding); - NS::SharedPtr vertexFunction = NS::TransferPtr(library->newFunction(nsVertName)); + MTLFunctionPtr vertexFunction = NS::TransferPtr(library->newFunction(nsVertName)); if (!vertexFunction) { Log::Error(Event::Shader, name + " missing vertex function " + vertexName.data()); assert(false); @@ -147,7 +152,7 @@ UniqueShaderProgram Context::createProgram(std::string name, } // fragment function is optional - NS::SharedPtr fragmentFunction; + MTLFunctionPtr fragmentFunction; if (!fragmentName.empty()) { const auto nsFragName = NS::String::string(fragmentName.data(), NS::UTF8StringEncoding); fragmentFunction = NS::TransferPtr(library->newFunction(nsFragName)); diff --git a/src/mbgl/mtl/render_pass.cpp b/src/mbgl/mtl/render_pass.cpp index 0841768a2..2ffee68c4 100644 --- a/src/mbgl/mtl/render_pass.cpp +++ b/src/mbgl/mtl/render_pass.cpp @@ -25,7 +25,7 @@ RenderPass::RenderPass(CommandEncoder& commandEncoder_, const char* name, const const auto& c = *descriptor.clearColor; colorTarget->setLoadAction(MTL::LoadActionClear); colorTarget->setClearColor(MTL::ClearColor::Make(c.r, c.g, c.b, c.a)); - rpd = copy; + rpd = std::move(copy); } } } diff --git a/src/mbgl/programs/line_program.hpp b/src/mbgl/programs/line_program.hpp index 198cac40e..122ebd6ad 100644 --- a/src/mbgl/programs/line_program.hpp +++ b/src/mbgl/programs/line_program.hpp @@ -4,9 +4,10 @@ #include #include #include -#include #include #include +#include + #include namespace mbgl { @@ -55,8 +56,8 @@ public: return LayoutVertex{ {{static_cast((p.x * 2) | (round ? 1 : 0)), static_cast((p.y * 2) | (up ? 1 : 0))}}, {{// add 128 to store a byte in an unsigned byte - static_cast(::round(extrudeScale * e.x) + 128), - static_cast(::round(extrudeScale * e.y) + 128), + static_cast(util::clamp(::round(extrudeScale * e.x) + 128, 0., 255.)), + static_cast(util::clamp(::round(extrudeScale * e.y) + 128, 0., 255.)), // Encode the -1/0/1 direction value into the first two bits of .z // of a_data. Combine it with the lower 6 bits of `linesofar` diff --git a/src/mbgl/style/layers/custom_drawable_layer.cpp b/src/mbgl/style/layers/custom_drawable_layer.cpp index 4afd3bcdb..b7ce43465 100644 --- a/src/mbgl/style/layers/custom_drawable_layer.cpp +++ b/src/mbgl/style/layers/custom_drawable_layer.cpp @@ -132,8 +132,8 @@ private: class WideVectorDrawableTweaker : public gfx::DrawableTweaker { public: - WideVectorDrawableTweaker(const CustomDrawableLayerHost::Interface::LineOptions& options) - : options(options) {} + WideVectorDrawableTweaker(const CustomDrawableLayerHost::Interface::LineOptions& options_) + : options(options_) {} void init(gfx::Drawable&) override {}; diff --git a/src/mbgl/util/thread_pool.cpp b/src/mbgl/util/thread_pool.cpp index 4db3e322b..d02963bf8 100644 --- a/src/mbgl/util/thread_pool.cpp +++ b/src/mbgl/util/thread_pool.cpp @@ -59,23 +59,28 @@ std::thread ThreadedSchedulerBase::makeSchedulerThread(size_t index) { lock.unlock(); if (function) { - const auto cleanup = [&] { - // destroy the function and release its captures before unblocking `waitForEmpty` - function = {}; - pendingItems--; - if (queue.empty() && !pendingItems) { - cvEmpty.notify_all(); - } - }; try { function(); - cleanup(); + + // destroy the function and release its captures before unblocking `waitForEmpty` + function = {}; + if (!--pendingItems) { + std::unique_lock inner_lock(mutex); + if (queue.empty()) { + cvEmpty.notify_all(); + } + } } catch (...) { - lock.lock(); + std::unique_lock inner_lock(mutex); if (handler) { handler(std::current_exception()); } - cleanup(); + + function = {}; + if (!--pendingItems && queue.empty()) { + cvEmpty.notify_all(); + } + if (handler) { continue; }