Sanitizer fixes (#2280)

This commit is contained in:
Tim Sylvester
2024-04-29 06:33:08 -07:00
committed by GitHub
parent 6ebbc3dd6a
commit cd9727b06f
14 changed files with 85 additions and 37 deletions

View File

@@ -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

View File

@@ -3,10 +3,11 @@
// Forward-declarations for metal-cpp types, allowing
// references without including any third-party headers
#include <memory>
#include <cstdint>
namespace CA {
class MetalDrawable;
class MetalLayer;
} // namespace CA
namespace NS {
@@ -39,7 +40,9 @@ class VertexDescriptor;
namespace mbgl {
namespace mtl {
using CAMetalDrawablePtr = NS::SharedPtr<CA::MetalDrawable>;
using CAMetalLayerPtr = NS::SharedPtr<CA::MetalLayer>;
using MTLBlitCommandEncoderPtr = NS::SharedPtr<MTL::BlitCommandEncoder>;
using MTLBlitPassDescriptorPtr = NS::SharedPtr<MTL::BlitPassDescriptor>;

View File

@@ -1,8 +1,8 @@
#pragma once
#include <mbgl/style/layers/custom_layer_render_parameters.hpp>
#include <mbgl/mtl/mtl_fwd.hpp>
#include <Foundation/Foundation.hpp>
#include <Metal/Metal.hpp>

View File

@@ -4,10 +4,11 @@
#include <mbgl/mtl/context.hpp>
#include <mbgl/gfx/backend_scope.hpp>
#include <Metal/Metal.hpp>
#include <cassert>
#include <stdexcept>
#include <type_traits>
#include <Metal/Metal.hpp>
namespace mbgl {
namespace mtl {

View File

@@ -1,11 +1,15 @@
#include "metal_backend.h"
#include <mbgl/mtl/mtl_fwd.hpp>
#include <mbgl/mtl/renderable_resource.hpp>
#include <Metal/Metal.hpp>
#include <QuartzCore/CAMetalLayer.hpp>
namespace mbgl {
using namespace mtl;
class MetalRenderableResource final: public mtl::RenderableResource {
public:
MetalRenderableResource(MetalBackend &backend):
@@ -92,17 +96,17 @@ public:
return renderPassDescriptor;
}
NS::SharedPtr<CA::MetalLayer> getSwapchain() {
const CAMetalLayerPtr& getSwapchain() const {
return swapchain;
}
private:
MetalBackend &rendererBackend;
NS::SharedPtr<MTL::CommandQueue> commandQueue;
NS::SharedPtr<MTL::CommandBuffer> commandBuffer;
NS::SharedPtr<MTL::RenderPassDescriptor> renderPassDescriptor;
NS::SharedPtr<CA::MetalDrawable> surface;
NS::SharedPtr<CA::MetalLayer> swapchain;
MTLCommandQueuePtr commandQueue;
MTLCommandBufferPtr commandBuffer;
MTLRenderPassDescriptorPtr renderPassDescriptor;
CAMetalDrawablePtr surface;
CAMetalLayerPtr swapchain;
gfx::Texture2DPtr depthTexture;
gfx::Texture2DPtr stencilTexture;
mbgl::Size size;

View File

@@ -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"],
)

View File

@@ -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

View File

@@ -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

View File

@@ -69,8 +69,8 @@ void FeatureIndex::query(std::unordered_map<std::string, std::vector<Feature>>&
// Determine query radius
const auto pixelsToTileUnits = static_cast<float>(util::EXTENT / tileSize / scale);
const int16_t additionalPadding = std::min<int16_t>(
util::EXTENT, static_cast<int16_t>(additionalQueryPadding * pixelsToTileUnits));
const int16_t additionalPadding = static_cast<int16_t>(
std::min(static_cast<float>(util::EXTENT), additionalQueryPadding * pixelsToTileUnits));
// Query the grid index
mapbox::geometry::box<int16_t> box = mapbox::geometry::envelope(queryGeometry);

View File

@@ -81,6 +81,11 @@ BufferResource Context::createBuffer(
return {const_cast<Context&>(*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<NS::Object*> rawDefines;
std::vector<const NS::Object*> 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<MTL::Function> 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<MTL::Function> fragmentFunction;
MTLFunctionPtr fragmentFunction;
if (!fragmentName.empty()) {
const auto nsFragName = NS::String::string(fragmentName.data(), NS::UTF8StringEncoding);
fragmentFunction = NS::TransferPtr(library->newFunction(nsFragName));

View File

@@ -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);
}
}
}

View File

@@ -4,9 +4,10 @@
#include <mbgl/programs/attributes.hpp>
#include <mbgl/programs/uniforms.hpp>
#include <mbgl/programs/textures.hpp>
#include <mbgl/util/geometry.hpp>
#include <mbgl/renderer/layers/render_line_layer.hpp>
#include <mbgl/renderer/cross_faded_property_evaluator.hpp>
#include <mbgl/util/geometry.hpp>
#include <cmath>
namespace mbgl {
@@ -55,8 +56,8 @@ public:
return LayoutVertex{
{{static_cast<int16_t>((p.x * 2) | (round ? 1 : 0)), static_cast<int16_t>((p.y * 2) | (up ? 1 : 0))}},
{{// add 128 to store a byte in an unsigned byte
static_cast<uint8_t>(::round(extrudeScale * e.x) + 128),
static_cast<uint8_t>(::round(extrudeScale * e.y) + 128),
static_cast<uint8_t>(util::clamp(::round(extrudeScale * e.x) + 128, 0., 255.)),
static_cast<uint8_t>(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`

View File

@@ -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 {};

View File

@@ -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<std::mutex> inner_lock(mutex);
if (queue.empty()) {
cvEmpty.notify_all();
}
}
} catch (...) {
lock.lock();
std::unique_lock<std::mutex> inner_lock(mutex);
if (handler) {
handler(std::current_exception());
}
cleanup();
function = {};
if (!--pendingItems && queue.empty()) {
cvEmpty.notify_all();
}
if (handler) {
continue;
}