writeonly fns cannot read memory, but fninputRefined violates it

[aqjune]

Consider this function:

define void @f(i8* %p) {
  %call = call i8* @g1(i8* %p)
  call void @g2(i8* %call)
  ret void
}

declare i8* @g1(i8*)
declare void @g2(i8*) writeonly

Running alive-tv with an identity input raises ERROR: Source is more defined than target, and the reason is pretty interesting.

When encoding the refinement of g2's inputs, Pointer::fninputRefined is called. fninputRefined is checking isBlockAlive().implies(other.isBlockAlive()), but this isn't well-defined because g2 is a writeonly function.
State::addFnCall is filling the source's input memory with just an initial memory.

Should Pointer::fninputRefined take an additional flag which doesn't check liveness? Pointer::fninputRefined already has is_byval_arg flag.

[nunoplopes]

nice catch!
Then I guess we do need to pass the memory. Even if a function only writes to memory, it has to write to valid blocks. So if it writes through an argument, that argument better be alive.
The whole problem is that the comparison function takes all fields of memory into account instead of a subset. e.g. for readonly it wouldn't compare the value field.

[aqjune]

Then I guess we do need to pass the memory. Even if a function only writes to memory, it has to write to valid blocks. So if it writes through an argument, that argument better be alive.

+1, it makes sense.

The whole problem is that the comparison function takes all fields of memory into account instead of a subset. e.g. for readonly it wouldn't compare the value field.

I didn't fully understand this, could you elaborate a bit more please?

[aqjune]

IIUC, calls_fn update in State::addFnCall should be fixed:

    auto call_data_pair
      = calls_fn.try_emplace(
          { move(inputs), move(ptr_inputs),
            reads_memory ? analysis.ranges_fn_calls
                         : State::ValueAnalysis::FnCallRanges(), <<< this and
            reads_memory ? memory : Memory(*this),            <<< this should consider `writes_memory` as well?
            reads_memory, argmemonly });

[nunoplopes]

Right. the point is that doing it right now will regress performance because calls_fn relies on Memory::operator< which compares all fields. While for e.g. readonly we shouldn't be comparing the local/non-local values. We achieve that right now by creating a fresh memory.

[aqjune]

What about introducing a new noread_fn_call_data field that uses a custom comparator that doesn't consider local/non-local values?
To minimize diff, I can create a State::MemoryOperator class, and make Memory declare it as a friend class. This would be a one-liner change in Memory.h .

[nunoplopes]

That would create a conflict with the new branch, as comparators are different. Let's hold this bug for a bit pls.