__libatomic_load might come at the end of the function, with no succeeding BB

Not exactly. It may come at the end of a BB.

In D82317#2200789, @rjmccall wrote:

Are you seriously adding an attribute to literally every argument and return value? Why is this the right representation?

.

LGTM

I can push this tomorrow unless anyone objects before then.

The problem with "a" is that we do not always know what it means exactly (could be a float), and it's dangerous to dereference the pointer.

LGTM

Right, that's what the isNoModRef check is for.

Testing this would be pretty hard - we'd need to touch a bunch of memory pages, and then somehow wait for the huge page daemon to merge them (or not).

You want to start scanning at the same point where poisoning is going to be inserted.

In D83595#2188282, @guiand wrote:

The code currently scans from the alloca, rather than from the lifetime_start. This might make only searching in single BB pretty limiting, since afaict an alloca can be detached from its lifetime region.

LGTM

LGTM

In general, this implementation looks pretty complex and easy to get wrong. I'd prefer something along the lines of AArch64StackTagging::collectInitializers - directly calculate the offset for each store/load instruction. It might do some extra work with unrelated memory instructions, but probably not too much.

Seems fine to me.

In D83595#2185554, @guiand wrote:

This actually has very significant effects on some, but not all, benchmarks.

Running grep I observed ~8% decrease in binary with this patch. But clang sees little effect: <1%.

On a few different benchmarks from a benchmark suite, this also decreased runtime overhead by a significant amount: 8% for sha512, and 13% for qsort.

LGTM

LGTM

Does it help to replace %clangxx_msan with %clang_msan instead?

LGTM

LGTM

In D84446#2170458, @guiand wrote:

I don't believe this happens in C++ but wanted to add it for completeness's sake (or if some other LLVM based language might use it).

Do you want to use this in eager checks when passing an array as a function argument (does that even happen in C++?) ?

LGTM

LGTM with 2 comments

The patch is missing context

Please add a test (instrumentation only, no need for a compiler-rt test).

Sure, no problem.
Thanks for the patch!

LGTM w/ nit

LGTM

Please upload with context.

LGTM

They are "errors reported by Control Flow Integrity" - that part is true. This is valid C++, but it triggers false positive errors in CFI.
This is not without precedent - see https://github.com/llvm/llvm-project/commit/3e58a6a7, https://github.com/llvm/llvm-project/commit/f11c00d7 and more.
The fact that CFI is stricter that the wording of the standard has been discussed many times before, but the conclusion is always that these reports are useful.
This type of false positive is not common at all outside of libc++.
@pcc
@kcc

In D82627#2158393, @ldionne wrote:

Before the lifetime of an object has started but after the storage which the object will occupy has been allocated [...] any pointer that represents the address of the storage location where the object will be or was located may be used but only in limited ways. [...]
The program has undefined behavior if:
[...]

• the pointer is used as the operand of a static_cast (8.2.9), except when the conversion is to pointer to cv void, or to pointer to cv void and subsequently to pointer to cv char, cv unsigned char, or cv std::byte (21.2.1),[...]

That's a good find. However, in this case, what we do is not a static_cast. It's a C-style cast from an unrelated type (std::aligned_storage<...>::type) to __base* (for example in the expression (__base*)&__buf_), which is actually equivalent to a reinterpret_cast unless I'm mistaken. Do you agree?

There is _LIBCPP_NO_CFI, if you prefer it that way.

@HAPPY
I'm not an expert, but I've heard that such casts are UB somewhere.
Looking at [basic.life] 6.8/6 in c++17

LGTM

LGTM with a nit

In D83595#2145447, @guiand wrote:

I figured if it's using GEP then it's likely not going to be storing to the entire shadow. Bitcast is overwhelmingly common though (especially bitcast->lifetime.start, which I realize I need to handle).

Allocas are often used through bitcast or GEP, we should handle them as well.

I don't think 1.5% is good enough to introduce new runtime functions for the off-by-default code path.

read also needs to be wrapped to do COMMON_INTERCEPTOR_UNPOISON_PARAM

LGTM

LGTM

This needs to include tests for bitcode writing and parsing.

This code generates a libcall out of thin air. My intuition says the safest thing to do is to drop all call site attributes, because they generally specify something about how an attribute must be passed to the callee, and not a property of the value being passed, so there is no reason for the attribute lists on pow and on exp to have anything in common at all.

Is unwinding actually broken on an all-clang, all-thumb system?

LGTM

In D82316#2117857, @jdoerfert wrote:

I think, we could even remove the padding sentence and say "undefined or poison" bits. The latter seems to match our existing language. The former should be OK because a value of type { i8, i32 } has 8 + 32 bits. The fact that the store and alloca size is different does not mean the value has undefined bits. If people agree with this interpretation we could also add a sentence explaining this difference.

Oh, it would be great to run libc++ tests with CFI on the buildbot, but it may be complicated, as CFI requires LTO.

This change look right to me, but I defer to libc++ maintainers.

LGTM

The sanitizer_common test seems excessive, but I'm inclined to take it anyway simply because it costs us very little to have it there.

Also, what's the plan to detect these cases in ubsan?

I don't think this has any practical impact on our goals with sanitizers. We should detect undefined behavior before it gets to the point of actually passing or returning an undef or poison value.

In D82249#2110054, @hctim wrote:

In D82249#2109920, @eugenis wrote:

I'm OK with this as a workaround, but it would be more natural to detect the unsupported IR pattern in globalisel and fall back instead of disabling it entirely. Is it difficult to do for some reason?

Eh, it's not an unsupported IR pattern that's the problem, it's that the IR is lowered into adrp + add so that the add can be folded into a ldr/str as an offset. IMO on the scale of "painting over the problem vs. fixing it", this patch is 100% paint, forced fallback with MO_TAGGED is 80% paint for maybe 60% of the work of just fixing it.

I'm working on fixing this properly now that we know we don't need to make a less-risky, fast-to-deploy patch. If that all falls into place this patchset will just become obsolete anyway.