[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

This revision was automatically updated to reflect the committed changes. Closed by commit rG10264a1b21ae: Introduce noundef attribute at call sites for stricter poison analysis (authored by guiand). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D8167

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 328060. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def clang/include/clang/Driver/Options.td clang/lib/CodeGen/C

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 328055. guiand added a comment. Reupload patch to trigger build Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def c

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

nlopes added a comment. @rsmith already gave his blessing, so please go ahead. I hope you guys have a plan to enable this by default at some point as features behind a flag get rotten and no one uses them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

eugenis added a comment. Any more comments/opinions? Gui, would you like to push it to main if no one objects? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 ___

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

eugenis accepted this revision. eugenis added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 ___

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 322899. guiand added a comment. Updated to use -enable-noundef-analysis Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

aqjune added a comment. I'm fine with any direction that helps people land test updates/implementations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 ___ cfe-c

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand added a comment. Changing the mechanism to explicit opt-in seems like a good idea, and bypasses the test issues. If everyone agrees about proceeding with this, I can fix up this patch within the next couple days and we can hopefully wrap this up! Repository: rG LLVM Github Monorepo

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

nikic added a comment. Herald added a reviewer: jansvoboda11. @guiand Do you plan to continue working on this patch? If not, I would try to finalize it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

eugenis added a comment. In D81678#2503931 , @nikic wrote: > As the discussion is spread out across multiple threads, do I understand > correctly that the current consensus is to introduce the > `-disable-noundef-analysis` flag, and explicitly add it to

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

nikic added a comment. As the discussion is spread out across multiple threads, do I understand correctly that the current consensus is to introduce the `-disable-noundef-analysis` flag, and explicitly add it to all the relevant tests (rather than adding it to the substitutions)? In any case,

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

aqjune added a comment. In D81678#2438264 , @eugenis wrote: > This change looks fine to me, but I'm slightly concerned about > https://reviews.llvm.org/D85788 - see my last comment on that revision. Thank you for the link! I left a comment there. Repos

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

eugenis added a comment. This change looks fine to me, but I'm slightly concerned about https://reviews.llvm.org/D85788 - see my last comment on that revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D8167

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

aqjune added a comment. Hello all, What is the status of this patch? Do we need someone who look into the lit change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

eugenis updated this revision to Diff 298692. eugenis added a comment. fix type size check for vscale types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

aqjune added a comment. I think it makes sense - IIUC, for most of the clang tests, noundef won't be the attribute of interest. For brevity of tests, I think the change is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://revi

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 284931. guiand added a comment. Made the compiler flag non-public Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand requested review of this revision. guiand added a comment. I think I'd like someone to take a look at the `llvm-lit` changes to see if this makes sense as an approach Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 284907. guiand added a comment. Herald added a subscriber: delcypher. 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. Repo

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand marked 5 inline comments as done. guiand added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2148-2150 +} else if (const VarDecl *VDecl = dyn_cast(TargetDecl)) { + // Function pointer + HasStrictReturn &= !VDecl->isExternC(); rsmi

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 281676. guiand added a comment. Addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def clang/include/

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 281679. guiand added a comment. Updated comment on disable-noundef-args option Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGen

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good with suggestions applied, and with the portability problems in the test fixed. (Maybe just add a `-triple`? Though it would be good to also test `inalloca` and ARM constructor `this

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 281345. guiand added a comment. Fix typo in MayDropFunctionReturn Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 281323. guiand edited the summary of this revision. guiand added a comment. Fixes regression; allows emitting noundef for non-FunctionDecls as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 281311. guiand added a comment. Incorporate C++'s more conservative checks for omitted return values Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/inclu

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 280622. guiand added a comment. 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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 279949. guiand added a comment. 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"). @rsmith, I just di

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand added a comment. 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. To make it easier to be sure this specific feat

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

rsmith added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2110 + DetermineNoUndef(RetTy, getTypes(), DL, RetAI)) { +RetAttrs.addAttribute(llvm::Attribute::NoUndef); + } rsmith wrote: > This only applies in C++. In C, it's valid for a functi

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. I'd like to see some testcases for the C++ side of this. The following things all seem like they might be interesting: passing and returning classes and unions, especially ones that

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand closed this revision. guiand added a comment. Merged in https://reviews.llvm.org/rG780528d9da707b15849d6c9711cc3ab19f6c7f00 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 _

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

jdoerfert resigned from this revision. jdoerfert added a comment. This revision is now accepted and ready to land. My concerns have been addressed. I am not the right one to look at the clang changes but what we merged into LLVM was really useful, thanks again for working on this! Repository:

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand reopened this revision. guiand added a comment. This revision is now accepted and ready to land. Sorry, closed this mistakenly! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 _

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand added a comment. Herald added a subscriber: dang. Is anything still pending here (besides the tests, of course)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 276487. guiand added a comment. Per @nikic's suggestion, I isolated the LLVM side of the changes to a separate revision D83412 , which should be good to go. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 274895. guiand added a comment. Addressed comments, added test for indirect calls Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/Code

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

jdoerfert added inline comments. Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:932 case Attribute::NoCfCheck: + case Attribute::NoUndef: break; nikic wrote: > Not familiar with this code, but based on the placement of other similar

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand added a comment. I've run across an issue when bootstrapping clang, that manifests itself in the following code when compiled with optimizations: #include float my_exp2(float a) { return pow(2.0, a); } With this code, the call to `pow` is lifted to a `exp2`, and then the

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

efriedma added a comment. In D81678#2120709 , @guiand wrote: > Could anyone point me to where I might need to make a change for that? LibCallSimplifier::optimizePow Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2082 + const Type *RetTyPtr = RetTy.getTypePtr(); + if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() && + RetAI.getKind() != ABIArgInfo::Indirect) { guiand wrote: > guiand wrote

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

nikic added inline comments. Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:932 case Attribute::NoCfCheck: + case Attribute::NoUndef: break; Not familiar with this code, but based on the placement of other similar attributes like no

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 273558. guiand added a comment. Made DetermineNoUndef more robust Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand marked an inline comment as done. guiand added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2082 + const Type *RetTyPtr = RetTy.getTypePtr(); + if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() && + RetAI.getKind() != ABIArgInfo::Indirect) { ---

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand updated this revision to Diff 273456. guiand retitled this revision from "Introduce frozen attribute at call sites for stricter poison analysis" to "Introduce noundef attribute at call sites for stricter poison analysis". guiand edited the summary of this revision. guiand added a comment.

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

guiand marked an inline comment as done. guiand added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2082 + const Type *RetTyPtr = RetTy.getTypePtr(); + if (!RetTy->isVoidType() && !RetTyPtr->isRecordType() && + RetAI.getKind() != ABIArgInfo::Indirect) { ---