[Commandline] Move general LLVMSupport options into Commandline.cpp [NFC]

[hintonda]

Summary

This patch moves general LLVMSupport options from Debug.cpp and
Signals.cpp into CommandLine.cpp so they are only constructed if
CommandLine.o is actually loaded when statically linked. This can
help in following cases:

1. LLVMSupport gets linked in multiple times, e.g., in a library and also in the final application, which will fail with an assert "Option 'X' registered more than once!". This is true whether or not any symbols from CommandLine.cpp are referenced in the final application, e.g., parseCommandLineOptions.
2. Applications that don't reference any symbols from CommandLine.cpp don't have to pay for constructing unused static Options -- these general options all use cl::location, so the options are only needed for parsing. This can result in smaller binaries and faster startup times.

This patch depends on D71169.

There are other options in LLVMSupport, e.g., timing, statistics,
debug counters, etc, but they mostly used outside of LLVMSupport,
and aren't applicable to this patch.

[serge-sans-paille]

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

[hintonda]

In D71228#1777711, @serge-sans-paille wrote:

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

When you statically link in LLVMSupport, you only load what you reference. In the case of Debug options, they are referenced all over, so that TU gets loaded. If it also included static Options, which register themselves with the global parser in CommandLine.cpp, that TU gets loaded as well. By moving the Debug options over to CommandLine.cpp, those statics won't be loaded unless the TU is loaded, which will only happened if the user calls function in CommandLine.cpp.

We have a case where we use functionality in LLVMSupport, but don't call parseCommandLineOptions. We can still access the DebugFlag, etc, but don't actually need the option since it is only used by the parser -- those options use cl::location, so the global already exists. Hope that makes sense.

[hintonda]

In D71228#1778285, @hintonda wrote:

In D71228#1777711, @serge-sans-paille wrote:

This patch moves Debug options into CommandLine.cpp so they are only constructed if parseCommandLineOptions is actually called.

This would be the case if the options were static to the parseCommandLineOptions, but I fail ti understand why current change gives this property. Can you explain me why?

When you statically link in LLVMSupport, you only load what you reference. In the case of Debug options, they are referenced all over, so that TU gets loaded. If it also included static Options, which register themselves with the global parser in CommandLine.cpp, that TU gets loaded as well. By moving the Debug options over to CommandLine.cpp, those statics won't be loaded unless the TU is loaded, which will only happened if the user calls function in CommandLine.cpp.

We have a case where we use functionality in LLVMSupport, but don't call parseCommandLineOptions. We can still access the DebugFlag, etc, but don't actually need the option since it is only used by the parser -- those options use cl::location, so the global already exists. Hope that makes sense.

Btw, this is true for any symbol in CommandLine.cpp, not just parseCommandLineOptions. Sorry if I wasn't clear.

[serge-sans-paille]

Thanks @hintonda for the clarification! I've added an extra question, LGTM otherwise.

llvm/lib/Support/Debug.cpp

81–82

Why did you keep that definition here?

[hintonda]

llvm/lib/Support/Debug.cpp

81–82

Originally, the option was named DebugBufferSize and only used in llvm::dbgs() below.

I renamed it to DebugBufferSizeOpt when I moved to CommandLine.cpp, and added a cl::location attribute referencing the new DebugBufferSize variable. Might make sense to put it in the llvm namespace though.

[hintonda]

• Also move over DisableSymbolication option from Signals.cpp, and put all variables in llvm namespace.

[hintonda]

post holiday ping...

[serge-sans-paille]

Giving some extra thoughts to that PR. I'm not a big fan of giving external visibility to flags like DisableSymbolicationFlag or DebugBufferSize. I certainly see value in activating them only on debug mode, and that could be a standalone patch.

Concerning the added value of having no options in LLVMSupport, isn't it an abstraction breakage to have debug-related stuff in the commandline implementation? It doesn't make more sense to me to have them in CL than in Support... And as you stated in the description, this doesn't solve the problem with other cl::... options in LLVMSupport.

If we want an option-less version of LLVMSupport, then I'd advocate for static variables with static initializer + getter/setter in LLVMSupport, and a separate library , say LLVMSupportCLOptions that would just use the setter/getter to properly modify LLVMSupport behavior. Just my opinion though, you probably want more insights from more people.

llvm/include/llvm/Support/CommandLine.h

public:
 
   template <class T> void addValue(const T &V) {
     assert(Location != 0 && "cl::location(...) not specified for a command "
                             "line option with external storage!");
     Location->push_back(V);
   }
 };
 
+// Specialization for ManagedStatic<std::vector<T>>.
+template <class DataType>
+class list_storage<DataType, ManagedStatic<std::vector<DataType>>> {
+  using StorageClass = ManagedStatic<std::vector<DataType>>;
+  StorageClass *Location = nullptr; // Where to store the object...
+
+public:
+  list_storage() = default;
+
+  void clear() {
+    getStorageClass()->clear();
+  }
+
+  bool setLocation(Option &O, StorageClass &L) {
+    if (Location)
+      return O.error("cl::location(x) specified more than once!");
+    Location = &L;
+    return false;
+  }
+
+  template <class T> void addValue(const T &V) {
+    getStorageClass()->push_back(V);
+  }
+private:
+  StorageClass &getStorageClass() {
+    assert(Location != 0 && "cl::location(...) not specified for a command "
+                            "line option with external storage!");
+    return *Location;
+  }
+};
+
 // Define how to hold a class type object, such as a string.
 // Originally this code inherited from std::vector. In transitioning to a new
 // API for command line options we should change this. The new implementation
 // of this list_storage specialization implements the minimum subset of the
 // std::vector API required for all the current clients.
 //
 // FIXME: Reduce this API to a more narrow subset of std::vector
 //

llvm/lib/Support/CommandLine.cpp

void cl::ResetAllOptionOccurrences() {
   GlobalParser->ResetAllOptionOccurrences();
 }
 
 void LLVMParseCommandLineOptions(int argc, const char *const *argv,
                                  const char *Overview) {
   llvm::cl::ParseCommandLineOptions(argc, argv, StringRef(Overview),
                                     &llvm::nulls());
 }
+
+// All Debug.h functionality is a no-op in NDEBUG mode.
+#ifndef NDEBUG
+
+namespace llvm {
+extern ManagedStatic<std::vector<std::string>> CurrentDebugType;
+
+extern unsigned DebugBufferSize;
+}
+
+// -debug - Command line option to enable the DEBUG statements in the passes.
+// This flag may only be enabled in debug builds.
+static cl::opt<bool, true> Debug("debug", cl::desc("Enable debug output"),
+                                 cl::Hidden, cl::location(DebugFlag));
+
+// -debug-buffer-size - Buffer the last N characters of debug output
+// until program termination.
+static cl::opt<unsigned, true>
+    DebugBufferSizeOpt("debug-buffer-size",
+                       cl::desc("Buffer the last N characters of debug output "
+                                "until program termination. "
+                                "[default 0 -- immediate print-out]"),
+                       cl::Hidden,
+                       cl::location(llvm::DebugBufferSize));
+
+static cl::list<std::string, ManagedStatic<std::vector<std::string>>>
+    DebugOnly("debug-only",
+              cl::desc("Enable a specific type of debug output (comma "
+                       "separated list of types)"),
+              cl::Hidden, cl::ZeroOrMore, cl::value_desc("debug string"),
+              cl::location(llvm::CurrentDebugType), cl::CommaSeparated,
+              cl::callback([](const std::string &) { DebugFlag = true; }));
+
+#endif
+
+namespace llvm {
+extern bool DisableSymbolicationFlag;
+}
+
+static cl::opt<bool, true> DisableSymbolication(
+    "disable-symbolication", cl::desc("Disable symbolizing crash backtraces."),
+    cl::location(llvm::DisableSymbolicationFlag), cl::Hidden);

llvm/lib/Support/Debug.cpp

 using namespace llvm;
 
 // Even though LLVM might be built with NDEBUG, define symbols that the code
 // built without NDEBUG can depend on via the llvm/Support/Debug.h header.
 namespace llvm {
 /// Exported boolean set by the -debug option.
 bool DebugFlag = false;
 
-static ManagedStatic<std::vector<std::string>> CurrentDebugType;
+ManagedStatic<std::vector<std::string>> CurrentDebugType;
 
 /// Return true if the specified string is the debug type
 /// specified on the command line, or if none was specified on the command line
 /// with the -debug-only=X option.
 bool isCurrentDebugType(const char *DebugType) {
   if (CurrentDebugType->empty())
     return true;
   // See if DebugType is in list. Note: do not use find() as that forces us to
void setCurrentDebugTypes(const char **Types, unsigned Count) {
   CurrentDebugType->clear();
   for (size_t T = 0; T < Count; ++T)
     CurrentDebugType->push_back(Types[T]);
 }
 } // namespace llvm
 
 // All Debug.h functionality is a no-op in NDEBUG mode.
 #ifndef NDEBUG
 
-// -debug - Command line option to enable the DEBUG statements in the passes.
-// This flag may only be enabled in debug builds.
-static cl::opt<bool, true>
-Debug("debug", cl::desc("Enable debug output"), cl::Hidden,
-      cl::location(DebugFlag));
-
-// -debug-buffer-size - Buffer the last N characters of debug output
-//until program termination.
-static cl::opt<unsigned>
-DebugBufferSize("debug-buffer-size",
-                cl::desc("Buffer the last N characters of debug output "
-                         "until program termination. "
-                         "[default 0 -- immediate print-out]"),
-                cl::Hidden,
-                cl::init(0));
-
-namespace {
-
-struct DebugOnlyOpt {
-  void operator=(const std::string &Val) const {
-    if (Val.empty())
-      return;
-    DebugFlag = true;
-    SmallVector<StringRef,8> dbgTypes;
-    StringRef(Val).split(dbgTypes, ',', -1, false);
-    for (auto dbgType : dbgTypes)
-      CurrentDebugType->push_back(dbgType);
-  }
-};
-
+namespace llvm {
+unsigned DebugBufferSize = 0;
 }
 
-static DebugOnlyOpt DebugOnlyOptLoc;
-
-static cl::opt<DebugOnlyOpt, true, cl::parser<std::string> >
-DebugOnly("debug-only", cl::desc("Enable a specific type of debug output (comma separated list of types)"),
-          cl::Hidden, cl::ZeroOrMore, cl::value_desc("debug string"),
-          cl::location(DebugOnlyOptLoc), cl::ValueRequired);
 // Signal handlers - dump debug output on termination.
 static void debug_user_sig_handler(void *Cookie) {
   // This is a bit sneaky.  Since this is under #ifndef NDEBUG, we
   // know that debug mode is enabled and dbgs() really is a
   // circular_raw_ostream.  If NDEBUG is defined, then dbgs() ==
   // errs() but this will never be invoked.
   llvm::circular_raw_ostream &dbgout =
       static_cast<circular_raw_ostream &>(llvm::dbgs());

llvm/lib/Support/Signals.cpp

 
 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only TRULY operating system
 //===          independent code.
 //===----------------------------------------------------------------------===//
 
 using namespace llvm;
 
+namespace llvm {
 // Use explicit storage to avoid accessing cl::opt in a signal handler.
-static bool DisableSymbolicationFlag = false;
-static cl::opt<bool, true>
-    DisableSymbolication("disable-symbolication",
-                         cl::desc("Disable symbolizing crash backtraces."),
-                         cl::location(DisableSymbolicationFlag), cl::Hidden);
+bool DisableSymbolicationFlag = false;
+}
 
 // Callbacks to run in signal handler must be lock-free because a signal handler
 // could be running as we add new callbacks. We don't add unbounded numbers of
 // callbacks, an array is therefore sufficient.
 struct CallbackAndCookie {
   sys::SignalHandlerCallback Callback;
   void *Cookie;
   enum class Status { Empty, Initializing, Initialized, Executing };

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, Callback) {
   EXPECT_FALSE(OptA);
   EXPECT_FALSE(OptB);
   EXPECT_FALSE(OptC);
   EXPECT_TRUE(List.size() == 1);
 
   cl::ResetAllOptionOccurrences();
 }
 
+TEST(CommandLineTest, ManagedStaticLocation) {
+  cl::ResetCommandLineParser();
+
+  ManagedStatic<std::vector<std::string>> ManagedStringVec;
+
+  StackOption<bool> OptA("a", cl::desc("option a"));
+  StackOption<std::string,
+              cl::list<std::string, ManagedStatic<std::vector<std::string>>>>
+      ManagedList(
+          "managedlist",
+          cl::desc("option managedlist -- This option turns on options a when "
+                   "'foo' is included in list"),
+          cl::location(ManagedStringVec), cl::CommaSeparated,
+          cl::callback([&](const std::string &Str) {
+            if (Str == "foo")
+              OptA = true;
+          }));
+
+  const char *args[] = {"prog", "--managedlist=foo"};
+  EXPECT_TRUE(cl::ParseCommandLineOptions(2, args));
+  EXPECT_TRUE(OptA);
+  EXPECT_TRUE(ManagedStringVec->size() == 1);
+
+  cl::ResetAllOptionOccurrences();
+}
+
 enum Enum { Val1, Val2 };
 static cl::bits<Enum> ExampleBits(
     cl::desc("An example cl::bits to ensure it compiles"),
     cl::values(
       clEnumValN(Val1, "bits-val1", "The Val1 value"),
       clEnumValN(Val1, "bits-val2", "The Val2 value")));
 
 } // anonymous namespace