Improve ScopedEnvironmentVar to distinguish between null and empty
Found while investigating Swiftshader support. Some Vulkan loaders
fail to create an instance if VK_ICD_FILENAMES is empty string
rather than entirely absent. It was set to empty string because
Dawn did not distinguish between nonexistent environment variables
and the empty string. This CL adds distinguishing between the two,
including tests for the behavior.
Bug: chromium:1266550
Change-Id: I1680a281f62e6b340009e01da65db9d485e2975e
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/69520
Auto-Submit: Austin Eng <enga@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>
diff --git a/examples/SampleUtils.cpp b/examples/SampleUtils.cpp
index 8c7ea0c..fff8840 100644
--- a/examples/SampleUtils.cpp
+++ b/examples/SampleUtils.cpp
@@ -92,8 +92,9 @@
static utils::TerribleCommandBuffer* s2cBuf = nullptr;
wgpu::Device CreateCppDawnDevice() {
- if (GetEnvironmentVar("ANGLE_DEFAULT_PLATFORM").empty()) {
- SetEnvironmentVar("ANGLE_DEFAULT_PLATFORM", "swiftshader");
+ ScopedEnvironmentVar angleDefaultPlatform;
+ if (GetEnvironmentVar("ANGLE_DEFAULT_PLATFORM").first.empty()) {
+ angleDefaultPlatform.Set("ANGLE_DEFAULT_PLATFORM", "swiftshader");
}
glfwSetErrorCallback(PrintGLFWError);
diff --git a/src/common/SystemUtils.cpp b/src/common/SystemUtils.cpp
index 58aac01..b56a08c 100644
--- a/src/common/SystemUtils.cpp
+++ b/src/common/SystemUtils.cpp
@@ -15,6 +15,7 @@
#include "common/SystemUtils.h"
#include "common/Assert.h"
+#include "common/Log.h"
#if defined(DAWN_PLATFORM_WINDOWS)
# include <Windows.h>
@@ -35,21 +36,29 @@
return "\\";
}
-std::string GetEnvironmentVar(const char* variableName) {
+std::pair<std::string, bool> GetEnvironmentVar(const char* variableName) {
// First pass a size of 0 to get the size of variable value.
- char* tempBuf = nullptr;
- DWORD result = GetEnvironmentVariableA(variableName, tempBuf, 0);
- if (result == 0) {
- return "";
+ DWORD sizeWithNullTerminator = GetEnvironmentVariableA(variableName, nullptr, 0);
+ if (sizeWithNullTerminator == 0) {
+ DWORD err = GetLastError();
+ if (err != ERROR_ENVVAR_NOT_FOUND) {
+ dawn::WarningLog() << "GetEnvironmentVariableA failed with code " << err;
+ }
+ return std::make_pair(std::string(), false);
}
// Then get variable value with its actual size.
- std::vector<char> buffer(result + 1);
- if (GetEnvironmentVariableA(variableName, buffer.data(), static_cast<DWORD>(buffer.size())) ==
- 0) {
- return "";
+ std::vector<char> buffer(sizeWithNullTerminator);
+ DWORD sizeStored =
+ GetEnvironmentVariableA(variableName, buffer.data(), static_cast<DWORD>(buffer.size()));
+ if (sizeStored + 1 != sizeWithNullTerminator) {
+ DWORD err = GetLastError();
+ if (err) {
+ dawn::WarningLog() << "GetEnvironmentVariableA failed with code " << err;
+ }
+ return std::make_pair(std::string(), false);
}
- return std::string(buffer.data());
+ return std::make_pair(std::string(buffer.data(), sizeStored), true);
}
bool SetEnvironmentVar(const char* variableName, const char* value) {
@@ -60,12 +69,16 @@
return "/";
}
-std::string GetEnvironmentVar(const char* variableName) {
+std::pair<std::string, bool> GetEnvironmentVar(const char* variableName) {
char* value = getenv(variableName);
- return value == nullptr ? "" : std::string(value);
+ return value == nullptr ? std::make_pair(std::string(), false)
+ : std::make_pair(std::string(value), true);
}
bool SetEnvironmentVar(const char* variableName, const char* value) {
+ if (value == nullptr) {
+ return unsetenv(variableName) == 0;
+ }
return setenv(variableName, value, 1) == 0;
}
#else
@@ -133,7 +146,8 @@
ScopedEnvironmentVar::~ScopedEnvironmentVar() {
if (mIsSet) {
- bool success = SetEnvironmentVar(mName.c_str(), mOriginalValue.c_str());
+ bool success = SetEnvironmentVar(
+ mName.c_str(), mOriginalValue.second ? mOriginalValue.first.c_str() : nullptr);
// If we set the environment variable in the constructor, we should never fail restoring it.
ASSERT(success);
}
diff --git a/src/common/SystemUtils.h b/src/common/SystemUtils.h
index 8e21dea..63cf541 100644
--- a/src/common/SystemUtils.h
+++ b/src/common/SystemUtils.h
@@ -20,7 +20,9 @@
#include <string>
const char* GetPathSeparator();
-std::string GetEnvironmentVar(const char* variableName);
+// Returns a pair of the environment variable's value, and a boolean indicating whether the variable
+// was present.
+std::pair<std::string, bool> GetEnvironmentVar(const char* variableName);
bool SetEnvironmentVar(const char* variableName, const char* value);
std::string GetExecutableDirectory();
@@ -42,7 +44,7 @@
private:
std::string mName;
- std::string mOriginalValue;
+ std::pair<std::string, bool> mOriginalValue;
bool mIsSet = false;
};
diff --git a/src/tests/DawnTest.cpp b/src/tests/DawnTest.cpp
index ff07cb1..4b7a62e 100644
--- a/src/tests/DawnTest.cpp
+++ b/src/tests/DawnTest.cpp
@@ -436,9 +436,11 @@
#ifdef DAWN_ENABLE_BACKEND_OPENGLES
- if (GetEnvironmentVar("ANGLE_DEFAULT_PLATFORM").empty()) {
- SetEnvironmentVar("ANGLE_DEFAULT_PLATFORM", "swiftshader");
+ ScopedEnvironmentVar angleDefaultPlatform;
+ if (GetEnvironmentVar("ANGLE_DEFAULT_PLATFORM").first.empty()) {
+ angleDefaultPlatform.Set("ANGLE_DEFAULT_PLATFORM", "swiftshader");
}
+
if (!glfwInit()) {
return instance;
}
diff --git a/src/tests/unittests/SystemUtilsTests.cpp b/src/tests/unittests/SystemUtilsTests.cpp
index 3730f45..e754701 100644
--- a/src/tests/unittests/SystemUtilsTests.cpp
+++ b/src/tests/unittests/SystemUtilsTests.cpp
@@ -12,32 +12,38 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+#include <gmock/gmock-matchers.h>
#include <gtest/gtest.h>
+#include "common/Assert.h"
#include "common/SystemUtils.h"
+using ::testing::_;
+using ::testing::Pair;
+
// Tests for GetEnvironmentVar
TEST(SystemUtilsTests, GetEnvironmentVar) {
// Test nonexistent environment variable
- ASSERT_EQ(GetEnvironmentVar("NonexistentEnvironmentVar"), "");
+ EXPECT_THAT(GetEnvironmentVar("NonexistentEnvironmentVar"), Pair("", false));
}
// Tests for SetEnvironmentVar
TEST(SystemUtilsTests, SetEnvironmentVar) {
// Test new environment variable
- ASSERT_TRUE(SetEnvironmentVar("EnvironmentVarForTest", "NewEnvironmentVarValue"));
- ASSERT_EQ(GetEnvironmentVar("EnvironmentVarForTest"), "NewEnvironmentVarValue");
+ EXPECT_TRUE(SetEnvironmentVar("EnvironmentVarForTest", "NewEnvironmentVarValue"));
+ EXPECT_THAT(GetEnvironmentVar("EnvironmentVarForTest"), Pair("NewEnvironmentVarValue", true));
// Test override environment variable
- ASSERT_TRUE(SetEnvironmentVar("EnvironmentVarForTest", "OverrideEnvironmentVarValue"));
- ASSERT_EQ(GetEnvironmentVar("EnvironmentVarForTest"), "OverrideEnvironmentVarValue");
+ EXPECT_TRUE(SetEnvironmentVar("EnvironmentVarForTest", "OverrideEnvironmentVarValue"));
+ EXPECT_THAT(GetEnvironmentVar("EnvironmentVarForTest"),
+ Pair("OverrideEnvironmentVarValue", true));
}
// Tests for GetExecutableDirectory
TEST(SystemUtilsTests, GetExecutableDirectory) {
// Test returned value is non-empty string
- ASSERT_NE(GetExecutableDirectory(), "");
+ EXPECT_NE(GetExecutableDirectory(), "");
// Test last charecter in path
- ASSERT_EQ(GetExecutableDirectory().back(), *GetPathSeparator());
+ EXPECT_EQ(GetExecutableDirectory().back(), *GetPathSeparator());
}
// Tests for ScopedEnvironmentVar
@@ -51,36 +57,59 @@
{
ScopedEnvironmentVar var;
var.Set("ScopedEnvironmentVarForTest", "NewEnvironmentVarValue");
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "NewEnvironmentVarValue");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"),
+ Pair("NewEnvironmentVarValue", true));
}
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("original", true));
// 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");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"),
+ Pair("NewEnvironmentVarValue", true));
}
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("original", true));
// Test nested scopes
{
ScopedEnvironmentVar outer("ScopedEnvironmentVarForTest", "outer");
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("outer", true));
{
ScopedEnvironmentVar inner("ScopedEnvironmentVarForTest", "inner");
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "inner");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("inner", true));
}
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "outer");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("outer", true));
}
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("original", true));
// Test redundantly setting scoped variables
{
ScopedEnvironmentVar var1("ScopedEnvironmentVarForTest", "var1");
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var1");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("var1", true));
ScopedEnvironmentVar var2("ScopedEnvironmentVarForTest", "var2");
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "var2");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("var2", true));
}
- ASSERT_EQ(GetEnvironmentVar("ScopedEnvironmentVarForTest"), "original");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("original", true));
+}
+
+// Test that restoring a scoped environment variable to the empty string.
+TEST(SystemUtilsTests, ScopedEnvironmentVarRestoresEmptyString) {
+ ScopedEnvironmentVar empty("ScopedEnvironmentVarForTest", "");
+ {
+ ScopedEnvironmentVar var1("ScopedEnvironmentVarForTest", "var1");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("var1", true));
+ }
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("", true));
+}
+
+// Test that restoring a scoped environment variable to not set (distuishable from empty string)
+// works.
+TEST(SystemUtilsTests, ScopedEnvironmentVarRestoresNotSet) {
+ ScopedEnvironmentVar null("ScopedEnvironmentVarForTest", nullptr);
+ {
+ ScopedEnvironmentVar var1("ScopedEnvironmentVarForTest", "var1");
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("var1", true));
+ }
+ EXPECT_THAT(GetEnvironmentVar("ScopedEnvironmentVarForTest"), Pair("", false));
}
