llvm dev - Mar 2021 - CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by design?

Hello Riyaz,

I did indeed answer here:
https://lists.llvm.org/pipermail/llvm-dev/2021-February/148299.html

If you have a fix, would you mind sending it on Phabricator please?

Thanks,
Alex.

De : llvm-dev <llvm-dev-bounces at lists.llvm.org> De la part de Riyaz
Puthiyapurayil via llvm-dev
Envoyé : March 24, 2021 2:08 AM
À : 'llvm-dev at lists.llvm.org' <llvm-dev at lists.llvm.org>
Objet : Re: [llvm-dev] CommandLine -- ResetAllOptionOccurrences with cl::bits --
Bug or by design?

I didn't see a response. But I fixed this in our downstream code and it is
working now as expected. It fixes two issues (1) clearing cl::bits and (2)
allowing an optional storage location for cl::bits (the reinterpret_cast is
wrong for an enum to unsigned cast; it should be a static_cast). If there is
interest in fixing this upstream, I can send the following patch for review
(note, the following is for our downstream branch but I can put together a patch
for master along with some unit tests that expose the issue).

diff --git a/llvm/include/llvm/Support/CommandLine.h
b/llvm/include/llvm/Support/CommandLine.h
index 05374e3..712d6d3 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -1720,7 +1720,7 @@ 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;
@@ -1744,6 +1744,11 @@ public:

   unsigned getBits() { return *Location; }

+  void clear() {
+    if (Location)
+      *Location = 0;
+  }
+
   template <class T> bool isSet(const T &V) {
     return (*Location & Bit(V)) != 0;
   }
@@ -1767,6 +1772,8 @@ public:

   unsigned getBits() { return Bits; }

+  void clear() { Bits = 0; }
+
   template <class T> bool isSet(const T &V) { return (Bits &
Bit(V)) != 0; }
};

@@ -1813,7 +1820,7 @@ class bits : public Option, public
bits_storage<DataType, Storage> {
   void printOptionValue(size_t /*GlobalWidth*/, bool /*Force*/) const override
{
   }

-  void setDefault() override {}
+  void setDefault() override { bits_storage<DataType, Storage>::clear();
}

   void done() {
     addArgument();

From: Riyaz Puthiyapurayil
Sent: Sunday, March 21, 2021 4:30 PM
To: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
Subject: CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by
design?

I have some unit tests where I want to parse different command lines where some
options use cl::bits with an enum type. I want to reset the option occurrences
in each unit test and reinvoke the command line parser with a new command line.
While ResetAllOptionOccurrences resets every other kind of option, it
doesn't do so for cl::bits. Is this by design?

I see setDefaultValue of cl::bits is an empty function. Why doesn't it set
the internal bit storage to 0 when ResetAllOptionOccurrences is called? I
thought I could instead use an external storage and set that to 0. But that
doesn't work either. It results in an error:

unsigned MyOptStorage;
static llvm::cl::bits<MyOpt, unsigned> MyOpts(
....
      llvm::cl::location(MyOptStorage),
...);

include/llvm/Support/CommandLine.h:1723:23: error: reinterpret_cast from
'MyOpt' to 'unsigned int' is not allowed
    unsigned BitPos = reinterpret_cast<unsigned>(V);

where 'MyOpt' is an enum type.

So how does one specify cl::bits with an external storage location if DataType
is an enum? There is not much information in Command Line Reference Manual. Note
that I used unsigned above just like the excerpt from the manual says:

The cl::bits class is the class used to represent a list of command line options
in the form of a bit vector. It is also a templated class which can take up to
three arguments:
namespace cl {
  template <class DataType, class Storage = bool,
            class ParserClass = parser<DataType> >
  class bits;
}
This class works the exact same as the cl::list class, except that the second
argument must be of type unsigned if external storage is used.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210324/7ce83f4e/attachment.html>

Sorry. I didn't see your answer to my email back in Feb. The email I recent
last week describes a second issue with reinterpret_cast and didn't see a
response. Thank you. I will send a patch on Phabricator.

/Riyaz

From: Alexandre Ganea <alexandre.ganea at ubisoft.com>
Sent: Wednesday, March 24, 2021 5:51 AM
To: Riyaz Puthiyapurayil <riyaz at synopsys.com>
Cc: llvm-dev at lists.llvm.org
Subject: RE: CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by
design?

Hello Riyaz,

I did indeed answer here:
https://lists.llvm.org/pipermail/llvm-dev/2021-February/148299.html<https://urldefense.com/v3/__https:/lists.llvm.org/pipermail/llvm-dev/2021-February/148299.html__;!!A4F2R9G_pg!NSCEGUMXZmeplomD4au94CTI82astNn26ICAJEpZGU9S1AeD2PvhE9HsuwZmAd5kWP-XcluuZqUU$>

If you have a fix, would you mind sending it on Phabricator please?

Thanks,
Alex.

De : llvm-dev <llvm-dev-bounces at lists.llvm.org<mailto:llvm-dev-bounces
at lists.llvm.org>> De la part de Riyaz Puthiyapurayil via llvm-dev
Envoyé : March 24, 2021 2:08 AM
À : 'llvm-dev at lists.llvm.org' <llvm-dev at
lists.llvm.org<mailto:llvm-dev at lists.llvm.org>>
Objet : Re: [llvm-dev] CommandLine -- ResetAllOptionOccurrences with cl::bits --
Bug or by design?

I didn't see a response. But I fixed this in our downstream code and it is
working now as expected. It fixes two issues (1) clearing cl::bits and (2)
allowing an optional storage location for cl::bits (the reinterpret_cast is
wrong for an enum to unsigned cast; it should be a static_cast). If there is
interest in fixing this upstream, I can send the following patch for review
(note, the following is for our downstream branch but I can put together a patch
for master along with some unit tests that expose the issue).

diff --git a/llvm/include/llvm/Support/CommandLine.h
b/llvm/include/llvm/Support/CommandLine.h
index 05374e3..712d6d3 100644
--- a/llvm/include/llvm/Support/CommandLine.h
+++ b/llvm/include/llvm/Support/CommandLine.h
@@ -1720,7 +1720,7 @@ 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;
@@ -1744,6 +1744,11 @@ public:

   unsigned getBits() { return *Location; }

+  void clear() {
+    if (Location)
+      *Location = 0;
+  }
+
   template <class T> bool isSet(const T &V) {
     return (*Location & Bit(V)) != 0;
   }
@@ -1767,6 +1772,8 @@ public:

   unsigned getBits() { return Bits; }

+  void clear() { Bits = 0; }
+
   template <class T> bool isSet(const T &V) { return (Bits &
Bit(V)) != 0; }
};

@@ -1813,7 +1820,7 @@ class bits : public Option, public
bits_storage<DataType, Storage> {
   void printOptionValue(size_t /*GlobalWidth*/, bool /*Force*/) const override
{
   }

-  void setDefault() override {}
+  void setDefault() override { bits_storage<DataType, Storage>::clear();
}

   void done() {
     addArgument();

From: Riyaz Puthiyapurayil
Sent: Sunday, March 21, 2021 4:30 PM
To: llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
Subject: CommandLine -- ResetAllOptionOccurrences with cl::bits -- Bug or by
design?

I have some unit tests where I want to parse different command lines where some
options use cl::bits with an enum type. I want to reset the option occurrences
in each unit test and reinvoke the command line parser with a new command line.
While ResetAllOptionOccurrences resets every other kind of option, it
doesn't do so for cl::bits. Is this by design?

I see setDefaultValue of cl::bits is an empty function. Why doesn't it set
the internal bit storage to 0 when ResetAllOptionOccurrences is called? I
thought I could instead use an external storage and set that to 0. But that
doesn't work either. It results in an error:

unsigned MyOptStorage;
static llvm::cl::bits<MyOpt, unsigned> MyOpts(
....
      llvm::cl::location(MyOptStorage),
...);

include/llvm/Support/CommandLine.h:1723:23: error: reinterpret_cast from
'MyOpt' to 'unsigned int' is not allowed
    unsigned BitPos = reinterpret_cast<unsigned>(V);

where 'MyOpt' is an enum type.

So how does one specify cl::bits with an external storage location if DataType
is an enum? There is not much information in Command Line Reference Manual. Note
that I used unsigned above just like the excerpt from the manual says:

The cl::bits class is the class used to represent a list of command line options
in the form of a bit vector. It is also a templated class which can take up to
three arguments:
namespace cl {
  template <class DataType, class Storage = bool,
            class ParserClass = parser<DataType> >
  class bits;
}
This class works the exact same as the cl::list class, except that the second
argument must be of type unsigned if external storage is used.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210324/585c5254/attachment.html>