[PATCH] D124699: [DeadArgElim] Set unused arguments for internal functions

Tue May 31 21:16:23 PDT 2022

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