Maybe then just keep as is to avoid new preprocessor branches?

The rest is LGTM

Could you please move refactoring (like extraction emitPrologue) into a separate CL?

In D114487#3593011, @fhahn wrote:

In D114487#3555041, @vitalybuka wrote:

@fhahn
This is reduced reproducer:

Thanks! I think I was able to isolate the bits that caused problems:

1. Unecessarily looking through selects outside the loop
2. Introducing branches on undef/poison when looking through selects.

I recommitted the change in e9cced27390ba38eac1144aa1240281a1edadec0. I couldn't test with msan though, as it's not available on macOS. Please let me know if you see further issues.

Thanks for the fix!

https://lab.llvm.org/buildbot/#/builders/105/builds/26496
and
https://lab.llvm.org/buildbot/#/builders/19/builds/11181

maybe #define _GNU_SOURCE as in documentation?

I believe "Depends on" should be in the summary, no in the title

No need to recover my snapshot, I'll just comment here.

In D126929#3567491, @hctim wrote:

Update.

In D127163#3564919, @hctim wrote:

We already have -fsanitize=memtag that implies -fsanitize=memtag-heap and -fsanitize=memtag-stack.

It makes the most sense IMHO in the world where we want heap MTE. -fsanitize=memtag-heap is the most reasonable instead of -fsanitize-memtag-heap, as "do i need -fsanitize=memtag as well, like MSan?" is never a question.

Up to you if you, if you want to wait for Peter. Feel free to remove and land it if you think it's done.
I added blocker only to make sure that review is shown to @phosek on Phabricator main page.

Why do we want this as a separate -fsanitize=
Maybe better to have some modifier flag like -fsanitize-memtag-globals=1 ?

I simplified this a little bit, but I guess this is effectively the same.

rebase

fix rebase

rebase

yes, however I have no idea if we need !TC.getTriple().isOSFreeBSD() !TC.getTriple().isOSNetBSD() !TC.getTriple().isOSOpenBSD()) etc.
I guess if make mistake it will be noticed and fixed.

In D86669#3561225, @efriedma wrote:

In D86669#3561125, @vitalybuka wrote:

In D86669#3560983, @efriedma wrote:

If you're seeing a miscompile, something is probably wrong with the caller. getUnderlyingObjects() is, in general, not guaranteed to produce an identifiable object. If the caller cares, it should check; for example, GlobalsAAResult::getModRefInfoForArgument checks all_of(Objects, isIdentifiedObject).

I am not sure why do don't want to make it guaranty with a patch like this?

How could we possibly guarantee that? In general, we're going to find some opaque thing that both getUnderlyingObjects() and its caller can't understand.

I mean, I guess we could define an API that narrowly guarantees it looks though all bitcast, addrspacecast, gep, phi, and select operations. But that would be expensive in general, and it's not obvious to me it solves anything.

In D86669#3560983, @efriedma wrote:

If you're seeing a miscompile, something is probably wrong with the caller. getUnderlyingObjects() is, in general, not guaranteed to produce an identifiable object. If the caller cares, it should check; for example, GlobalsAAResult::getModRefInfoForArgument checks all_of(Objects, isIdentifiedObject).

Thanks.
LGTM with few nits

LGTM, with Asm test and bitcode stuff moved as explained.

Can you please rebase this patch onto others two?

@fhahn
This is reduced reproducer:

This patch breaks msan bots: https://lab.llvm.org/buildbot/#/builders/5/builds/24307 and https://lab.llvm.org/buildbot/#/builders/74

In D124699#3549191, @kstoimenov wrote:

This change is breaking memory sanitizer in some cases. We observed that when the argument is not actually removed the pass is dropping the noundef attribute. See the diff snippet below. Is that an intended behavior?

We would like to revert this as it is breaking MSan tests.

Thanks,
Kirill

< define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* noundef %args) #0 !dbg !1065 {
---
> define internal %struct._object* @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object* %args) #0 !dbg !1065 {

In D114487#3547560, @fhahn wrote:

In D114487#3546674, @vitalybuka wrote:

I will continue to minimize the reproducer.
Should we revert the patch, because it breaks Msan and maybe there is a chance the bounds are calculated incorrectly?

Thanks for the report, I'll take a look. I already have suspicion of what's going wrong.

This patch is causing false reports with msan. I am trying to creduce the code. It's with TensorFlow, so far it's quite complicated.

all stuff under llvm/... here needs testing, Bitcode and AsmParser have many tests usable as examples

Do you have estimate of how often this happend? How many different instrumented functions which can be called from uninstrumented code?

In D126102#3537519, @paulkirth wrote:

@vitalybuka what if we just changed the implementation to return whatever it had read so far?

so in https://github.com/llvm/llvm-project/blob/06fee478d217a9fbd2ba31f92bc595ed327635a5/compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_libcdep.cpp#L535, we just break instead of setting the read_len = 0, and allow the filled buffer to be output?

In D126102#3536097, @paulkirth wrote:

In D126102#3536038, @vitalybuka wrote:

I would prefer you just disable the test for your platform.

The failures we see are for AArch64 and x86_64 Linux... so are you saying we should disable the test for Linux?

Yes, even "// UNSUPPORTED: *" with FIXME is fine

In D126102#3536037, @paulkirth wrote:

@vitalybuka I'm hoping to just address the flaky test for now. There's some value in ensuring that long output doesn't crash symbolization w/in sanitizer runtimes, so I've opted to keep the test, and try to make it less prone to flake. For now, I'm happy just to remove the flake so we can update our Toolchain.

In D123822#3534734, @leonardchan wrote:

*ping* any more comments?

Please also link patches related to #1508 into review "Stack" (see "Edit Related Revisions" on the menu)

We need a test here as well

We need a test for that