inline expansion reuses functionals from incompatible policies

Bug #309141 reported by Nikodemus Siivola on 2008-12-17
2
Affects Status Importance Assigned to Milestone
SBCL
High
Nikodemus Siivola

Bug Description

  a.
  (declaim (ftype (function (cons) number) acc))
  (declaim (inline acc))
  (defun acc (c)
    (the number (car c)))

  (defun foo (x y)
    (values (locally (declare (optimize (safety 0)))
              (acc x))
            (locally (declare (optimize (safety 3)))
              (acc y))))

  (foo '(nil) '(t)) => NIL, T.

  As of 0.9.15.41 this seems to be due to ACC being inlined only once
  inside FOO, which results in the second call reusing the FUNCTIONAL
  resulting from the first -- which doesn't check the type.

Changed in sbcl:
importance: Undecided → Medium
status: New → Confirmed
Nikodemus Siivola (nikodemus) wrote :

Heap corruption potential in safe code, bumping to HIGH.

Changed in sbcl:
importance: Medium → High
Nikodemus Siivola (nikodemus) wrote :

The problem is that FIND-FREE-FUN returns the defined-fun which has the functional from the SAFETY 0 call for the SAFETY 0 call, and consequently the functional converted in the earlier policy gets used.

This might be a point in favor of not using the policy of the call site for inline expansions.

Alternatively we need to make sure we get a distinct functional for each distinct policy a defined-fun is used in.

Nikodemus Siivola (nikodemus) wrote :

Mean to say:

"The problem is that FIND-FREE-FUN returns the defined-fun which has the functional from the SAFETY 0 call for the SAFETY 3 call, and consequently the functional converted in the earlier policy gets used."

Nikodemus Siivola (nikodemus) wrote :

Attached patch changes DEFINED-FUN-FUNCTIONAL into DEFINED-FUN-FUNCTIONALS, an alist indexed by the LEXENV-POLICY.

It fixes the symptoms and doesn't cause any (apparent) regressions, but I don't really like it: not only does it feel kludgy, it also seems to me that DEFINED-FUN-FUNCTIONAL should not really hold inline expansions at all, just NAMED-LAMBDA functionals for use with block compilation.

Note how in the original bug the two ACCs have been coalesced as if they had been declared MAYBE-INLINE: this will still happen with the patch, if the policies match. (I am not strictly opposed to such coalescing, but ...)

Removing inline expansions from DEFINED-FUN-FUNCTIONAL and instead caching them in a component-local hash-table indexed by policy and function name seems like a nicer way to handle this, maybe.

Nikodemus Siivola (nikodemus) wrote :

In 1.0.24.42, based on the patch attached to an earlier comment. More comprehensive treatment seems best left for a time when block compilation is being worked on, after all.

Changed in sbcl:
status: Confirmed → Fix Committed
assignee: nobody → nikodemus
Nikodemus Siivola (nikodemus) wrote :

Released as 1.0.25.

Changed in sbcl:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers