[PATCH] D81678: Introduce frozen 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 frozen attribute at call sites for stricter poison analysis

rsmith 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) { Other types that we should

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

jdoerfert added a comment. > We don't have any optimizations that infer either "speculatable" or "frozen", > though, so I'm not sure there's any practical impact here. `speculatable` is on the shortlist for the Attributor. Will happen eventually. I am sure `frozen` will follow once it is in.

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

efriedma added a comment. In D81678#2109059 , @nlopes wrote: > I'm a bit concerned with this patch as it increases the amount of UB that > LLVM exploits without any study of the impact. > For example, right now it's ok do this with clang (not with

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

eugenis added a comment. >> 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. MSan

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

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4095 +} + } call->setCallingConv(getRuntimeCC()); eugenis wrote: > guiand wrote: > > jdoerfert wrote: > > > Why would we do this? Function attributes are valid at the call site, no

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

jdoerfert added a comment. In D81678#2109059 , @nlopes wrote: > I'm a bit concerned with this patch as it increases the amount of UB that > LLVM exploits without any study of the impact. > For example, right now it's ok do this with clang (not with

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

nlopes added a comment. I'm a bit concerned with this patch as it increases the amount of UB that LLVM exploits without any study of the impact. For example, right now it's ok do this with clang (not with constants; make it less trivial so clang doesn't fold it right away): int f() { return

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

guiand marked 2 inline comments as done. guiand added inline comments. Comment at: clang/include/clang/Driver/CC1Options.td:507 +def disable_frozen_args : Flag<["-"], "disable-frozen-args">, + HelpText<"Disable emitting frozen attribute in LLVM IR">; def load : Separate<["-"],

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

eugenis added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4095 +} + } call->setCallingConv(getRuntimeCC()); guiand wrote: > jdoerfert wrote: > > Why would we do this? Function attributes are valid at the call site, no > > need to copy

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

jdoerfert added inline comments. Comment at: clang/include/clang/Driver/CC1Options.td:507 +def disable_frozen_args : Flag<["-"], "disable-frozen-args">, + HelpText<"Disable emitting frozen attribute in LLVM IR">; def load : Separate<["-"], "load">, MetaVarName<"">,

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

guiand added a comment. I've added the test change to yet another diff, which you can find here: https://reviews.llvm.org/D82317 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678

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

guiand added a comment. @jdoerfert I've separated out changes to the language reference to https://reviews.llvm.org/D82316 as you suggested. I kept the name `frozen` for now while we reach a consensus regarding its final name. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

guiand updated this revision to Diff 272489. guiand added a comment. I've updated this patch to only include the actual implementation of `frozen`, for easier review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/

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

aqjune added a comment. In D81678#2105077 , @eugenis wrote: > > Could you explicitly state that if it is aggregate or vector type all > > elements and paddings should be frozen too? > > If an aggregate is passed as an aggregate at IR level, we should not

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

jdoerfert requested changes to this revision. jdoerfert added a comment. This revision now requires changes to proceed. First, apologies for being late, I didn't properly monitor the list recently. --- This diff is impossible to review and later to understand. I would ask you to split it, at

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

aqjune accepted this revision. aqjune added a comment. This revision is now accepted and ready to land. For the LangRef part, LGTM modulo the suggested change. Could anyone review clang and LLVM's updated part? Comment at: llvm/docs/LangRef.rst:1644 +and return values.

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

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

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

eugenis added a comment. > Could you explicitly state that if it is aggregate or vector type all > elements and paddings should be frozen too? If an aggregate is passed as an aggregate at IR level, we should not care about the padding. Unless it's coerced to an integer. Repository: rG LLVM