Page MenuHomePhabricator

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

Authored by pgousseau on Feb 7 2019, 11:00 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pgousseau updated this revision to Diff 187258.Feb 18 2019, 9:45 AM
pgousseau edited the summary of this revision. (Show Details)

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

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.

riccibruno added inline comments.Feb 18 2019, 10:03 AM
include/clang/Basic/Sanitizers.h
43 ↗(On Diff #187258)

Why not use a fixed size array of uint64_ts, 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.

pgousseau marked an inline comment as done.Feb 18 2019, 10:15 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
43 ↗(On Diff #187258)

Sounds good! I will give it a try.

Updated to use an array as suggested.

riccibruno marked an inline comment as done.Feb 19 2019, 11:53 AM

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

include/clang/Basic/Sanitizers.h
39 ↗(On Diff #187400)

struct -> class and get rid of the private ?

46 ↗(On Diff #187400)

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

51 ↗(On Diff #187400)

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.

54 ↗(On Diff #187400)

SanitizerOrdinal should probably be passed by value here and elsewhere.

61 ↗(On Diff #187400)

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

96 ↗(On Diff #187400)

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

lib/Driver/SanitizerArgs.cpp
53 ↗(On Diff #187400)

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) ?

pgousseau updated this revision to Diff 187553.Feb 20 2019, 5:17 AM

Applied suggested changes.

pgousseau marked 7 inline comments as done.Feb 20 2019, 5:24 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
96 ↗(On Diff #187400)

clang 6 or gcc 7.3 does not vectorize but clang 8 does!

riccibruno accepted this revision.Feb 20 2019, 6:34 AM
riccibruno marked an inline comment as done.

LGTM with an additional comment.

include/clang/Basic/Sanitizers.h
81 ↗(On Diff #187553)

This operator should be const-qualified.

96 ↗(On Diff #187553)

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

This revision is now accepted and ready to land.Feb 20 2019, 6:34 AM
riccibruno requested changes to this revision.Feb 20 2019, 6:46 AM

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

This revision now requires changes to proceed.Feb 20 2019, 6:46 AM

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

A yes I better move the definitions, thanks!

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.

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

riccibruno added a comment.EditedFeb 21 2019, 8:46 AM

To define the masks as constexpr variables you will have to make at least bitPosToMask, operator|, operator& and operator~ constexpr. 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.

More explicitly something like:

in Sanitizer.h:

template <typename T = void> struct SanitizerMasks {
  static const SanitizerMask SomeMask;
  /* and so on for each mask*/
};

template <typename T> const SanitizerMask SanitizerMasks<T>::SomeMask = the definition;

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

More explicitly something like:

in Sanitizer.h:

template <typename T = void> struct SanitizerMasks {
  static const SanitizerMask SomeMask;
  /* and so on for each mask*/
};

template <typename T> const SanitizerMask SanitizerMasks<T>::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!

pgousseau updated this revision to Diff 187913.Feb 22 2019, 2:41 AM

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

pgousseau updated this revision to Diff 187917.Feb 22 2019, 3:26 AM

Rework FIXME comment.

riccibruno added inline comments.Feb 22 2019, 3:58 AM
include/clang/Basic/Sanitizers.h
148 ↗(On Diff #187917)

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.

173 ↗(On Diff #187917)

You are back to the same issue with the references.

pgousseau marked 2 inline comments as done.Feb 22 2019, 4:19 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
148 ↗(On Diff #187917)

Makes sense thanks!

173 ↗(On Diff #187917)

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?

riccibruno added inline comments.Feb 22 2019, 4:22 AM
include/clang/Basic/Sanitizers.h
173 ↗(On Diff #187917)

Nope, unfortunately.

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

pgousseau updated this revision to Diff 188128.Feb 25 2019, 4:04 AM

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

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

include/clang/Basic/Sanitizers.h
133 ↗(On Diff #188128)

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

169 ↗(On Diff #188128)

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

pgousseau updated this revision to Diff 188175.Feb 25 2019, 7:33 AM

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

pgousseau marked 2 inline comments as done.Feb 25 2019, 7:36 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
54 ↗(On Diff #187400)

Yes I meant to initially, done now thanks!

riccibruno accepted this revision.Feb 26 2019, 4:31 AM

I think this looks good now. Thanks !

This revision is now accepted and ready to land.Feb 26 2019, 4:31 AM

I think this looks good now. Thanks !

Pushed at r354873, thanks for the help!

This revision was automatically updated to reflect the committed changes.

reverted at r354875 at this break lldb build.

pgousseau reopened this revision.Feb 27 2019, 10:00 AM

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<clang::SanitizerMask>' 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<clang::SanitizerMask>' 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<llvm::hash_code, clang::SanitizerMask>' 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<llvm::hash_code, clang::SanitizerMask>' 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 &Arg);
          ^
1 error generated.
This revision is now accepted and ready to land.Feb 27 2019, 10:00 AM

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

riccibruno requested changes to this revision.Feb 28 2019, 6:51 AM
riccibruno added inline comments.
include/clang/Basic/Sanitizers.h
29 ↗(On Diff #188576)

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

This revision now requires changes to proceed.Feb 28 2019, 6:51 AM
pgousseau marked an inline comment as done.Feb 28 2019, 7:49 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
29 ↗(On Diff #188576)

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.

riccibruno added inline comments.Feb 28 2019, 8:44 AM
include/clang/Basic/Sanitizers.h
29 ↗(On Diff #188576)

Never mind, I overlooked that it is only the declaration of llvm::hash_code hash_value(const clang::SanitizerMask &Arg); 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 &Arg); just after SanitizerMask so that you don't have to forward-declare SanitizerMask (but keep the forward-declaration of hash_code in llvm).

pgousseau updated this revision to Diff 188748.Feb 28 2019, 9:03 AM

Change hash_value() declaration's location as suggested.

pgousseau marked 2 inline comments as done.Feb 28 2019, 9:04 AM
pgousseau added inline comments.
include/clang/Basic/Sanitizers.h
29 ↗(On Diff #188576)

Sounds good, done!

riccibruno accepted this revision.Feb 28 2019, 10:16 AM

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

This revision is now accepted and ready to land.Feb 28 2019, 10:16 AM
This revision was automatically updated to reflect the committed changes.
pgousseau marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 2:04 AM
jyknight added a subscriber: jyknight.EditedMar 2 2019, 1:03 PM

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.

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!

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!