LGTM!

Thank you vitalybuka eugenis for reviewing! I have pushed the change at https://reviews.llvm.org/rG27830bc2b1b8
Sorry I forgot to add the reviewers and phabricator url to the commit message -_-

In D95859#2575618, @vitalybuka wrote:

In D95859#2568230, @pgousseau wrote:

In D95859#2568106, @vitalybuka wrote:

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Thank you for the comment, yes, this change can potentially break the assumption that odr-indicator=0 reduces binary size, as it will prevent certain linker (eg bfd 2.30), to dead strip unused asan globals’ metadata. Do I understand your point correctly?

But I think correct semantic should take priority over binary size optimizations, please let me know your thoughts on this.

Sanitizers don't mind false-negatives if it can noticeably help with some performance metrics. But I can't reproduce expected size regression.

In D95859#2568106, @vitalybuka wrote:

I guess odr-indicator=1 is for reliable ODR violation detection and odr-indicator=0 of smaller binary size.

Ping

Ping

fix lint issue

• Always use MWAITX_DAG
• Remove uneeded Glue

Updated the patch to handle the case described in PR42064 where a separate basic block demands the use of a base pointer.
In that case we do the codegen of mwaitx during custom insertion.

In D77393#1969419, @probinson wrote:

It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with -verify -fsyntax-only and // expected-no-diagnostics.

Fix false negative test introduce while attempting to add label check.

Reopening to fix false negative test.

Added label

Renaming test and including x86intrin.h instead.

In D73475#1843805, @craig.topper wrote:

Is this subject to the same problem as PR42064?

In D62245#1526733, @jdoerfert wrote:

I think adding all these comments and updating the format is a great thing!

Minor comments to consider:

• I thought we used backslash instead of @ now but I might be wrong. The coding standard uses backslashs, if compiler-rt is not using @ all over the place you should change it.
• Mention in the commit message that you also add descriptions to undocumented functions.
• Mention in the commit message that you improved spelling, grammar, and formatting as well.

I'm not a compiler-rt person but if there is no objection in the next few days, you should go ahead (LGTM).

Replaced doxygen's '@' character by '\'

Ping!

Ping!

In D59815#1442868, @probinson wrote:

This is fine for PS4, I'm just curious whether anyone knows if there's a predicate that means something like "target does not use GNU tools" so we don't have to itemize Fuschia and PS4 this way.

Update patch to reflect changes to the llvm side of the patch.

In D59220#1443749, @vitalybuka wrote:

What is exactly going to be checked by -fsanitize=pointer-compare and -fsanitize=pointer-subtract?
Why this needs to be done in AddressSanitizer?
Is this going just checheck that pointers are from the same allocation?

Use backend option directly rather than introducing new parameters.
Add missing ALL check to test.

Ping!

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.

Change hash_value() declaration's location as suggested.

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

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

reverted at r354875 at this break lldb build.

In D57914#1410387, @riccibruno wrote:

I think this looks good now. Thanks !

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

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

Rework FIXME comment.

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

In D57914#1405955, @riccibruno wrote:

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.

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 ?

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.

In D57914#1404053, @riccibruno wrote:

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

Applied suggested changes.

Updated to use an array as suggested.

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

Move num bits constant inside class.