vitalybuka added a comment.
In D124699#3549191 <https://reviews.llvm.org/D124699#3549191>, @kstoimenov
wrote:
> This change is breaking memory sanitizer in some cases. We observed that
> when the argument is not actually removed the pass is dropping the noundef
> attribute. See the diff snippet below. Is that an intended behavior?
>
> We would like to revert this as it is breaking MSan tests.
>
> Thanks,
> Kirill
>
> < define internal %struct._object*
> @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object*
> noundef %args) #0 !dbg !1065 {
> ---
> > define internal %struct._object*
> @coro_wrapper_close(%struct.PyCoroWrapper* noundef %cw, %struct._object*
> %args) #0 !dbg !1065 {
I guess I realized why msan, is broken. It's indirectly cased by this patch,
but the problem is in our code.
This patch poisons more msan arguments (see NOTE below), but still correctly.
However I just noticed that the issues we are working with Kirill involves a
peace of notinstrumented assembly, which fails to propagate Msan shadow.
@kstoimenov and me will find a different solution.
*NOTE* This optimization is negative for msan with noundef mode
(-fsanitizer-memory-param-retval)
If we remove noundef, then msan must to setup TLS for arguments. If it's undef,
we set that into -1 to poison arg.
For msan it could be more efficient to keep noundef and pass null into that
argument.
But that's not a problem of this patch, and we will address that later if
needed.
================
Comment at: llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp:268
// }
if (!Fn.hasExactDefinition())
return false;
----------------
@kstoimenov Offline I guessed that maybe our problem because we lost noundef on
weak function arg, however this check protects against that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124699/new/
https://reviews.llvm.org/D124699
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
