Initialize common options in `getRegisteredOptions`

[zsrkmyn]

Summary

This allows users accessing options in libSupport before invoking
cl::ParseCommandLineOptions, and also matches the behavior before
D105959.

[zsrkmyn]

I'm not sure if we need a test for it. I tried to add one but the test passed even without this patch, as those options were already initialized before running the test.

diff --git a/llvm/unittests/Support/CommandLineTest.cpp b/llvm/unittests/Support/CommandLineTest.cpp
index a0352bc8a4c5..c18d34ab4323 100644
--- a/llvm/unittests/Support/CommandLineTest.cpp
+++ b/llvm/unittests/Support/CommandLineTest.cpp
@@ -89,6 +89,12 @@ public:
   ~StackSubCommand() { unregisterSubCommand(); }
 };
 
+TEST(CommandLineTest, RetrievePresetOptions) {
+  StringMap<cl::Option *> &Map =
+      cl::getRegisteredOptions(*cl::TopLevelSubCommand);
+
+  ASSERT_TRUE(Map.count("help") == 1) << "Could not get preset option `help`";
+}
 
 cl::OptionCategory TestCategory("Test Options", "Description");
 TEST(CommandLineTest, ModifyExisitingOption) {

[dexonsmith]

In D106334#2889260, @zsrkmyn wrote:

I'm not sure if we need a test for it. I tried to add one but the test passed even without this patch, as those options were already initialized before running the test.

Seems nice to have a test for this...

One idea would be to split this test out of SupportTests to avoid being affected by other tests' running the initializer... assuming that's what's preventing the test from being useful?

Another idea would be to add a new llvm-clopt-test executable that exposes this codepath somehow. The tool could check if the first command-line argument is -register, and avoid parsing the command-line in that case.

[jhenderson]

Is there a way to clear all the registered options, prior to reregistering them again? Alternatively, would it be simple to add? I'm guessing even if it were, it might not solve our problems though, since the variables are local statics, so resetting wouldn't cause them to be reinstantiated...

[zsrkmyn]

In D106334#2889571, @jhenderson wrote:

I'm guessing even if it were, it might not solve our problems though, since the variables are local statics, so resetting wouldn't cause them to be reinstantiated...

I believe so. So I think I need to split the test from SupportTests as @dexonsmith suggested.

[mehdi_amini]

LGTM, thanks!
(better with a test as @dexonsmith suggested)

[MaskRay]

(unittest request)

[zsrkmyn]

Test added ;-)

[zsrkmyn]

A soft ping.

[MaskRay]

llvm/unittests/Support/CommandLineInit/CMakeLists.txt

2

This file looks a bit complex. Is all stuff here needed?

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp

32

State where this piece of code is copied from.

unittest/UnitTestMain/TestMain.cpp ?

[zsrkmyn]

llvm/unittests/Support/CommandLineInit/CMakeLists.txt

2

I believe so. Most part of the file was copied from add_unittest in llvm/cmake/modules/AddLLVM.cmake, except that gtest_main was excluded from target_link_libraries to prevent the test linking against TestMain.cpp.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp

32

Yep. Should I add a comment in the source code?

[MaskRay]

llvm/unittests/Support/CommandLineInit/CMakeLists.txt

2

OK. Then state where the code is copied from.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp

32

Yes

[zsrkmyn]

Would you mind helping me commit it if there's no further comments? Many thanks!

llvm/unittests/Support/CommandLineInit/CMakeLists.txt

2

Done. Added at line 7.

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp

32

Done. Added at line 14.

llvm/lib/Support/CommandLine.cpp

void cl::SetVersionPrinter(VersionPrinterTy func) {
   CommonOptions->OverrideVersionPrinter = func;
 }
 
 void cl::AddExtraVersionPrinter(VersionPrinterTy func) {
   CommonOptions->ExtraVersionPrinters.push_back(func);
 }
 
 StringMap<Option *> &cl::getRegisteredOptions(SubCommand &Sub) {
+  initCommonOptions();
   auto &Subs = GlobalParser->RegisteredSubCommands;
   (void)Subs;
   assert(is_contained(Subs, &Sub));
   return Sub.OptionsMap;
 }
 
 iterator_range<typename SmallPtrSet<SubCommand *, 4>::iterator>
 cl::getRegisteredSubcommands() {

llvm/unittests/Support/CMakeLists.txt

 
 # ManagedStatic.cpp uses <pthread>.
 target_link_libraries(SupportTests PRIVATE LLVMTestingSupport ${LLVM_PTHREAD_LIB})
 
 if(NOT LLVM_INTEGRATED_CRT_ALLOC)
   # The test doesn't pass when using a custom allocator, PR47881.
   add_subdirectory(DynamicLibrary)
 endif()
+
+add_subdirectory(CommandLineInit)

llvm/unittests/Support/CommandLineInit/CMakeLists.txt

+set(test_name CommandLineInitTests)
+set(test_suite UnitTests)
+
+# We don't call add_llvm_unittest() here, because the function automatically
+# links the test against TestMain.cpp, in which main() function calls
+# llvm::cl::ParseCommandLineOptions, and it makes the test always pass.
+# The following code mainly comes from `add_unittest` in
+# llvm/cmake/modules/AddLLVM.cmake, except that gtest_main is excluded from
+# target_link_libraries to prevent the test linking against TestMain.cpp.
+
+if (NOT LLVM_BUILD_TESTS)
+  set(EXCLUDE_FROM_ALL ON)
+endif()
+
+if (SUPPORTS_VARIADIC_MACROS_FLAG)
+  list(APPEND LLVM_COMPILE_FLAGS "-Wno-variadic-macros")
+endif ()
+# Some parts of gtest rely on this GNU extension, don't warn on it.
+if(SUPPORTS_GNU_ZERO_VARIADIC_MACRO_ARGUMENTS_FLAG)
+  list(APPEND LLVM_COMPILE_FLAGS "-Wno-gnu-zero-variadic-macro-arguments")
+endif()
+
+list(APPEND LLVM_LINK_COMPONENTS Support)
+
+add_llvm_executable(${test_name}
+  IGNORE_EXTERNALIZE_DEBUGINFO NO_INSTALL_RPATH
+  CommandLineInitTest.cpp)
+
+target_link_libraries(${test_name} PRIVATE gtest)
+
+add_dependencies(${test_suite} ${test_name})
+
+set(outdir ${CMAKE_CURRENT_BINARY_DIR}/${CMAKE_CFG_INTDIR})
+set_output_directory(${test_name} BINARY_DIR ${outdir} LIBRARY_DIR ${outdir})
+
+get_target_property(test_suite_folder ${test_suite} FOLDER)
+if (test_suite_folder)
+  set_property(TARGET ${test_name} PROPERTY FOLDER "${test_suite_folder}")
+endif ()

llvm/unittests/Support/CommandLineInit/CommandLineInitTest.cpp

+//===- llvm/unittest/Support/CommandLineInit/CommandLineInitTest.cpp ------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+/// \file
+/// Check if preset options in libSupport -- e.g., "help", "version", etc. --
+/// are correctly initialized and registered before getRegisteredOptions is
+/// invoked.
+///
+/// Most code here comes from llvm/utils/unittest/UnitTestMain/TestMain.cpp,
+/// except that llvm::cl::ParseCommandLineOptions() call is removed.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/CommandLine.h"
+#include "gtest/gtest.h"
+
+#if defined(_WIN32)
+#include <windows.h>
+#if defined(_MSC_VER)
+#include <crtdbg.h>
+#endif
+#endif
+
+using namespace llvm;
+
+int main(int argc, char **argv) {
+  testing::InitGoogleTest(&argc, argv);
+
+#if defined(_WIN32)
+  // Disable all of the possible ways Windows conspires to make automated
+  // testing impossible.
+  ::SetErrorMode(SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX);
+#if defined(_MSC_VER)
+  ::_set_error_mode(_OUT_TO_STDERR);
+  _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+  _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
+  _CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+  _CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
+  _CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
+  _CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
+#endif
+#endif
+
+  return RUN_ALL_TESTS();
+}
+
+TEST(CommandLineInitTest, GetPresetOptions) {
+  StringMap<cl::Option *> &Map =
+      cl::getRegisteredOptions(*cl::TopLevelSubCommand);
+
+  for (auto *Str :
+       {"help", "help-hidden", "help-list", "help-list-hidden", "version"})
+    EXPECT_EQ(Map.count(Str), (size_t)1)
+        << "Could not get preset option `" << Str << '`';
+}