[m-dev.] [Mercury-Language/mercury] Segfault in only Mercury after solutions in a particular code path (#72)

[Zoltan Somogyi]

2019-08-26 15:00 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
> On Mon, 26 Aug 2019 13:36:33 +1000 (AEST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>> 
>> The root of the problem is the parameter passing. In the inlined version,
>> when we fill in the argument of R_4, we update R_4. In the non-inlined version,
>> when we fill in the argument of HeadVar__1, for which main passes R_4,
>> we update HeadVar__1, but do NOT update R_4 in main. Normally, this is
>> not a problem: the filled in argument is normally on the heap, and the caller's
>> pointer also points to it, so the caller also sees the field as being filled in.
>> However, with the direct arg optimization, the field being filled in is NOT
>> on the heap; it is in the pointer, next to the primary tag. It is the lack of
>> any update to this field in the caller's R_4 that leaves the value of R_4 as a
>> tagged NULL pointer, whose dereferencing leads to the crash.
>> 
>> I see two obvious approaches to fixing this. The quick-and-dirty fix would be
>> to insist that the calls to unify predicates that have this problem (i.e. they are
>> for a type with one or more direct arg functors, and some args of those direct-arg
>> functors are initially free) be inlined even in the presence of --no-inlining.
>> The other would be to modify the parameter passing conventions for such
>> unifications, which would require not just nontrivial changes to the code
>> that handles both the caller and the callee sides of such calls, but probably
>> also significant changes in all the code generators. Currently all code generators
>> assume that a call's outputs define *new* variables; making them handle situations
>> in which a call has one or two outputs that update *existing* variables would also
>> not be trivial.
>> 
>> On that basis, the first approach would seem more pragmatic, even though
>> conceptually it is far from satisfying.
>> 
>> Opinions?
> 
> Unfortunately the problem is not limited to unify predicates, as in the
> attached test case. We may need to disable the direct arg optimization
> until the calling convention can be changed.

I have come up with what I think is an easier way to fix this bug.
(Yes, this bug is more than a year old now; we last discussed it on m-dev
around 2019 aug 26.)

The reason why I didn't work on it at the time was that I didn't think that
either approach I could see was feasible. Changing both the LLDS and MLDS
code generators to handle an extremely rare corner case looked to be both
very fragile (i.e. likely to break as other changes were made to those code
generators), and therefore likely to impose extra work on anyone making
such changes in order to avoid such breakage.

I think I have come up with a way to avoid all that. Instead of teaching
the code generators that some arguments need to be passed by
value/result, with an initial value as input and the final value as output,
we could have a compiler pass that runs sometime before code generation
that replaces all arguments that are potentially subject to this requirement
(i.e. args whose top function symbol is input, but this function may be
a direct_arg functor with a free argument that the predicate binds)
with a pair of arguments, one input and one output.

The idea is the following.

- A compiler pass looks for such arguments in every procedure.
  If a procedure has one or more such arguments, we create a clone
  of that procedure in which (a) we add a new output headvar after
  each such headvar, and (b) we add a unification to the end of the
  procedure body that unifies each such pair of arguments.
  (The latter is needed only if we will generate code for the procedure.)
  For each such procedure, we record the id of its clone, and
  the list of argument positions that were "twinned".

- In almost all cases, we won't find any such arguments, and the
  pass is done. In cases such as tests/hard_coded/gh72b.m,
  when we do find such arguments, we have to scan the bodies
  of all procedures in the module, looking for calls to any of
  the cloned procedures. When we find, such a call, we would
  replace the callee with the clone procedure, and create new
  variables to hold the output twins. If e.g. such a call passed
  variable A as a partially instantiated input, then we would create
  a new variable A' to hold its grounded (or at least further instantiated)
  version. We would then replace all later occurrences of A with A'.

  One tricky part here is dealing with situations in which different arms
  of a branched control structure require such replacements for different
  sets of variables, but I think the technique we use to implement
  state variables would would here as well. It should also handle the
  other tricky part. Even though the free argument of a direct arg
  function symbol can be bound just once, the type of A may have
  more than one direct arg function symbol, so it is possible for
  one call in a conjunction to bind the argument of one function symbol
  only, and then a later call bind the argument of another. Therefore
  we may need A'', A''' and so on.

The idea is that this pass would transform the gh72b.m test case,
which crashes, to the attached code, which works.

The only fly in the ointment I can see is that not all references to
a cloned procedure are calls to that procedure. This approach
would not handle the case where the program constructs a closure
using a cloned procedure. In most cases, the closure is then passed
to code that knows its declared signature, and whose implementation
works only with that signature, so this transformation, which changes
the signature, wouldn't work. But at least, the scan of the procedure
bodies can easily be made to find such situations, so we can at least
generate a "sorry, not implemented" error message from the compiler,
instead of a runtime crash.

The very first task above, looking for arguments that should be twinned,
could be done by the simplification pass at the end of semantic analysis,
since it has to look at all procedures anyway. This should make the cost
of all this almost nothing in the usual case. The win is that we can keep
on using the direct arg optimization.

Thoughts?

Zoltan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gh72c.m
Type: application/octet-stream
Size: 1355 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/developers/attachments/20201116/81641c5e/attachment.obj>