[Support] Fix for two issues with clearing of the internal storage for cl::bits

[RVP]

Summary

This patch fixes two issues with clearing of the internal storage for cl::bits

1. The internal bits storage for cl::bits is uninitialized. This is a problem if a cl::bits option is not defined with static lifetime.
2. ResetAllOptionOccurrences does not reset cl::bits options.

The latter is also discussed in:

https://lists.llvm.org/pipermail/llvm-dev/2021-February/148299.html

[RVP]

NOTE: I have no commit access. If this is good to land, I will need someone to commit this. Thanks!

[RVP]

Changed the declaration of Args in unit test to use const char* Args[] which is how it was originally. std::array seems to break Windows build.

[RVP]

The pre-merge build shows a failure due to some unit tests timing out. I am not able to see any relation. Please let me know otherwise. Thanks.

llvm/include/llvm/Support/CommandLine.h

1731

Previously, uninitialized if cl::bits has non-static lifetime

llvm/unittests/Support/CommandLineTest.cpp

1954

Without the initialization, this EXPECT will fail.

1963

Without the fix in setDefault, this EXPECT will fail.

[serge-sans-paille]

Thanks for this patch, it looks correct to me.

[RVP]

@serge-sans-paille Thanks for reviewing. Please note that I have no commit access and I will need someone to do it for me.

[serge-sans-paille]

(running extra checks and landing the patch)

llvm/include/llvm/Support/CommandLine.h

 // Default storage class definition: external storage.  This implementation
 // assumes the user will specify a variable to store the data into with the
 // cl::location(x) modifier.
 //
 template <class DataType, class StorageClass> class bits_storage {
   unsigned *Location = nullptr; // Where to store the bits...
 
   template <class T> static unsigned Bit(const T &V) {
-    unsigned BitPos = reinterpret_cast<unsigned>(V);
+    unsigned BitPos = static_cast<unsigned>(V);
     assert(BitPos < sizeof(unsigned) * CHAR_BIT &&
            "enum exceeds width of bit vector!");
     return 1 << BitPos;
   }
 
 public:
   bits_storage() = default;
 
   bool setLocation(Option &O, unsigned &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) {
     assert(Location != nullptr &&
            "cl::location(...) not specified for a command "
            "line option with external storage!");
     *Location |= Bit(V);
   }
 
   unsigned getBits() { return *Location; }
 
+  void clear() {
+    if (Location)
+      *Location = 0;
+  }
+
   template <class T> bool isSet(const T &V) {
     return (*Location & Bit(V)) != 0;
   }
 };
 
 // Define how to hold bits.  Since we can inherit from a class, we do so.
 // This makes us exactly compatible with the bits in all cases that it is used.
 //
 template <class DataType> class bits_storage<DataType, bool> {
-  unsigned Bits; // Where to store the bits...
+  unsigned Bits{0}; // Where to store the bits...
 
   template <class T> static unsigned Bit(const T &V) {
-    unsigned BitPos = (unsigned)V;
+    unsigned BitPos = static_cast<unsigned>(V);
     assert(BitPos < sizeof(unsigned) * CHAR_BIT &&
            "enum exceeds width of bit vector!");
     return 1 << BitPos;
   }
 
 public:
   template <class T> void addValue(const T &V) { Bits |= Bit(V); }
 
   unsigned getBits() { return Bits; }
 
+  void clear() { Bits = 0; }
+
   template <class T> bool isSet(const T &V) { return (Bits & Bit(V)) != 0; }
 };
 
 //===----------------------------------------------------------------------===//
 // A bit vector of command options.
 //
 template <class DataType, class Storage = bool,
           class ParserClass = parser<DataType>>
class bits : public Option, public bits_storage<DataType, Storage> {
   void printOptionInfo(size_t GlobalWidth) const override {
     Parser.printOptionInfo(*this, GlobalWidth);
   }
 
   // Unimplemented: bits options don't currently store their default values.
   void printOptionValue(size_t /*GlobalWidth*/, bool /*Force*/) const override {
   }
 
-  void setDefault() override {}
+  void setDefault() override { bits_storage<DataType, Storage>::clear(); }
 
   void done() {
     addArgument();
     Parser.initialize();
   }
 
 public:
   // Command line options should not be copyable

llvm/unittests/Support/CommandLineTest.cpp

TEST(CommandLineTest, ConsumeAfterTwoPositionals) {
   EXPECT_EQ(ExtraArgs[0], "arg1");
   EXPECT_EQ(ExtraArgs[1], "arg2");
   EXPECT_TRUE(Errs.empty());
 }
 
 TEST(CommandLineTest, ResetAllOptionOccurrences) {
   cl::ResetCommandLineParser();
 
-  // -option [sink] input [args]
+  // -option -enableA -enableC [sink] input [args]
   StackOption<bool> Option("option");
+  enum Vals { ValA, ValB, ValC };
+  StackOption<Vals, cl::bits<Vals>> Bits(
+      cl::values(clEnumValN(ValA, "enableA", "Enable A"),
+                 clEnumValN(ValB, "enableB", "Enable B"),
+                 clEnumValN(ValC, "enableC", "Enable C")));
   StackOption<std::string, cl::list<std::string>> Sink(cl::Sink);
   StackOption<std::string> Input(cl::Positional);
   StackOption<std::string, cl::list<std::string>> ExtraArgs(cl::ConsumeAfter);
 
-  const char *Args[] = {"prog", "-option", "-unknown", "input", "-arg"};
+  const char *Args[] = {"prog",     "-option", "-enableA", "-enableC",
+                        "-unknown", "input",   "-arg"};
 
   std::string Errs;
   raw_string_ostream OS(Errs);
-  EXPECT_TRUE(cl::ParseCommandLineOptions(5, Args, StringRef(), &OS));
+  EXPECT_TRUE(cl::ParseCommandLineOptions(7, Args, StringRef(), &OS));
   EXPECT_TRUE(OS.str().empty());
 
   EXPECT_TRUE(Option);
+  EXPECT_EQ((1u << ValA) | (1u << ValC), Bits.getBits());
   EXPECT_EQ(1u, Sink.size());
   EXPECT_EQ("-unknown", Sink[0]);
   EXPECT_EQ("input", Input);
   EXPECT_EQ(1u, ExtraArgs.size());
   EXPECT_EQ("-arg", ExtraArgs[0]);
 
   cl::ResetAllOptionOccurrences();
   EXPECT_FALSE(Option);
+  EXPECT_EQ(0u, Bits.getBits());
   EXPECT_EQ(0u, Sink.size());
   EXPECT_EQ(0, Input.getNumOccurrences());
   EXPECT_EQ(0u, ExtraArgs.size());
 }
 
 } // anonymous namespace