From 295a4177aebc5c306a84b0d2c49218496299b2ce Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Thu, 28 May 2020 16:05:04 +0000 Subject: [PATCH] Add ScopedEnvironmentVar class for setting env variables in BackendVk In the Vulkan backend, we only need to set VK_LAYER_PATH and VK_ICD_FILENAMES while we're gathering the device information. After this, we should unset the environment variables or they will persist and affect loading Vulkan again (from Dawn or another client) in the future. Bug: dawn:406 Change-Id: I30c38b0980e181126fcd7fa911bbf9e8aa35f3b8 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/22021 Reviewed-by: Stephen White Reviewed-by: Corentin Wallez Commit-Queue: Austin Eng --- src/common/SystemUtils.cpp | 26 ++++++++++++++ src/common/SystemUtils.h | 17 +++++++++ src/dawn_native/vulkan/BackendVk.cpp | 12 ++++--- src/tests/unittests/SystemUtilsTests.cpp | 45 ++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 5 deletions(-) diff --git a/src/common/SystemUtils.cpp b/src/common/SystemUtils.cpp index 73aa4ee640..f8282eb414 100644 --- a/src/common/SystemUtils.cpp +++ b/src/common/SystemUtils.cpp @@ -14,6 +14,8 @@ #include "common/SystemUtils.h" +#include "common/Assert.h" + #if defined(DAWN_PLATFORM_WINDOWS) # include # include @@ -115,3 +117,27 @@ std::string GetExecutableDirectory() { size_t lastPathSepLoc = exePath.find_last_of(GetPathSeparator()); return lastPathSepLoc != std::string::npos ? exePath.substr(0, lastPathSepLoc + 1) : ""; } + +// ScopedEnvironmentVar + +ScopedEnvironmentVar::ScopedEnvironmentVar(const char* variableName, const char* value) + : mName(variableName), + mOriginalValue(GetEnvironmentVar(variableName)), + mIsSet(SetEnvironmentVar(variableName, value)) { +} + +ScopedEnvironmentVar::~ScopedEnvironmentVar() { + if (mIsSet) { + bool success = SetEnvironmentVar(mName.c_str(), mOriginalValue.c_str()); + // If we set the environment variable in the constructor, we should never fail restoring it. + ASSERT(success); + } +} + +bool ScopedEnvironmentVar::Set(const char* variableName, const char* value) { + ASSERT(!mIsSet); + mName = variableName; + mOriginalValue = GetEnvironmentVar(variableName); + mIsSet = SetEnvironmentVar(variableName, value); + return mIsSet; +} diff --git a/src/common/SystemUtils.h b/src/common/SystemUtils.h index 2edf1e3a25..ed18c31e66 100644 --- a/src/common/SystemUtils.h +++ b/src/common/SystemUtils.h @@ -24,4 +24,21 @@ std::string GetEnvironmentVar(const char* variableName); bool SetEnvironmentVar(const char* variableName, const char* value); std::string GetExecutableDirectory(); +class ScopedEnvironmentVar { + public: + ScopedEnvironmentVar() = default; + ScopedEnvironmentVar(const char* variableName, const char* value); + ~ScopedEnvironmentVar(); + + ScopedEnvironmentVar(const ScopedEnvironmentVar& rhs) = delete; + ScopedEnvironmentVar& operator=(const ScopedEnvironmentVar& rhs) = delete; + + bool Set(const char* variableName, const char* value); + + private: + std::string mName; + std::string mOriginalValue; + bool mIsSet = false; +}; + #endif // COMMON_SYSTEMUTILS_H_ diff --git a/src/dawn_native/vulkan/BackendVk.cpp b/src/dawn_native/vulkan/BackendVk.cpp index eef62e8d39..fe9eba6dbc 100644 --- a/src/dawn_native/vulkan/BackendVk.cpp +++ b/src/dawn_native/vulkan/BackendVk.cpp @@ -109,14 +109,16 @@ namespace dawn_native { namespace vulkan { MaybeError Backend::Initialize(bool useSwiftshader) { DAWN_TRY(LoadVulkan(useSwiftshader)); - // TODO(crbug.com/dawn/406): In order to not modify the environment variables of - // the rest of an application embedding Dawn, we should set these only - // in the scope of this function. See ANGLE's ScopedVkLoaderEnvironment + // These environment variables need only be set while loading procs and gathering device + // info. + ScopedEnvironmentVar vkICDFilenames; + ScopedEnvironmentVar vkLayerPath; + if (useSwiftshader) { #if defined(DAWN_SWIFTSHADER_VK_ICD_JSON) std::string fullSwiftshaderICDPath = GetExecutableDirectory() + DAWN_SWIFTSHADER_VK_ICD_JSON; - if (!SetEnvironmentVar("VK_ICD_FILENAMES", fullSwiftshaderICDPath.c_str())) { + if (!vkICDFilenames.Set("VK_ICD_FILENAMES", fullSwiftshaderICDPath.c_str())) { return DAWN_INTERNAL_ERROR("Couldn't set VK_ICD_FILENAMES"); } #else @@ -128,7 +130,7 @@ namespace dawn_native { namespace vulkan { if (GetInstance()->IsBackendValidationEnabled()) { #if defined(DAWN_ENABLE_VULKAN_VALIDATION_LAYERS) std::string vkDataDir = GetExecutableDirectory() + DAWN_VK_DATA_DIR; - if (!SetEnvironmentVar("VK_LAYER_PATH", vkDataDir.c_str())) { + if (!vkLayerPath.Set("VK_LAYER_PATH", vkDataDir.c_str())) { return DAWN_INTERNAL_ERROR("Couldn't set VK_LAYER_PATH"); } #else diff --git a/src/tests/unittests/SystemUtilsTests.cpp b/src/tests/unittests/SystemUtilsTests.cpp index 540f4ef931..3730f45f13 100644 --- a/src/tests/unittests/SystemUtilsTests.cpp +++ b/src/tests/unittests/SystemUtilsTests.cpp @@ -39,3 +39,48 @@ TEST(SystemUtilsTests, GetExecutableDirectory) { // Test last charecter in path ASSERT_EQ(GetExecutableDirectory().back(), *GetPathSeparator()); } + +// Tests for ScopedEnvironmentVar +TEST(SystemUtilsTests, ScopedEnvironmentVar) { + SetEnvironmentVar("ScopedEnvironmentVarForTest", "original"); + + // Test empty environment variable doesn't crash + { ScopedEnvironmentVar var; } + + // Test setting empty environment variable + { + ScopedEnvironmentVar var; + var.Set("ScopedEnvironmentVarForTest", "NewEnvironmentVarValue"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "NewEnvironmentVarValue"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test that the environment variable can be set, and it is unset at the end of the scope. + { + ScopedEnvironmentVar var("ScopedEnvironmentVarForTest", "NewEnvironmentVarValue"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "NewEnvironmentVarValue"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test nested scopes + { + ScopedEnvironmentVar outer("ScopedEnvironmentVarForTest", "outer"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer"); + { + ScopedEnvironmentVar inner("ScopedEnvironmentVarForTest", "inner"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "inner"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); + + // Test redundantly setting scoped variables + { + ScopedEnvironmentVar var1("ScopedEnvironmentVarForTest", "var1"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var1"); + + ScopedEnvironmentVar var2("ScopedEnvironmentVarForTest", "var2"); + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var2"); + } + ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original"); +}