[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57914#1416215 , @jyknight wrote:

> The intricate initialization-order workarounds apparently don't work in all 
> build modes, so I've updated this code to have constexpr functions and 
> initializations in rL355278 .


Looking at what guarantees are given for dynamic initialization (in 
[basic.start.dynamic]), it seems that no guarantees are given if the variable 
is an explicit or implicit instantiation. So unless I am mistaken the original 
code was not guaranteed to work indeed. Apologies for missing that!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1416215 , @jyknight wrote:

> The intricate initialization-order workarounds apparently don't work in all 
> build modes, so I've updated this code to have constexpr functions and 
> initializations in rL355278 .


Thanks for finding the issue! All constexpr sounds good!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

The intricate initialization-order workarounds apparently don't work in all 
build modes, so I've updated this code to have constexpr functions and 
initializations in r355278.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
pgousseau marked an inline comment as done.
Closed by commit rL355190: [Driver] Allow enum SanitizerOrdinal to represent 
more than 64 different… (authored by pgousseau, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57914?vs=188748=188866#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/Sanitizers.def
  cfe/trunk/include/clang/Basic/Sanitizers.h
  cfe/trunk/include/clang/Driver/ToolChain.h
  cfe/trunk/lib/Basic/SanitizerSpecialCaseList.cpp
  cfe/trunk/lib/Basic/Sanitizers.cpp
  cfe/trunk/lib/CodeGen/CGExpr.cpp
  cfe/trunk/lib/Driver/SanitizerArgs.cpp
  cfe/trunk/lib/Driver/ToolChain.cpp
  cfe/trunk/lib/Frontend/CompilerInvocation.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp

Index: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
===
--- cfe/trunk/lib/Sema/SemaDeclAttr.cpp
+++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp
@@ -6301,7 +6301,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: cfe/trunk/lib/Basic/SanitizerSpecialCaseList.cpp
===
--- cfe/trunk/lib/Basic/SanitizerSpecialCaseList.cpp
+++ cfe/trunk/lib/Basic/SanitizerSpecialCaseList.cpp
@@ -36,7 +36,7 @@
 
 void SanitizerSpecialCaseList::createSanitizerSections() {
   for (auto  : Sections) {
-SanitizerMask Mask = 0;
+SanitizerMask Mask;
 
 #define SANITIZER(NAME, ID)\
   if (S.SectionMatcher->match(NAME))   \
Index: cfe/trunk/lib/Basic/Sanitizers.cpp
===
--- cfe/trunk/lib/Basic/Sanitizers.cpp
+++ cfe/trunk/lib/Basic/Sanitizers.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "clang/Basic/Sanitizers.h"
+#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringSwitch.h"
 
 using namespace clang;
@@ -19,9 +20,9 @@
   SanitizerMask ParsedKind = llvm::StringSwitch(Value)
 #define SANITIZER(NAME, ID) .Case(NAME, SanitizerKind::ID)
 #define SANITIZER_GROUP(NAME, ID, ALIAS)   \
-  .Case(NAME, AllowGroups ? SanitizerKind::ID##Group : 0)
+  .Case(NAME, AllowGroups ? SanitizerKind::ID##Group : SanitizerMask())
 #include "clang/Basic/Sanitizers.def"
-.Default(0);
+.Default(SanitizerMask());
   return ParsedKind;
 }
 
@@ -33,3 +34,13 @@
 #include "clang/Basic/Sanitizers.def"
   return Kinds;
 }
+
+llvm::hash_code SanitizerMask::hash_value() const {
+  return llvm::hash_combine_range([0], [kNumElem]);
+}
+
+namespace clang {
+llvm::hash_code hash_value(const clang::SanitizerMask ) {
+  return Arg.hash_value();
+}
+} // namespace clang
Index: cfe/trunk/lib/Frontend/CompilerInvocation.cpp
===
--- cfe/trunk/lib/Frontend/CompilerInvocation.cpp
+++ cfe/trunk/lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: cfe/trunk/lib/CodeGen/CGExpr.cpp
===
--- cfe/trunk/lib/CodeGen/CGExpr.cpp
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp
@@ -2855,16 +2855,13 @@
 }
 
 static CheckRecoverableKind getRecoverableKind(SanitizerMask Kind) {
-  assert(llvm::countPopulation(Kind) == 1);
-  switch (Kind) {
-  case SanitizerKind::Vptr:
+  assert(Kind.countPopulation() == 1);
+  if (Kind == SanitizerKind::Vptr)
 return CheckRecoverableKind::AlwaysRecoverable;
-  case SanitizerKind::Return:
-  case SanitizerKind::Unreachable:
+  else if (Kind == SanitizerKind::Return || Kind == SanitizerKind::Unreachable)
 return CheckRecoverableKind::Unrecoverable;
-  default:
+  else
 return CheckRecoverableKind::Recoverable;
-  }
 }
 
 namespace {
Index: cfe/trunk/lib/Driver/SanitizerArgs.cpp
===
--- cfe/trunk/lib/Driver/SanitizerArgs.cpp
+++ 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno accepted this revision.
riccibruno added a comment.
This revision is now accepted and ready to land.

Looks good, hopefully this time it will stick. thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked 2 inline comments as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:29
+class hash_code;
+}
 

riccibruno wrote:
> pgousseau wrote:
> > riccibruno wrote:
> > > What ? You are forward-declaring `hash_code` here and using it as a value 
> > > a few lines later. Just include `llvm/ADT/Hashing.h`.
> > Thanks for reviewing!
> > I am happy to include `Hashing.h` but I thought it was generally frown upon 
> > to add includes if avoidable?
> > `hash_code` is only used in the return type of the `hash_value()` 
> > declaration here no?
> > I saw StringRef.h or APInt.h also forward declare `hash_code`.
> > Thanks again for reviewing and sorry for the many iterations.
> Never mind, I overlooked that it is only the declaration of `llvm::hash_code 
> hash_value(const clang::SanitizerMask );` and not the definition. Sorry 
> about that.
> 
> I believe that is it correct to have the overload of `hash_value` in the 
> namespace `clang`, so that it can be found by ADL since `SanitizerMask` is in 
> `clang`. However you can probably move the declaration of `llvm::hash_code 
> hash_value(const clang::SanitizerMask );` just after `SanitizerMask` so 
> that you don't have to forward-declare `SanitizerMask` (but keep the 
> forward-declaration of `hash_code` in `llvm`).
Sounds good, done!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 188748.
pgousseau added a comment.

Change `hash_value()` declaration's location as suggested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -818,21 +818,23 @@
   // Return sanitizers which don't require runtime support and are not
   // platform dependent.
 
-  using namespace SanitizerKind;
-
-  SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
-  CFICastStrict | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds;
+  SanitizerMask Res = (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
+   ~SanitizerKind::Function) |
+  (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
+  SanitizerKind::CFICastStrict |
+  SanitizerKind::UnsignedIntegerOverflow |
+  SanitizerKind::ImplicitConversion |
+  SanitizerKind::Nullability | SanitizerKind::LocalBounds;
   if (getTriple().getArch() == llvm::Triple::x86 ||
   getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::arm ||
   getTriple().getArch() == llvm::Triple::aarch64 ||
   getTriple().getArch() == llvm::Triple::wasm32 ||
   getTriple().getArch() == llvm::Triple::wasm64)
-Res |= CFIICall;
+Res |= SanitizerKind::CFIICall;
   if (getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::aarch64)
-Res |= ShadowCallStack;
+Res |= SanitizerKind::ShadowCallStack;
   return Res;
 }
 
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -21,33 +21,52 @@
 #include 
 
 using namespace clang;
-using namespace clang::SanitizerKind;
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask NeedsUbsanRt =
+SanitizerKind::Undefined | SanitizerKind::Integer |
+

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:29
+class hash_code;
+}
 

pgousseau wrote:
> riccibruno wrote:
> > What ? You are forward-declaring `hash_code` here and using it as a value a 
> > few lines later. Just include `llvm/ADT/Hashing.h`.
> Thanks for reviewing!
> I am happy to include `Hashing.h` but I thought it was generally frown upon 
> to add includes if avoidable?
> `hash_code` is only used in the return type of the `hash_value()` declaration 
> here no?
> I saw StringRef.h or APInt.h also forward declare `hash_code`.
> Thanks again for reviewing and sorry for the many iterations.
Never mind, I overlooked that it is only the declaration of `llvm::hash_code 
hash_value(const clang::SanitizerMask );` and not the definition. Sorry 
about that.

I believe that is it correct to have the overload of `hash_value` in the 
namespace `clang`, so that it can be found by ADL since `SanitizerMask` is in 
`clang`. However you can probably move the declaration of `llvm::hash_code 
hash_value(const clang::SanitizerMask );` just after `SanitizerMask` so 
that you don't have to forward-declare `SanitizerMask` (but keep the 
forward-declaration of `hash_code` in `llvm`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked an inline comment as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:29
+class hash_code;
+}
 

riccibruno wrote:
> What ? You are forward-declaring `hash_code` here and using it as a value a 
> few lines later. Just include `llvm/ADT/Hashing.h`.
Thanks for reviewing!
I am happy to include `Hashing.h` but I thought it was generally frown upon to 
add includes if avoidable?
`hash_code` is only used in the return type of the `hash_value()` declaration 
here no?
I saw StringRef.h or APInt.h also forward declare `hash_code`.
Thanks again for reviewing and sorry for the many iterations.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno requested changes to this revision.
riccibruno added inline comments.
This revision now requires changes to proceed.



Comment at: include/clang/Basic/Sanitizers.h:29
+class hash_code;
+}
 

What ? You are forward-declaring `hash_code` here and using it as a value a few 
lines later. Just include `llvm/ADT/Hashing.h`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 188576.
pgousseau added a comment.

move hash_value declaration to clang's namespace to solve lldb cmake bot build 
error.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -818,21 +818,23 @@
   // Return sanitizers which don't require runtime support and are not
   // platform dependent.
 
-  using namespace SanitizerKind;
-
-  SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
-  CFICastStrict | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds;
+  SanitizerMask Res = (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
+   ~SanitizerKind::Function) |
+  (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
+  SanitizerKind::CFICastStrict |
+  SanitizerKind::UnsignedIntegerOverflow |
+  SanitizerKind::ImplicitConversion |
+  SanitizerKind::Nullability | SanitizerKind::LocalBounds;
   if (getTriple().getArch() == llvm::Triple::x86 ||
   getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::arm ||
   getTriple().getArch() == llvm::Triple::aarch64 ||
   getTriple().getArch() == llvm::Triple::wasm32 ||
   getTriple().getArch() == llvm::Triple::wasm64)
-Res |= CFIICall;
+Res |= SanitizerKind::CFIICall;
   if (getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::aarch64)
-Res |= ShadowCallStack;
+Res |= SanitizerKind::ShadowCallStack;
   return Res;
 }
 
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -21,33 +21,52 @@
 #include 
 
 using namespace clang;
-using namespace clang::SanitizerKind;
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask NeedsUbsanRt =
+SanitizerKind::Undefined | 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau reopened this revision.
pgousseau added a comment.
This revision is now accepted and ready to land.

Reopening because of buildbot failure 
http://green.lab.llvm.org/green/job/lldb-cmake/20537/

I have not been able to reproduce the error, the order of includes must be 
different on the failing bot...
As suggested by the compiler warning, moving `hash_value(const 
clang::SanitizerMask&)` declaration into clang's namespace should fix it.

  FAILED: 
tools/clang/lib/Frontend/CMakeFiles/clangFrontend.dir/CompilerInvocation.cpp.o 
  /Users/buildslave/jenkins/workspace/lldb-cmake/host-compiler/bin/clang++  
-DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS 
-D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/Frontend 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/lib/Frontend 
-I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/include 
-Itools/clang/include 
-I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/usr/include/libxml2
 -Iinclude -I/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/include -fPIC 
-fvisibility-inlines-hidden -Werror=date-time 
-Werror=unguarded-availability-new -std=c++11 -fmodules 
-fmodules-cache-path=/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/module.cache
 -fcxx-modules -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -fno-common 
-Woverloaded-virtual -Wno-nested-anon-types -O3  -isysroot 
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk
 -mmacosx-version-min=10.9   -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT 
tools/clang/lib/Frontend/CMakeFiles/clangFrontend.dir/CompilerInvocation.cpp.o 
-MF 
tools/clang/lib/Frontend/CMakeFiles/clangFrontend.dir/CompilerInvocation.cpp.o.d
 -o 
tools/clang/lib/Frontend/CMakeFiles/clangFrontend.dir/CompilerInvocation.cpp.o 
-c 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/lib/Frontend/CompilerInvocation.cpp
  In module 'LLVM_Utils' imported from 
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/lib/Frontend/TestModuleFileExtension.h:13:
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/include/llvm/ADT/Hashing.h:374:10:
 error: call to function 'hash_value' that is neither visible in the template 
definition nor found by argument-dependent lookup
return hash_value(value);
   ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/include/llvm/ADT/Hashing.h:555:63:
 note: in instantiation of function template specialization 
'llvm::hashing::detail::get_hashable_data' requested here
  buffer_ptr = combine_data(length, buffer_ptr, buffer_end, 
get_hashable_data(arg));
^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/include/llvm/ADT/Hashing.h:558:12:
 note: in instantiation of function template specialization 
'llvm::hashing::detail::hash_combine_recursive_helper::combine'
 requested here
  return combine(length, buffer_ptr, buffer_end, args...);
 ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/include/llvm/ADT/Hashing.h:603:17:
 note: in instantiation of function template specialization 
'llvm::hashing::detail::hash_combine_recursive_helper::combine' requested here
return helper.combine(0, helper.buffer, helper.buffer + 64, args...);
  ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/lib/Frontend/CompilerInvocation.cpp:3430:12:
 note: in instantiation of function template specialization 
'llvm::hash_combine' requested here
  code = hash_combine(code, SanHash.Mask);
 ^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm/tools/clang/include/clang/Basic/Sanitizers.h:29:11:
 note: 'hash_value' should be declared prior to the call site or in namespace 
'clang'
  hash_code hash_value(const clang::SanitizerMask );
^
  1 error generated.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

reverted at r354875 at this break lldb build.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC354873: [Driver] Allow enum SanitizerOrdinal to represent 
more than 64 different… (authored by pgousseau, committed by ).

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: include/clang/Basic/Attr.td
===
--- include/clang/Basic/Attr.td
+++ include/clang/Basic/Attr.td
@@ -2366,7 +2366,7 @@
   let Documentation = [NoSanitizeDocs];
   let AdditionalMembers = [{
 SanitizerMask getMask() const {
-  SanitizerMask Mask = 0;
+  SanitizerMask Mask;
   for (auto SanitizerName : sanitizers()) {
 SanitizerMask ParsedMask =
 parseSanitizerValue(SanitizerName, /*AllowGroups=*/true);
Index: include/clang/Basic/Sanitizers.h
===
--- include/clang/Basic/Sanitizers.h
+++ include/clang/Basic/Sanitizers.h
@@ -21,44 +21,167 @@
 #include 
 
 namespace clang {
+class SanitizerMask;
+}
 
-using SanitizerMask = uint64_t;
+namespace llvm {
+class hashcode;
+hash_code hash_value(const clang::SanitizerMask );
+} // namespace llvm
 
-namespace SanitizerKind {
+namespace clang {
 
-// Assign ordinals to possible values of -fsanitize= flag, which we will use as
-// bit positions.
-enum SanitizerOrdinal : uint64_t {
-#define SANITIZER(NAME, ID) SO_##ID,
-#define SANITIZER_GROUP(NAME, ID, ALIAS) SO_##ID##Group,
-#include "clang/Basic/Sanitizers.def"
-  SO_Count
+class SanitizerMask {
+  /// Number of array elements.
+  static constexpr unsigned kNumElem = 2;
+  /// Mask value initialized to 0.
+  uint64_t maskLoToHigh[kNumElem]{};
+  /// Number of bits in a mask.
+  static constexpr unsigned kNumBits = sizeof(decltype(maskLoToHigh)) * 8;
+  /// Number of bits in a mask element.
+  static constexpr unsigned kNumBitElem = sizeof(decltype(maskLoToHigh[0])) * 8;
+
+public:
+  static constexpr bool checkBitPos(const unsigned Pos) {
+return Pos < kNumBits;
+  }
+
+  /// Create a mask with a bit enabled at position Pos.
+  static SanitizerMask bitPosToMask(const unsigned Pos) {
+assert(Pos < kNumBits && "Bit position too big.");
+SanitizerMask mask;
+mask.maskLoToHigh[Pos / kNumBitElem] = 1ULL << Pos % kNumBitElem;
+return mask;
+  }
+
+  unsigned countPopulation() const {
+unsigned total = 0;
+for (const auto  : maskLoToHigh)
+  total += llvm::countPopulation(Val);
+return total;
+  }
+
+  void flipAllBits() {
+for (auto  : maskLoToHigh)
+  Val = ~Val;
+  }
+
+  bool isPowerOf2() const {
+return countPopulation() == 1;
+  }
+
+  llvm::hash_code hash_value() const;
+
+  explicit operator bool() const {
+for (const auto  : maskLoToHigh)
+  if (Val)
+return true;
+return false;
+  };
+
+  bool operator==(const SanitizerMask ) const {
+for (unsigned k = 0; k < kNumElem; k++) {
+  if (maskLoToHigh[k] != V.maskLoToHigh[k])
+return false;
+}
+return true;
+  }
+
+  SanitizerMask &=(const SanitizerMask ) {
+for (unsigned k = 0; k < kNumElem; k++)
+  maskLoToHigh[k] &= RHS.maskLoToHigh[k];
+return *this;
+  }
+
+  SanitizerMask |=(const SanitizerMask ) {
+for (unsigned k = 0; k < kNumElem; k++)
+  maskLoToHigh[k] |= RHS.maskLoToHigh[k];
+return *this;
+  }
+
+  bool operator!() const {
+for (const auto  : maskLoToHigh)
+  if (Val)
+return false;
+return true;
+  }
+
+  bool operator!=(const SanitizerMask ) const { return !((*this) == RHS); }
 };
 
+inline SanitizerMask operator~(SanitizerMask v) {
+  v.flipAllBits();
+  return v;
+}
+
+inline SanitizerMask operator&(SanitizerMask a, const SanitizerMask ) {
+  a &= b;
+  return a;
+}
+
+inline SanitizerMask operator|(SanitizerMask a, const SanitizerMask ) {
+  a |= b;
+  return a;
+}
+
 // Define the set of sanitizer kinds, as well as the set of sanitizers each
 // sanitizer group expands into.
-#define SANITIZER(NAME, ID) \
-  const SanitizerMask ID = 1ULL << SO_##ID;
-#define SANITIZER_GROUP(NAME, ID, ALIAS) \
-  const SanitizerMask ID = ALIAS; \
-  const SanitizerMask ID##Group = 1ULL << SO_##ID##Group;
+// Uses static data member of a class template as recommended in second
+// workaround from n4424 to avoid odr issues.
+// FIXME: Can be marked as constexpr once c++14 can be used in llvm.
+// FIXME: n4424 workaround can be replaced by c++17 inline variable.
+template  struct SanitizerMasks {
+
+  // Assign ordinals to possible values of -fsanitize= flag, which we will use
+  // 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1410387 , @riccibruno wrote:

> I think this looks good now. Thanks !


Pushed at r354873, thanks for the help!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno accepted this revision.
riccibruno added a comment.
This revision is now accepted and ready to land.

I think this looks good now. Thanks !


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked 2 inline comments as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:54
+  static constexpr bool
+  checkBitPos(const SanitizerKind::SanitizerOrdinal ) {
+return Pos < kNumBits;

riccibruno wrote:
> `SanitizerOrdinal` should probably be passed by value here and elsewhere.
Yes I meant to initially, done now thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 188175.
pgousseau added a comment.

Keep `enum SanitizerOrdinal` as it was inside `SanitizerKind` namespace, fix 
comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -818,21 +818,23 @@
   // Return sanitizers which don't require runtime support and are not
   // platform dependent.
 
-  using namespace SanitizerKind;
-
-  SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
-  CFICastStrict | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds;
+  SanitizerMask Res = (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
+   ~SanitizerKind::Function) |
+  (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
+  SanitizerKind::CFICastStrict |
+  SanitizerKind::UnsignedIntegerOverflow |
+  SanitizerKind::ImplicitConversion |
+  SanitizerKind::Nullability | SanitizerKind::LocalBounds;
   if (getTriple().getArch() == llvm::Triple::x86 ||
   getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::arm ||
   getTriple().getArch() == llvm::Triple::aarch64 ||
   getTriple().getArch() == llvm::Triple::wasm32 ||
   getTriple().getArch() == llvm::Triple::wasm64)
-Res |= CFIICall;
+Res |= SanitizerKind::CFIICall;
   if (getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::aarch64)
-Res |= ShadowCallStack;
+Res |= SanitizerKind::ShadowCallStack;
   return Res;
 }
 
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -21,33 +21,52 @@
 #include 
 
 using namespace clang;
-using namespace clang::SanitizerKind;
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask NeedsUbsanRt =
+SanitizerKind::Undefined | 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

It looks better now as far as I can see. I like the idea of aliasing 
`SanitizerKind` to `SanitizerMasks<>;`.




Comment at: include/clang/Basic/Sanitizers.h:133
 // bit positions.
 enum SanitizerOrdinal : uint64_t {
 #define SANITIZER(NAME, ID) SO_##ID,

`SanitizerOrdinal` used to be in `SanitizerKind`. Would it make sense to keep 
this by moving it to `SanitizerMasks` ?



Comment at: include/clang/Basic/Sanitizers.h:169
 
-} // namespace SanitizerKind
+// Force instantiate here to ensure correct initialization order.
+template struct SanitizerMasks<>;

"Force instantiate here [...]" -> "Explicit instantiation here [...]" ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 188128.
pgousseau added a comment.
Herald added a subscriber: jfb.

Fix bad use of reference as pointed out, aliased SanitizerKind to 
SanitizerMasks<> instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Driver/ToolChain.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/ToolChain.cpp
===
--- lib/Driver/ToolChain.cpp
+++ lib/Driver/ToolChain.cpp
@@ -818,21 +818,23 @@
   // Return sanitizers which don't require runtime support and are not
   // platform dependent.
 
-  using namespace SanitizerKind;
-
-  SanitizerMask Res = (Undefined & ~Vptr & ~Function) | (CFI & ~CFIICall) |
-  CFICastStrict | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds;
+  SanitizerMask Res = (SanitizerKind::Undefined & ~SanitizerKind::Vptr &
+   ~SanitizerKind::Function) |
+  (SanitizerKind::CFI & ~SanitizerKind::CFIICall) |
+  SanitizerKind::CFICastStrict |
+  SanitizerKind::UnsignedIntegerOverflow |
+  SanitizerKind::ImplicitConversion |
+  SanitizerKind::Nullability | SanitizerKind::LocalBounds;
   if (getTriple().getArch() == llvm::Triple::x86 ||
   getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::arm ||
   getTriple().getArch() == llvm::Triple::aarch64 ||
   getTriple().getArch() == llvm::Triple::wasm32 ||
   getTriple().getArch() == llvm::Triple::wasm64)
-Res |= CFIICall;
+Res |= SanitizerKind::CFIICall;
   if (getTriple().getArch() == llvm::Triple::x86_64 ||
   getTriple().getArch() == llvm::Triple::aarch64)
-Res |= ShadowCallStack;
+Res |= SanitizerKind::ShadowCallStack;
   return Res;
 }
 
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -21,33 +21,52 @@
 #include 
 
 using namespace clang;
-using namespace clang::SanitizerKind;
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:173
+  SanitizerMask::bitPosToMask(SO_##ID##Group); 
\
+  static const SanitizerMask ##Group = SanitizerMasks<>::ID##Group;
 #include "clang/Basic/Sanitizers.def"

pgousseau wrote:
> riccibruno wrote:
> > You are back to the same issue with the references.
> A yes I should have read the odr definition more carefully! I am trying to 
> find a way to not have to add `SanitizerMasks<>::` all over the place, any 
> ideas?
Nope, unfortunately.

(And I am wondering how many other similar issues exist in clang/llvm; I would 
bet it is > 0).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked 2 inline comments as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:148
+// workaround from n4424 to avoid odr issues.
+// FIXME: Can be replaced by constexpr once c++14 can be used in llvm.
+template  struct SanitizerMasks {

riccibruno wrote:
> Not replaced. `constexpr` is nearly orthogonal to the odr issue. It can be 
> replaced by `inline` variables in C++17. I think that you should leave two 
> FIXME here: the first one about marking the masks`constexpr` in C++14, and 
> the second one about replacing the workaround by marking the masks `inline`.
Makes sense thanks!



Comment at: include/clang/Basic/Sanitizers.h:173
+  SanitizerMask::bitPosToMask(SO_##ID##Group); 
\
+  static const SanitizerMask ##Group = SanitizerMasks<>::ID##Group;
 #include "clang/Basic/Sanitizers.def"

riccibruno wrote:
> You are back to the same issue with the references.
A yes I should have read the odr definition more carefully! I am trying to find 
a way to not have to add `SanitizerMasks<>::` all over the place, any ideas?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:148
+// workaround from n4424 to avoid odr issues.
+// FIXME: Can be replaced by constexpr once c++14 can be used in llvm.
+template  struct SanitizerMasks {

Not replaced. `constexpr` is nearly orthogonal to the odr issue. It can be 
replaced by `inline` variables in C++17. I think that you should leave two 
FIXME here: the first one about marking the masks`constexpr` in C++14, and the 
second one about replacing the workaround by marking the masks `inline`.



Comment at: include/clang/Basic/Sanitizers.h:173
+  SanitizerMask::bitPosToMask(SO_##ID##Group); 
\
+  static const SanitizerMask ##Group = SanitizerMasks<>::ID##Group;
 #include "clang/Basic/Sanitizers.def"

You are back to the same issue with the references.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 187917.
pgousseau added a comment.

Rework FIXME comment.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+static const SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+static const SanitizerMask NotAllowedWithTrap = Vptr;
+static const SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+static const SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+static const SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+static const SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+static const SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+static const SanitizerMask Unrecoverable = Unreachable | Return;
+static const SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+static const SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+static const SanitizerMask NeedsLTO = CFI;
+static const SanitizerMask TrappingSupported =
+(Undefined & ~Vptr) | UnsignedIntegerOverflow | ImplicitConversion |
+Nullability | LocalBounds | CFI;
+static const SanitizerMask TrappingDefault = CFI;
+static const SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+static const SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 187913.
pgousseau added a comment.

Fix odr violation issue using static data member of a class template as 
suggested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6211,7 +6211,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+static const SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+static const SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+static const SanitizerMask NotAllowedWithTrap = Vptr;
+static const SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+static const SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+static const SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+static const SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+static const SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+static const SanitizerMask Unrecoverable = Unreachable | Return;
+static const SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+static const SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+static const SanitizerMask NeedsLTO = CFI;
+static const SanitizerMask TrappingSupported =
+(Undefined & ~Vptr) | UnsignedIntegerOverflow | ImplicitConversion |
+Nullability | LocalBounds | CFI;
+static const SanitizerMask TrappingDefault = CFI;
+static const SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+static const SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1405955 , @riccibruno wrote:

> More explicitly something like:
>
> in `Sanitizer.h`:
>
>   template  struct SanitizerMasks {
> static const SanitizerMask SomeMask;
> /* and so on for each mask*/
>   };
>  
>   template  const SanitizerMask SanitizerMasks::SomeMask = the 
> definition;
>
>
> And then you can write `SanitizerMasks<>::SomeMask` when you want to use this 
> mask.


Sounds good to me, I will try that thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

More explicitly something like:

in `Sanitizer.h`:

  template  struct SanitizerMasks {
static const SanitizerMask SomeMask;
/* and so on for each mask*/
  };
  
  template  const SanitizerMask SanitizerMasks::SomeMask = the 
definition;

And then you can write `SanitizerMasks<>::SomeMask` when you want to use this 
mask.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

To define the masks as `constexpr` variables you will have to make at least 
`bitPosToMask`, `operator|`, `operator&` and `operator~`. Unfortunately 
`constexpr` functions in c++11 are rather restricted. It seems to me however 
that you could do the following:

1. Wait until we can use C++14 in llvm (soon hopefully) to mark the masks 
`contexpr` (and leave a FIXME for now).
2. Solve the odr-violation issue by using one of the work-around in n4424. In 
particular I like the second work-around consisting of putting the static 
members in a class template.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1405781 , @riccibruno wrote:

> In D57914#1405665 , @pgousseau wrote:
>
> > In D57914#1405615 , @riccibruno 
> > wrote:
> >
> > > I think what you would really want to do is mark the masks as `inline 
> > > constexpr`, but sadly inline variables are C++17 only. I have seen some 
> > > ways to emulate inline variables but I am not sure whether it is worth 
> > > doing so in this case.
> >
> >
> > Yes thanks for the advice.
> >  I have tried moving the definitions to the cpp file but ran into 
> > initialization order fiasco issue because of SanitizerArgs.cpp global 
> > definitions.
> >  If using a constexpr SanitizerMask ctor then that would work around the 
> > initialization fiasco issue hopefully?
> >  Although I am not having much luck using constexpr ctor in VS2015 because 
> > of the array member, so might have to revert back to lo/hi mask members?
>
>
> I would prefer to avoid doing this. What is the problem with `constexpr` ?


I thought of using `constexpr SanitizerMask Foo = {lo, hi};` as this seems 
supported in VS2015 but I cant seem to find a way to get this to work with 
array members...
Now I also realised I would need something like `constexpr SanitizerMask 
FooGroup = Foo + Bar;`
And I havent found a way yet, arrays or no arrays, any ideas?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57914#1405665 , @pgousseau wrote:

> In D57914#1405615 , @riccibruno 
> wrote:
>
> > I think what you would really want to do is mark the masks as `inline 
> > constexpr`, but sadly inline variables are C++17 only. I have seen some 
> > ways to emulate inline variables but I am not sure whether it is worth 
> > doing so in this case.
>
>
> Yes thanks for the advice.
>  I have tried moving the definitions to the cpp file but ran into 
> initialization order fiasco issue because of SanitizerArgs.cpp global 
> definitions.
>  If using a constexpr SanitizerMask ctor then that would work around the 
> initialization fiasco issue hopefully?
>  Although I am not having much luck using constexpr ctor in VS2015 because of 
> the array member, so might have to revert back to lo/hi mask members?


I would prefer to avoid doing this. What is the problem with `constexpr` ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1405615 , @riccibruno wrote:

> I think what you would really want to do is mark the masks as `inline 
> constexpr`, but sadly inline variables are C++17 only. I have seen some ways 
> to emulate inline variables but I am not sure whether it is worth doing so in 
> this case.


Yes thanks for the advice.
I have tried moving the definitions to the cpp file but ran into initialization 
order fiasco issue because of SanitizerArgs.cpp global definitions.
If using a constexpr SanitizerMask ctor then that would work around the 
initialization fiasco issue hopefully?
Although I am not having much luck using constexpr ctor in VS2015 because of 
the array member, so might have to revert back to lo/hi mask members?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I think what you would really want to do is mark the masks as `inline 
constexpr`, but sadly inline variables are C++17 only. I have seen some ways to 
emulate inline variables but I am not sure whether it is worth doing so in this 
case.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau added a comment.

In D57914#1404053 , @riccibruno wrote:

> Wait no, can you really define the `SanitizerMask`s in the header ? Isn't 
> that an odr violation ?


A yes I better move the definitions, thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno requested changes to this revision.
riccibruno added a comment.
This revision now requires changes to proceed.

Wait no, can you really define the `SanitizerMask`s in the header ? Isn't that 
an odr violation ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno accepted this revision.
riccibruno marked an inline comment as done.
riccibruno added a comment.
This revision is now accepted and ready to land.

LGTM with an additional comment.




Comment at: include/clang/Basic/Sanitizers.h:81
+
+  explicit operator bool() {
+for (const auto  : maskLoToHigh)

This operator should be const-qualified.



Comment at: include/clang/Basic/Sanitizers.h:96
+
+  SanitizerMask &=(const SanitizerMask ) {
+for (unsigned k = 0; k < kNumElem; k++)

Nice, a round of applause for clang 8 I guess then. Thanks for looking!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked 7 inline comments as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:96
+return false;
+}
+return true;

riccibruno wrote:
> Is that vectorized by gcc/clang, or is that a sequence of branches ?
clang 6 or gcc 7.3 does not vectorize but clang 8 does!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 187553.
pgousseau added a comment.

Applied suggested changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6199,7 +6199,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+const SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+const SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+const SanitizerMask NotAllowedWithTrap = Vptr;
+const SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+const SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+const SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+const SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+const SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+const SanitizerMask Unrecoverable = Unreachable | Return;
+const SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+const SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+const SanitizerMask NeedsLTO = CFI;
+const SanitizerMask TrappingSupported =
+(Undefined & ~Vptr) | UnsignedIntegerOverflow | ImplicitConversion |
+Nullability | LocalBounds | CFI;
+const SanitizerMask TrappingDefault = CFI;
+const SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+const SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  SanitizerMask TrapRemove; // During the loop below, the accumulated set of
 // 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno marked an inline comment as done.
riccibruno added a comment.

Looks mostly fine to me. I have added a few inline comments but they are all 
little nits.




Comment at: include/clang/Basic/Sanitizers.h:39
+struct SanitizerMask {
+private:
+  // Number of array elements.

`struct` -> `class` and get rid of the `private` ?



Comment at: include/clang/Basic/Sanitizers.h:46
+  static constexpr unsigned kNumBits = sizeof(maskLoToHigh) * 8;
+  // Number of bits in a mask element.
+  static constexpr unsigned kNumBitElem = sizeof(maskLoToHigh[0]) * 8;

/// instead to have doxygen handle these comments (here and elsewhere).



Comment at: include/clang/Basic/Sanitizers.h:51
+  // Mask is initialized to 0.
+  SanitizerMask() : maskLoToHigh{} {}
+

You don't have to explicitly write the default ctor; you can just instead write 
`uint64_t maskLoToHigh[kNumElem]{};` above to value-initialize each element in 
the array.



Comment at: include/clang/Basic/Sanitizers.h:54
+  static constexpr bool
+  checkBitPos(const SanitizerKind::SanitizerOrdinal ) {
+return Pos < kNumBits;

`SanitizerOrdinal` should probably be passed by value here and elsewhere.



Comment at: include/clang/Basic/Sanitizers.h:61
+  bitPosToMask(const SanitizerKind::SanitizerOrdinal ) {
+assert(Pos < kNumBits);
+SanitizerMask mask;

Could you please add a relevant message to the assertion (here and elsewhere) ?



Comment at: include/clang/Basic/Sanitizers.h:96
+return false;
+}
+return true;

Is that vectorized by gcc/clang, or is that a sequence of branches ?



Comment at: lib/Driver/SanitizerArgs.cpp:53
+SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 

Shouldn't all of these masks be const to be sure that they are not modified by 
mistake (unless I a missing something and they are modified somewhere else) ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 187400.
pgousseau added a comment.

Updated to use an array as suggested.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6199,7 +6199,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+SanitizerMask NotAllowedWithTrap = Vptr;
+SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+SanitizerMask Unrecoverable = Unreachable | Return;
+SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+SanitizerMask NeedsLTO = CFI;
+SanitizerMask TrappingSupported = (Undefined & ~Vptr) |
+  UnsignedIntegerOverflow | ImplicitConversion |
+  Nullability | LocalBounds | CFI;
+SanitizerMask TrappingDefault = CFI;
+SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  SanitizerMask TrapRemove; // During the loop below, the accumulated set of
 // sanitizers disabled by 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau marked an inline comment as done.
pgousseau added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:43
+  // Low bits of mask value.
+  uint64_t maskLo;
+  // High bits of mask value.

riccibruno wrote:
> Why not use a fixed size array of `uint64_t`s, and then compute the index 
> without any branch with `ArrayIndex = BitPosition / 64` (with an assertion 
> that `ArrayIndex < MaxArrayIndex` ? This has the advantage that in the future 
> all that is needed to do is bump up the size of the array.
Sounds good! I will give it a try.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/Basic/Sanitizers.h:43
+  // Low bits of mask value.
+  uint64_t maskLo;
+  // High bits of mask value.

Why not use a fixed size array of `uint64_t`s, and then compute the index 
without any branch with `ArrayIndex = BitPosition / 64` (with an assertion that 
`ArrayIndex < MaxArrayIndex` ? This has the advantage that in the future all 
that is needed to do is bump up the size of the array.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D57914#1401367 , @pgousseau wrote:

> Updated patch to not use APInt for representing the mask as it feels like a 
> hammer solution.


Ah! I was going to say exactly this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 187258.
pgousseau edited the summary of this revision.
pgousseau added a comment.

Updated patch to not use APInt for representing the mask as it feels like a 
hammer solution.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.def
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp
  lib/Frontend/CompilerInvocation.cpp
  lib/Sema/SemaDeclAttr.cpp

Index: lib/Sema/SemaDeclAttr.cpp
===
--- lib/Sema/SemaDeclAttr.cpp
+++ lib/Sema/SemaDeclAttr.cpp
@@ -6199,7 +6199,8 @@
 if (!S.checkStringLiteralArgumentAttr(AL, I, SanitizerName, ))
   return;
 
-if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) == 0)
+if (parseSanitizerValue(SanitizerName, /*AllowGroups=*/true) ==
+SanitizerMask())
   S.Diag(LiteralLoc, diag::warn_unknown_sanitizer_ignored) << SanitizerName;
 else if (isGlobalVar(D) && SanitizerName != "address")
   S.Diag(D->getLocation(), diag::err_attribute_wrong_decl_type)
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -551,7 +551,7 @@
 DiagnosticsEngine , SanitizerSet ) {
   for (const auto  : Sanitizers) {
 SanitizerMask K = parseSanitizerValue(Sanitizer, /*AllowGroups=*/false);
-if (K == 0)
+if (K == SanitizerMask())
   Diags.Report(diag::err_drv_invalid_value) << FlagName << Sanitizer;
 else
   S.set(K, true);
Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+SanitizerMask NotAllowedWithTrap = Vptr;
+SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+SanitizerMask Unrecoverable = Unreachable | Return;
+SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+SanitizerMask NeedsLTO = CFI;
+SanitizerMask TrappingSupported = (Undefined & ~Vptr) |
+  UnsignedIntegerOverflow | ImplicitConversion |
+  Nullability | LocalBounds | CFI;
+SanitizerMask TrappingDefault = CFI;
+SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  SanitizerMask TrapRemove; // 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau updated this revision to Diff 186201.
pgousseau added a comment.

Move num bits constant inside class.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57914/new/

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp

Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+SanitizerMask NotAllowedWithTrap = Vptr;
+SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+SanitizerMask Unrecoverable = Unreachable | Return;
+SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+SanitizerMask NeedsLTO = CFI;
+SanitizerMask TrappingSupported = (Undefined & ~Vptr) |
+  UnsignedIntegerOverflow | ImplicitConversion |
+  Nullability | LocalBounds | CFI;
+SanitizerMask TrappingDefault = CFI;
+SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  SanitizerMask TrapRemove(0);  // During the loop below, the accumulated set of
 // sanitizers disabled by the current sanitizer
 // argument or any argument after it.
-  SanitizerMask TrappingKinds = 0;
+  SanitizerMask TrappingKinds(0);
   SanitizerMask TrappingSupportedWithGroups = setGroupBits(TrappingSupported);
 
   for (ArgList::const_reverse_iterator I = Args.rbegin(), E = Args.rend();
@@ -203,24 +206,26 @@
 }
 
 bool SanitizerArgs::needsUnwindTables() const {
-  return Sanitizers.Mask & NeedsUnwindTables;
+  return static_cast(Sanitizers.Mask & NeedsUnwindTables);
 }
 
-bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+bool SanitizerArgs::needsLTO() const {
+  return static_cast(Sanitizers.Mask & NeedsLTO);
+}
 
 SanitizerArgs::SanitizerArgs(const ToolChain ,
  const llvm::opt::ArgList ) {
-  SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
+  SanitizerMask AllRemove(0);   // During the loop below, the accumulated set of
 // sanitizers disabled by the current sanitizer
 // argument or any argument after it.
-  SanitizerMask AllAddedKinds = 0;  // Mask of all sanitizers ever enabled by
+  SanitizerMask AllAddedKinds(0);   // Mask of all sanitizers ever enabled by
 // 

[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[pierre gousseau via Phabricator via cfe-commits]

pgousseau created this revision.
pgousseau added reviewers: rsmith, probinson, gbedwell, filcab, lebedev.ri, 
wristow.
Herald added subscribers: cfe-commits, cryptoad.
Herald added a project: clang.

enum SanitizerOrdinal has reached maximum capacity, this change extends the 
capacity to 128 sanitizer checks.
Uses APInt to represent the bit mask.
This can eventually allow us to add gcc 8's options 
"-fsanitize=pointer-substract" and "-fsanitize=pointer-compare".

Fixes: https://llvm.org/PR39425


Repository:
  rC Clang

https://reviews.llvm.org/D57914

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/Sanitizers.h
  include/clang/Driver/ToolChain.h
  lib/Basic/SanitizerSpecialCaseList.cpp
  lib/Basic/Sanitizers.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Driver/SanitizerArgs.cpp

Index: lib/Driver/SanitizerArgs.cpp
===
--- lib/Driver/SanitizerArgs.cpp
+++ lib/Driver/SanitizerArgs.cpp
@@ -25,29 +25,32 @@
 using namespace clang::driver;
 using namespace llvm::opt;
 
-enum : SanitizerMask {
-  NeedsUbsanRt = Undefined | Integer | ImplicitConversion | Nullability | CFI,
-  NeedsUbsanCxxRt = Vptr | CFI,
-  NotAllowedWithTrap = Vptr,
-  NotAllowedWithMinimalRuntime = Vptr,
-  RequiresPIE = DataFlow | HWAddress | Scudo,
-  NeedsUnwindTables = Address | HWAddress | Thread | Memory | DataFlow,
-  SupportsCoverage = Address | HWAddress | KernelAddress | KernelHWAddress |
- Memory | KernelMemory | Leak | Undefined | Integer |
- ImplicitConversion | Nullability | DataFlow | Fuzzer |
- FuzzerNoLink,
-  RecoverableByDefault = Undefined | Integer | ImplicitConversion | Nullability,
-  Unrecoverable = Unreachable | Return,
-  AlwaysRecoverable = KernelAddress | KernelHWAddress,
-  LegacyFsanitizeRecoverMask = Undefined | Integer,
-  NeedsLTO = CFI,
-  TrappingSupported = (Undefined & ~Vptr) | UnsignedIntegerOverflow |
-  ImplicitConversion | Nullability | LocalBounds | CFI,
-  TrappingDefault = CFI,
-  CFIClasses =
-  CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast,
-  CompatibleWithMinimalRuntime = TrappingSupported | Scudo | ShadowCallStack,
-};
+SanitizerMask NeedsUbsanRt =
+Undefined | Integer | ImplicitConversion | Nullability | CFI;
+SanitizerMask NeedsUbsanCxxRt = Vptr | CFI;
+SanitizerMask NotAllowedWithTrap = Vptr;
+SanitizerMask NotAllowedWithMinimalRuntime = Vptr;
+SanitizerMask RequiresPIE = DataFlow | HWAddress | Scudo;
+SanitizerMask NeedsUnwindTables =
+Address | HWAddress | Thread | Memory | DataFlow;
+SanitizerMask SupportsCoverage =
+Address | HWAddress | KernelAddress | KernelHWAddress | Memory |
+KernelMemory | Leak | Undefined | Integer | ImplicitConversion |
+Nullability | DataFlow | Fuzzer | FuzzerNoLink;
+SanitizerMask RecoverableByDefault =
+Undefined | Integer | ImplicitConversion | Nullability;
+SanitizerMask Unrecoverable = Unreachable | Return;
+SanitizerMask AlwaysRecoverable = KernelAddress | KernelHWAddress;
+SanitizerMask LegacyFsanitizeRecoverMask = Undefined | Integer;
+SanitizerMask NeedsLTO = CFI;
+SanitizerMask TrappingSupported = (Undefined & ~Vptr) |
+  UnsignedIntegerOverflow | ImplicitConversion |
+  Nullability | LocalBounds | CFI;
+SanitizerMask TrappingDefault = CFI;
+SanitizerMask CFIClasses =
+CFIVCall | CFINVCall | CFIMFCall | CFIDerivedCast | CFIUnrelatedCast;
+SanitizerMask CompatibleWithMinimalRuntime =
+TrappingSupported | Scudo | ShadowCallStack;
 
 enum CoverageFeature {
   CoverageFunc = 1 << 0,
@@ -136,10 +139,10 @@
 
 static SanitizerMask parseSanitizeTrapArgs(const Driver ,
const llvm::opt::ArgList ) {
-  SanitizerMask TrapRemove = 0; // During the loop below, the accumulated set of
+  SanitizerMask TrapRemove(0);  // During the loop below, the accumulated set of
 // sanitizers disabled by the current sanitizer
 // argument or any argument after it.
-  SanitizerMask TrappingKinds = 0;
+  SanitizerMask TrappingKinds(0);
   SanitizerMask TrappingSupportedWithGroups = setGroupBits(TrappingSupported);
 
   for (ArgList::const_reverse_iterator I = Args.rbegin(), E = Args.rend();
@@ -203,24 +206,26 @@
 }
 
 bool SanitizerArgs::needsUnwindTables() const {
-  return Sanitizers.Mask & NeedsUnwindTables;
+  return static_cast(Sanitizers.Mask & NeedsUnwindTables);
 }
 
-bool SanitizerArgs::needsLTO() const { return Sanitizers.Mask & NeedsLTO; }
+bool SanitizerArgs::needsLTO() const {
+  return static_cast(Sanitizers.Mask & NeedsLTO);
+}
 
 SanitizerArgs::SanitizerArgs(const ToolChain ,
  const llvm::opt::ArgList ) {
-  SanitizerMask AllRemove = 0;  // During the loop below, the accumulated set of
+  SanitizerMask AllRemove(0);   // During the loop below, the accumulated set of