[SYCL] Fix SYCL config processing for DPC++ RT (#4366)

Fix SYCL configuration file processing for DPCPP runtime.
The reason for this change is that the config for SYCL broke
from the simplest changes, for example, from a comment in
the config. Without these changes, it was a dangerous code
that may fail and even don't send an error.
What's new:

 1. Rewritten line processing;
 2. Supported format:
 3. ConfigName=Value #comment;
 4. Skip lines which do not contain variables;
 5. Throw error message if variable is incorrect;
 6. Several tests for new functionality.

Author: denis-kabanov

sycl/source/detail/config.cpp

@@ -33,8 +33,6 @@ namespace detail {
 #include "detail/config.def"
 #undef CONFIG
 
-#define MAX_CONFIG_NAME 256
-
 static void initValue(const char *Key, const char *Value) {
 #define CONFIG(Name, MaxSize, CompileTimeDef)                                  \
   if (0 == strncmp(Key, SYCLConfigBase<Name>::MConfigName, MAX_CONFIG_NAME)) { \
@@ -47,10 +45,11 @@ static void initValue(const char *Key, const char *Value) {
 #undef CONFIG
 }
 
-void readConfig() {
+void readConfig(bool ForceInitialization) {
   static bool Initialized = false;
-  if (Initialized)
+  if (!ForceInitialization && Initialized) {
     return;
+  }
 
   std::fstream File;
   if (const char *ConfigFile = getenv("SYCL_CONFIG_FILE_NAME"))
@@ -62,39 +61,85 @@ void readConfig() {
   }
 
   if (File.is_open()) {
-    // TODO: Use max size from macro instead of 256
-    char Key[MAX_CONFIG_NAME] = {0}, Value[256] = {0};
+    char Key[MAX_CONFIG_NAME] = {0}, Value[MAX_CONFIG_VALUE] = {0};
+    std::string BufString;
+    std::size_t Position = std::string::npos;
     while (!File.eof()) {
-      // Expected fromat:
+      // Expected format:
       // ConfigName=Value\r
+      // ConfigName=Value #comment
       // ConfigName=Value
       // TODO: Skip spaces before and after '='
-      File.getline(Key, sizeof(Key), '=');
-      if (File.fail()) {
-        // Fail to process the line. Skip it completely and try next one.
-        // Do we want to restore here? Or just throw an exception?
+      std::getline(File, BufString);
+      if (File.fail() && !File.eof()) {
+        // Fail to process the line.
         File.clear(File.rdstate() & ~std::ios_base::failbit);
         File.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+        throw sycl::exception(
+            make_error_code(errc::runtime),
+            "An error occurred while attempting to read a line");
+      }
+      // Handle '\r'
+      if ((BufString.length() > 0) &&
+          (BufString[BufString.length() - 1] == '\r')) {
+        BufString.pop_back();
+      }
+      // Handle comments
+      if (BufString.find("#") != std::string::npos) {
+        BufString.erase(BufString.find("#"));
+        while ((BufString.length() > 0) &&
+               (BufString[BufString.length() - 1] == ' ')) {
+          BufString.pop_back();
+        }
+      }
+      // Skip lines with a length = 0 or which don't have "="
+      if ((BufString.length() == 0) ||
+          (BufString.find("=") == std::string::npos)) {
         continue;
       }
-      File.getline(Value, sizeof(Value), '\n');
-
-      if (File.fail()) {
-        // Fail to process the value while config name is OK. It's likely that
-        // value is too long. Currently just deal what we have got and ignore
-        // remaining characters on the line.
-        // Do we want to restore here? Or just throw an exception?
-        File.clear(File.rdstate() & ~std::ios_base::failbit);
-        File.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+      // Finding the position of '='
+      Position = BufString.find("=");
+      // Checking that the variable name is less than MAX_CONFIG_NAME and more
+      // than zero character
+      if ((Position <= MAX_CONFIG_NAME) && (Position > 0)) {
+        // Checking that the value is less than MAX_CONFIG_VALUE and
+        // more than zero character
+        if ((BufString.length() - (Position + 1) <= MAX_CONFIG_VALUE) &&
+            (BufString.length() != Position + 1)) {
+          // Checking for spaces at the beginning and end of the line,
+          // before and after '='
+          if ((BufString[0] == ' ') ||
+              (BufString[BufString.length() - 1] == ' ') ||
+              (BufString[Position - 1] == ' ') ||
+              (BufString[Position + 1] == ' ')) {
+            throw sycl::exception(
+                make_error_code(errc::runtime),
+                "SPACE found at the beginning/end of the line "
+                "or before/after '='");
+          }
+          // Creating pairs of (key, value)
+          BufString.copy(Key, Position, 0);
+          Key[Position] = '\0';
+          BufString.copy(Value, BufString.length() - (Position + 1),
+                         Position + 1);
+          Value[BufString.length() - (Position + 1)] = '\0';
+        } else {
+          throw sycl::exception(
+              make_error_code(errc::runtime),
+              "The value contains more than " +
+                  std::to_string(MAX_CONFIG_VALUE) +
+                  " characters or does not contain them at all");
+        }
+      } else {
+        throw sycl::exception(make_error_code(errc::runtime),
+                              "Variable name is more than " +
+                                  std::to_string(MAX_CONFIG_NAME) +
+                                  " or less than one character");
       }
 
-      // Handle '\r' by nullifying it
-      const std::streamsize ReadSybmols = File.gcount();
-      if (ReadSybmols > 1 && '\r' == Value[ReadSybmols - 2])
-        Value[ReadSybmols - 2] = '\0';
-
       initValue(Key, Value);
     }
+    File.close();
   }
   Initialized = true;
 }

sycl/source/detail/config.hpp

@@ -43,6 +43,9 @@ constexpr bool ConfigFromCompileDefEnabled = false;
 constexpr bool ConfigFromCompileDefEnabled = true;
 #endif // DISABLE_CONFIG_FROM_COMPILE_TIME
 
+constexpr int MAX_CONFIG_NAME = 256;
+constexpr int MAX_CONFIG_VALUE = 256;
+
 // Enum of config IDs for accessing other arrays
 enum ConfigID {
   START = 0,
@@ -58,7 +61,7 @@ constexpr const char *getStrOrNullptr(const char *Str) {
 }
 
 // Intializes configs from the configuration file
-void readConfig();
+void readConfig(bool ForceInitialization = false);
 
 template <ConfigID Config> class SYCLConfigBase;
 

sycl/test/regression/check_config_processing.cpp

@@ -0,0 +1,33 @@
+// RUN: %clangxx -fsycl %s -o %t.out
+//
+// RUN: env TEST=SET_CORRECT_ENVIRONMENT %t.out > %t.conf
+// RUN: env TEST=CORRECT_CONFIG_FILE env SYCL_CONFIG_FILE_NAME=%t.conf %t.out
+// RUN: ls *.dot
+// RUN: rm *.dot
+
+#include <CL/sycl.hpp>
+#include <iostream>
+#include <regex>
+
+int main() {
+  std::string testEnvVarValue = getenv("TEST");
+
+  // Сonfig creation
+  if (testEnvVarValue == "SET_CORRECT_ENVIRONMENT") {
+    std::cout << "#\n"
+              << "#abc\n"
+              << "# abc = cba \r\n"
+              << "\r\n"
+              << "SYCL_PRINT_EXECUTION_GRAPH=always\r\n"
+              << std::endl;
+    return 0;
+  }
+
+  // Config check
+  if (testEnvVarValue == "CORRECT_CONFIG_FILE") {
+    sycl::buffer<int, 1> Buf(sycl::range<1>{1});
+    auto Acc = Buf.get_access<sycl::access::mode::read>();
+    return 0;
+  }
+  throw std::logic_error("Environment is incorrect");
+}

sycl/unittests/CMakeLists.txt

@@ -12,6 +12,7 @@ string(TOLOWER "${CMAKE_BUILD_TYPE}" build_type_lower)
 include(AddSYCLUnitTest)
 
 add_subdirectory(allowlist)
+add_subdirectory(config)
 add_subdirectory(misc)
 add_subdirectory(pi)
 add_subdirectory(kernel-and-program)

sycl/unittests/config/CMakeLists.txt

@@ -0,0 +1,7 @@
+set(CMAKE_CXX_EXTENSIONS OFF)
+
+# Enable exception handling for these unit tests
+set(LLVM_REQUIRES_EH 1)
+add_sycl_unittest(ConfigTests OBJECT 
+    ConfigTests.cpp
+)