bug#39529: 28.0.50; Metahelp does not contain help text

bug#39529: 28.0.50; Metahelp does not contain help text

[Federico Tedin]

On Emacs 28.0.50 (5e7bead8ea), typing C-h C-h will display the
*Metahelp* buffer, but instead of containing the usual help information,
it will contain just the following:

Automatically break line at a previous space, in insertion of text.

(which appears to be the document string for `auto-fill-function' in
simple.el).

bug#39529: 28.0.50; Metahelp does not contain help text

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmail.com>
> Date: Sun, 09 Feb 2020 20:06:40 +0100
> 
> On Emacs 28.0.50 (5e7bead8ea), typing C-h C-h will display the
> *Metahelp* buffer, but instead of containing the usual help information,
> it will contain just the following:
> 
> Automatically break line at a previous space, in insertion of text.

It's related to dumping in some way, because in temacs this works as
expected.

bug#39529: 28.0.50; Metahelp does not contain help text

[Eli Zaretskii]

> Date: Sun, 09 Feb 2020 21:22:54 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 39529@debbugs.gnu.org
> 
> > From: Federico Tedin <federicotedin@gmail.com>
> > Date: Sun, 09 Feb 2020 20:06:40 +0100
> > 
> > On Emacs 28.0.50 (5e7bead8ea), typing C-h C-h will display the
> > *Metahelp* buffer, but instead of containing the usual help information,
> > it will contain just the following:
> > 
> > Automatically break line at a previous space, in insertion of text.
> 
> It's related to dumping in some way, because in temacs this works as
> expected.

I suspect the sxhash changes on Jan 7.  This problem started happening
in that day's build.

bug#39529: 28.0.50; Metahelp does not contain help text

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On 2/9/20 11:34 AM, Eli Zaretskii wrote:

> I suspect the sxhash changes on Jan 7.  This problem started happening
> in that day's build.

Right you are. The problem was Emacs was using a hash table to unify identical 
Lisp compiled objects when purecopying them, even though their doc strings were 
not set up yet (and so were 0 placeholders that always compared equal). Formerly 
this bug was masked because sxhash simply returned the objects' hashed 
addresses, but the January 7 changes unveiled the bug. I install the attached 
patch to fix the problem.

[-- Attachment #2: 0001-Fix-C-h-C-h-bug-due-to-mutating-a-hash-key.patch --]
[-- Type: text/x-patch, Size: 2051 bytes --]

From 3480071dfab30eaca7f1d014600b864d2ea22f62 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 15 Feb 2020 15:12:34 -0800
Subject: [PATCH] Fix C-h C-h bug due to mutating a hash key
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Federico Tedin (Bug#39529).
The problem was that dumping uses a hash table based on 'equal'
when purecopying compiled objects, but then modifies the compiled
objects while they are keys in the table.  This no-no was uncovered
by the sxhash fixes in 2020-01-07T19:23:11Z!eggert@cs.ucla.edu.
Eli Zaretski pinpointed the patch that triggered the bug.
* src/lread.c (read1): When reading a compiled object, replace
its docstring with a unique negative integer instead of with 0,
so that purecopy doesn’t unify it with some other compiled object
that happens to have the same Lisp code.
---
 src/lread.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/src/lread.c b/src/lread.c
index c124d5a1d8..f39e81ae2c 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -2974,6 +2974,21 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
 	  vec = XVECTOR (tmp);
 	  if (vec->header.size == 0)
 	    invalid_syntax ("Empty byte-code object");
+
+	  if (COMPILED_DOC_STRING < vec->header.size
+	      && AREF (tmp, COMPILED_DOC_STRING) == make_fixnum (0))
+	    {
+	      /* read_list found a docstring like '(#$ . 5521)' and treated it
+		 as 0.  This placeholder 0 would lead to accidental sharing in
+		 purecopy's hash-consing, so replace it with a (hopefully)
+		 unique integer placeholder, which is negative so that it is
+		 not confused with a DOC file offset.  Eventually
+		 Snarf-documentation should replace the placeholder with the
+		 actual docstring.  */
+	      EMACS_UINT hash = XHASH (tmp) | (INTMASK - INTMASK / 2);
+	      ASET (tmp, COMPILED_DOC_STRING, make_ufixnum (hash));
+	    }
+
 	  make_byte_code (vec);
 	  return tmp;
 	}
-- 
2.17.1

bug#39529: 28.0.50; Metahelp does not contain help text

[Eli Zaretskii]

> Cc: federicotedin@gmail.com, 39529-done@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sat, 15 Feb 2020 15:25:44 -0800
> 
> > I suspect the sxhash changes on Jan 7.  This problem started happening
> > in that day's build.
> 
> Right you are. The problem was Emacs was using a hash table to unify identical 
> Lisp compiled objects when purecopying them, even though their doc strings were 
> not set up yet (and so were 0 placeholders that always compared equal). Formerly 
> this bug was masked because sxhash simply returned the objects' hashed 
> addresses, but the January 7 changes unveiled the bug. I install the attached 
> patch to fix the problem.

Thanks, the problem is gone now.

bug#39529: 28.0.50; Metahelp does not contain help text

[Pip Cet]

On Sat, Feb 15, 2020 at 11:26 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> > I suspect the sxhash changes on Jan 7.  This problem started happening
> > in that day's build.
>
> Right you are. The problem was Emacs was using a hash table to unify identical
> Lisp compiled objects when purecopying them, even though their doc strings were
> not set up yet (and so were 0 placeholders that always compared equal). Formerly
> this bug was masked because sxhash simply returned the objects' hashed
> addresses, but the January 7 changes unveiled the bug. I install the attached
> patch to fix the problem.

That works as a quick fix, but it's not perfectly correct, since it
relies on different objects having different hashes, and they might,
by pure chance, have the same hash.

We need to fix the underlying issue, or at least go for the correct
"quick fix", which is to make equal = eq for bytecode objects (this is
almost indistinguishable from the previous behavior, before
sxhash-equal was "fixed").

The pure-cons hash, and many other places, assume "equal" means
"equivalent" in some way. That's not true for bytecode objects, where
a function always returning `nil' can be equal to one always returning
`t'.

And I still suspect many other people have made the same "mistake" as
I, of assuming the previous behavior of equal-based hashes for markers
and overlays was guaranteed. To recap, the January 7 changes broke
code that uses markers or overlays as hash keys (or as components of
hash keys) in equal-based hash tables; before the changes, it was safe
to do so. After the changes, it's only safe to do if the character
positions don't change between storage and lookup, and there's no hint
at what might be happening.

In summary, I still think the right fix is not to make equal look at
more state than sxhash-equal used to, particularly for Emacs 27. We've
now seen that the January 7 changes cause follow-up bugs, such as this
one, so maybe it's time to reconsider what the right thing to do is
here.

bug#39529: 28.0.50; Metahelp does not contain help text

[Paul Eggert]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

On 2/16/20 8:51 AM, Pip Cet wrote:

> it relies on different objects having different hashes, and they might,
> by pure chance, have the same hash.

That could happen only if two Lisp vector addresses disagree only in the 
high-order bit during dumping while on a USE_LSB_TAG platform, something that I 
didn't think possible on practical Emacs targets. However, the potential problem 
is easy enough to work around; I installed the attached.

> We need to fix the underlying issue, or at least go for the correct
> "quick fix", which is to make equal = eq for bytecode objects (this is
> almost indistinguishable from the previous behavior, before
> sxhash-equal was "fixed").

Give the attached patch I hope no such quick fix is needed.

For what it's worth, the patched code is similar to what Fautoload does; so if 
there's something wrong here, a similar wrongness has likely been present in 
Fautoload for some time.

> The pure-cons hash, and many other places, assume "equal" means
> "equivalent" in some way. That's not true for bytecode objects, where
> a function always returning `nil' can be equal to one always returning
> `t'.

Could you give an example of this? When I byte-compiled this:

(defun always-t () t)
(defun always-nil () nil)

the .elc file had different bytecode objects #[nil "\300\207" [t] 1] and #[nil 
"\300\207" [nil] 1]. (The strings are the same, but that's not the issue here.)

> the right fix is not to make equal look at
> more state than sxhash-equal used to, particularly for Emacs 27.

That would indeed make 'equal' and 'sxhash-equal' consistent. But wouldn't it 
potentially break a different set of user programs, that (say) rely on 'equal' 
returning nil when markers point to different locations? So I'd be leery about 
messing with Emacs 27 in this area.

For the master branch, it's more of a question of what 'equal' should mean. 
Obviously 'sxhash-equal' must be consistent with 'equal'. If we decide that 
'equal' should not inspect marker contents etc., then we should change 
'sxhash-equal' accordingly.

(To my mind a better fix would be to introduce the notion of immutability, and 
for sxhash-equal to inspect only the immutable parts of key contents. But now 
it's time for me to duck and run....)

[-- Attachment #2: 0001-Improve-C-h-C-h-bug-fix.patch --]
[-- Type: text/x-patch, Size: 1482 bytes --]

From 556cc727e5076d590f8286406e4f46cff3cee41e Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 16 Feb 2020 11:36:19 -0800
Subject: [PATCH] Improve C-h C-h bug fix

* src/lread.c (read1): Guard against two 'struct Lisp_Vector *'
pointers differing only in their most significant bit.  Problem
reported by Pip Cet (Bug#39529#22).
---
 src/lread.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/src/lread.c b/src/lread.c
index 1613719eb1..70984d37e1 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -2982,10 +2982,13 @@ read1 (Lisp_Object readcharfun, int *pch, bool first_in_list)
 		 as 0.  This placeholder 0 would lead to accidental sharing in
 		 purecopy's hash-consing, so replace it with a (hopefully)
 		 unique integer placeholder, which is negative so that it is
-		 not confused with a DOC file offset.  Eventually
-		 Snarf-documentation should replace the placeholder with the
-		 actual docstring.  */
-	      EMACS_UINT hash = XHASH (tmp) | (INTMASK - INTMASK / 2);
+		 not confused with a DOC file offset (the USE_LSB_TAG shift
+		 relies on the fact that VALMASK is one bit narrower than
+		 INTMASK).  Eventually Snarf-documentation should replace the
+		 placeholder with the actual docstring.  */
+	      verify (INTMASK & ~VALMASK);
+	      EMACS_UINT hash = ((XHASH (tmp) >> USE_LSB_TAG)
+				 | (INTMASK - INTMASK / 2));
 	      ASET (tmp, COMPILED_DOC_STRING, make_ufixnum (hash));
 	    }
 
-- 
2.17.1

bug#39529: 28.0.50; Metahelp does not contain help text

[Pip Cet]

On Sun, Feb 16, 2020 at 7:43 PM Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 2/16/20 8:51 AM, Pip Cet wrote:
> > it relies on different objects having different hashes, and they might,
> > by pure chance, have the same hash.

Just to be clear about that: I was wrong here, and had misread the
code. It does not, in fact, rely on arbitrary different objects having
different hashes; it relies on different vectors having different
XHASHes, which you correctly point out they do, on practical targets.

> That could happen only if two Lisp vector addresses disagree only in the
> high-order bit during dumping while on a USE_LSB_TAG platform, something that I
> didn't think possible on practical Emacs targets. However, the potential problem
> is easy enough to work around; I installed the attached.

You're absolutely correct.

> > We need to fix the underlying issue, or at least go for the correct
> > "quick fix", which is to make equal = eq for bytecode objects (this is
> > almost indistinguishable from the previous behavior, before
> > sxhash-equal was "fixed").
>
> Give the attached patch I hope no such quick fix is needed.

Let me turn around that argument: if we rely on the precise
definitions of XHASH etc. that were in place prior to the January
changes, no "quick fix" was needed; it was simply the case that the
predicate used by hash tables created with :test equal diverged from
the built-in `equal' predicate in one more way than was already
documented. (Those documented deviations are that mutations of hash
keys won't cause rehashing, and that equal will sometimes signal for
(equal a b) but not for (equal b a)).

> For what it's worth, the patched code is similar to what Fautoload does; so if
> there's something wrong here, a similar wrongness has likely been present in
> Fautoload for some time.

Thanks for pointing that out, I wasn't aware of it.

> > The pure-cons hash, and many other places, assume "equal" means
> > "equivalent" in some way. That's not true for bytecode objects, where
> > a function always returning `nil' can be equal to one always returning
> > `t'.
>
> Could you give an example of this? When I byte-compiled this:

(defun f () (let ((x '(#1=(nil) . #1#)))
          (eq (car x) (cdr x))))
(defun g () (let ((x '((nil) . (nil)))
          (eq (car x) (cdr x))))

That's somewhat contrived; more realistic examples where this might
actually be a problem would use string properties.

> > the right fix is not to make equal look at
> > more state than sxhash-equal used to, particularly for Emacs 27.
>
> That would indeed make 'equal' and 'sxhash-equal' consistent. But wouldn't it
> potentially break a different set of user programs, that (say) rely on 'equal'
> returning nil when markers point to different locations?

You're right, that wouldn't work.

I'll propose one more thing, which sounds horrible at first but might
be the least horrible option: accept that `equal', the function, and
`:test equal', the hash table predicate, have diverged significantly
already. Keep them separate but similar in Emacs 27, and introduce
more useful hash table predicates in Emacs 28.

That would allow us to restore the previous behavior for Emacs 27,
document the inconsistency there, fix the pdumper bug that caused the
original bug report, remove the hacky XHASH implementation dependency
from Fautoload and read1, and return to cleaner semantics for Emacs
28.

> So I'd be leery about messing with Emacs 27 in this area.

I think we already have messed with Emacs 27 in ways that we shouldn't have.

> (To my mind a better fix would be to introduce the notion of immutability, and
> for sxhash-equal to inspect only the immutable parts of key contents. But now
> it's time for me to duck and run....)

immutably-equal would indeed be one hash table predicate that I'd like to see.

bug#39529: 28.0.50; Metahelp does not contain help text

[Paul Eggert]

On 2/18/20 11:56 AM, Pip Cet wrote:

> if we rely on the precise
> definitions of XHASH etc. that were in place prior to the January
> changes, no "quick fix" was needed; it was simply the case that the
> predicate used by hash tables created with :test equal diverged from
> the built-in `equal' predicate in one more way than was already
> documented. (Those documented deviations are that mutations of hash
> keys won't cause rehashing, and that equal will sometimes signal for
> (equal a b) but not for (equal b a)).

I'm not aware of these deviations - where are they documented?

What I see in the Emacs 27 documentation, under "Defining Hash 
Comparisons", is the statement that the hash and equality functions' 
"behavior should depend on only on properties of the keys that do not 
change". This is true of 'equal' and 'sxhash-equal' if one does not 
mutate any key that's in the hash table. So from what I can see, the 
predicates are supposed to be the same, it's just that one can't mutate 
keys.

Also, I don't understand what is meant by "the predicate used by hash 
tables created with :test equal diverged from the built-in `equal' 
predicate". cmpfn_equal simply calls Fequal, so how can their behaviors 
diverge?

>>> The pure-cons hash, and many other places, assume "equal" means
>>> "equivalent" in some way. That's not true for bytecode objects, where
>>> a function always returning `nil' can be equal to one always returning
>>> `t'.
>>
>> Could you give an example of this? When I byte-compiled this:
> 
> (defun f () (let ((x '(#1=(nil) . #1#)))
>            (eq (car x) (cdr x))))
> (defun g () (let ((x '((nil) . (nil)))
>            (eq (car x) (cdr x))))
> 
> That's somewhat contrived; more realistic examples where this might
> actually be a problem would use string properties.

Although this example is a good one that illustrates a bug (or at least 
a poorly-documented feature) in dumping, it hasn't changed because of 
the January 7 changes to sxhash. The same issue exists in Emacs 27 (and 
in Emacs 26 for that matter). So I'd rather address this issue 
separately, perhaps simply by documenting it for now.

> I'll propose one more thing, which sounds horrible at first but might
> be the least horrible option: accept that `equal', the function, and
> `:test equal', the hash table predicate, have diverged significantly
> already.
Again I'm not following, because the two functions haven't diverged as 
far as I can see.

bug#39529: 28.0.50; Metahelp does not contain help text

[Pip Cet]

On Wed, Feb 19, 2020 at 1:22 AM Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 2/18/20 11:56 AM, Pip Cet wrote:
>
> > if we rely on the precise
> > definitions of XHASH etc. that were in place prior to the January
> > changes, no "quick fix" was needed; it was simply the case that the
> > predicate used by hash tables created with :test equal diverged from
> > the built-in `equal' predicate in one more way than was already
> > documented. (Those documented deviations are that mutations of hash
> > keys won't cause rehashing, and that equal will sometimes signal for
> > (equal a b) but not for (equal b a)).
>
> I'm not aware of these deviations - where are they documented?
>
> What I see in the Emacs 27 documentation, under "Defining Hash
> Comparisons", is the statement that the hash and equality functions'
> "behavior should depend on only on properties of the keys that do not
> change".

That sentence is what I was referring to by "mutations of hash keys
won't cause rehashing", which is one of the deviations. The other one
is documented in objects.texi:

Comparing circular lists may therefore cause deep recursion that leads
to an error, and this may result in counterintuitive behavior such as
@code{(equal a b)} returning @code{t} whereas @code{(equal b a)}
signals an error.

> This is true of 'equal' and 'sxhash-equal' if one does not
> mutate any key that's in the hash table. So from what I can see, the
> predicates are supposed to be the same, it's just that one can't mutate
> keys.
>
> Also, I don't understand what is meant by "the predicate used by hash
> tables created with :test equal diverged from the built-in `equal'
> predicate". cmpfn_equal simply calls Fequal, so how can their behaviors
> diverge?

cmpfn_equal is only called when the numeric hashes are equal, so the
actual predicate used is sxhash_equal (a) == sxhash_equal (b) &&
Fequal (a, b). That diverged from Fequal (a, b) in the case where a
and b are markers, for example.

>
> >>> The pure-cons hash, and many other places, assume "equal" means
> >>> "equivalent" in some way. That's not true for bytecode objects, where
> >>> a function always returning `nil' can be equal to one always returning
> >>> `t'.
> >>
> >> Could you give an example of this? When I byte-compiled this:
> >
> > (defun f () (let ((x '(#1=(nil) . #1#)))
> >            (eq (car x) (cdr x))))
> > (defun g () (let ((x '((nil) . (nil)))
> >            (eq (car x) (cdr x))))
> >
> > That's somewhat contrived; more realistic examples where this might
> > actually be a problem would use string properties.
>
> Although this example is a good one that illustrates a bug (or at least
> a poorly-documented feature) in dumping, it hasn't changed because of
> the January 7 changes to sxhash.

True. My point was that "equal" is a problematic hash predicate, at
least currently.

> The same issue exists in Emacs 27 (and
> in Emacs 26 for that matter). So I'd rather address this issue
> separately, perhaps simply by documenting it for now.

Agreed.

> > I'll propose one more thing, which sounds horrible at first but might
> > be the least horrible option: accept that `equal', the function, and
> > `:test equal', the hash table predicate, have diverged significantly
> > already.
> Again I'm not following, because the two functions haven't diverged as
> far as I can see.

(defun hash-equal (a b)
  (let ((ht (make-hash-table :test 'equal)))
    (puthash a t ht)
    (gethash b ht)))

(hash-equal (point-marker) (point-marker))
(equal (point-marker) (point-marker))

bug#39529: 28.0.50; Metahelp does not contain help text

[Paul Eggert]

On 2/18/20 6:34 PM, Pip Cet wrote:
> (defun hash-equal (a b)
>    (let ((ht (make-hash-table :test 'equal)))
>      (puthash a t ht)
>      (gethash b ht)))
> 
> (hash-equal (point-marker) (point-marker))
> (equal (point-marker) (point-marker))

Ah, OK, then I think we're in fundamental agreement about the problem.

However, I don't agree that the Emacs 27-and-earlier approach is tenable, as 
means that (and (equal a b) (gethash a ht) (not (gethash b ht))) can return t, 
and similarly unexpected and undesirable behavior elsewhere - even if there are 
no side effects on hash keys and no cycles in hash key values. For now it should 
suffice if Emacs handles the case where hash keys don't change and don't have 
cycles; we can worry about the fancier problems later (if ever).