Skip to content
  • mevenson@1c010e3e-69d0-11dd-93a8-456734b0d56f's avatar
    compiler: fix stack inconsistency errors · 2a07d4e6
    (somewhat-functional-programmer) Mark
    
    Lately I have been starting to dive deeper into Common Lisp and have started to
    use ABCL more than SBCL or CCL lately. It is a very impressive project.
    
    I want to post a patch for review/comments and hopefully have it be worthwhile
    to eventually include it in ABCL. The patch attempts to fix a couple of stack
    inconsistency bugs in the compiler. I came across the stack inconsistency issue
    in one of my projects and started to try to find the root cause of the problem
    based on a nice minimal reproduction of the bug found in
    https://github.com/armedbear/abcl/issues/69.
    
    However, my particular bug was slightly different. It had to do with using a
    return-from in the cleanup form of an unwind-protect. The following two forms
    also result in a stack inconsistency problem (and are a more minimal
    reproduction of the bug my code introduced):
    
    (defun two-arg-fn (one two)
     (format t "Two args: ~S and ~S~%" one two))
    
    (let ((fn (compile nil '(lambda ()
                            (two-arg-fn
                             (block test-block
                               (unwind-protect
                                    30
                                 (return-from test-block 8)))
                             -1)))))
     (funcall fn))
    
    My patch handles both the github issue and the stack inconsistency in the form
    above. It also fixes jvm::print-code to print string representations of values
    from the constant pool which I found useful in debugging the output.
    
    Anyhow, let me attempt to quickly summarize the stack inconsistency problem in
    general:
     - Certain common lisp control flow forms
       (tagbody/go/unwind-protect/block/return-from/throw/catch) require the use
       of JVM exceptions to implement in bytecode
     - When the JVM throws an exception, the operand stack is cleared and the
       exception is pushed onto the operand stack (see jvms8, 6.5/athrow, p378)
     - Therefore, any form which pushes values onto the operand stack for
       further use is confounded when these control flow forms are child
       forms
     - To properly handle these alternate control flows we need to save the
       result of the control flow form to a local variable in the stack frame
       (distinct from the operand stack, and not destroyed by an exception
       (well at least not until the exception passes /out/ of the method)
       and then reload for use by the parent form to push on the operand stack
    
    Take the case of the ash function (from the github issue). Bytecode for ash is
    emitted from jvm::p2-ash. It compiles its arguments to the operand stack, and is
    therefore vulnerable to the problem discussed above. Other low level functions
    (like + for example in p2-plus) use the following forms to overcome this issue:
    jvm::with-operand-accumulation and jvm::compile-operand. These forms save the
    results of "unsafe" forms (opstack unsafe) to "registers" (local variables in
    the stack frame). This technique allows for these complicated control flow forms
    to be a child form of + with no issues, but not the ash function (which does not
    do this). See my patch for how I added these already present
    with-operand-accumulation and compile-operand forms to ash so it is no longer
    vulnerable to stack inconsistency bugs.
    
    Generally, function calls in ABCL are not vulnerable to these stack
    inconsistency bugs. Function arguments are processed in jvm::process-args, and
    in this function, the opstack safety of child forms is checked, and values are
    saved to "registers" when a form is known to be unsafe. My case (the return-from
    in the cleanup form of an unwind-protect) simply wasn't properly being marked as
    opstack unsafe. I modified jvm::p1-unwind-protect to mark all direct children of
    the unwind-protect as opstack unsafe, which eliminated my problem. I believe
    there may have been confusion here in the code (speculation of course on my
    part) of 'protected' form referring to the form actually protected by the
    unwind-protect (which is totally different than being unsafe or needing opstack
    /protection/) (or I'm misinterpreting this function and potentially causing
    additional bugs!).
    
    The only other item in my patch is fixing how bytecode is printed for debugging.
    Basically, most items in a class constant pool are referenced with a 2 byte
    index, but one (ldc) uses a one byte index). This has been accounted for in the
    new function jvm::constant-pool-index.
    
    Let me know what you think, and again please review, there aren't many lines
    changed but I am new to the internals of the project. I think I ran all tests
    (ant abcl.test) but while the ant task completed successfully, my output
    complained of a missing dependency and I'm not sure what actually ran.
    
    -Mark
    
    The attached patch was produced against b3cfee66
    from https://github.com/easye/abcl (latest master branch).
    
    [jvms8] https://docs.oracle.com/javase/specs/jvms/se8/jvms8.pdf
    
    
    Addresses <https://github.com/armedbear/abcl/issues/69>.
    
    In response to
    <https://mailman.common-lisp.net/pipermail/armedbear-devel/2019-May/003977.html>.
    2a07d4e6