bug#34318: 26.1.90; Strange behavior of two line message with running shell

[Stefan Monnier]

> The problem is with
>
> 2018-06-03  Stefan Monnier  <monnier@iro.umontreal.ca>
>
>       Fix bug#30846, along with misc cleanups found along the way
>
> and in particular
>
>       Don't call swap_in_symval_forwarding since the currently swapped
>       binding is never one we've modified.

Indeed, good spotting.  I installed the patch below which mostly reverts
this part of the commit.

> So please, pretty please review that entire patch again - maybe it
> contains additional sorts of problems.

This is a rather tricky part of the code, indeed.  I reviewed it
thoroughly before installing it and yet here we are.
I re-reviewed it now and couldn't spot any further mistakes, but you
know what this means.


        Stefan


diff --git a/src/data.c b/src/data.c
index 571114802a..ed6dedbe24 100644
--- a/src/data.c
+++ b/src/data.c
@@ -1954,6 +1954,16 @@ Instead, use `add-hook' and specify t for the LOCAL 
argument.  */)
        (current_buffer,
         Fcons (Fcons (variable, XCDR (blv->defcell)),
                BVAR (current_buffer, local_var_alist)));
+
+      /* If the symbol forwards into a C variable, then load the binding
+         for this buffer now, to preserve the invariant that forwarded
+         variables must always hold the value corresponding to the
+         current buffer (they are swapped eagerly).
+         Otherwise, if C code modifies the variable before we load the
+         binding in, then that new value would clobber the default binding
+         the next time we unload it.  See bug#34318.  */
+      if (blv->fwd)
+        swap_in_symval_forwarding (sym, blv);
     }
 
   return variable;
diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index 0069ee84fe..f3b4262de4 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -508,4 +508,22 @@ binding-test-some-local
                        (bound-and-true-p data-tests-foo2)
                        (bound-and-true-p data-tests-foo3)))))))
 
+(ert-deftest data-tests-make-local-forwarded-var () ;bug#34318
+  ;; Boy, this bug is tricky to trigger.  You need to:
+  ;; - call make-local-variable on a forwarded var (i.e. one that
+  ;;   has a corresponding C var linked via DEFVAR_(LISP|INT|BOOL))
+  ;; - cause the C code to modify this variable from the C side of the
+  ;;   forwarding, but this needs to happen before the var is accessed
+  ;;   from the Lisp side and before we switch to another buffer.
+  ;; The trigger in bug#34318 doesn't exist any more because the C code has
+  ;; changes.  Instead I found the trigger below.
+  (with-temp-buffer
+    (setq last-coding-system-used 'bug34318)
+    (make-local-variable 'last-coding-system-used)
+    ;; This should set last-coding-system-used to `no-conversion'.
+    (decode-coding-string "hello" nil)
+    (should (equal (list last-coding-system-used
+                         (default-value 'last-coding-system-used))
+                   '(no-conversion bug34318)))))
+
 ;;; data-tests.el ends here