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

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

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

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

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

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/

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

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: >

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

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:

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

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)

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

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/

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

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"

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

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 {

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

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`

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

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

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

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

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

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*/ > }; > >

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

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

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

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

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

pgousseau added a comment. In D57914#1405781 , @riccibruno wrote: > In D57914#1405665 , @pgousseau wrote: > > > In D57914#1405615 , @riccibruno > > wrote: > > > > > I think

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

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

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

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

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

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

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

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

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

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/

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

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() { +

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

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 ?

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

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

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

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.

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

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

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

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

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

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

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

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

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

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/

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

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

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

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