Switch from llvm::StringRef to char* for Toggle name to prevent static initializers

[chrdavis]

Small change to switch from llvm::StringRef to char* to prevent incurring a large number of static initializers. See dump of static initializers before and after this change from a release build of dxcompiler.dll.

static_initializers_after.txt
static_initializers_before.txt

Fixes #6896

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvm-beanz]

I think this is the wrong approach. In modern LLVM StringRef's constructor is a constant expression, so it doesn't have a global initializer.

We should probably fix the problem that way rather than giving up the safety that StringRef provides over unbound character pointers.

[llvm-beanz]

This change should have a potentially larger impact on static initialization without any loss of safety:

diff --git a/include/llvm/ADT/StringRef.h b/include/llvm/ADT/StringRef.h
index 957707f427c..911a7b6bc9b 100644
--- a/include/llvm/ADT/StringRef.h
+++ b/include/llvm/ADT/StringRef.h
@@ -74,7 +74,7 @@ namespace llvm {
       }
 
     /// Construct a string ref from a pointer and length.
-    /*implicit*/ StringRef(const char *data, size_t length)
+    /*implicit*/ constexpr StringRef(const char *data, size_t length)
       : Data(data), Length(length) {
         assert((data || length == 0) &&
         "StringRef cannot be built from a NULL argument with non-null length");

[bogner]

Upstream LLVM has StringLiteral which would be appropriate here, but it looks like that was added in late 2016 and isn't available in DXC. Using a constexpr StringRef should probably do the trick though.

[chrdavis]

This change should have a potentially larger impact on static initialization without any loss of safety:

diff --git a/include/llvm/ADT/StringRef.h b/include/llvm/ADT/StringRef.h
index 957707f427c..911a7b6bc9b 100644
--- a/include/llvm/ADT/StringRef.h
+++ b/include/llvm/ADT/StringRef.h
@@ -74,7 +74,7 @@ namespace llvm {
       }
 
     /// Construct a string ref from a pointer and length.
-    /*implicit*/ StringRef(const char *data, size_t length)
+    /*implicit*/ constexpr StringRef(const char *data, size_t length)
       : Data(data), Length(length) {
         assert((data || length == 0) &&
         "StringRef cannot be built from a NULL argument with non-null length");

I made that change but it does not result in removal of the static initializers.

[llvm-beanz]

I made that change but it does not result in removal of the static initializers.

What compiler are you using and how are you checking for static initializers? Technically neither of these approaches eliminates static initializers, both just make the initializer trivial so the compiler can replace a non-trivial initialization with a compile-time constant.

If the patch I provided doesn't eliminate the non-trivial initializer, that's likely a bug/limitation in the compiler you're building with. I'm not sure I like the tradeoff of replacing a bounds-checked data type with a raw pointer given the number of security-focused issues we've been addressing in DXC this year.

[llvm-beanz]

Nvm... my patch put the constexpr tag on the wrong constructor. You also need to mark the Toggle constructor as constexpr and frankly, we might as well mark the variables as constexpr too, because they're static constants and we don't want them to have constructors.

This works for me locally:

diff --git a/include/dxc/Support/DxcOptToggles.h b/include/dxc/Support/DxcOptToggles.h
index 04c4c684966..de40cfbcf09 100644
--- a/include/dxc/Support/DxcOptToggles.h
+++ b/include/dxc/Support/DxcOptToggles.h
@@ -27,7 +27,8 @@ namespace options {
 struct Toggle {
   llvm::StringRef Name;
   bool Default = false;
-  Toggle(llvm::StringRef Name, bool Default) : Name(Name), Default(Default) {}
+  constexpr Toggle(llvm::StringRef Name, bool Default)
+      : Name(Name), Default(Default) {}
 };
 
 enum {
@@ -35,20 +36,20 @@ enum {
   DEFAULT_OFF = 0,
 };
 
-static const Toggle TOGGLE_GVN = {"gvn", DEFAULT_ON};
-static const Toggle TOGGLE_LICM = {"licm", DEFAULT_ON};
-static const Toggle TOGGLE_SINK = {"sink", DEFAULT_ON};
-static const Toggle TOGGLE_ENABLE_AGGRESSIVE_REASSOCIATION = {
+static constexpr Toggle TOGGLE_GVN = {"gvn", DEFAULT_ON};
+static constexpr Toggle TOGGLE_LICM = {"licm", DEFAULT_ON};
+static constexpr Toggle TOGGLE_SINK = {"sink", DEFAULT_ON};
+static constexpr Toggle TOGGLE_ENABLE_AGGRESSIVE_REASSOCIATION = {
     "aggressive-reassociation", DEFAULT_ON};
-static const Toggle TOGGLE_LIFETIME_MARKERS = {"lifetime-markers", DEFAULT_ON};
-static const Toggle TOGGLE_PARTIAL_LIFETIME_MARKERS = {
+static constexpr Toggle TOGGLE_LIFETIME_MARKERS = {"lifetime-markers",
+                                                   DEFAULT_ON};
+static constexpr Toggle TOGGLE_PARTIAL_LIFETIME_MARKERS = {
     "partial-lifetime-markers", DEFAULT_OFF};
-static const Toggle TOGGLE_STRUCTURIZE_LOOP_EXITS_FOR_UNROLL = {
+static constexpr Toggle TOGGLE_STRUCTURIZE_LOOP_EXITS_FOR_UNROLL = {
     "structurize-loop-exits-for-unroll", DEFAULT_ON};
-static const Toggle TOGGLE_DEBUG_NOPS = {"debug-nops", DEFAULT_ON};
-static const Toggle TOGGLE_STRUCTURIZE_RETURNS = {"structurize-returns",
-                                                  DEFAULT_OFF};
-
+static constexpr Toggle TOGGLE_DEBUG_NOPS = {"debug-nops", DEFAULT_ON};
+static constexpr Toggle TOGGLE_STRUCTURIZE_RETURNS = {"structurize-returns",
+                                                      DEFAULT_OFF};
 struct OptimizationToggles {
   // Optimization pass enables, disables and selects
   std::map<std::string, bool> Toggles; // OPT_opt_enable & OPT_opt_disable
diff --git a/include/llvm/ADT/StringRef.h b/include/llvm/ADT/StringRef.h
index 957707f427c..c103fdbf3b9 100644
--- a/include/llvm/ADT/StringRef.h
+++ b/include/llvm/ADT/StringRef.h
@@ -66,19 +66,20 @@ namespace llvm {
     /*implicit*/ StringRef() : Data(nullptr), Length(0) {}
     StringRef(std::nullptr_t) = delete; // HLSL Change - So we don't accidentally pass `false` again
 
+    // HLSL Change - Begin - Make StringRef constructors constexpr.
     /// Construct a string ref from a cstring.
-    /*implicit*/ StringRef(const char *Str)
-      : Data(Str) {
-        assert(Str && "StringRef cannot be built from a NULL argument");
-        Length = ::strlen(Str); // invoking strlen(NULL) is undefined behavior
-      }
+    /*implicit*/ constexpr StringRef(const char *Str)
+        : Data(Str), Length(Str ? std::char_traits<char>::length(Str) : 0) {
+      assert(Str && "StringRef cannot be built from a NULL argument");
+    }
 
     /// Construct a string ref from a pointer and length.
-    /*implicit*/ StringRef(const char *data, size_t length)
-      : Data(data), Length(length) {
-        assert((data || length == 0) &&
-        "StringRef cannot be built from a NULL argument with non-null length");
-      }
+    /*implicit*/ constexpr StringRef(const char *data, size_t length)
+        : Data(data), Length(length) {
+      assert((data || length == 0) && "StringRef cannot be built from a NULL "
+                                      "argument with non-null length");
+    }
+    // HLSL Change - End - Make StringRef constructors constexpr.
 
     /// Construct a string ref from an std::string.
     /*implicit*/ StringRef(const std::string &Str)

[chrdavis]

Thanks I've updated the change and verified the static initializers are gone after building clean.

[chrdavis]

I need one more reviewer. Would love to close this out before the holidays.