bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

As I do not understand the true nature of this bug, the title of this
debbugs could be confusing. Please read the below in entirety.

Related discussion is also on emacs-devel:
https://lists.gnu.org/archive/html/emacs-devel/2016-07/msg00519.html

Evaluating the below in emacs 24.5 emacs -Q results in the below error
backtrace as expected:

=====
Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  string-match("." nil nil)
  string-match-p("." nil)
  (progn (require (quote package)) (if (member (quote ("melpa" . "
http://melpa.org/packages/")) package-archives) package-archives (setq
package-archives (append package-archives (list (quote ("melpa" . "
http://melpa.org/packages/")))))) (package-initialize)
(package-refresh-contents) (package-install (quote projectile)) (require
(quote projectile)) (projectile-global-mode) (string-match-p "." nil))
  eval((progn (require (quote package)) (if (member (quote ("melpa" . "
http://melpa.org/packages/")) package-archives) package-archives (setq
package-archives (append package-archives (list (quote ("melpa" . "
http://melpa.org/packages/")))))) (package-initialize)
(package-refresh-contents) (package-install (quote projectile)) (require
(quote projectile)) (projectile-global-mode) (string-match-p "." nil)) nil)
  eval-last-sexp-1(nil)
  eval-last-sexp(nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)
=====

Minimal code to reproduce the issue:
=====
(progn
  (require 'package)
  (add-to-list 'package-archives '("melpa" . "http://melpa.org/packages/")
t)
  (package-initialize)
  (package-refresh-contents)
  (package-install 'projectile)

  (require 'projectile)
  (projectile-global-mode)
  (string-match-p "." nil))
=====

But evaluating the same on emacs-25 build results in:

=====
Entering debugger...
help-function-arglist: End of file during parsing
=====

Also evaluating the above after M-x toggle-debug-on-error does not give a
backtrace on the "help-function-arglist: End of file during parsing" error.

This is a regression from how the actual error (wrong-type-argument stringp
nil) backtrace showed up correctly on emacs 24.5 but is giving confusing
and difficult to debug error on emacs-25.

emacs-25 build info:

In GNU Emacs 25.0.95.11 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.23)
 of 2016-06-29 built on ulcf41.cld.analog.com
Repository revision: 6192b6c3a4374b2cb6e02ca865e1899a04a7f7dc
Windowing system distributor 'The X.Org Foundation', version 11.0.60900000
System Description: Red Hat Enterprise Linux Workstation release 6.6
(Santiago)

Configured using:
 'configure --with-modules
 --prefix=/home/kmodi/usr_local/apps/6/emacs/emacs-25
 'CPPFLAGS=-fgnu89-inline -I/home/kmodi/usr_local/6/include
 -I/usr/include/freetype2 -I/usr/include' 'CFLAGS=-ggdb3 -O0'
 'CXXFLAGS=-ggdb3 -O0' 'LDFLAGS=-L/home/kmodi/usr_local/6/lib
 -L/home/kmodi/usr_local/6/lib64 -ggdb3'
 PKG_CONFIG_PATH=/home/kmodi/usr_local/6/lib/pkgconfig:/home/kmodi/usr_local/6/lib64/pkgconfig:/cad/adi/apps/gnu/linux/x86_64/6/lib/pkgconfig:/cad/adi/apps/gnu/linux/x86_64/6/lib64/pkgconfig:/usr/lib/pkgconfig:/usr/lib64/pkgconfig:/usr/share/pkgconfig:/lib/pkgconfig:/lib64/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK2 X11 MODULES

-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 5215 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

I evaluated the below after:

> gdb ./emacs
gdb> r -Q

====
(defmacro emacs-q-template (pkgs &rest body)
  "Install packages in PKGS list and evaluate BODY.

Example usage:

1. Launch 'emacs -Q'.
2. Copy this macro definition to its scratch buffer and evaluate it.
3. Evaluate a minimum working example using this macro as below:
     (emacs-q-template '(projectile)
       (projectile-global-mode)) "
  (declare (indent 1) (debug t))
  `(progn
     (require 'package)
     (setq package-user-dir (concat (getenv "HOME") "/.emacs.d/elpa_test/"))
     (add-to-list 'package-archives '("melpa" . "http://melpa.org/packages/")
t)
     (package-initialize)
     (package-refresh-contents)

     (dolist (pkg ,pkgs)
       (package-install pkg)
       (require pkg))

     ,@body))

(emacs-q-template '(projectile)
  (projectile-global-mode)
  (string-match-p "." nil))
=====

Below is the backtrace at the point where I get the unexpected end-of-file
error

(gdb) c
Continuing.
[New Thread 0x7fffec504700 (LWP 11972)]
[Thread 0x7fffec504700 (LWP 11972) exited]

Breakpoint 3, Fsignal (error_symbol=19104, data=0) at eval.c:1472
1472        = (NILP (error_symbol) ? Fcar (data) : error_symbol);
(gdb) p error_symbol
$15 = 19104
(gdb) xsymbol
$16 = (struct Lisp_Symbol *) 0xcae060 <lispsym+19104>
"end-of-file"
(gdb) bt
#0  Fsignal (error_symbol=19104, data=0) at eval.c:1472
#1  0x00000000005f4a06 in xsignal (error_symbol=19104, data=0) at
eval.c:1578
#2  0x00000000005f4a33 in xsignal0 (error_symbol=19104) at eval.c:1587
#3  0x0000000000623284 in end_of_file_error () at lread.c:1738
#4  0x0000000000624dcd in read1 (readcharfun=57797668, pch=0x7fffffff353c,
first_in_list=false) at lread.c:2553
#5  0x00000000006278e3 in read_list (flag=false, readcharfun=57797668) at
lread.c:3659
#6  0x0000000000624dfc in read1 (readcharfun=57797668, pch=0x7fffffff38ac,
first_in_list=false) at lread.c:2558
#7  0x00000000006241b0 in read0 (readcharfun=57797668) at lread.c:2139
#8  0x0000000000624112 in read_internal_start (stream=57797668, start=0,
end=0) at lread.c:2112
#9  0x0000000000623fab in Fread_from_string (string=57797668, start=0,
end=0) at lread.c:2075
#10 0x00000000005f7627 in Ffuncall (nargs=2, args=0x7fffffff3a28) at
eval.c:2708
#11 0x000000000063df57 in exec_byte_code (bytestr=10617580,
vector=10617613, maxdepth=34, args_template=0, nargs=0, args=0x0) at
bytecode.c:880
#12 0x00000000005f817a in funcall_lambda (fun=10617485, nargs=2,
arg_vector=0xa2030d <pure+1081741>) at eval.c:2929
#13 0x00000000005f783a in Ffuncall (nargs=3, args=0x7fffffff3f80) at
eval.c:2750
#14 0x000000000063df57 in exec_byte_code (bytestr=40406500,
vector=35307789, maxdepth=18, args_template=1030, nargs=1,
args=0x7fffffff44c8) at bytecode.c:880
#15 0x00000000005f7e3e in funcall_lambda (fun=35307829, nargs=1,
arg_vector=0x7fffffff44c0) at eval.c:2863
#16 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff44b8) at
eval.c:2750
#17 0x000000000063df57 in exec_byte_code (bytestr=41942932,
vector=35797685, maxdepth=66, args_template=1030, nargs=1,
args=0x7fffffff4a78) at bytecode.c:880
#18 0x00000000005f7e3e in funcall_lambda (fun=26917541, nargs=1,
arg_vector=0x7fffffff4a70) at eval.c:2863
#19 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff4a68) at
eval.c:2750
#20 0x000000000063df57 in exec_byte_code (bytestr=32991236,
vector=36233277, maxdepth=50, args_template=2058, nargs=2,
args=0x7fffffff4fd0) at bytecode.c:880
#21 0x00000000005f7e3e in funcall_lambda (fun=36233437, nargs=2,
arg_vector=0x7fffffff4fc0) at eval.c:2863
#22 0x00000000005f783a in Ffuncall (nargs=3, args=0x7fffffff4fb8) at
eval.c:2750
#23 0x000000000063df57 in exec_byte_code (bytestr=29885572,
vector=35430989, maxdepth=26, args_template=2054, nargs=1,
args=0x7fffffff5508) at bytecode.c:880
#24 0x00000000005f7e3e in funcall_lambda (fun=35431157, nargs=1,
arg_vector=0x7fffffff5500) at eval.c:2863
#25 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff54f8) at
eval.c:2750
#26 0x000000000063df57 in exec_byte_code (bytestr=29509364,
vector=35472261, maxdepth=34, args_template=1026, nargs=0,
args=0x7fffffff5a50) at bytecode.c:880
#27 0x00000000005f7e3e in funcall_lambda (fun=37586485, nargs=0,
arg_vector=0x7fffffff5a50) at eval.c:2863
#28 0x00000000005f783a in Ffuncall (nargs=1, args=0x7fffffff5a48) at
eval.c:2750
#29 0x000000000063df57 in exec_byte_code (bytestr=29347988,
vector=37183445, maxdepth=18, args_template=2, nargs=0,
args=0x7fffffff6010) at bytecode.c:880
#30 0x00000000005f7e3e in funcall_lambda (fun=37183509, nargs=0,
arg_vector=0x7fffffff6010) at eval.c:2863
#31 0x00000000005f783a in Ffuncall (nargs=1, args=0x7fffffff6008) at
eval.c:2750
#32 0x00000000005f6a9b in funcall_nil (nargs=1, args=0x7fffffff6008) at
eval.c:2340
#33 0x00000000005f6e9f in run_hook_with_args (nargs=1, args=0x7fffffff6008,
funcall=0x5f6a78 <funcall_nil>) at eval.c:2517
#34 0x00000000005f6b22 in Frun_hook_with_args (nargs=1,
args=0x7fffffff6008) at eval.c:2382
#35 0x00000000005f6efb in run_hook (hook=28871488) at eval.c:2530
#36 0x00000000005f6adf in Frun_hooks (nargs=1, args=0x7fffffff6118) at
eval.c:2364
#37 0x00000000005f74ba in Ffuncall (nargs=2, args=0x7fffffff6110) at
eval.c:2681
#38 0x000000000063df57 in exec_byte_code (bytestr=9658076, vector=9658109,
maxdepth=22, args_template=514, nargs=1, args=0x7fffffff6640) at
bytecode.c:880
#39 0x00000000005f7e3e in funcall_lambda (fun=9658029, nargs=1,
arg_vector=0x7fffffff6640) at eval.c:2863
#40 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff6638) at
eval.c:2750
#41 0x000000000063df57 in exec_byte_code (bytestr=26545060,
vector=26696973, maxdepth=10, args_template=2, nargs=0,
args=0x7fffffff6c50) at bytecode.c:880
#42 0x00000000005f7e3e in funcall_lambda (fun=26697125, nargs=0,
arg_vector=0x7fffffff6c50) at eval.c:2863
#43 0x00000000005f783a in Ffuncall (nargs=1, args=0x7fffffff6c48) at
eval.c:2750
#44 0x000000000063df57 in exec_byte_code (bytestr=26480452,
vector=26654173, maxdepth=166, args_template=514, nargs=2,
args=0x7fffffff71a8) at bytecode.c:880
#45 0x00000000005f7e3e in funcall_lambda (fun=26654805, nargs=2,
arg_vector=0x7fffffff71a8) at eval.c:2863
#46 0x00000000005f783a in Ffuncall (nargs=3, args=0x7fffffff71a0) at
eval.c:2750
#47 0x00000000005f6a41 in Fapply (nargs=2, args=0x7fffffff7270) at
eval.c:2329
#48 0x00000000005f6f90 in apply1 (fn=16848, arg=56618787) at eval.c:2545
#49 0x00000000005f23b6 in call_debugger (arg=56618787) at eval.c:308
#50 0x00000000005f4e2b in maybe_call_debugger (conditions=9578427,
sig=51984, data=56618835) at eval.c:1723
#51 0x00000000005f48dd in Fsignal (error_symbol=51984, data=56618835) at
eval.c:1541
#52 0x00000000005f4a06 in xsignal (error_symbol=51984, data=56618835) at
eval.c:1578
#53 0x00000000005f4a9a in xsignal2 (error_symbol=51984, arg1=44400, arg2=0)
at eval.c:1599
#54 0x00000000005d7967 in wrong_type_argument (predicate=44400, value=0) at
data.c:151
#55 0x000000000055388b in CHECK_STRING (x=0) at lisp.h:2807
#56 0x00000000005b2278 in string_match_1 (regexp=55505044, string=0,
start=0, posix=false) at search.c:373
#57 0x00000000005b25e5 in Fstring_match (regexp=55505044, string=0,
start=0) at search.c:444
#58 0x00000000005f7627 in Ffuncall (nargs=4, args=0x7fffffff75c0) at
eval.c:2708
#59 0x000000000063df57 in exec_byte_code (bytestr=9684412, vector=9684445,
maxdepth=30, args_template=3082, nargs=2, args=0x7fffffff7a40) at
bytecode.c:880
#60 0x00000000005f7e3e in funcall_lambda (fun=9684365, nargs=2,
arg_vector=0x7fffffff7a30) at eval.c:2863
#61 0x00000000005f7bca in apply_lambda (fun=9684365, args=55521603,
count=15) at eval.c:2802
#62 0x00000000005f623e in eval_sub (form=55521587) at eval.c:2219
#63 0x00000000005f2647 in Fprogn (body=55521667) at eval.c:427
#64 0x00000000005f5c69 in eval_sub (form=55520739) at eval.c:2127
#65 0x00000000005f5786 in Feval (form=55520739, lexical=0) at eval.c:1996
#66 0x00000000005f75ec in Ffuncall (nargs=3, args=0x7fffffff7e28) at
eval.c:2704
#67 0x000000000063df57 in exec_byte_code (bytestr=11219292,
vector=11219325, maxdepth=22, args_template=1030, nargs=1,
args=0x7fffffff8360) at bytecode.c:880
#68 0x00000000005f7e3e in funcall_lambda (fun=11219245, nargs=1,
arg_vector=0x7fffffff8358) at eval.c:2863
#69 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff8350) at
eval.c:2750
#70 0x000000000063df57 in exec_byte_code (bytestr=11220028,
vector=11220061, maxdepth=18, args_template=1030, nargs=1,
args=0x7fffffff8968) at bytecode.c:880
#71 0x00000000005f7e3e in funcall_lambda (fun=11219973, nargs=1,
arg_vector=0x7fffffff8960) at eval.c:2863
#72 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff8958) at
eval.c:2750
#73 0x00000000005ef8fc in Ffuncall_interactively (nargs=2,
args=0x7fffffff8958) at callint.c:252
#74 0x00000000005f74ba in Ffuncall (nargs=3, args=0x7fffffff8950) at
eval.c:2681
#75 0x00000000005f1c82 in Fcall_interactively (function=4203120,
record_flag=0, keys=13589173) at callint.c:840
#76 0x00000000005f7627 in Ffuncall (nargs=4, args=0x7fffffff8c78) at
eval.c:2708
#77 0x000000000063df57 in exec_byte_code (bytestr=10517116,
vector=10517149, maxdepth=54, args_template=4102, nargs=1,
args=0x7fffffff91d0) at bytecode.c:880
#78 0x00000000005f7e3e in funcall_lambda (fun=10517069, nargs=1,
arg_vector=0x7fffffff91c8) at eval.c:2863
#79 0x00000000005f783a in Ffuncall (nargs=2, args=0x7fffffff91c0) at
eval.c:2750
#80 0x00000000005f6fe2 in call1 (fn=15072, arg1=4203120) at eval.c:2560
#81 0x000000000055b646 in command_loop_1 () at keyboard.c:1479
#82 0x00000000005f424a in internal_condition_case (bfun=0x55ae8d
<command_loop_1>, handlers=19392, hfun=0x55a677 <cmd_error>) at eval.c:1310
#83 0x000000000055ab95 in command_loop_2 (ignore=0) at keyboard.c:1107
#84 0x00000000005f3b6a in internal_catch (tag=46560, func=0x55ab6c
<command_loop_2>, arg=0) at eval.c:1075
#85 0x000000000055ab35 in command_loop () at keyboard.c:1086
#86 0x000000000055a246 in recursive_edit_1 () at keyboard.c:692
#87 0x000000000055a3d9 in Frecursive_edit () at keyboard.c:763
#88 0x000000000055825a in main (argc=2, argv=0x7fffffff9688) at emacs.c:1656

Lisp Backtrace:
"read-from-string" (0xffff3a30)
"help-function-arglist" (0xffff3f88)
"ad-arglist" (0xffff44c0)
"ad-make-advised-definition" (0xffff4a70)
"ad-activate-advised-definition" (0xffff4fc0)
"ad-activate" (0xffff5500)
"projectile-mode" (0xffff5a50)
"projectile-global-mode-enable-in-buffers" (0xffff6010)
"run-hooks" (0xffff6118)
"run-mode-hooks" (0xffff6640)
"debugger-mode" (0xffff6c50)
"debug" (0xffff71a8)
"string-match" (0xffff75c8)
"string-match-p" (0xffff7a30)
"progn" (0xffff7c70)
"eval" (0xffff7e30)
"elisp--eval-last-sexp" (0xffff8358)
"eval-last-sexp" (0xffff8960)
"funcall-interactively" (0xffff8958)
"call-interactively" (0xffff8c80)
"command-execute" (0xffff91c8)
(gdb)
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 13067 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 12:29:25 +0000
> 
> Lisp Backtrace:
> "read-from-string" (0xffff3a30)
> "help-function-arglist" (0xffff3f88)

This says it tried to read from a string.  What string was that?  And
why does help-function-arglist try to read from that string?

> #9 0x0000000000623fab in Fread_from_string (string=57797668, start=0, end=0) at lread.c:2075

In this frame, please show the value of 'string', like this:

 (gdb) frame 9
 (gdb) p string
 (gdb) xstring

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

Hi Eli,

This is what I get (obviously odd-looking string):

(gdb) frame 9
#9  0x0000000000624397 in Fread_from_string (string=54138084, start=0,
end=0) at lread.c:2075
2075      ret = read_internal_start (string, start, end);
(gdb) p string
$15 = 54138084
(gdb) xstring
$16 = (struct Lisp_String *) 0x33a14e0
"(nilory is relative, it is com"

Looks like the 'nil' argument in '(string-match-p "." nil)' somehow did
this?

(BTW this debug session is on the 2f67f8a commit of master (very recent,
today). But the bug is present on emacs-25 too.)

On Tue, Jul 12, 2016 at 9:14 AM Eli Zaretskii <eliz@gnu.org> wrote:

> In this frame, please show the value of 'string', like this:
>
>  (gdb) frame 9
>  (gdb) p string
>  (gdb) xstring
>
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1323 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

Some interesting discovery .. hope this helps debug this:

Searched the whole emacs source and my ~/.emacs.d for 'is relative, it is
com'. And the only place I found it was in lisp/progmodes/compile.el, line
2679:

=====
km²~/downloads/:git/emacs> ag 'is relative, it is com'
lisp/progmodes/compile.el
2679:If DIRECTORY is relative, it is combined with `default-directory'.
=====

Somehow "(nil" replaced "If DIRECT", the whole thing got lower-cased,
string after "com" in "combined" got truncated, and we got:

"(nilory is relative, it is com"




On Tue, Jul 12, 2016 at 9:33 AM Kaushal Modi <kaushal.modi@gmail.com> wrote:

> Hi Eli,
>
> This is what I get (obviously odd-looking string):
>
> (gdb) frame 9
> #9  0x0000000000624397 in Fread_from_string (string=54138084, start=0,
> end=0) at lread.c:2075
> 2075      ret = read_internal_start (string, start, end);
> (gdb) p string
> $15 = 54138084
> (gdb) xstring
> $16 = (struct Lisp_String *) 0x33a14e0
> "(nilory is relative, it is com"
>
> Looks like the 'nil' argument in '(string-match-p "." nil)' somehow did
> this?
>
> (BTW this debug session is on the 2f67f8a commit of master (very recent,
> today). But the bug is present on emacs-25 too.)
>
> On Tue, Jul 12, 2016 at 9:14 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
>> In this frame, please show the value of 'string', like this:
>>
>>  (gdb) frame 9
>>  (gdb) p string
>>  (gdb) xstring
>
> Kaushal Modi
>
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 2695 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 13:33:05 +0000
> Cc: 23949@debbugs.gnu.org
> 
> (gdb) frame 9
> #9 0x0000000000624397 in Fread_from_string (string=54138084, start=0, end=0) at lread.c:2075
> 2075 ret = read_internal_start (string, start, end);
> (gdb) p string
> $15 = 54138084
> (gdb) xstring
> $16 = (struct Lisp_String *) 0x33a14e0
> "(nilory is relative, it is com"
> 
> Looks like the 'nil' argument in '(string-match-p "." nil)' somehow did this?

string-match-p just signals an error, because its 2nd arg must be a
string.  Look up the backtrace, and you will see that Emacs is trying
to signal an error:

  #47 0x00000000005f6a41 in Fapply (nargs=2, args=0x7fffffff7270) at
  eval.c:2329
  #48 0x00000000005f6f90 in apply1 (fn=16848, arg=56618787) at eval.c:2545
  #49 0x00000000005f23b6 in call_debugger (arg=56618787) at eval.c:308
  #50 0x00000000005f4e2b in maybe_call_debugger (conditions=9578427,
  sig=51984, data=56618835) at eval.c:1723
  #51 0x00000000005f48dd in Fsignal (error_symbol=51984, data=56618835) at
  eval.c:1541
  #52 0x00000000005f4a06 in xsignal (error_symbol=51984, data=56618835) at
  eval.c:1578
  #53 0x00000000005f4a9a in xsignal2 (error_symbol=51984, arg1=44400, arg2=0)
  at eval.c:1599
  #54 0x00000000005d7967 in wrong_type_argument (predicate=44400, value=0) at
  data.c:151
  #55 0x000000000055388b in CHECK_STRING (x=0) at lisp.h:2807
  #56 0x00000000005b2278 in string_match_1 (regexp=55505044, string=0,
  start=0, posix=false) at search.c:373
  #57 0x00000000005b25e5 in Fstring_match (regexp=55505044, string=0,
  start=0) at search.c:444

What happens next is that Emacs calls the debugger, and then your
advices kick in, starting at apply1.  And that's where the trouble
begins.

I asked why help-function-arglist is trying to read from a string, but
got no answer.

> (BTW this debug session is on the 2f67f8a commit of master (very recent, today). But the bug is present on
> emacs-25 too.)

Sorry, I see no bug yet, just a lot of ad-FOO stuff that tries to do
something silly during an error, when it should have moved out of the
way.  If there is a bug, its root cause hides inside
help-function-arglist, and the way it is called by the advices you (or
maybe it's Projectile?) have set up.  That's where we should be
looking, not in string-match-p, which did what it was supposed to do:
signaled an error when called with nil instead of a string.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 13:37:57 +0000
> Cc: 23949@debbugs.gnu.org
> 
> Some interesting discovery .. hope this helps debug this:
> 
> Searched the whole emacs source and my ~/.emacs.d for 'is relative, it is com'. And the only place I found it
> was in lisp/progmodes/compile.el, line 2679:
> 
> =====
> km²~/downloads/:git/emacs> ag 'is relative, it is com' 
> lisp/progmodes/compile.el
> 2679:If DIRECTORY is relative, it is combined with `default-directory'.
> =====
> 
> Somehow "(nil" replaced "If DIRECT", the whole thing got lower-cased, string after "com" in "combined" got
> truncated, and we got:
> 
> "(nilory is relative, it is com"

It's probably just some random garbage left on the stack.

Once again, the root cause lies in help-function-arglist or its
immediate callers.  Why does all this get called when Emacs signals an
error?

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Andreas Schwab]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> (gdb) frame 9
> #9  0x0000000000624397 in Fread_from_string (string=54138084, start=0,
> end=0) at lread.c:2075
> 2075      ret = read_internal_start (string, start, end);
> (gdb) p string
> $15 = 54138084
> (gdb) xstring

This is only valid if $ is a Lisp_String.  Use xtype to find out.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

On Tue, Jul 12, 2016 at 10:02 AM Eli Zaretskii <eliz@gnu.org> wrote:

> string-match-p just signals an error, because its 2nd arg must be a
> string.  Look up the backtrace, and you will see that Emacs is trying
> to signal an error:
>

Correct, but that error does not show up within emacs. All the user sees is:

Entering debugger...
help-function-arglist: End of file during parsing

In any case, I believe that that should not happen.

Also concerning is the fact that,

- (string-match "." nil) gives the expected error backtrace.
- But (string-match-p "." nil) gives the help-function-arglist error.

What happens next is that Emacs calls the debugger, and then your
> advices kick in, starting at apply1.  And that's where the trouble
> begins.
>

Correct. That's exactly what I do not understand.


> I asked why help-function-arglist is trying to read from a string, but
> got no answer.
>

Sorry, I missed answering that question in my earlier emails; I got too
busy with gdb.

I do not have answer for that question. I filed this bug report to learn
why this happens, and help in any way I can to fix it.

From the below:

> "help-function-arglist" (0xffff3f88)
> "ad-arglist" (0xffff44c0)
> "ad-make-advised-definition" (0xffff4a70)
> "ad-activate-advised-definition" (0xffff4fc0)
> "ad-activate" (0xffff5500)
> "projectile-mode" (0xffff5a50)

, the advices in the projectile package are causing this. I grepped the
projectile source, and it contains only 2 advices.

(1)
(defadvice compilation-find-file (around projectile-compilation-find-file)
  "Try to find a buffer for FILENAME, if we cannot find it,
fallback to the original function."
  (let ((filename (ad-get-arg 1)))
    (ad-set-arg 1
                (or
                 (if (file-exists-p (expand-file-name filename))
                     filename)
                 ;; Try to find the filename using projectile
                 (and (projectile-project-p)
                      (let ((root (projectile-project-root))
                            (dirs (cons ""
(projectile-current-project-dirs))))
                        (-when-let (full-filename (->> dirs
                                                       (--map
(expand-file-name filename (expand-file-name it root)))
                                                       (-filter
#'file-exists-p)
                                                       (-first-item)))
                          full-filename)))
                 ;; Fall back to the old argument
                 filename))
    ad-do-it))

(2)
(defadvice delete-file (before purge-from-projectile-cache (filename
&optional trash))
  (if (and projectile-enable-caching (projectile-project-p))
      (let* ((project-root (projectile-project-root))
             (true-filename (file-truename filename))
             (relative-filename (file-relative-name true-filename
project-root)))
        (if (projectile-file-cached-p relative-filename project-root)
            (projectile-purge-file-from-cache relative-filename)))))

As I started using advices only after emacs 24.4, I never learned the old
advice style. So I hoped someone experienced with these would help connect
the dots between invalid arg error for string-match-p and
help-function-arglist error. @Stefan?

Also, with the exact same projectile version, I do *not* get the
misleading help-function-arglist
error on emacs 24.5. So something probably changed in the way major mode
hooks are run in the debugger since then?

Sorry, I see no bug yet, just a lot of ad-FOO stuff that tries to do
> something silly during an error, when it should have moved out of the
> way.  If there is a bug, its root cause hides inside
> help-function-arglist,


Copying Stefan to help throw some light on this.


> and the way it is called by the advices you (or
> maybe it's Projectile?) have set up.


The minimal code I posted was run in emacs -Q. So the only effective
advices are the ones in Projectile; I have pasted the code for those 2
advices above for reference.


> That's where we should be
> looking, not in string-match-p, which did what it was supposed to do:
> signaled an error when called with nil instead of a string.
>

But then it would be interesting to know why (string-match "." nil) instead
does not cause the help-function-arglist error.

Here is the backtrace on evaluating

=====
(emacs-q-template '(projectile)
  (projectile-global-mode)
  (string-match-p "." nil))
=====
(Code for the emacs-q-template macro is in my previous email in this
thread.)

Backtrace:
=====
Debugger entered--Lisp error: (wrong-type-argument stringp nil)
string-match("." nil nil)
string-match-p("." nil)
(progn (require (quote package)) (setq package-user-dir (concat ...
eval((progn (require (quote package)) (setq package-user-dir (concat ...
eval-last-sexp-1(nil)
eval-last-sexp(nil)
call-interactively(eval-last-sexp nil nil)
command-execute(eval-last-sexp)
=====
(I do not get this backtrace in emacs 25.x; above is from emacs 24.5.)

And here is the backtrace for evaluating the same when using string-match
instead of string-match-p in emacs 24.5:

=====
(emacs-q-template '(projectile)
  (projectile-global-mode)
  (string-match "." nil))
=====

Backtrace:
=====

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  string-match("." nil)
  (progn (require (quote package)) (setq package-user-dir (concat ...
  eval((progn (require (quote package)) (setq package-user-dir (concat ...
  eval-last-sexp-1(nil)
  eval-last-sexp(nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)
=====

In the former case, it was

  string-match("." nil nil)

In the latter case, it was,

  string-match("." nil)

Do those 2 consecutive 'nil's somehow throw off the
debugger/ad-arglist/help-function-arglist
in emacs 25.x?




On Tue, Jul 12, 2016 at 10:15 AM Andreas Schwab <schwab@suse.de> wrote:

> > $15 = 54138084
> > (gdb) xstring
>
> This is only valid if $ is a Lisp_String.  Use xtype to find out.
>

 Hi Andreas,

Thanks for looking into this bug thread. I have close to 0 experience with
gdb. I typed 'xtype' for the same frame and this is what I got:

(gdb) xstring
$16 = (struct Lisp_String *) 0x33a14e0
"(nilory is relative, it is com"
(gdb) xtype
Argument to arithmetic operation not a number or boolean.
(gdb)

I do not know what to make out of that. Let me know if there are any other
gdb commands that I can use to give you more helpful debug info.

Thanks everyone.
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 18386 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Noam Postavsky]

On Tue, Jul 12, 2016 at 2:35 PM, Kaushal Modi <kaushal.modi@gmail.com> wrote:
> (gdb) xstring
> $16 = (struct Lisp_String *) 0x33a14e0
> "(nilory is relative, it is com"
> (gdb) xtype
> Argument to arithmetic operation not a number or boolean.
> (gdb)
>
> I do not know what to make out of that.

I think you have to use xtype before xstring (otherwise it applies to
$16, the return value from xstring).

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

Hi Noam,

Thanks. Now it says "Lisp_String".

(gdb) xstring
$18 = (struct Lisp_String *) 0x33a14e0
"(nilory is relative, it is com"
(gdb) p string
$19 = 54138084
(gdb) xtype
Lisp_String
(gdb)

On Tue, Jul 12, 2016 at 2:55 PM Noam Postavsky <
npostavs@users.sourceforge.net> wrote:

> I think you have to use xtype before xstring (otherwise it applies to
> $16, the return value from xstring).
>
-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 909 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 18:35:02 +0000
> Cc: 23949@debbugs.gnu.org
> 
> On Tue, Jul 12, 2016 at 10:15 AM Andreas Schwab <schwab@suse.de> wrote:
> 
>  > $15 = 54138084
>  > (gdb) xstring
> 
>  This is only valid if $ is a Lisp_String. Use xtype to find out.
> 
> Hi Andreas,
> 
> Thanks for looking into this bug thread. I have close to 0 experience with gdb. I typed 'xtype' for the same
> frame and this is what I got:
> 
> (gdb) xstring
> $16 = (struct Lisp_String *) 0x33a14e0
> "(nilory is relative, it is com"
> (gdb) xtype
> Argument to arithmetic operation not a number or boolean.
> (gdb)

No, the other way around:

 (gdb) p string
 (gdb) xtype

"xtype" works on the last value that was printed, it should do that on
the value produced by the p(rint) command.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 19:00:20 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>, 
> 	Andreas Schwab <schwab@suse.de>, 23949@debbugs.gnu.org
> 
> Thanks. Now it says "Lisp_String".
> 
> (gdb) xstring
> $18 = (struct Lisp_String *) 0x33a14e0
> "(nilory is relative, it is com"
> (gdb) p string
> $19 = 54138084
> (gdb) xtype
> Lisp_String
> (gdb)

If it says "Lisp_String", then using "xstring" is correct.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 12 Jul 2016 18:35:02 +0000
> Cc: 23949@debbugs.gnu.org
> 
>  string-match-p just signals an error, because its 2nd arg must be a
>  string. Look up the backtrace, and you will see that Emacs is trying
>  to signal an error:
> 
> Correct, but that error does not show up within emacs. All the user sees is:
> 
> Entering debugger...
> help-function-arglist: End of file during parsing

Because Emacs hits a second error while trying to show the backtrace
of the first one.

> In any case, I believe that that should not happen.

Indeed, it shouldn't, but the question is: what code is responsible
for that which shouldn't happen?  If some package or your own
customizations cause the debugger to call extra code, and that extra
code signals an error, then that extra code needs to be fixed, not
Emacs.

> Also concerning is the fact that,
> 
> - (string-match "." nil) gives the expected error backtrace.
> - But (string-match-p "." nil) gives the help-function-arglist error. 

Sorry, I fail to see the significance of this to the issue at hand.
They are two different functions, and we still don't know which
functions were advised and how.  Perhaps the advice will explain the
difference.  Or perhaps we understand the reason  for the difference
once we get to the bottom of investigating the problem.  Either way,
the efficient method of looking into this problem is to understand
what are those advices and where do they come from.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

On Tue, Jul 12, 2016 at 3:20 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Indeed, it shouldn't, but the question is: what code is responsible
> for that which shouldn't happen?  If some package or your own
> customizations cause the debugger to call extra code, and that extra
> code signals an error, then that extra code needs to be fixed, not
> Emacs.
>

It should also be considered that the help-function-arglist error does not
happen on emacs 24.5, using the exact same projectile version.


> > Also concerning is the fact that,
> >
> > - (string-match "." nil) gives the expected error backtrace.
> > - But (string-match-p "." nil) gives the help-function-arglist error.
>
> Sorry, I fail to see the significance of this to the issue at hand.
>

I find the above to be important because both forms give the expected error
backtrace on emacs 24.5. But on emacs 25.x, string-match-p gives the
unintended help-function-arglist error without any backtrace, while
string-match gives the intended error backtrace. The same projectile
advices are in effect for all of these.


> They are two different functions, and we still don't know which
> functions were advised and how.


Projectile is advising the delete-file and compilation-find-file functions.
The full advice definitions were posted in my previous email.


> Perhaps the advice will explain the
> difference.  Or perhaps we understand the reason  for the difference
> once we get to the bottom of investigating the problem.  Either way,
> the efficient method of looking into this problem is to understand
> what are those advices and where do they come from.

-- 

-- 
Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 2542 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Stefan Monnier]

> Correct, but that error does not show up within emacs. All the user sees is:
> Entering debugger...
> help-function-arglist: End of file during parsing

Clearly, the problem is that string-match-p uses
"(let ((inhibit-changing-match-data t))", so the debugger is run with
inhibit-changing-match-data bound to t and that breaks lots of
Elisp code.

That's a general problem with the use dynamic binding to pass extra
parameters: you end up passing them not just to that one function but
also to all other functions called from that one.


        Stefan

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Kaushal Modi]

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

On Tue, Jul 12, 2016 at 4:27 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> Clearly, the problem is that string-match-p uses
> "(let ((inhibit-changing-match-data t))", so the debugger is run with
> inhibit-changing-match-data bound to t and that breaks lots of
> Elisp code.
>
> That's a general problem with the use dynamic binding to pass extra
> parameters: you end up passing them not just to that one function but
> also to all other functions called from that one.
>

Thanks Stefan.

So what is the way forward?

Fixing just string-match-p and string-match does not seem to the complete
solution, because I have seen just let-bound dynamic vars at many places.

As I posted in the emacs-devel thread (
https://lists.gnu.org/archive/html/emacs-devel/2016-07/msg00544.html ),
while this string-match-p issue causes a confusing seemingly unrelated
error, some other packages cause emacs to freeze up (check the drag-stuff
example in the above link). When I get a change I will add a minimum
working example for the drag-stuff package causing emacs freeze too.

Certainly there was a lower level change after emacs 24.5 that changed the
behavior of how the run-hooks behave or how the advices are executed in
general or when called within a debugger?
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1909 bytes --]

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Stefan Monnier]

>> That's a general problem with the use dynamic binding to pass extra
>> parameters: you end up passing them not just to that one function but
>> also to all other functions called from that one.
> So what is the way forward?

I think it's a fairly fundamental problem that's hard/impossible to "fix".

> Fixing just string-match-p and string-match does not seem to the complete
> solution, because I have seen just let-bound dynamic vars at many places.

A way to fix *some* of the problems is to use concurrency (not yet in
"master"), and make the debugger run in another thread so it's not
affected by the dynamic bindings of the code that signaled the error.

For the more general problem, the only "fix" is to try and remove those
uses of dynamic bindings and replace them with something else, which
seems like a very large undertaking and whose benefits are not
necessarily that clear (the problem being to decide what that "something
else" should be, and the fact that this "something else" will come with
its own problems).

In the case of string-match-p I think we should get rid of
inhibit-changing-match-data and implement string-match-p (and
looking-at-p) some other way.


        Stefan

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  schwab@suse.de,  23949@debbugs.gnu.org
> Date: Tue, 12 Jul 2016 16:27:03 -0400
> 
> Clearly, the problem is that string-match-p uses
> "(let ((inhibit-changing-match-data t))",

Since this is the only difference between string-match and
string-match-p, yes, that's pretty much obvious.  But saying that
doesn't yet point out the code which is affected by this binding in a
way that breaks popping the debugger.

> so the debugger is run with inhibit-changing-match-data bound to t
> and that breaks lots of Elisp code.

Then perhaps the solution is to avoid the effect of binding
inhibit-changing-match-data on the debugger.

> That's a general problem with the use dynamic binding to pass extra
> parameters: you end up passing them not just to that one function but
> also to all other functions called from that one.

This is a strange thing to hear, from you of all the people.
Dynamically binding variables around some expression is standard Emacs
Lisp programming technique, used all over the place.  The doc string
of this particular variable even says so.  How come it is suddenly a
problem?

I also think that the "breaks a lot of Elisp code" part is at least a
tad exaggerated.  We bind this particular variable in 2 functions that
are called from more than 300 different places in the Emacs sources,
so if doing so indeed breaks a lot of Lisp, we are in deep trouble,
which I don't think is the case.

Anyway, a cursory glance at help-function-arglist points to the
problematic code.  Compare this:

  (help-split-fundoc (documentation 'delete-file) nil)
    => ("(nil FILENAME &optional TRASH)" . "Delete file named FILENAME.  If it is a symlink, remove the symlink.
  If file has multiple names, it continues to exist with the other names.
  TRASH non-nil means to trash the file instead of deleting, provided
  ‘delete-by-moving-to-trash’ is non-nil.

  When called interactively, TRASH is t if no prefix argument is given.
  With a prefix argument, TRASH is nil.")

with this:

  (let ((inhibit-changing-match-data t))
    (help-split-fundoc (documentation 'delete-file) nil))
    => ("(nilnil" . "Delete fi")

The latter is clearly bogus.  Now, help-function-arglist calls
help-split-fundoc, and then reads from the string produced by the
latter:

          (let* ((doc (condition-case nil (documentation def) (error nil)))
                 (docargs (if doc (car (help-split-fundoc doc nil))))
                 (arglist (if docargs
                              (cdar (read-from-string (downcase docargs)))))

I hope the reason for EOF is now clear.  (I have no idea why it only
happens on master: the above bogus value shows on emacs-25 as well.)

Does the following variant of string-match-p look right?  Its intent
is to limit the effect of inhibit-changing-match-data to the call to
string-match only, leaving the error handling, if any is needed,
outside of that binding.

(defsubst string-match-p (regexp string &optional start)
  "\
Same as `string-match' except this function does not change the match data."
  (condition-case err
      (let ((inhibit-changing-match-data t))
	(string-match regexp string start))
    (error (signal (car err) (cdr err)))))

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Stefan Monnier]

> Dynamically binding variables around some expression is standard Emacs
> Lisp programming technique, used all over the place.

For sure.  But it's risky and does cause problems in lots of corner
cases (which luckily for us are very rarely triggered).

E.g. we often use re-search-forward or string-match without binding
case-fold-search explicitly, so we depend on the particular setting it
happens to have.  In some cases it truly doesn't matter, while in others
it happens to work because in 99% of the cases the code is run with the
right setting.

Same for completion-ignore-case, completion-regexp-list,
inhibit-read-only, ...

That's just part of life in Elisp.
And I'm not sure what a real "fix" would look like.  E.g. for
minibuffer-setup-hook, we have minibuffer-with-setup-hook but it's
pretty hackish.

Instead, we just live with it and fix the corner cases that we actually
bump into.

> I also think that the "breaks a lot of Elisp code" part is at least a
> tad exaggerated.

Binding inhibit-changing-match-data to t will pretty much break any
function that uses match-beginning or match-end.  I think that counts as
"a lot of Elisp code".  Of course, we currently don't have any code that
binds inhibit-changing-match-data to t around calls to those functions,
except when the calls happen via the debugger.  But it could rear its
ugly head in other cases, e.g. if/when we make it possible for the
regexp-engine to run Elisp code (there can be various reasons to do
that.  Such as to setup the syntax-table property lazily, or to allow
the regexp primitives to be expanded in Elisp [I've a long term desire
to do so for the zero-length primitives such as \< ]) or to pause and
run timers or process filters.

> (defsubst string-match-p (regexp string &optional start)
>   "\
> Same as `string-match' except this function does not change the match data."
>   (condition-case err
>       (let ((inhibit-changing-match-data t))
> 	(string-match regexp string start))
>     (error (signal (car err) (cdr err)))))

That will still cause the same problems when debug-on-signal is non-nil.


        Stefan

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Wed, 13 Jul 2016 13:10:46 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, schwab@suse.de, 23949@debbugs.gnu.org
> 
> So what is the way forward?

I just proposed one way forward.

> Certainly there was a lower level change after emacs 24.5 that changed the behavior of how the run-hooks
> behave or how the advices are executed in general or when called within a debugger? 

The underlying program with binding inhibit-changing-match-data when
help-function-arglist is called exists on the emacs-25 branch as
well.  It also exists in Emacs 24.5, except that there
help-split-fundoc signals an error, instead of returning bogus data:

  (let ((inhibit-changing-match-data t))
    (help-split-fundoc (documentation 'delete-file) nil))
    => error->Args out of range: 3004

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> Does the following variant of string-match-p look right?  Its intent
> is to limit the effect of inhibit-changing-match-data to the call to
> string-match only, leaving the error handling, if any is needed,
> outside of that binding.
>
> (defsubst string-match-p (regexp string &optional start)
>   "\
> Same as `string-match' except this function does not change the match data."
>   (condition-case err
>       (let ((inhibit-changing-match-data t))
> 	(string-match regexp string start))
>     (error (signal (car err) (cdr err)))))

This optimizes for the rare case that string-match throws an error.
Better would be to bind inhibit-changing-match-data in call_debugger.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  schwab@suse.de,  23949@debbugs.gnu.org
> Date: Wed, 13 Jul 2016 09:59:33 -0400
> 
> In the case of string-match-p I think we should get rid of
> inhibit-changing-match-data and implement string-match-p (and
> looking-at-p) some other way.

Why?  The only problem with the current implementation is when
string-match signals an error.  We need to make sure the error is
handled not under the binding of inhibit-changing-match-data.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: kaushal.modi@gmail.com,  schwab@suse.de,  23949@debbugs.gnu.org
> Date: Wed, 13 Jul 2016 10:48:15 -0400
> 
> > I also think that the "breaks a lot of Elisp code" part is at least a
> > tad exaggerated.
> 
> Binding inhibit-changing-match-data to t will pretty much break any
> function that uses match-beginning or match-end.

But those functions aren't supposed to run when string-match is
called.

> > (defsubst string-match-p (regexp string &optional start)
> >   "\
> > Same as `string-match' except this function does not change the match data."
> >   (condition-case err
> >       (let ((inhibit-changing-match-data t))
> > 	(string-match regexp string start))
> >     (error (signal (car err) (cdr err)))))
> 
> That will still cause the same problems when debug-on-signal is non-nil.

So you don't consider this an improvement that should be installed?

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  23949@debbugs.gnu.org,  kaushal.modi@gmail.com
> Date: Wed, 13 Jul 2016 17:03:22 +0200
> 
> > (defsubst string-match-p (regexp string &optional start)
> >   "\
> > Same as `string-match' except this function does not change the match data."
> >   (condition-case err
> >       (let ((inhibit-changing-match-data t))
> > 	(string-match regexp string start))
> >     (error (signal (car err) (cdr err)))))
> 
> This optimizes for the rare case that string-match throws an error.
> Better would be to bind inhibit-changing-match-data in call_debugger.

Fine, let's do that, then.

Thanks.

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Stefan Monnier]

>> > I also think that the "breaks a lot of Elisp code" part is at least a
>> > tad exaggerated.
>> Binding inhibit-changing-match-data to t will pretty much break any
>> function that uses match-beginning or match-end.
> But those functions aren't supposed to run when string-match is
> called.

Yet they are in bug#23949.

In practice, there's always a discrepancy between what is supposed to
happen and what can happen in the general case ;-)

>> > (defsubst string-match-p (regexp string &optional start)
>> >   "\
>> > Same as `string-match' except this function does not change the match data."
>> >   (condition-case err
>> >       (let ((inhibit-changing-match-data t))
>> > 	(string-match regexp string start))
>> >     (error (signal (car err) (cdr err)))))
>> That will still cause the same problems when debug-on-signal is non-nil.
> So you don't consider this an improvement that should be installed?

No.

A simpler and more robust solution would be
(save-match-data (string-match regexp string start))

Of course, with either solution, it means that string-match-p is even
worse in terms of efficiency, whereas the unsuspecting coder would
rightfully expect string-match-p to be (slightly) *more* efficient than
string-match.


        Stefan

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: kaushal.modi@gmail.com,  schwab@suse.de,  23949@debbugs.gnu.org
> Date: Wed, 13 Jul 2016 12:00:40 -0400
> 
> >> > I also think that the "breaks a lot of Elisp code" part is at least a
> >> > tad exaggerated.
> >> Binding inhibit-changing-match-data to t will pretty much break any
> >> function that uses match-beginning or match-end.
> > But those functions aren't supposed to run when string-match is
> > called.
> 
> Yet they are in bug#23949.

No, they aren't.  They run from the debugger.

> >> > (defsubst string-match-p (regexp string &optional start)
> >> >   "\
> >> > Same as `string-match' except this function does not change the match data."
> >> >   (condition-case err
> >> >       (let ((inhibit-changing-match-data t))
> >> > 	(string-match regexp string start))
> >> >     (error (signal (car err) (cdr err)))))
> >> That will still cause the same problems when debug-on-signal is non-nil.
> > So you don't consider this an improvement that should be installed?
> 
> No.

What about the suggestion made by Andreas?

> A simpler and more robust solution would be
> (save-match-data (string-match regexp string start))
> 
> Of course, with either solution, it means that string-match-p is even
> worse in terms of efficiency, whereas the unsuspecting coder would
> rightfully expect string-match-p to be (slightly) *more* efficient than
> string-match.

Exactly.  And we are punishing the innocent (the calls that don't
signal an error) for fear of the guilty (those that do).

bug#23949: 25.0.95; Regression in handling error caused by (string-match-p "." nil)

[Stefan Monnier]

>> >> > I also think that the "breaks a lot of Elisp code" part is at least a
>> >> > tad exaggerated.
>> >> Binding inhibit-changing-match-data to t will pretty much break any
>> >> function that uses match-beginning or match-end.
>> > But those functions aren't supposed to run when string-match is
>> > called.
>> Yet they are in bug#23949.
> No, they aren't.  They run from the debugger.

I guess it depends what you call "run when string-match is called".

For me, I look at the C backtrace and see that Fstring_match calls
functions which end up calling the debugger which ends up running
the rest.

>> >> > (defsubst string-match-p (regexp string &optional start)
>> >> >   "\
>> >> > Same as `string-match' except this function does not change the match data."
>> >> >   (condition-case err
>> >> >       (let ((inhibit-changing-match-data t))
>> >> > 	(string-match regexp string start))
>> >> >     (error (signal (car err) (cdr err)))))
>> >> That will still cause the same problems when debug-on-signal is non-nil.
>> > So you don't consider this an improvement that should be installed?
>> No.
> What about the suggestion made by Andreas?

I think that would also fix this problem, yes.  It won't fix other
possible cases where the string-match could end up running Elisp code,
but AFAIK there aren't any currently.

> Exactly.  And we are punishing the innocent (the calls that don't
> signal an error) for fear of the guilty (those that do).

Right.  A more efficient solution would be to implement string-match-p
in C alongside string-match rather than on top of it, so we can get rid of
inhibit-changing-match-data (replaced by some other mechanism to
propagate this info where we need it).  But it's more work.


        Stefan

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 1194 bytes --]

Hey bug-gnu-emacs,

Is the following a bug?

    # No backtrace?!
    $ emacs -Q  --batch --eval '(setq debug-on-error t)' --eval '(string-match-p nil "A")'
    Wrong type argument: stringp, nil

    # Yes backtrace?!?
    $ emacs -Q  --batch --eval '(setq debug-on-error t)' --eval '(string-match nil "A")'
    Debugger entered--Lisp error: (wrong-type-argument stringp nil)
      string-match(nil "A")
      eval((string-match nil "A"))
      ...

This isn't new, but it behaved differently in Emacs 24.5:

    $ emacs-24.5 -Q  --batch --eval '(setq debug-on-error t)' --eval '(string-match nil "A")'
    Debugger entered--Lisp error: (wrong-type-argument stringp nil)
      string-match(nil "A")
      eval((string-match nil "A"))
      ...

    $ emacs-24.5 -Q  --batch --eval '(setq debug-on-error t)' --eval '(string-match-p nil "A")'
    Args out of range: 0

Clément.

In GNU Emacs 25.1.50.7 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
 of 2016-07-20 built on clem-w50-mint
Repository revision: a1a0c208e3e895a6ea0942e8e5c4077faf12c7ad
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description:	Linux Mint 18 Sarah


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[npostavs]

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

merge 23949 24166
quit

Clément Pit--Claudel <clement.pitclaudel@live.com> writes:

> Hey bug-gnu-emacs,
>
> Is the following a bug?

Yes, it's another instance of #23949

Actually, I wonder why the trivial fix suggested there wasn't applied already:


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 938 bytes --]

From d7c19f35861f12a16e7571340f3f0802a7e85adb Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 5 Aug 2016 22:11:00 -0400
Subject: [PATCH v1] Fix debugging of string-match-p errors

* lisp/emacs-lisp/debug.el (debug): Bind inhibit-changing-match-data to
nil so that debugger code that needs to do regexp match won't break
(Bug #23949, Bug #24166, Bug#16294).
---
 lisp/emacs-lisp/debug.el | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lisp/emacs-lisp/debug.el b/lisp/emacs-lisp/debug.el
index 22a3f39..22f86e4 100644
--- a/lisp/emacs-lisp/debug.el
+++ b/lisp/emacs-lisp/debug.el
@@ -177,6 +177,7 @@ debug
 	     (or enable-recursive-minibuffers (> (minibuffer-depth) 0)))
 	    (standard-input t) (standard-output t)
 	    inhibit-redisplay
+            inhibit-changing-match-data
 	    (cursor-in-echo-area nil)
 	    (window-configuration (current-window-configuration)))
 	(unwind-protect
-- 
2.8.0

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 476 bytes --]

On 2016-08-05 22:15, npostavs@users.sourceforge.net wrote:
> merge 23949 24166
> quit
> 
> Clément Pit--Claudel <clement.pitclaudel@live.com> writes:
> 
>> Hey bug-gnu-emacs,
>>
>> Is the following a bug?
> 
> Yes, it's another instance of #23949
> 
> Actually, I wonder why the trivial fix suggested there wasn't applied already:

Thanks Noam. I did try to search for dupes before posting, but my debbugs-fu is still limited; sorry about this!

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Fri, 05 Aug 2016 22:15:09 -0400
> Cc: 24166@debbugs.gnu.org
> 
> > Is the following a bug?
> 
> Yes, it's another instance of #23949

Indeed.

> Actually, I wonder why the trivial fix suggested there wasn't applied already:

I think Andreas's suggestion to do this in call_debugger is more
robust, because it does that for _any_ debugger whose value is placed
in the 'debugger' variable, not just for debug.el.

Would you please make that change?

TIA

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Noam Postavsky]

On Sat, Aug 6, 2016 at 3:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> I think Andreas's suggestion to do this in call_debugger is more
> robust, because it does that for _any_ debugger whose value is placed
> in the 'debugger' variable, not just for debug.el.

What about all the other variables that debug.el is binding? (I see
that they both bind inhibit-redisplay to nil)

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 6 Aug 2016 06:28:10 -0400
> Cc: Clément Pit--Claudel <clement.pitclaudel@live.com>, 
> 	24166@debbugs.gnu.org
> 
> On Sat, Aug 6, 2016 at 3:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> > I think Andreas's suggestion to do this in call_debugger is more
> > robust, because it does that for _any_ debugger whose value is placed
> > in the 'debugger' variable, not just for debug.el.
> 
> What about all the other variables that debug.el is binding? (I see
> that they both bind inhibit-redisplay to nil)

Sorry, I don't understand how those variables are related to the issue
at hand.  What am I missing?

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Noam Postavsky]

On Sat, Aug 6, 2016 at 6:34 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Noam Postavsky <npostavs@users.sourceforge.net>
>> Date: Sat, 6 Aug 2016 06:28:10 -0400
>> Cc: Clément Pit--Claudel <clement.pitclaudel@live.com>,
>>       24166@debbugs.gnu.org
>>
>> On Sat, Aug 6, 2016 at 3:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> > I think Andreas's suggestion to do this in call_debugger is more
>> > robust, because it does that for _any_ debugger whose value is placed
>> > in the 'debugger' variable, not just for debug.el.
>>
>> What about all the other variables that debug.el is binding? (I see
>> that they both bind inhibit-redisplay to nil)
>
> Sorry, I don't understand how those variables are related to the issue
> at hand.  What am I missing?

Not related to this issue as such, but if we want to bind
inhibit-changing-match-data for all debuggers, why not the others too?

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sat, 6 Aug 2016 06:49:41 -0400
> Cc: Clément Pit--Claudel <clement.pitclaudel@live.com>, 
> 	24166@debbugs.gnu.org
> 
> On Sat, Aug 6, 2016 at 6:34 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> From: Noam Postavsky <npostavs@users.sourceforge.net>
> >> Date: Sat, 6 Aug 2016 06:28:10 -0400
> >> Cc: Clément Pit--Claudel <clement.pitclaudel@live.com>,
> >>       24166@debbugs.gnu.org
> >>
> >> On Sat, Aug 6, 2016 at 3:25 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> >> > I think Andreas's suggestion to do this in call_debugger is more
> >> > robust, because it does that for _any_ debugger whose value is placed
> >> > in the 'debugger' variable, not just for debug.el.
> >>
> >> What about all the other variables that debug.el is binding? (I see
> >> that they both bind inhibit-redisplay to nil)
> >
> > Sorry, I don't understand how those variables are related to the issue
> > at hand.  What am I missing?
> 
> Not related to this issue as such, but if we want to bind
> inhibit-changing-match-data for all debuggers, why not the others too?

Which ones specifically did you have in mind?  We need to consider
each one carefully, in order to determine whether every debugger needs
that, or just debug.el.

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[npostavs]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Not related to this issue as such, but if we want to bind
>> inhibit-changing-match-data for all debuggers, why not the others too?
>
> Which ones specifically did you have in mind?  We need to consider
> each one carefully, in order to determine whether every debugger needs
> that, or just debug.el.

Well I thought all of the ones in the let-binding except
enable-recursive-minibuffers (which has a comment about a specific
debug.el command) and window-configuration (which is just a local
lexical variable), as they don't look debug.el specific. But I don't
really have the patience for careful consideration of each variable just
to solve some hypothetical problems, so let's leave it for another time.

Here's the new patch.  Is it correct to put the DEFSYM next to the
DEFVAR_LISP?  I saw a few places where they were separated, but that
seems odd since it's then documented twice, and I thought maybe it's
just because they happened to be added at different times by people who
didn't notice the existing definition.


[-- Attachment #2: patch v2 --]
[-- Type: text/plain, Size: 1619 bytes --]

From 9249fece6255615b63cd6848250003051f21cd1c Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Fri, 5 Aug 2016 22:11:00 -0400
Subject: [PATCH v2] Fix debugging of string-match-p errors

* src/eval.c (call_debugger): Bind inhibit-changing-match-data to nil so
that debugger code that needs to do regexp match won't break
(Bug #23949, Bug #24166, Bug#16294).
---
 src/eval.c   | 4 ++++
 src/search.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/src/eval.c b/src/eval.c
index 33b82f7..bf5bed7 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -300,6 +300,10 @@ call_debugger (Lisp_Object arg)
   specbind (Qinhibit_redisplay, Qnil);
   specbind (Qinhibit_debugger, Qt);
 
+  /* If we are debugging an error within a call to `string-match-p',
+     then make sure debugger code can still use match data.  */
+  specbind (Qinhibit_changing_match_data, Qnil);
+
 #if 0 /* Binding this prevents execution of Lisp code during
 	 redisplay, which necessarily leads to display problems.  */
   specbind (Qinhibit_eval_during_redisplay, Qt);
diff --git a/src/search.c b/src/search.c
index 5dc4d35..9b8fc58 100644
--- a/src/search.c
+++ b/src/search.c
@@ -3390,6 +3390,7 @@ or other such regexp constructs are not replaced with this.
 A value of nil (which is the normal value) means treat spaces literally.  */);
   Vsearch_spaces_regexp = Qnil;
 
+  DEFSYM (Qinhibit_changing_match_data, "inhibit-changing-match-data");
   DEFVAR_LISP ("inhibit-changing-match-data", Vinhibit_changing_match_data,
       doc: /* Internal use only.
 If non-nil, the primitive searching and matching functions
-- 
2.8.0

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: 24166@debbugs.gnu.org,  clement.pitclaudel@live.com
> Date: Sat, 06 Aug 2016 08:25:17 -0400
> 
> Here's the new patch.

Thanks.  I have one minor comment below.

> Is it correct to put the DEFSYM next to the DEFVAR_LISP?

It isn't incorrect, and even eval.c already does that.

> +  /* If we are debugging an error within a call to `string-match-p',
> +     then make sure debugger code can still use match data.  */

Please mention in the comment that string-match-p binds
inhibit-changing-match-data, and that it is only one example of that.

Otherwise, LGTM, thanks.

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[npostavs]

tags 24166 fixed
close 24166 25.2
quit

Pushed as 7fb75680

bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!)

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 163 bytes --]

On 2016-08-07 10:27, npostavs@users.sourceforge.net wrote:
> tags 24166 fixed
> close 24166 25.2
> quit
> 
> Pushed as 7fb75680

Yay! Thanks everyone :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

bug#23949: acknowledged by developer (Re: bug#24166: With --eval, errors in string-match-p do not produce backtraces (but errors in string-match do?!))

[Kaushal Modi]

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

Hi Noam,

I appreciate the work you put on this. Thanks for fixing this.

On Sun, Aug 7, 2016 at 10:28 AM GNU bug Tracking System <
help-debbugs@gnu.org> wrote:

> This is an automatic notification regarding your bug report
> #23949: 25.0.95; Regression in handling error caused by (string-match-p
> "." nil),
> which was filed against the emacs package.
>
> Thank you for your report, which has now been closed.
> You can view the full report at
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23949
>
> If you require further information, please followup to
> 23949@debbugs.gnu.org.
>
> debbugs.gnu.org maintainers
> (administrator, GNU bugs database)
>
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1318 bytes --]