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.

In D55712#1334603, @probinson wrote:

I missed that filcab had said ok internally. Go ahead Pierre.

Updated patch with requested changes.

In D31457#769039, @vitalybuka wrote:

LGTM

In D31457#767980, @eugenis wrote:

In D31457#759229, @pgousseau wrote:

Invert arguments positions in Min call.

I don't think the order of the arguments matters here. Vitaly's comment was about READ_STRING vs READ_RANGE.

Replaced READ_RANGE by READ_STRING

Ping! Latest comment has been addressed, ok to commit?

Invert arguments positions in Min call.

• Fix off by one issue using Vitaly's fix.
• Expanded msan and asan unit tests to check for the correct handling of non zero terminated strings.

Reopening after issue was found and reverted in rL303339

strndup_oob_test.cc also fails on armv7l-unknown-linux-gnueabihf (cf http://lab.llvm.org:8011/builders/clang-cmake-thumbv7-a15-full-sh/builds/1521).
I have marked it as unsupported in rL302807 as this is a known issue.

One of the test I have added (strndup_oob_test.cc) is failing on the s390 linux bot (cf http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/8096) .
I have marked the test as unsupported on s390 for now in rL302789 if that's ok?