[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. Herald added a subscriber: StephenFan. The ThinLTO related breakage I mentioned above should be fixed as of https://github.com/llvm/llvm-project/commit/451799bb8261bde52bbfef226d019caf1d82aa42. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks added a comment. I'll hold off on submitting this until that bug is figured out Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/ https://reviews.llvm.org/D133036 ___ cfe-commits

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks updated this revision to Diff 475017. aeubanks added a comment. I somehow missed the previous comments rebased on top of fixing UB in tests changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. I'm sorry for the noise. Further investigation has shown that this happens when Rust is doing (thin) LTO, and I don't think this patch can be considered in any way "at fault" here, so this is the last you'll hear from me on the topic here. I don't know whether the fault

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. I didn't manage to repro with `opt`, so still no compilable IR. I did some more debugging, though: - Inside `removeDeadArgumentsFromCallers`, `CB->getCalledFunction()->dump()` (after the modification) is `define void

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. I've included excerpts from the IR below. It will take me a bit to provide something compilable. Though you are right, the `noundef` did indeed get removed from the `call`. *** IR Dump Before DeadArgumentEliminationPass on [module] *** ; Function Attrs: nonlazybind

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

nikic added a comment. @TimNN Do you have an IR sample for this? DAE is supposed to strip UB-implying attributes when converting arguments to poison here:

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. I'm still trying to properly minimize this, but this definitely interacts badly with other optimizations (which triggers the Rust CI failure I mentioned above): - We start with two functions, `outer` and `inner`. `outer` calls `inner`. `outer` has a `noundef` argument

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

nikic accepted this revision. nikic added a comment. This revision is now accepted and ready to land. The default switch has happened, so unblocking this. Comment at: clang/test/CodeGenOpenCL/overload.cl:23 generic int *local *genloc; generic int *global *genglob; + //

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks added a comment. In D133036#3816002 , @aeubanks wrote: > @vitalybuka can we turn on -fsanitize-memory-param-retval by default upstream? attempt in https://reviews.llvm.org/D134669 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks added a comment. @vitalybuka can we turn on -fsanitize-memory-param-retval by default upstream? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/ https://reviews.llvm.org/D133036 ___

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

nikic requested changes to this revision. nikic added a comment. This revision now requires changes to proceed. I'd like to block this on `-fsanitize-memory-param-retval` (aka msan eager checks) being enabled by default. It seems pretty clear that there is a *lot* of UB due to uninitialized

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks updated this revision to Diff 462249. aeubanks added a comment. extra parens Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/ https://reviews.llvm.org/D133036 Files: clang/test/CodeGenOpenCL/overload.cl

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks updated this revision to Diff 462247. aeubanks added a comment. add flag to disable (to be removed in the future) "fix" lldb test skip on msan instrumented functions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks updated this revision to Diff 462245. aeubanks added a comment. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. "fix" lldb test (hopefully) skip when msan add flag to disable (to be removed in the future) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

sbc100 added a comment. This change also broken emscripten in some odd ways: https://app.circleci.com/pipelines/github/emscripten-core/emscripten/23359/workflows/4080be5f-bd82-45b9-a355-3a5d4f4ef977/jobs/564543. The revert seems to have fixed it. Repository: rG LLVM Github Monorepo

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

TimNN added a comment. This also broke Rust when compiled at LLVM head: https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/13166#0182fb4a-0f2d-4f2e-830f-f62b463b8d48. (I don't know whether this was some existing UB that got only exposed by this patch or not, but wanted to

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

omjavaid added a comment. Apparently some problem occuring due to trouble with ASAN exception https://lab.llvm.org/buildbot/#/builders/96/builds/28300 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/ https://reviews.llvm.org/D133036

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

omjavaid reopened this revision. omjavaid added a comment. This revision is now accepted and ready to land. This change has broken LLDB Arm/AArch64 Linux buildbots. I dont really understand the underlying reason. Reverting for now to make buildbot green. Repository: rG LLVM Github Monorepo

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

mstorsjo added a comment. FYI, this change broke Wine (at least on arm and aarch64). Not saying that it is legit breakage of code that actually was UB to begin with though - it's going to take some time to figure out what's broken though. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc911befaec49: [InstCombine] Treat passing undef to noundef params as UB (authored by aeubanks). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D133036: [InstCombine] Treat passing undef to noundef params as UB

aeubanks updated this revision to Diff 457383. aeubanks added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. fix clang test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133036/new/