Sorry, I must've missed this. @aqjune, if you'd be willing to take on this change, that would be amazing (I haven't got much time to update these patches lately).

We decided to go with a positive flag for enabling noundef, so I'm closing this.

Reupload patch to trigger build

For sure. I'll upload a rebased patch shortly and give it another day or so for people to look, and then push if there aren't any issues.

Updated to use -enable-noundef-analysis

Changing the mechanism to explicit opt-in seems like a good idea, and bypasses the test issues.

This whole thing is a little unfortunate, but maybe a better substitution would be leaving %clang as referring to the pure clang binary, with default arguments. Then we can have a %clang_cc which may only be used for a standard C compiler invocation.

Did we decide that we wanted this change then? I remember there being discussion around whether it's the right approach.

Sorry about the mix-up. This seems like exactly what we should do to clean up here.

@rjmccall We've discussed several different possibilities here. Does any of them strike you as a good step forward here?

LGTM, although I'm just curious about the omissions here. why not mark something like round noundef?

Emit a nop instruction to always mark the end of the MSan prologue, and insert prologue instructions before that.

Caught by http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/41009/steps/ninja%20check%201/logs/FAIL%3A%20MemorySanitizer-POWERPC64%3A%3A%20chained_origin_memcpy.cpp

Move removeUnreachableBlocks before inserting the prologue. This makes sure there's no issue with the ActualFnStart instruction being deleted before the visitor loop.

Committed as 97de0188dd5d845ff90c8ac779a2ea09688b17df

Remove BB splitting for KMSAN

Added a test for hash codegen.

Updated to depend on https://reviews.llvm.org/D85985 (ActualFnStart becomes a Instruction *)

Addressed comments

In D82317#2212145, @jdoerfert wrote:

TBH, I don't see how this solves any problem. It just makes it a problem for someone in the future... (FWIW, I say this being in full support of noundef)

Made the compiler flag non-public

Allows passing different extra arguments for different clang expansions

Added willreturn. I think nocapture is probably best left to a future change since it's more invasive -- it would require keeping tabs of which arguments to each function is a pointer arg.

I think I'd like someone to take a look at the llvm-lit changes to see if this makes sense as an approach

To try to alleviate the tests issue, @eugenis and I discussed that it might be best to take it slow. So now this patch will mask off emitting the attribute on clang tests by default.

After discussing with @eugenis, for the meantime it might be best to do the following:

• Change the masking attribute to be -fdisable-noundef-analysis (name notwithstanding), and have it completely turn off all noundefs
• Change the llvm-lit configuration to use the new codegen flag for all the tests by default
• Have noundef emitted in the frontend by default (when the codegen flag isn't present)

LGTM, but I don't really maintain this code.

I don't think we can necessarily guarantee argmemonly/readonly/writeonly, particularly since these library calls can take a lock somewhere inside. I'll definitely add nocapture and willreturn though.

Addressed comments

Addressed comments. For handling number of uses per stack trace, this uses a bit of a heuristic:

Separated out the frontend change. Addressed other comments.

Rebased on master (again)

Rebased on master

Simplified by returning to the old implementation, but having libatomic calls made nounwind (so we never see them as invokes).

Modified to just use __sanitizer::mem_is_zero

Most of the discussion has been in https://reviews.llvm.org/D81678 (implementing the attribute in clang) and https://reviews.llvm.org/D82316 (adding the attribute to langref).

In D82317#2200809, @jrtc27 wrote:

At the very least, make whatever script you used to update these public, as I don't want to have to recreate it from scratch when merging this in. I had enough "fun" with the LLD mass-renaming (UpperCamel -> lowerCamel) and that was _with_ a supposedly-working script (it didn't quite do the right thing and I seem to recall there being two refactoring commits, only one of which had a script); I do not want a repeat of that experience.

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. Does this support the a format specifier as well, or just m?

LGTM

Integrated with Alias Analyzer, uses simpler mechanism for walking through BB and determining stores to alloca

It seems like collectInitializers leans heavily on the isPointerOffset function, which returns an offset if two pointers have a constant difference, nullopt if they don't. The problem here is that we can't distinguish isPointerOffset == nullopt happening because the offset is determined at runtime, or because the two pointers are completely unrelated.

Updated.

Oh, wait, the patch didn't update. One sec.

Fixed to actually set the shadow and origin. Sorry for the mix-up!

Rebased; all tests passing again. Removed the change to the ppc-*mmintrin.c tests, instead I just use the -disable-noundef-args flag`. Cleaned up typos.

Oh! I need to test that it's not checked as well.

In D83595#2188021, @eugenis wrote:

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.

Flattened some control flow, updated to properly use StoreOffs, and updated tests to cover chained GEPs

I've cut it down to only within a basic block as @eugenis and @vitalybuka suggested.

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

@jdoerfert what would the procedure be for reviewing these test changes / getting this landed with the noundef patch?

I agree, this probably isn't going to be worth it.

Updated comment on disable-noundef-args option

Addressed comments

All tests up to date. Of particular note are the ppc-*mmintrin.c tests, which seemed to drastically change upon rerunning the test autogen script.

Fix typo in MayDropFunctionReturn

Fixes regression; allows emitting noundef for non-FunctionDecls as well.