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.

Incorporate C++'s more conservative checks for omitted return values

LGTM. Thanks!

LGTM

Added an across-the-board test with several different interesting cases. @rsmith, I'm not sure how to test things like VTables/VTTs, since afaik the way they would be exposed to function attributes would be if a struct is being passed by value. But in that case, we currently never emit noundef.

Update tests to reflect more strict noundef rules

I think we can actually just drop the #include <stdatomic.h> line here. It was used for a previous version of the test but I forgot to remove it after some changes.

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).

@amyk Sorry about that! The issue should be fixed now.

Added a test case

Just saw your comment about tests as well. The idea was to have all tests ported over as part of a separate commit (I linked it in the main patch description) and then only to push either commit once both are ready to land.

Adds additional constraints on noundef: Not a nullptr_t, not a member pointer, and not coerced to a type of larger size. Disabled emitting in return position for non-C++ languages (or inside extern "C").

Reuploaded with context

Sorry about that. Reuploaded with full context.

Kept only core of patch: adding new LIT configration for eager-checks

This patch actually has no dependency on clang emitting noundef, so it should be good to go.