bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Mallchad Skeghyeph]

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

Date: Mon, 28 Jun 2021 18:36:19 +0100
Message-ID: <878s2tx564.fsf@gmail.com>
I've found for a little while now that a few default behaviors of GNU
emacs pollutes directories.
In the worst cases this can actually break programs, especially software
toolchains, where they assume a partition kind of file should be
pretend, and mistakenly tries to work with the file lock.
Another one of this files is backups and autosaves.

Ideally I would like the file lock and backup files to be placed in a
central directory,
rather than the same directory as the file in question.
After researching this problem I can see I'm not alone, and others have
felt this way for far longer than I have.

I have looked into seeing if I could change this manually,
for backups I can,
for file locks I have to dive into the source code, and after looking I
fear I would be knee deep in a very long rabbit hole without having
something more to work with.
I've not touched the source for this project before, only looked.

In GNU Emacs 28.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.29,
cairo version 1.17.4)
 of 2021-06-20 built on m-arch-asus
Repository revision: 01b0a909b5ca858a09484821cc866127652f4153
Repository branch: makepkg
Windowing system distributor 'System Description: Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --mandir=/usr/share/man --with-gameuser=:games
 --with-sound=alsa --with-modules --without-gconf --without-gsettings
 --with-native-compilation --with-pgtk --enable-link-time-optimization
 --with-x-toolkit=gtk3 --without-xaw3d --without-m17n-flt --with-cairo
 --with-xwidgets --without-compress-install 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt -g -flto -fuse-linker-plugin
 -fuse-ld=gold -g -flto -fuse-linker-plugin -fuse-ld=gold'
 CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM HARFBUZZ JPEG JSON LCMS2
LIBOTF LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER
PGTK PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XIM
XWIDGETS GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
  company-tng-mode: t
  beacon-mode: t
  centaur-tabs-mode: t
  global-highlight-parentheses-mode: t
  smartparens-global-mode: t
  smooth-scrolling-mode: t
  sublimity-mode: t
  global-whitespace-mode: t
  global-subword-mode: t
  treemacs-filewatch-mode: t
  treemacs-follow-mode: t
  treemacs-git-mode: deferred
  treemacs-fringe-indicator-mode: t
  global-hl-line-mode: t
  helm-mode: t
  projectile-mode: t
  zoom-mode: t
  ws-butler-global-mode: t
  volatile-highlights-mode: t
  global-undo-tree-mode: t
  helm-autoresize-mode: t
  helm--remap-mouse-mode: t
  global-git-commit-mode: t
  magit-auto-revert-mode: t
  global-hungry-delete-mode: t
  global-hl-todo-mode: t
  recentf-mode: t
  shell-dirtrack-mode: t
  async-bytecomp-package-mode: t
  override-global-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  global-visual-line-mode: t

Load-path shadows:
/home/mallchad/.emacs.d/elpa/cmake-mode-20210104.1831/cmake-mode hides
/usr/share/emacs/site-lisp/cmake-mode
/home/mallchad/.emacs.d/elpa/transient-20210525.1141/transient hides
/usr/share/emacs/28.0.50/lisp/transient

Features:
(gnus-async gnus-agent gnus-srvr gnus-score score-mode nnvirtual nntp
gnus-ml gnus-msg nndoc gnus-cache gnus-dup debbugs-gnu debbugs
soap-client rng-xsd rng-dt rng-util xsd-regexp tar-mode arc-mode
archive-mode mm-archive url-http url-gw url-cache url-auth mailclient
shadow sort mail-extr emacsbug sendmail vc bug-reference mule-util
cl-print help-fns tabify image-file image-converter magit-extras
org-indent ol-eww eww xdg url-queue mm-url ol-rmail ol-mhe ol-irc
ol-info ol-gnus nnselect gnus-search eieio-opt speedbar ezimage dframe
gnus-art mm-uu mml2015 mm-view mml-smime smime dig gnus-sum shr kinsoku
svg gnus-group gnus-undo gnus-start gnus-dbus gnus-cloud nnimap nnmail
mail-source utf7 netrc nnoo gnus-spec gnus-int gnus-range gnus-win gnus
nnheader ol-docview doc-view jka-compr ol-bibtex bibtex ol-bbdb ol-w3m
sh-script smie executable backup-each-save cus-edit cus-start cus-load
ffap dabbrev find-file helm-command helm-elisp helm-eval edebug
backtrace lsp-diagnostics lsp-headerline lsp-icons lsp-modeline vc-git
vc-dispatcher lsp-zig lsp-steep lsp-svelte lsp-sqls lsp-yaml lsp-xml
lsp-vimscript lsp-vhdl lsp-vetur lsp-html lsp-verilog lsp-vala lsp-v
lsp-terraform lsp-tex lsp-sorbet lsp-solargraph lsp-rust lsp-rf lsp-r
lsp-purescript lsp-pylsp lsp-pyls lsp-pwsh lsp-php lsp-perl lsp-ocaml
lsp-nix lsp-nim lsp-markdown lsp-lua lsp-kotlin lsp-json lsp-javascript
lsp-haxe lsp-groovy lsp-hack lsp-go lsp-completion lsp-gdscript
lsp-fsharp lsp-fortran lsp-eslint lsp-erlang lsp-elixir lsp-elm
lsp-dockerfile lsp-dhall lsp-d lsp-css lsp-csharp lsp-crystal lsp-cmake
lsp-clojure lsp-clangd dom lsp-beancount lsp-bash lsp-angular lsp-ada
lsp-actionscript winner tramp-archive tramp-gvfs dbus helm-x-files
helm-for-files helm-bookmark helm-adaptive helm-info helm-external
helm-net xml aggressive-indent company-oddmuse company-keywords
company-etags company-gtags company-dabbrev-code company-dabbrev
company-files company-clang company-capf company-cmake company-semantic
company-template company-bbdb company-tng company rainbow-delimiters
rainbow-mode page-break-lines display-line-numbers linum init beacon
centaur-tabs centaur-tabs-interactive centaur-tabs-functions
centaur-tabs-elements powerline powerline-separators powerline-themes
highlight-parentheses smartparens-config smartparens-javascript
smartparens-rst smartparens-org smartparens-markdown smartparens-text
smartparens-c smartparens smooth-scrolling sublimity-scroll sublimity
disp-table whitespace cap-words superword subword lsp-treemacs
lsp-treemacs-themes treemacs treemacs-header-line treemacs-compatibility
treemacs-mode treemacs-bookmarks treemacs-interface treemacs-extensions
treemacs-mouse-interface treemacs-tags treemacs-persistence
treemacs-filewatch-mode treemacs-follow-mode treemacs-rendering
treemacs-async treemacs-workspaces treemacs-dom treemacs-visuals
treemacs-fringe-indicator treemacs-scope pulse treemacs-faces
treemacs-icons treemacs-themes treemacs-core-utils pfuture hl-line
treemacs-logging treemacs-customization treemacs-macros lsp-origami
lsp-ui lsp-ui-flycheck lsp-ui-doc xwidget image-mode exif magit-bookmark
bookmark goto-addr lsp-ui-imenu lsp-ui-peek lsp-ui-sideline face-remap
lsp-ui-util lsp-mode lsp-protocol spinner network-stream nsm
markdown-mode inline ewoc origami origami-parsers helm-mode helm-config
helm-swoop helm-rg pcase helm-projectile helm-files helm-tags
helm-buffers helm-occur helm-locate helm-types projectile grep ibuf-ext
ibuffer ibuffer-loaddefs helm-flycheck flycheck-inline csproj-mode
org-cliplink org-cliplink-transport org-cliplink-string em-glob esh-util
zoom ws-butler volatile-highlights visual-fill-column vlf-setup vlf
vlf-base vlf-tune visible-mark undo-tree smart-yank restart-emacs
desktop frameset rainbow-blocks org-super-agenda ts org-habit org-agenda
org-refile org-noter org-element avl-tree org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-footnote org-src ob-comint org-pcomplete
org-list org-faces org-entities noutline outline org-version
ob-emacs-lisp ob-core ob-eval org-table ol org-keys org-compat org-macs
org-loaddefs cal-menu calendar cal-loaddefs omnisharp
omnisharp-unit-test-actions omnisharp-code-structure
omnisharp-server-installation gnutls omnisharp-format-actions
omnisharp-solution-actions omnisharp-helm-integration helm-grep
helm-regexp helm-utils helm-help helm helm-global-bindings helm-easymenu
helm-source helm-multi-match helm-lib omnisharp-navigation-actions
omnisharp-current-symbol-actions omnisharp-auto-complete-actions
omnisharp-server-actions omnisharp-http-utils omnisharp-utils
omnisharp-server-management omnisharp-settings f s flycheck find-func
etags fileloop generator xref project popup ido csharp-mode
csharp-compilation cc-langs json-mode json-reformat json-snatcher js
cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align cc-engine
cc-vars cc-defs multiple-cursors mc-separate-operations
rectangular-region-mode mc-mark-pop mc-edit-lines
mc-hide-unmatched-lines-mode mc-mark-more mc-cycle-cursors
multiple-cursors-core advice rect magit-submodule magit-obsolete
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log which-func imenu magit-diff smerge-mode
diff diff-mode git-commit log-edit message rmc puny rfc822 mml mml-sec
epa derived epg epg-config gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045 mm-util
ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils mailheader
pcvs-util add-log magit-core magit-autorevert autorevert filenotify
magit-margin magit-transient magit-process with-editor server magit-mode
transient magit-git magit-section magit-utils crm hydra lv hungry-delete
hl-todo god-mode free-keys fireplace dashboard dashboard-widgets time
recentf tree-widget wid-edit cmake-mode rst compile text-property-search
crux tramp tramp-loaddefs trampver tramp-integration files-x
tramp-compat shell pcomplete comint ansi-color parse-time iso8601
time-date ls-lisp format-spec thingatpt edmacro kmacro avy ring
sanityinc-tomorrow-night-theme color-theme-sanityinc-tomorrow color
all-the-icons all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons
async-bytecomp async flyspell ispell req-package view req-package-cycles
req-package-args req-package-hooks ht log4e dash el-get
el-get-autoloading el-get-list-packages el-get-dependencies el-get-build
el-get-status pp el-get-methods el-get-fossil el-get-svn el-get-pacman
el-get-github-zip el-get-github-tar el-get-http-zip el-get-http-tar
el-get-hg el-get-go el-get-git-svn el-get-fink el-get-emacswiki
el-get-http el-get-notify el-get-emacsmirror el-get-github el-get-git
el-get-elpa el-get-darcs el-get-cvs el-get-bzr el-get-brew
el-get-builtin el-get-apt-get el-get-recipes el-get-byte-compile
el-get-custom el-get-core autoload radix-tree lisp-mnt dired
dired-loaddefs use-package use-package-ensure use-package-delight
use-package-diminish use-package-bind-key bind-key easy-mmode
use-package-core cl cemacs-utility finder-inf info package browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap url-handlers url-parse auth-source eieio
eieio-core eieio-loaddefs password-cache json map url-vars comp
comp-cstr warnings subr-x rx cl-seq cl-macs cl-extra help-mode seq
byte-opt gv cl-loaddefs cl-lib bytecomp byte-compile cconv iso-transl
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/pgtk-win pgtk-win term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads
xwidget-internal dbusbind inotify dynamic-setting font-render-setting
cairo move-toolbar gtk x-toolkit pgtk lcms2 multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 1366260 1278287)
 (symbols 48 71661 1845)
 (strings 32 821605 302207)
 (string-bytes 1 15366299)
 (vectors 16 808670)
 (vector-slots 8 8340068 966354)
 (floats 8 3607 3481)
 (intervals 56 38157 23043)
 (buffers 992 69))

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

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Mallchad Skeghyeph <ncaprisunfan@gmail.com> writes:

> I've found for a little while now that a few default behaviors of GNU
> emacs pollutes directories.

Yes, this is a question that comes up from time to time, and I don't
think we've documented in a comprehensive way how to make Emacs not
write anything in the directories it visits.

For auto-save files, that's auto-save-file-name-transforms.
For backup files, that's backup-directory-alist.
For lock files, they can be switched off with create-lockfiles.

Would it make sense to allow the user to control where the lockfiles are
written?  The lockfiles are symlinks, so it should theoretically be
possible to have them elsewhere without being any racier than the code
currently is, I think.

Any opinions?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 30 Jun 2021 15:00:41 +0200
> Cc: 49261@debbugs.gnu.org
> 
> Would it make sense to allow the user to control where the lockfiles are
> written?

Yes, I think so.  But it is not as trivial as it could sound,
because...

> The lockfiles are symlinks, so it should theoretically be
> possible to have them elsewhere without being any racier than the code
> currently is, I think.

...even if the lock files are symlinks (which they not necessarily
are), we need to handle the case of several files with identical
basenames in different directories.  (Their being symlinks is
unimportant, because the target of the symlink doesn't exist.)

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The lockfiles are symlinks, so it should theoretically be
>> possible to have them elsewhere without being any racier than the code
>> currently is, I think.
>
> ...even if the lock files are symlinks (which they not necessarily
> are), we need to handle the case of several files with identical
> basenames in different directories.

We could perhaps reuse the auto-save file name logic?  I.e., extend
`make-auto-save-file-name' so that we can use it to compute the lock
file names, too.

Or just use the UNIIFY logic from auto-save-file-name-transforms
unconditionally...

> (Their being symlinks is unimportant, because the target of the
> symlink doesn't exist.)

Right; I'd forgotten that we just use the lock symlink to stash some
extra data about the locking Emacs process.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

(Please keep the debbugs address in the CCs -- otherwise the message
won't reach the bug tracker.)

Mallchad Skeghyeph <ncaprisunfan@gmail.com> writes:

>  ...even if the lock files are symlinks (which they not necessarily
>  are), we need to handle the case of several files with identical
>  basenames in different directories.  (Their being symlinks is
>  unimportant, because the target of the symlink doesn't exist.)
>
> Actually, is there even a good reason to keep relying on symlinks in the future?
> Considering that.. ahem, some Operating Systems cannot do symlinks for the
> user?
> If it were treated as a normal file you could load it up with whatever metadata
> you want.

Using a normal file should also work, I think?  But it'd be slightly
less efficient on some common popular file systems.

>  Or just use the UNIIFY logic from auto-save-file-name-transforms
>  unconditionally...
>
> It seems most of `make-auto-save-file-name` makes very little assumptions 
> about how we're modifying the file, actually,
> so it shouldn't be too too problematic to make it work for both.
> Though, I would like an optional single variable for a directory for both,
> as oppose to a slightly opaqueregex.
>
>  Remote, I think, otherwise the lock can be easily ignored from another
>  machine.
>
> Yes. I think we need to keep compatibility with the current lock, if its noticed. 
> Remote or local.
> In the case of remote files, we'd have to assume others might be using the
> current 
> lock behaviour.

Remote files are a problem, but what to do here would be up to the user
(as it is with autosave files).

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Thu, 01 Jul 2021 12:55:55 +0200
> Cc: 49261@debbugs.gnu.org
> 
> Mallchad Skeghyeph <ncaprisunfan@gmail.com> writes:
> 
> >  ...even if the lock files are symlinks (which they not necessarily
> >  are), we need to handle the case of several files with identical
> >  basenames in different directories.  (Their being symlinks is
> >  unimportant, because the target of the symlink doesn't exist.)
> >
> > Actually, is there even a good reason to keep relying on symlinks in the future?
> > Considering that.. ahem, some Operating Systems cannot do symlinks for the
> > user?
> > If it were treated as a normal file you could load it up with whatever metadata
> > you want.
> 
> Using a normal file should also work, I think?  But it'd be slightly
> less efficient on some common popular file systems.

We already use regular files on systems on which symlinks are either
unsupported or otherwise problematic.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Would it make sense to allow the user to control where the lockfiles are
>> written?
>
> Yes, I think so.  But it is not as trivial as it could sound,
> because...
>
>> The lockfiles are symlinks, so it should theoretically be
>> possible to have them elsewhere without being any racier than the code
>> currently is, I think.
>
> ...even if the lock files are symlinks (which they not necessarily
> are), we need to handle the case of several files with identical
> basenames in different directories.  (Their being symlinks is
> unimportant, because the target of the symlink doesn't exist.)

And there is the case of remote files. Do we want locking of remote
files? Should the respective lock file be local (better for performance)
or also remote (better for locking a file being accessed from different
Emacsen somewhere)?

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Wed, 30 Jun 2021 18:07:41 +0200
> 
> And there is the case of remote files. Do we want locking of remote
> files?

I think we do.  maybe as an opt-in behavior?

> Should the respective lock file be local (better for performance) or
> also remote (better for locking a file being accessed from different
> Emacsen somewhere)?

Remote, I think, otherwise the lock can be easily ignored from another
machine.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> Should the respective lock file be local (better for performance) or
>> also remote (better for locking a file being accessed from different
>> Emacsen somewhere)?
>
> Remote, I think, otherwise the lock can be easily ignored from another
> machine.

Auto-save files are local by default, so they have the same problem
(being ignored on other systems).  However, lock files are tiny, while
auto-save files can be arbitrarily big, so from a practical point of
view there's less reason to default lock files to being local.

So I agree that having lock files being remote would be a good default.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Juri Linkov]

> For auto-save files, that's auto-save-file-name-transforms.
> For backup files, that's backup-directory-alist.
> For lock files, they can be switched off with create-lockfiles.
>
> Would it make sense to allow the user to control where the lockfiles are
> written?  The lockfiles are symlinks, so it should theoretically be
> possible to have them elsewhere without being any racier than the code
> currently is, I think.
>
> Any opinions?

This is a real problem.  To avoid syncing temporary files,
it's easy to add a pair of lines for every such directory:

  (add-to-list 'auto-save-file-name-transforms '("\\`/dir1/dir2/.*" "/tmp/" t))
  (add-to-list 'backup-directory-alist         '("\\`/dir1/dir2/.*" . "/tmp/"))

Then it creates temporary files whose names contain absolute paths:

  /tmp/!dir1/dir2!file~
  /tmp/#!dir1/dir2!file#

But currently no way to do the same for lock files, so need to take care
not to forget to add the same pattern in every .stignore in every sync directory
to ignore lock files that is extra trouble:

  .#*

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

> Yes, this is a question that comes up from time to time, and I don't
> think we've documented in a comprehensive way how to make Emacs not
> write anything in the directories it visits.
>
> For auto-save files, that's auto-save-file-name-transforms.
> For backup files, that's backup-directory-alist.
> For lock files, they can be switched off with create-lockfiles.
>
> Would it make sense to allow the user to control where the lockfiles are
> written?  The lockfiles are symlinks, so it should theoretically be
> possible to have them elsewhere without being any racier than the code
> currently is, I think.
>
> Any opinions?

Thinking about, it makes much sense to write the lockfile into the same
directory as the file it locks. Think about mounted directories. If
there is, for example, an NFS server which exports home directories, and
you are editing /home/user/.emacs with different Emacs instances on
different machines, where /home/user is mounted, you want to protect
~/.emacs for write access from any Emacs instance when needed.

A similar case would be for remote files, where a file could also be
locked for access with Emacs from different machines, which use Tramp.

Note, that file lock does not exist yet for remote files, but as Eli
said, we want this feature.

So the very recommended default lock file name shall be what we have
now. We could give the user a configuration variable to change this
behaviour, but with explicit warning about consequences.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Thu, 01 Jul 2021 18:57:12 +0200
> Cc: Mallchad Skeghyeph <ncaprisunfan@gmail.com>, 49261@debbugs.gnu.org
> 
> So the very recommended default lock file name shall be what we have
> now. We could give the user a configuration variable to change this
> behaviour, but with explicit warning about consequences.

I think this was indeed the intent.  There's no intent to change the
default behavior wrt where the lock file is written, only to introduce
a new opt-in behavior.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think this was indeed the intent.  There's no intent to change the
> default behavior wrt where the lock file is written, only to introduce
> a new opt-in behavior.

Yup.

I was going to take a stab at implementing this today, but I find that I
don't have the time (probably for the next couple of days), so if
somebody else wants to take a stab at it, please be my guest.  :-)

If not, I'll get to it early next week.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

>> I think this was indeed the intent.  There's no intent to change the
>> default behavior wrt where the lock file is written, only to introduce
>> a new opt-in behavior.
>
> I was going to take a stab at implementing this today, but I find that I
> don't have the time (probably for the next couple of days), so if
> somebody else wants to take a stab at it, please be my guest.  :-)

I've pushed on my TODO to implement file locking for remote files first,
which does not exist yet. If nothing breaks my priorities, it should be
ready next days.

After that, we could see what does it need for configuration.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> If not, I'll get to it early next week.

Is Wednesday still "early"?

Anyway, I've now implemented this.  The biggest part of this is
refactoring out the `auto-save-file-name-transforms' handling so that it
can be used by the lock handling code, too.

I'd like to have more eyes on this before I commit.  It seems to work
fine after some light testing, but I'm not completely confident about
the ENCODE_FILE/ALLOCA bits in the C code.

So if somebody could give that a look-over while I'm writing up the
documentation, that'd be great.  :-)

diff --git a/lisp/files.el b/lisp/files.el
index 859c193db9..ba588842a2 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -412,6 +412,21 @@ auto-save-file-name-transforms
   :initialize 'custom-initialize-delay
   :version "21.1")
 
+(defcustom lock-file-name-transforms nil
+  "Transforms to apply to buffer file name before making a lock file name.
+This has the same syntax as
+`auto-save-file-name-transforms' (which see), but instead of
+applying to auto-save file names, it's applied to lock file names.
+
+By default, a lock file is put into the same directory as the
+file it's locking, and it has the same name, but with \".#\" prepended."
+  :group 'files
+  :type '(repeat (list (regexp :tag "Regexp")
+                       (string :tag "Replacement")
+		       (boolean :tag "Uniquify")))
+  :initialize 'custom-initialize-delay
+  :version "28.1")
+
 (defvar auto-save--timer nil "Timer for `auto-save-visited-mode'.")
 
 (defcustom auto-save-visited-interval 5
@@ -6668,63 +6683,11 @@ make-auto-save-file-name
 					     'make-auto-save-file-name)))
 	(if handler
 	    (funcall handler 'make-auto-save-file-name)
-	  (let ((list auto-save-file-name-transforms)
-		(filename buffer-file-name)
-		result uniq)
-	    ;; Apply user-specified translations
-	    ;; to the file name.
-	    (while (and list (not result))
-	      (if (string-match (car (car list)) filename)
-		  (setq result (replace-match (cadr (car list)) t nil
-					      filename)
-			uniq (car (cddr (car list)))))
-	      (setq list (cdr list)))
-	    (if result
-                (setq filename
-                      (cond
-                       ((memq uniq (secure-hash-algorithms))
-                        (concat
-                         (file-name-directory result)
-                         (secure-hash uniq filename)))
-                       (uniq
-                        (concat
-			 (file-name-directory result)
-			 (subst-char-in-string
-			  ?/ ?!
-			  (replace-regexp-in-string
-                           "!" "!!" filename))))
-		       (t result))))
-	    (setq result
-		  (if (and (eq system-type 'ms-dos)
-			   (not (msdos-long-file-names)))
-		      ;; We truncate the file name to DOS 8+3 limits
-		      ;; before doing anything else, because the regexp
-		      ;; passed to string-match below cannot handle
-		      ;; extensions longer than 3 characters, multiple
-		      ;; dots, and other atrocities.
-		      (let ((fn (dos-8+3-filename
-				 (file-name-nondirectory buffer-file-name))))
-			(string-match
-			 "\\`\\([^.]+\\)\\(\\.\\(..?\\)?.?\\|\\)\\'"
-			 fn)
-			(concat (file-name-directory buffer-file-name)
-				"#" (match-string 1 fn)
-				"." (match-string 3 fn) "#"))
-		    (concat (file-name-directory filename)
-			    "#"
-			    (file-name-nondirectory filename)
-			    "#")))
-	    ;; Make sure auto-save file names don't contain characters
-	    ;; invalid for the underlying filesystem.
-	    (if (and (memq system-type '(ms-dos windows-nt cygwin))
-		     ;; Don't modify remote filenames
-                     (not (file-remote-p result)))
-		(convert-standard-filename result)
-	      result))))
-
+          (auto-save--transform-file-name buffer-file-name
+                                          auto-save-file-name-transforms
+                                          "#" "#")))
     ;; Deal with buffers that don't have any associated files.  (Mail
     ;; mode tends to create a good number of these.)
-
     (let ((buffer-name (buffer-name))
 	  (limit 0)
 	  file-name)
@@ -6772,6 +6735,71 @@ make-auto-save-file-name
 	(file-error nil))
       file-name)))
 
+(defun auto-save--transform-file-name (filename transforms
+                                                prefix suffix)
+  "Transform FILENAME according to TRANSFORMS.
+See `auto-save-file-name-transforms' for the format of
+TRANSFORMS.  PREFIX is prepended to the non-directory portion of
+the resulting file name, and SUFFIX is appended."
+  (let (result uniq)
+    ;; Apply user-specified translations
+    ;; to the file name.
+    (while (and transforms (not result))
+      (if (string-match (car (car transforms)) filename)
+	  (setq result (replace-match (cadr (car transforms)) t nil
+				      filename)
+		uniq (car (cddr (car transforms)))))
+      (setq transforms (cdr transforms)))
+    (when result
+      (setq filename
+            (cond
+             ((memq uniq (secure-hash-algorithms))
+              (concat
+               (file-name-directory result)
+               (secure-hash uniq filename)))
+             (uniq
+              (concat
+	       (file-name-directory result)
+	       (subst-char-in-string
+		?/ ?!
+		(replace-regexp-in-string
+                 "!" "!!" filename))))
+	     (t result))))
+    (setq result
+	  (if (and (eq system-type 'ms-dos)
+		   (not (msdos-long-file-names)))
+	      ;; We truncate the file name to DOS 8+3 limits
+	      ;; before doing anything else, because the regexp
+	      ;; passed to string-match below cannot handle
+	      ;; extensions longer than 3 characters, multiple
+	      ;; dots, and other atrocities.
+	      (let ((fn (dos-8+3-filename
+			 (file-name-nondirectory buffer-file-name))))
+		(string-match
+		 "\\`\\([^.]+\\)\\(\\.\\(..?\\)?.?\\|\\)\\'"
+		 fn)
+		(concat (file-name-directory buffer-file-name)
+			prefix (match-string 1 fn)
+			"." (match-string 3 fn) suffix))
+	    (concat (file-name-directory filename)
+		    prefix
+		    (file-name-nondirectory filename)
+		    suffix)))
+    ;; Make sure auto-save file names don't contain characters
+    ;; invalid for the underlying filesystem.
+    (if (and (memq system-type '(ms-dos windows-nt cygwin))
+	     ;; Don't modify remote filenames
+             (not (file-remote-p result)))
+	(convert-standard-filename result)
+      result)))
+
+(defun make-lock-file-name (filename)
+  "Make a lock file name for FILENAME.
+By default, this just prepends \".*\" to the non-directory part
+of FILENAME, but the transforms in `lock-file-name-transforms'
+are done first."
+  (auto-save--transform-file-name filename lock-file-name-transforms ".#" ""))
+
 (defun auto-save-file-name-p (filename)
   "Return non-nil if FILENAME can be yielded by `make-auto-save-file-name'.
 FILENAME should lack slashes.
diff --git a/src/filelock.c b/src/filelock.c
index 446a262a1c..3c6e6b4942 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -294,25 +294,6 @@ get_boot_time_1 (const char *filename, bool newest)
   char user[MAX_LFINFO + 1 + sizeof " (pid )" - sizeof "."];
 } lock_info_type;
 
-/* Write the name of the lock file for FNAME into LOCKNAME.  Length
-   will be that of FNAME plus two more for the leading ".#", plus one
-   for the null.  */
-#define MAKE_LOCK_NAME(lockname, fname) \
-  (lockname = SAFE_ALLOCA (SBYTES (fname) + 2 + 1), \
-   fill_in_lock_file_name (lockname, fname))
-
-static void
-fill_in_lock_file_name (char *lockfile, Lisp_Object fn)
-{
-  char *last_slash = memrchr (SSDATA (fn), '/', SBYTES (fn));
-  char *base = last_slash + 1;
-  ptrdiff_t dirlen = base - SSDATA (fn);
-  memcpy (lockfile, SSDATA (fn), dirlen);
-  lockfile[dirlen] = '.';
-  lockfile[dirlen + 1] = '#';
-  strcpy (lockfile + dirlen + 2, base);
-}
-
 /* For some reason Linux kernels return EPERM on file systems that do
    not support hard or symbolic links.  This symbol documents the quirk.
    There is no way to tell whether a symlink call fails due to
@@ -639,6 +620,12 @@ lock_if_free (lock_info_type *clasher, char *lfname)
   return err;
 }
 
+static Lisp_Object
+make_lock_file_name (Lisp_Object fn)
+{
+  return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn));
+}
+
 /* lock_file locks file FN,
    meaning it serves notice on the world that you intend to edit that file.
    This should be done only when about to modify a file-visiting
@@ -660,10 +647,9 @@ lock_if_free (lock_info_type *clasher, char *lfname)
 void
 lock_file (Lisp_Object fn)
 {
-  Lisp_Object orig_fn, encoded_fn;
+  Lisp_Object orig_fn;
   char *lfname = NULL;
   lock_info_type lock_info;
-  USE_SAFE_ALLOCA;
 
   /* Don't do locking while dumping Emacs.
      Uncompressing wtmp files uses call-process, which does not work
@@ -672,29 +658,25 @@ lock_file (Lisp_Object fn)
     return;
 
   orig_fn = fn;
-  fn = Fexpand_file_name (fn, Qnil);
+  fn = make_lock_file_name (Fexpand_file_name (fn, Qnil));
 #ifdef WINDOWSNT
   /* Ensure we have only '/' separators, to avoid problems with
      looking (inside fill_in_lock_file_name) for backslashes in file
      names encoded by some DBCS codepage.  */
   dostounix_filename (SSDATA (fn));
 #endif
-  encoded_fn = ENCODE_FILE (fn);
-  if (create_lockfiles)
-    /* Create the name of the lock-file for file fn */
-    MAKE_LOCK_NAME (lfname, encoded_fn);
-
+  lfname = SSDATA (fn);
   /* See if this file is visited and has changed on disk since it was
      visited.  */
   Lisp_Object subject_buf = get_truename_buffer (orig_fn);
   if (!NILP (subject_buf)
       && NILP (Fverify_visited_file_modtime (subject_buf))
       && !NILP (Ffile_exists_p (fn))
-      && !(lfname && current_lock_owner (NULL, lfname) == -2))
+      && !(create_lockfiles && current_lock_owner (NULL, lfname) == -2))
     call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
 
   /* Don't do locking if the user has opted out.  */
-  if (lfname)
+  if (create_lockfiles)
     {
       /* Try to lock the lock.  FIXME: This ignores errors when
 	 lock_if_free returns a positive errno value.  */
@@ -715,7 +697,6 @@ lock_file (Lisp_Object fn)
 	  if (!NILP (attack))
 	    lock_file_1 (lfname, 1);
 	}
-      SAFE_FREE ();
     }
 }
 
@@ -723,12 +704,9 @@ lock_file (Lisp_Object fn)
 unlock_file_body (Lisp_Object fn)
 {
   char *lfname;
-  USE_SAFE_ALLOCA;
-
-  Lisp_Object filename = Fexpand_file_name (fn, Qnil);
-  fn = ENCODE_FILE (filename);
 
-  MAKE_LOCK_NAME (lfname, fn);
+  Lisp_Object filename = make_lock_file_name (Fexpand_file_name (fn, Qnil));
+  lfname = SSDATA (filename);
 
   int err = current_lock_owner (0, lfname);
   if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
@@ -736,7 +714,6 @@ unlock_file_body (Lisp_Object fn)
   if (0 < err)
     report_file_errno ("Unlocking file", filename, err);
 
-  SAFE_FREE ();
   return Qnil;
 }
 
@@ -842,11 +819,10 @@ DEFUN ("file-locked-p", Ffile_locked_p, Sfile_locked_p, 1, 1, 0,
   char *lfname;
   int owner;
   lock_info_type locker;
-  USE_SAFE_ALLOCA;
 
   filename = Fexpand_file_name (filename, Qnil);
-  Lisp_Object encoded_filename = ENCODE_FILE (filename);
-  MAKE_LOCK_NAME (lfname, encoded_filename);
+  Lisp_Object lockname = make_lock_file_name (filename);
+  lfname = SSDATA (lockname);
 
   owner = current_lock_owner (&locker, lfname);
   switch (owner)
@@ -857,7 +833,6 @@ DEFUN ("file-locked-p", Ffile_locked_p, Sfile_locked_p, 1, 1, 0,
     default: report_file_errno ("Testing file lock", filename, owner);
     }
 
-  SAFE_FREE ();
   return ret;
 #endif
 }
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 257cbc2d32..b97e0256fb 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -949,6 +949,44 @@ files-tests-file-name-non-special-make-auto-save-file-name
                              (make-auto-save-file-name)
                            (kill-buffer)))))))
 
+(ert-deftest files-test-auto-save-name-default ()
+  (with-temp-buffer
+    (let ((auto-save-file-name-transforms nil))
+      (setq buffer-file-name "/tmp/foo.txt")
+      (should (equal (make-auto-save-file-name) "/tmp/#foo.txt#")))))
+
+(ert-deftest files-test-auto-save-name-transform ()
+  (with-temp-buffer
+    (setq buffer-file-name "/tmp/foo.txt")
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" nil))))
+      (should (equal (make-auto-save-file-name) "/var/tmp/#foo.txt#")))))
+
+(ert-deftest files-test-auto-save-name-unique ()
+  (with-temp-buffer
+    (setq buffer-file-name "/tmp/foo.txt")
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" t))))
+      (should (equal (make-auto-save-file-name) "/var/tmp/#!tmp!foo.txt#")))
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" sha1))))
+      (should (equal (make-auto-save-file-name)
+                     "/var/tmp/#b57c5a04f429a83305859d3350ecdab8315a9037#")))))
+
+(ert-deftest files-test-lock-name-default ()
+  (let ((lock-file-name-transforms nil))
+    (should (equal (make-lock-file-name "/tmp/foo.txt") "/tmp/.#foo.txt"))))
+
+(ert-deftest files-test-lock-name-unique ()
+  (let ((lock-file-name-transforms
+         '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" t))))
+    (should (equal (make-lock-file-name "/tmp/foo.txt")
+                   "/var/tmp/.#!tmp!foo.txt")))
+  (let ((lock-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" sha1))))
+    (should (equal (make-lock-file-name "/tmp/foo.txt")
+                   "/var/tmp/.#b57c5a04f429a83305859d3350ecdab8315a9037"))))
+
 (ert-deftest files-tests-file-name-non-special-make-directory ()
   (files-tests--with-temp-non-special (tmpdir nospecial-dir t)
     (let ((default-directory nospecial-dir))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> If not, I'll get to it early next week.
>
> Is Wednesday still "early"?

Uuhh, I'm almost finished. I'm running some final tests, my plan is to
commit tonight.

Would it be OK for you? I promise, I will check your patch immediately
after this.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Uuhh, I'm almost finished. I'm running some final tests, my plan is to
> commit tonight.
>
> Would it be OK for you? I promise, I will check your patch immediately
> after this.

Sure.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Uuhh, I'm almost finished. I'm running some final tests, my plan is to
>> commit tonight.
>>
>> Would it be OK for you? I promise, I will check your patch immediately
>> after this.
>
> Sure.  :-)

There are still two failed test cases. But I guess it will be better to
hunt them together with your applied patch.

I've committed my work as d35868bec9. Go on and push your changes, and
let's puzzle the final bits afterwards.

(Ohh, Glenn will beat me)

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> There are still two failed test cases. But I guess it will be better to
> hunt them together with your applied patch.
>
> I've committed my work as d35868bec9. Go on and push your changes, and
> let's puzzle the final bits afterwards.

I have some errands to run, so I'm not pushing right away -- but I'll do
so later tonight, unless I discover major problems...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

> So if somebody could give that a look-over while I'm writing up the
> documentation, that'd be great.  :-)

Just some first thoughts, by dry reading.

>  	(if handler
>  	    (funcall handler 'make-auto-save-file-name)

We have a file name handler for make-auto-save-file-name. Shall we use
also a handler for make-lock-file-name?

> +(defun make-lock-file-name (filename)
> +  "Make a lock file name for FILENAME.
> +By default, this just prepends \".*\" to the non-directory part
> +of FILENAME, but the transforms in `lock-file-name-transforms'
> +are done first."
> +  (auto-save--transform-file-name filename lock-file-name-transforms ".#" ""))

Hmm, maybe not, because the lock file name must be the *same* over
different Emacs sessions.

Furthermore, there is auto-save-mode, which toggles auto-saving. Shall
we use something similar for file locks? Perhaps, people might not want
to lock all files, for example they might want to disable this feature
for remote files (possible performance degradation).

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Just some first thoughts, by dry reading.
>
>>  	(if handler
>>  	    (funcall handler 'make-auto-save-file-name)
>
> We have a file name handler for make-auto-save-file-name. Shall we use
> also a handler for make-lock-file-name?

Yeah, I wondered about that, too.  I'm not quite sure what the use case
would be, but for symmetry's sake, it might make sense anyway.

>> +(defun make-lock-file-name (filename)
>> +  "Make a lock file name for FILENAME.
>> +By default, this just prepends \".*\" to the non-directory part
>> +of FILENAME, but the transforms in `lock-file-name-transforms'
>> +are done first."
>> + (auto-save--transform-file-name filename lock-file-name-transforms
>> ".#" ""))
>
> Hmm, maybe not, because the lock file name must be the *same* over
> different Emacs sessions.

That would be up to the person that writes the handler to take care of,
I think?

> Furthermore, there is auto-save-mode, which toggles auto-saving. Shall
> we use something similar for file locks? Perhaps, people might not want
> to lock all files, for example they might want to disable this feature
> for remote files (possible performance degradation).

Sure; makes sense.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>>>  	(if handler
>>>  	    (funcall handler 'make-auto-save-file-name)
>>
>> We have a file name handler for make-auto-save-file-name. Shall we use
>> also a handler for make-lock-file-name?
>
> Yeah, I wondered about that, too.  I'm not quite sure what the use case
> would be, but for symmetry's sake, it might make sense anyway.

OK, I'll care.

One use case could be a file lock for a file archive (something like
foo.tar), when you change a file inside the archive.

>> Hmm, maybe not, because the lock file name must be the *same* over
>> different Emacs sessions.
>
> That would be up to the person that writes the handler to take care of,
> I think?

:-)

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

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

While looking at this code, I'm puzzled by:

-  orig_fn = fn;
-  fn = Fexpand_file_name (fn, Qnil);
-#ifdef WINDOWSNT
-  /* Ensure we have only '/' separators, to avoid problems with
-     looking (inside fill_in_lock_file_name) for backslashes in file
-     names encoded by some DBCS codepage.  */
-  dostounix_filename (SSDATA (fn));
-#endif
-  encoded_fn = ENCODE_FILE (fn);
-  if (create_lockfiles)
-    /* Create the name of the lock-file for file fn */
-    MAKE_LOCK_NAME (lfname, encoded_fn);
-

So here we (possibly destructively) alter the data in the fn string on
WINDOWSNT, because we want to avoid problems in fill_in_lock_file_name.
OK, but we call MAKE_LOCK_NAME (which calls fill_in_lock_file_name) in
two other places, and in those places the call isn't guarded by a call
to dostounix_filename.

This is moot after my patch, since MAKE_LOCK_NAME is gone, but I'm still
worried that there's something I don't understand here...  The
dostounix_filename call was added by Eli in 2013.

So I think I'll wait to push this patch until Eli has given it a
once-over.  I've included the current state of the patch below as an
attachment.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lock.patch --]
[-- Type: text/x-diff, Size: 18421 bytes --]

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 912980b688..98b6b194d2 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -789,7 +789,9 @@ Interlocking
 @vindex create-lockfiles
   You can prevent the creation of lock files by setting the variable
 @code{create-lockfiles} to @code{nil}.  @strong{Caution:} by
-doing so you will lose the benefits that this feature provides.
+doing so you will lose the benefits that this feature provides.  You
+can also control where lock files are written by using the
+@code{lock-file-name-transforms} variable.
 
 @cindex collision
   If you begin to modify the buffer while the visited file is locked by
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 5238597a46..d73c502924 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -772,6 +772,20 @@ File Locks
 If this variable is @code{nil}, Emacs does not lock files.
 @end defopt
 
+@defopt lock-file-name-transforms
+By default, Emacs creates the lock files in the same directory as the
+files that are being locked.  This can be changed by customizing this
+variable.  Is has the same syntax as
+@code{auto-save-file-name-transforms} (@pxref{Auto-Saving}).  For
+instance, to make Emacs write all the lock files to @file{/var/tmp/},
+you could say something like:
+
+@lisp
+(setq lock-file-name-transforms
+      '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" t)))
+@end lisp
+@end defopt
+
 @defun ask-user-about-lock file other-user
 This function is called when the user tries to modify @var{file}, but it
 is locked by another user named @var{other-user}.  The default
diff --git a/doc/misc/efaq.texi b/doc/misc/efaq.texi
index 53a3af4b78..caf5438edb 100644
--- a/doc/misc/efaq.texi
+++ b/doc/misc/efaq.texi
@@ -407,9 +407,9 @@ Mailing list archives
 @cindex Old mailing list posts for GNU lists
 @cindex Mailing list archives for GNU lists
 
-The FSF has maintained archives of all of the GNU mailing lists for many
-years, although there may be some unintentional gaps in coverage.  The
-archive can be browsed over the web at
+The FSF has maintained archives of all of the GNU mailing lists for
+many years, although there may be some unintentional gaps in coverage.
+The archive can be browsed over the web at
 @uref{https://lists.gnu.org/r/, the GNU mail archive}.
 
 Some web-based Usenet search services also archive the @code{gnu.*}
@@ -1519,6 +1519,7 @@ Common requests
 * Documentation for etags::
 * Disabling backups::
 * Disabling auto-save-mode::
+* Not writing files to the current directory::
 * Going to a line by number::
 * Modifying pull-down menus::
 * Deleting menus and menu options::
@@ -2620,6 +2621,39 @@ Disabling auto-save-mode
 To disable or change how @code{auto-save-mode} works,
 @pxref{Auto Save,,, emacs, The GNU Emacs Manual}.
 
+@node Not writing files to the current directory
+@section Making Emacs write all auxiliary files somewhere else
+@cindex Writing all auxiliary files to the same directory
+
+By default, Emacs may create many new files in the directory where
+you're editing a file.  If you're editing the file
+@file{/home/user/foo.txt}, Emacs will create the lock file
+@file{/home/user/.#foo.txt}, the auto-save file
+@file{/home/user/#foo.txt#}, and when you save the file, Emacs will
+create the backup file @file{/home/user/foo.txt~}.  (The first two
+files are deleted when you save the file.)
+
+This may be inconvenient in some setups, so Emacs has mechanisms for
+changing the locations of all these files.
+
+@table @code
+@item auto-save-file-name-transforms (@pxref{Auto-Saving,,,elisp, GNU Emacs Lisp Reference Manual}).
+@item lock-file-name-transforms (@pxref{File Locks,,,elisp, GNU Emacs Lisp Reference Manual}).
+@item backup-directory-alist (@pxref{Making Backups,,,elisp, GNU Emacs Lisp Reference Manual}).
+@end table
+
+For instance, to write all these things to
+@file{~/.emacs.d/aux/}:
+
+@lisp
+(setq lock-file-name-transforms
+      '(("\\`/.*/\\([^/]+\\)\\'" "~/.emacs.d/aux/\\1" t)))
+(setq auto-save-file-name-transforms
+      '(("\\`/.*/\\([^/]+\\)\\'" "~/.emacs.d/aux/\\1" t)))
+(setq backup-directory-alist
+      '((".*" . "~/.emacs.d/aux/")))
+@end lisp
+
 @node Going to a line by number
 @section How can I go to a certain line given its number?
 @cindex Going to a line by number
diff --git a/etc/NEWS b/etc/NEWS
index 7bf8c1d8f5..e398e3c789 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2165,6 +2165,11 @@ summaries will include the failing condition.
 
 ** Miscellaneous
 
++++
+*** New user option 'lock-file-name-transforms'.
+This option allows controlling where lock files are written.  It uses
+the same syntax as 'auto-save-file-name-transforms'.
+
 +++
 *** New user option 'kill-transform-function'.
 This can be used to transform (and suppress) strings from entering the
diff --git a/lisp/files.el b/lisp/files.el
index 859c193db9..205b429f9d 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -412,6 +412,21 @@ auto-save-file-name-transforms
   :initialize 'custom-initialize-delay
   :version "21.1")
 
+(defcustom lock-file-name-transforms nil
+  "Transforms to apply to buffer file name before making a lock file name.
+This has the same syntax as
+`auto-save-file-name-transforms' (which see), but instead of
+applying to auto-save file names, it's applied to lock file names.
+
+By default, a lock file is put into the same directory as the
+file it's locking, and it has the same name, but with \".#\" prepended."
+  :group 'files
+  :type '(repeat (list (regexp :tag "Regexp")
+                       (string :tag "Replacement")
+		       (boolean :tag "Uniquify")))
+  :initialize 'custom-initialize-delay
+  :version "28.1")
+
 (defvar auto-save--timer nil "Timer for `auto-save-visited-mode'.")
 
 (defcustom auto-save-visited-interval 5
@@ -6668,63 +6683,11 @@ make-auto-save-file-name
 					     'make-auto-save-file-name)))
 	(if handler
 	    (funcall handler 'make-auto-save-file-name)
-	  (let ((list auto-save-file-name-transforms)
-		(filename buffer-file-name)
-		result uniq)
-	    ;; Apply user-specified translations
-	    ;; to the file name.
-	    (while (and list (not result))
-	      (if (string-match (car (car list)) filename)
-		  (setq result (replace-match (cadr (car list)) t nil
-					      filename)
-			uniq (car (cddr (car list)))))
-	      (setq list (cdr list)))
-	    (if result
-                (setq filename
-                      (cond
-                       ((memq uniq (secure-hash-algorithms))
-                        (concat
-                         (file-name-directory result)
-                         (secure-hash uniq filename)))
-                       (uniq
-                        (concat
-			 (file-name-directory result)
-			 (subst-char-in-string
-			  ?/ ?!
-			  (replace-regexp-in-string
-                           "!" "!!" filename))))
-		       (t result))))
-	    (setq result
-		  (if (and (eq system-type 'ms-dos)
-			   (not (msdos-long-file-names)))
-		      ;; We truncate the file name to DOS 8+3 limits
-		      ;; before doing anything else, because the regexp
-		      ;; passed to string-match below cannot handle
-		      ;; extensions longer than 3 characters, multiple
-		      ;; dots, and other atrocities.
-		      (let ((fn (dos-8+3-filename
-				 (file-name-nondirectory buffer-file-name))))
-			(string-match
-			 "\\`\\([^.]+\\)\\(\\.\\(..?\\)?.?\\|\\)\\'"
-			 fn)
-			(concat (file-name-directory buffer-file-name)
-				"#" (match-string 1 fn)
-				"." (match-string 3 fn) "#"))
-		    (concat (file-name-directory filename)
-			    "#"
-			    (file-name-nondirectory filename)
-			    "#")))
-	    ;; Make sure auto-save file names don't contain characters
-	    ;; invalid for the underlying filesystem.
-	    (if (and (memq system-type '(ms-dos windows-nt cygwin))
-		     ;; Don't modify remote filenames
-                     (not (file-remote-p result)))
-		(convert-standard-filename result)
-	      result))))
-
+          (auto-save--transform-file-name buffer-file-name
+                                          auto-save-file-name-transforms
+                                          "#" "#")))
     ;; Deal with buffers that don't have any associated files.  (Mail
     ;; mode tends to create a good number of these.)
-
     (let ((buffer-name (buffer-name))
 	  (limit 0)
 	  file-name)
@@ -6772,6 +6735,72 @@ make-auto-save-file-name
 	(file-error nil))
       file-name)))
 
+(defun auto-save--transform-file-name (filename transforms
+                                                prefix suffix)
+  "Transform FILENAME according to TRANSFORMS.
+See `auto-save-file-name-transforms' for the format of
+TRANSFORMS.  PREFIX is prepended to the non-directory portion of
+the resulting file name, and SUFFIX is appended."
+  (let (result uniq)
+    ;; Apply user-specified translations
+    ;; to the file name.
+    (while (and transforms (not result))
+      (if (string-match (car (car transforms)) filename)
+	  (setq result (replace-match (cadr (car transforms)) t nil
+				      filename)
+		uniq (car (cddr (car transforms)))))
+      (setq transforms (cdr transforms)))
+    (when result
+      (setq filename
+            (cond
+             ((memq uniq (secure-hash-algorithms))
+              (concat
+               (file-name-directory result)
+               (secure-hash uniq filename)))
+             (uniq
+              (concat
+	       (file-name-directory result)
+	       (subst-char-in-string
+		?/ ?!
+		(replace-regexp-in-string
+                 "!" "!!" filename))))
+	     (t result))))
+    (setq result
+	  (if (and (eq system-type 'ms-dos)
+		   (not (msdos-long-file-names)))
+	      ;; We truncate the file name to DOS 8+3 limits
+	      ;; before doing anything else, because the regexp
+	      ;; passed to string-match below cannot handle
+	      ;; extensions longer than 3 characters, multiple
+	      ;; dots, and other atrocities.
+	      (let ((fn (dos-8+3-filename
+			 (file-name-nondirectory buffer-file-name))))
+		(string-match
+		 "\\`\\([^.]+\\)\\(\\.\\(..?\\)?.?\\|\\)\\'"
+		 fn)
+		(concat (file-name-directory buffer-file-name)
+			prefix (match-string 1 fn)
+			"." (match-string 3 fn) suffix))
+	    (concat (file-name-directory filename)
+		    prefix
+		    (file-name-nondirectory filename)
+		    suffix)))
+    ;; Make sure auto-save file names don't contain characters
+    ;; invalid for the underlying filesystem.
+    (expand-file-name
+     (if (and (memq system-type '(ms-dos windows-nt cygwin))
+	      ;; Don't modify remote filenames
+              (not (file-remote-p result)))
+	 (convert-standard-filename result)
+       result))))
+
+(defun make-lock-file-name (filename)
+  "Make a lock file name for FILENAME.
+By default, this just prepends \".*\" to the non-directory part
+of FILENAME, but the transforms in `lock-file-name-transforms'
+are done first."
+  (auto-save--transform-file-name filename lock-file-name-transforms ".#" ""))
+
 (defun auto-save-file-name-p (filename)
   "Return non-nil if FILENAME can be yielded by `make-auto-save-file-name'.
 FILENAME should lack slashes.
diff --git a/src/filelock.c b/src/filelock.c
index 446a262a1c..cda7e2f8f6 100644
--- a/src/filelock.c
+++ b/src/filelock.c
@@ -51,7 +51,6 @@ Copyright (C) 1985-1987, 1993-1994, 1996, 1998-2021 Free Software
 #ifdef WINDOWSNT
 #include <share.h>
 #include <sys/socket.h>	/* for fcntl */
-#include "w32.h"	/* for dostounix_filename */
 #endif
 
 #ifndef MSDOS
@@ -294,25 +293,6 @@ get_boot_time_1 (const char *filename, bool newest)
   char user[MAX_LFINFO + 1 + sizeof " (pid )" - sizeof "."];
 } lock_info_type;
 
-/* Write the name of the lock file for FNAME into LOCKNAME.  Length
-   will be that of FNAME plus two more for the leading ".#", plus one
-   for the null.  */
-#define MAKE_LOCK_NAME(lockname, fname) \
-  (lockname = SAFE_ALLOCA (SBYTES (fname) + 2 + 1), \
-   fill_in_lock_file_name (lockname, fname))
-
-static void
-fill_in_lock_file_name (char *lockfile, Lisp_Object fn)
-{
-  char *last_slash = memrchr (SSDATA (fn), '/', SBYTES (fn));
-  char *base = last_slash + 1;
-  ptrdiff_t dirlen = base - SSDATA (fn);
-  memcpy (lockfile, SSDATA (fn), dirlen);
-  lockfile[dirlen] = '.';
-  lockfile[dirlen + 1] = '#';
-  strcpy (lockfile + dirlen + 2, base);
-}
-
 /* For some reason Linux kernels return EPERM on file systems that do
    not support hard or symbolic links.  This symbol documents the quirk.
    There is no way to tell whether a symlink call fails due to
@@ -639,6 +619,13 @@ lock_if_free (lock_info_type *clasher, char *lfname)
   return err;
 }
 
+static Lisp_Object
+make_lock_file_name (Lisp_Object fn)
+{
+  return ENCODE_FILE (call1 (intern ("make-lock-file-name"),
+			     Fexpand_file_name (fn, Qnil)));
+}
+
 /* lock_file locks file FN,
    meaning it serves notice on the world that you intend to edit that file.
    This should be done only when about to modify a file-visiting
@@ -660,10 +647,9 @@ lock_if_free (lock_info_type *clasher, char *lfname)
 void
 lock_file (Lisp_Object fn)
 {
-  Lisp_Object orig_fn, encoded_fn;
+  Lisp_Object lock_filename;
   char *lfname = NULL;
   lock_info_type lock_info;
-  USE_SAFE_ALLOCA;
 
   /* Don't do locking while dumping Emacs.
      Uncompressing wtmp files uses call-process, which does not work
@@ -671,30 +657,19 @@ lock_file (Lisp_Object fn)
   if (will_dump_p ())
     return;
 
-  orig_fn = fn;
-  fn = Fexpand_file_name (fn, Qnil);
-#ifdef WINDOWSNT
-  /* Ensure we have only '/' separators, to avoid problems with
-     looking (inside fill_in_lock_file_name) for backslashes in file
-     names encoded by some DBCS codepage.  */
-  dostounix_filename (SSDATA (fn));
-#endif
-  encoded_fn = ENCODE_FILE (fn);
-  if (create_lockfiles)
-    /* Create the name of the lock-file for file fn */
-    MAKE_LOCK_NAME (lfname, encoded_fn);
-
+  lock_filename = make_lock_file_name (fn);
+  lfname = SSDATA (lock_filename);
   /* See if this file is visited and has changed on disk since it was
      visited.  */
-  Lisp_Object subject_buf = get_truename_buffer (orig_fn);
+  Lisp_Object subject_buf = get_truename_buffer (fn);
   if (!NILP (subject_buf)
       && NILP (Fverify_visited_file_modtime (subject_buf))
-      && !NILP (Ffile_exists_p (fn))
-      && !(lfname && current_lock_owner (NULL, lfname) == -2))
+      && !NILP (Ffile_exists_p (lock_filename))
+      && !(create_lockfiles && current_lock_owner (NULL, lfname) == -2))
     call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
 
   /* Don't do locking if the user has opted out.  */
-  if (lfname)
+  if (create_lockfiles)
     {
       /* Try to lock the lock.  FIXME: This ignores errors when
 	 lock_if_free returns a positive errno value.  */
@@ -715,7 +690,6 @@ lock_file (Lisp_Object fn)
 	  if (!NILP (attack))
 	    lock_file_1 (lfname, 1);
 	}
-      SAFE_FREE ();
     }
 }
 
@@ -723,12 +697,9 @@ lock_file (Lisp_Object fn)
 unlock_file_body (Lisp_Object fn)
 {
   char *lfname;
-  USE_SAFE_ALLOCA;
-
-  Lisp_Object filename = Fexpand_file_name (fn, Qnil);
-  fn = ENCODE_FILE (filename);
 
-  MAKE_LOCK_NAME (lfname, fn);
+  Lisp_Object filename = make_lock_file_name (fn);
+  lfname = SSDATA (filename);
 
   int err = current_lock_owner (0, lfname);
   if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
@@ -736,7 +707,6 @@ unlock_file_body (Lisp_Object fn)
   if (0 < err)
     report_file_errno ("Unlocking file", filename, err);
 
-  SAFE_FREE ();
   return Qnil;
 }
 
@@ -842,11 +812,9 @@ DEFUN ("file-locked-p", Ffile_locked_p, Sfile_locked_p, 1, 1, 0,
   char *lfname;
   int owner;
   lock_info_type locker;
-  USE_SAFE_ALLOCA;
 
-  filename = Fexpand_file_name (filename, Qnil);
-  Lisp_Object encoded_filename = ENCODE_FILE (filename);
-  MAKE_LOCK_NAME (lfname, encoded_filename);
+  Lisp_Object lockname = make_lock_file_name (filename);
+  lfname = SSDATA (lockname);
 
   owner = current_lock_owner (&locker, lfname);
   switch (owner)
@@ -857,7 +825,6 @@ DEFUN ("file-locked-p", Ffile_locked_p, Sfile_locked_p, 1, 1, 0,
     default: report_file_errno ("Testing file lock", filename, owner);
     }
 
-  SAFE_FREE ();
   return ret;
 #endif
 }
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 257cbc2d32..a6b0c900be 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -949,6 +949,44 @@ files-tests-file-name-non-special-make-auto-save-file-name
                              (make-auto-save-file-name)
                            (kill-buffer)))))))
 
+(ert-deftest files-test-auto-save-name-default ()
+  (with-temp-buffer
+    (let ((auto-save-file-name-transforms nil))
+      (setq buffer-file-name "/tmp/foo.txt")
+      (should (equal (make-auto-save-file-name) "/tmp/#foo.txt#")))))
+
+(ert-deftest files-test-auto-save-name-transform ()
+  (with-temp-buffer
+    (setq buffer-file-name "/tmp/foo.txt")
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" nil))))
+      (should (equal (make-auto-save-file-name) "/var/tmp/#foo.txt#")))))
+
+(ert-deftest files-test-auto-save-name-unique ()
+  (with-temp-buffer
+    (setq buffer-file-name "/tmp/foo.txt")
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" t))))
+      (should (equal (make-auto-save-file-name) "/var/tmp/#!tmp!foo.txt#")))
+    (let ((auto-save-file-name-transforms
+           '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" sha1))))
+      (should (equal (make-auto-save-file-name)
+                     "/var/tmp/#b57c5a04f429a83305859d3350ecdab8315a9037#")))))
+
+(ert-deftest files-test-lock-name-default ()
+  (let ((lock-file-name-transforms nil))
+    (should (equal (make-lock-file-name "/tmp/foo.txt") "/tmp/.#foo.txt"))))
+
+(ert-deftest files-test-lock-name-unique ()
+  (let ((lock-file-name-transforms
+         '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" t))))
+    (should (equal (make-lock-file-name "/tmp/foo.txt")
+                   "/var/tmp/.#!tmp!foo.txt")))
+  (let ((lock-file-name-transforms
+         '(("\\`/.*/\\([^/]+\\)\\'" "/var/tmp/\\1" sha1))))
+    (should (equal (make-lock-file-name "/tmp/foo.txt")
+                   "/var/tmp/.#b57c5a04f429a83305859d3350ecdab8315a9037"))))
+
 (ert-deftest files-tests-file-name-non-special-make-directory ()
   (files-tests--with-temp-non-special (tmpdir nospecial-dir t)
     (let ((default-directory nospecial-dir))

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 20:08:48 +0200
> 
> While looking at this code, I'm puzzled by:
> 
> -  orig_fn = fn;
> -  fn = Fexpand_file_name (fn, Qnil);
> -#ifdef WINDOWSNT
> -  /* Ensure we have only '/' separators, to avoid problems with
> -     looking (inside fill_in_lock_file_name) for backslashes in file
> -     names encoded by some DBCS codepage.  */
> -  dostounix_filename (SSDATA (fn));
> -#endif
> -  encoded_fn = ENCODE_FILE (fn);
> -  if (create_lockfiles)
> -    /* Create the name of the lock-file for file fn */
> -    MAKE_LOCK_NAME (lfname, encoded_fn);
> -
> 
> So here we (possibly destructively) alter the data in the fn string on
> WINDOWSNT, because we want to avoid problems in fill_in_lock_file_name.
> OK, but we call MAKE_LOCK_NAME (which calls fill_in_lock_file_name) in
> two other places, and in those places the call isn't guarded by a call
> to dostounix_filename.
> 
> This is moot after my patch, since MAKE_LOCK_NAME is gone, but I'm still
> worried that there's something I don't understand here...  The
> dostounix_filename call was added by Eli in 2013.

It's a bug.  Or maybe it was a bug, back then, because I think
nowadays expand-file-name always converts backslashes to forward
slashes.  And actually the fact that MAKE_LOCK_NAME looks for slashes
in encoded file names is also a subtle bug (or at least unsafe code):
some coding-systems don't guarantee that a '/' byte can never be part
of a multibyte sequence.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It's a bug.  Or maybe it was a bug, back then, because I think
> nowadays expand-file-name always converts backslashes to forward
> slashes.  And actually the fact that MAKE_LOCK_NAME looks for slashes
> in encoded file names is also a subtle bug (or at least unsafe code):
> some coding-systems don't guarantee that a '/' byte can never be part
> of a multibyte sequence.

Ah, cool, then I wasn't totally misreading the code.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

I've now pushed this change.  Michael, there were some conflicts in
filelock.c with your changes from earlier this evening -- can you check
whether I resolved them correctly?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

> I've now pushed this change.  Michael, there were some conflicts in
> filelock.c with your changes from earlier this evening -- can you check
> whether I resolved them correctly?

I see no regressions. Three test cases still fail
(file-notify-test03-events-remote, shadow-test08-shadow-todo,
shadow-test09-shadow-copy-files), but the most important one,
tramp-test39-lock-file, still succeeds.

I will check tomorrow why these test cases fail, and how to integrate
the new lock-file-name-transforms into Tramp.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Lars,

> I see no regressions. Three test cases still fail
> (file-notify-test03-events-remote, shadow-test08-shadow-todo,
> shadow-test09-shadow-copy-files), but the most important one,
> tramp-test39-lock-file, still succeeds.

I've pushed another change which let all test cases succeed, as far as I
am involved. Furthermore, I have renamed auto-save--transform-file-name
to files--transform-file-name, because it isn't restricted to auto-save files.

I don't apply make bootstrap, so I'm not plagued with the segfault
problem.

> I will check tomorrow why these test cases fail, and how to integrate
> the new lock-file-name-transforms into Tramp.

This remains open for later today, or tomorrow.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Lars,

>> I will check tomorrow why these test cases fail, and how to integrate
>> the new lock-file-name-transforms into Tramp.
>
> This remains open for later today, or tomorrow.

This is done. ATM, I've decided not to write an own Tramp handler for
make-lock-file-name, the default function is good enough. But the
infrastructure for such a handler is prepared, when needed.

One point I am missing is to suppress lock files, customised by
users. There is create-lockfiles, but this is all-or-nothing. One idea
we've discussed shortly would be to add a file-lock-mode, similar to the
existing auto-save-mode. Would be useful, but it is restricted to the
current buffer.

Another idea I have is to use the new lock-file-name-transforms user
option. It contains entries (REGEXP REPLACEMENT UNIQUIFY) .

What if we allow REPLACEMENT to be nil, meaning that there shoudn't be
file locks for files matching REGEXP? An entry
("\\`/[^/]*:\\([^/]*/\\)*\\([^/]*\\)\\'") of that variable would
suppress file locks for remote files then.

A similar meaning could be offered for auto-save-file-name-transforms.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Thu, 08 Jul 2021 21:53:08 +0200
> 
> One point I am missing is to suppress lock files, customised by
> users. There is create-lockfiles, but this is all-or-nothing.

What other degrees besides "all" and "nothing" would you like to have?
And why?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> One point I am missing is to suppress lock files, customised by
>> users. There is create-lockfiles, but this is all-or-nothing.
>
> What other degrees besides "all" and "nothing" would you like to have?
> And why?

Remote files (all files which match tramp-file-name-regexp). For
backward compatibility, and possibly due to performance reasons.

Mounted files (all files which match `mounted-file-systems'). Possibly
due to performance reasons.

These are just examples. One could imagine more fine-grained choices
(for example, only files located on a given host), or completely other
selections.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: larsi@gnus.org,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Fri, 09 Jul 2021 10:28:29 +0200
> 
> > What other degrees besides "all" and "nothing" would you like to have?
> > And why?
> 
> Remote files (all files which match tramp-file-name-regexp). For
> backward compatibility, and possibly due to performance reasons.
> 
> Mounted files (all files which match `mounted-file-systems'). Possibly
> due to performance reasons.

You want a capability to exempt different kinds of files from locking?
Why would that be a good idea?  Performance doesn't cut it, IMO,
because if one wants to be protected from clobbering, one doesn't care
about performance.  And if one cares about performance for those
special kinds of files, it most probably means one doesn't care about
file locking at all.

So I submit that a binary switch is good enough.

> These are just examples. One could imagine more fine-grained choices
> (for example, only files located on a given host), or completely other
> selections.

I suggest not to imagine potential features.  We already have too many
features, so we should only add new ones if they are really required.
just because someone could use a finer-grained setting doesn't yet
mean we need to cater to that in core.

In any case, now that make-lock-file-name is exposed to Lisp, they can
override or advise it to do whatever they like, right?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Remote files (all files which match tramp-file-name-regexp). For
>> backward compatibility, and possibly due to performance reasons.
>
> You want a capability to exempt different kinds of files from locking?
> Why would that be a good idea?  Performance doesn't cut it, IMO,
> because if one wants to be protected from clobbering, one doesn't care
> about performance.  And if one cares about performance for those
> special kinds of files, it most probably means one doesn't care about
> file locking at all.
>
> So I submit that a binary switch is good enough.

Until now, there are no file locks for remote files at all. I thought disabling
it for remote files would be requested by some users for backward compatibility.

> In any case, now that make-lock-file-name is exposed to Lisp, they can
> override or advise it to do whatever they like, right?

Yes. But they cannot disable it. Advising the function is always
possible, but it is less convenient than a user option setting.

But well, let's see whether people shout ...

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Until now, there are no file locks for remote files at all. I thought
> disabling it for remote files would be requested by some users for
> backward compatibility.

Yeah, I think it's a good idea, and allowing REPLACEMENT to be nil seems
like the natural way to implement it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Until now, there are no file locks for remote files at all. I thought
>> disabling it for remote files would be requested by some users for
>> backward compatibility.
>
> Yeah, I think it's a good idea, and allowing REPLACEMENT to be nil seems
> like the natural way to implement it.

And now? You and Eli disagree in this point, and I don't know what to do.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 15:53:12 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> >> Until now, there are no file locks for remote files at all. I thought
> >> disabling it for remote files would be requested by some users for
> >> backward compatibility.
> >
> > Yeah, I think it's a good idea, and allowing REPLACEMENT to be nil seems
> > like the natural way to implement it.
> 
> And now? You and Eli disagree in this point, and I don't know what to do.

Which disagreement between us leaves you at an impasse?  If there is
indeed such a disagreement, maybe we could find a compromise which
will allow some way forward.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>>
>> >> Until now, there are no file locks for remote files at all. I thought
>> >> disabling it for remote files would be requested by some users for
>> >> backward compatibility.
>> >
>> > Yeah, I think it's a good idea, and allowing REPLACEMENT to be nil seems
>> > like the natural way to implement it.
>>
>> And now? You and Eli disagree in this point, and I don't know what to do.
>
> Which disagreement between us leaves you at an impasse?  If there is
> indeed such a disagreement, maybe we could find a compromise which
> will allow some way forward.

Lars agrees my proposal to use lock-file-name-transforms for disabling
file locks. You did argue against.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: larsi@gnus.org,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 16:37:45 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> Hi Eli,
> 
> >> Lars Ingebrigtsen <larsi@gnus.org> writes:
> >>
> >> >> Until now, there are no file locks for remote files at all. I thought
> >> >> disabling it for remote files would be requested by some users for
> >> >> backward compatibility.
> >> >
> >> > Yeah, I think it's a good idea, and allowing REPLACEMENT to be nil seems
> >> > like the natural way to implement it.
> >>
> >> And now? You and Eli disagree in this point, and I don't know what to do.
> >
> > Which disagreement between us leaves you at an impasse?  If there is
> > indeed such a disagreement, maybe we could find a compromise which
> > will allow some way forward.
> 
> Lars agrees my proposal to use lock-file-name-transforms for disabling
> file locks. You did argue against.

Are we talking about disabling remote locks by default?  If so, how
about disabling them via a more user-friendly option?  It strikes me
as weird and un-intuitive to _disable_ something via a "transform"
function.  And it will definitely be more complex to set up such a
"transform" function than just use a boolean flag.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Are we talking about disabling remote locks by default?  If so, how
> about disabling them via a more user-friendly option?  It strikes me
> as weird and un-intuitive to _disable_ something via a "transform"
> function.  And it will definitely be more complex to set up such a
> "transform" function than just use a boolean flag.

The thing is: In Emacs 27, we didn't have file locking for Tramp files.
This has now been introduced, and we're discussing how to disable them
(and only them) in Emacs 28.

Doing that as a transform seems natural, since that's very parallel to
what we're doing in `auto-save-file-name-transforms' (where Tramp file
names are made local).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 19:35:38 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Are we talking about disabling remote locks by default?  If so, how
> > about disabling them via a more user-friendly option?  It strikes me
> > as weird and un-intuitive to _disable_ something via a "transform"
> > function.  And it will definitely be more complex to set up such a
> > "transform" function than just use a boolean flag.
> 
> The thing is: In Emacs 27, we didn't have file locking for Tramp files.
> This has now been introduced, and we're discussing how to disable them
> (and only them) in Emacs 28.
> 
> Doing that as a transform seems natural, since that's very parallel to
> what we're doing in `auto-save-file-name-transforms' (where Tramp file
> names are made local).

I suggested a compromise: disable it by default, but not via the
transform function.  I think it's more natural, but even if you
disagree, would it do as a compromise?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggested a compromise: disable it by default, but not via the
> transform function.  I think it's more natural, but even if you
> disagree, would it do as a compromise?

I'm OK with another user option to disable it, it doesn't matter too
much. But I'm not sure whether we shall disable it by default: nobody(*)
will enable it ever, and all the effort would be wasted.

(*): except me, of course :-)

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 20:00:34 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I suggested a compromise: disable it by default, but not via the
> > transform function.  I think it's more natural, but even if you
> > disagree, would it do as a compromise?
> 
> I'm OK with another user option to disable it, it doesn't matter too
> much. But I'm not sure whether we shall disable it by default: nobody(*)
> will enable it ever, and all the effort would be wasted.

Now _I_ am confused: didn't you say that we _should_ disable it by
default for remote files, because that was how Emacs worked until now?
Or what did I miss?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > I suggested a compromise: disable it by default, but not via the
>> > transform function.  I think it's more natural, but even if you
>> > disagree, would it do as a compromise?
>>
>> I'm OK with another user option to disable it, it doesn't matter too
>> much. But I'm not sure whether we shall disable it by default: nobody(*)
>> will enable it ever, and all the effort would be wasted.
>
> Now _I_ am confused: didn't you say that we _should_ disable it by
> default for remote files, because that was how Emacs worked until now?
> Or what did I miss?

No. I meant we shall provide the *possibility* to disable it, because it
didn't exist before.

If I have written something which reads different, it is because of my
limited English.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: larsi@gnus.org,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Mon, 12 Jul 2021 20:46:57 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > I suggested a compromise: disable it by default, but not via the
> >> > transform function.  I think it's more natural, but even if you
> >> > disagree, would it do as a compromise?
> >>
> >> I'm OK with another user option to disable it, it doesn't matter too
> >> much. But I'm not sure whether we shall disable it by default: nobody(*)
> >> will enable it ever, and all the effort would be wasted.
> >
> > Now _I_ am confused: didn't you say that we _should_ disable it by
> > default for remote files, because that was how Emacs worked until now?
> > Or what did I miss?
> 
> No. I meant we shall provide the *possibility* to disable it, because it
> didn't exist before.

Ah, okay.  I only said it should be disabled by default because I
thought that was what you suggested.  I have no problem whatsoever to
have remote locking enabled by default.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> >> I'm OK with another user option to disable it, it doesn't matter too
>> >> much. But I'm not sure whether we shall disable it by default: nobody(*)
>> >> will enable it ever, and all the effort would be wasted.
>> >
>> > Now _I_ am confused: didn't you say that we _should_ disable it by
>> > default for remote files, because that was how Emacs worked until now?
>> > Or what did I miss?
>>
>> No. I meant we shall provide the *possibility* to disable it, because it
>> didn't exist before.
>
> Ah, okay.  I only said it should be disabled by default because I
> thought that was what you suggested.  I have no problem whatsoever to
> have remote locking enabled by default.

Finally, I've added `remote-file-name-inhibit-locks'. The name shall
look similar to the existing `remote-file-name-inhibit-cache'.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggested a compromise: disable it by default, but not via the
> transform function.  I think it's more natural, but even if you
> disagree, would it do as a compromise?

I don't mind adding a special Tramp variable here (perhaps there should
be one for auto save file names, too, for symmetry)?

But I do think allowing people to inhibit locks/auto save files on a
path name basis is good functionality in general, Tramp no Tramp.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But I do think allowing people to inhibit locks/auto save files on a
> path name basis is good functionality in general, Tramp no Tramp.

(The last bit should be "Tramp or no Tramp".)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Tue, 13 Jul 2021 18:30:58 +0200
> 
> But I do think allowing people to inhibit locks/auto save files on a
> path name basis is good functionality in general, Tramp no Tramp.

What would a lock-file-name-transforms look like that inhibits file
locking?

And apart of remote file names, what other practical use cases can you
think of where disabling locking selectively based on the file name
would make sense?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> And apart of remote file names, what other practical use cases can you
> think of where disabling locking selectively based on the file name
> would make sense?

All file names which match `mounted-file-systems'.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Tue, 13 Jul 2021 19:59:12 +0200
> 
> > And apart of remote file names, what other practical use cases can you
> > think of where disabling locking selectively based on the file name
> > would make sense?
> 
> All file names which match `mounted-file-systems'.

Shouldn't file locking be disabled there by default, and never
enabled?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> > And apart of remote file names, what other practical use cases can you
>> > think of where disabling locking selectively based on the file name
>> > would make sense?
>> 
>> All file names which match `mounted-file-systems'.
>
> Shouldn't file locking be disabled there by default, and never
> enabled?

Disabling file locking there (and backup files too, perhaps?) would make
sense, but the user should be able to override it.  (They may have
directories called "/media" that's not a mounted file system.)

As for other use cases -- I have, for instance, some automated processes
on several systems that write to the same file, and file locking of that
file has tripped me up before.  I've now rewritten those processes to
work differently, but if `lock-file-name-transforms' had existed before
(and allowed disabling locking based on regexps) I would have used that
instead.

I'm sure there's as many use cases for disabling auto-save/locking based
on regexps as there are for transforming the file names.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > And apart of remote file names, what other practical use cases can you
>> > think of where disabling locking selectively based on the file name
>> > would make sense?
>>
>> All file names which match `mounted-file-systems'.
>
> Shouldn't file locking be disabled there by default, and never
> enabled?

Don't know. At least now, it isn't disabled, and people do not protest.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> I suggested a compromise: disable it by default, but not via the
>> transform function.  I think it's more natural, but even if you
>> disagree, would it do as a compromise?
>
> I don't mind adding a special Tramp variable here (perhaps there should
> be one for auto save file names, too, for symmetry)?
>
> But I do think allowing people to inhibit locks/auto save files on a
> path name basis is good functionality in general, Tramp no Tramp.

Well, there is auto-save-mode, which works over the current buffer. Maybe
we could use something similar for file locks?

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Well, there is auto-save-mode, which works over the current buffer. Maybe
> we could use something similar for file locks?

Yeah, I think that's a good idea (but in addition to disabling based on
file name regexps).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

Hi Lars,

>> Well, there is auto-save-mode, which works over the current buffer. Maybe
>> we could use something similar for file locks?
>
> Yeah, I think that's a good idea (but in addition to disabling based on
> file name regexps).

I've pushed this to master.

Best regards, Michael.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> I've pushed this to master.

Great!  I think we've covered this bug report, then, and I'm closing it.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Eli Zaretskii <eliz@gnu.org>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 21:40:34 +0200
> 
> I've now pushed this change.

The build seems to be broken:

  Finding pointers to doc strings...done
    SCRAPE   ./calendar
  Debugger entered--Lisp error: (void-function make-lock-file-name)
    make-lock-file-name("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
    insert-file-contents("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda..." t)
    find-file-noselect-1(#<buffer cal-loaddefs.el> "/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda..." nil nil "/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda..." (51128996 64768))
    find-file-noselect("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
    autoload-find-generated-file("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
    make-directory-autoloads(("./calendar") "/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
    batch-update-autoloads()
    command-line-1(("-l" "autoload" "--eval" "(setq generate-autoload-cookie \";;;###cal-autoload..." "--eval" "(setq generated-autoload-file (expand-file-name (u..." "-f" "batch-update-autoloads" "./calendar"))
    command-line()
    normal-top-level()
    eval((normal-top-level) t)
    load("loadup.el")

  make[2]: *** [calendar/cal-loaddefs.el] Error 255

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> The build seems to be broken:
>
>   Finding pointers to doc strings...done
>     SCRAPE   ./calendar
>   Debugger entered--Lisp error: (void-function make-lock-file-name)
>     make-lock-file-name("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
>     insert-file-contents("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda..." t)

And I'm seeing a segfault, too, ending with:

Loading /home/larsi/src/emacs/trunk/lisp/cus-face.el (source)...
Loading /home/larsi/src/emacs/trunk/lisp/faces.el (source)...
Fatal error 11: Segmentation fault

I'm investigating.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 22:09:53 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The build seems to be broken:
> >
> >   Finding pointers to doc strings...done
> >     SCRAPE   ./calendar
> >   Debugger entered--Lisp error: (void-function make-lock-file-name)
> >     make-lock-file-name("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda...")
> >     insert-file-contents("/srv/data/home/e/eliz/git/emacs/trunk/lisp/calenda..." t)
> 
> And I'm seeing a segfault, too, ending with:
> 
> Loading /home/larsi/src/emacs/trunk/lisp/cus-face.el (source)...
> Loading /home/larsi/src/emacs/trunk/lisp/faces.el (source)...
> Fatal error 11: Segmentation fault

My stupid typo, should be fixed now.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> Date: Wed, 07 Jul 2021 23:05:33 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: michael.albinus@gmx.de, ncaprisunfan@gmail.com, 49261@debbugs.gnu.org
> 
> > From: Lars Ingebrigtsen <larsi@gnus.org>
> > Cc: Eli Zaretskii <eliz@gnu.org>,  ncaprisunfan@gmail.com,
> >   49261@debbugs.gnu.org
> > Date: Wed, 07 Jul 2021 21:40:34 +0200
> > 
> > I've now pushed this change.
> 
> The build seems to be broken:

Sorry, that was my fault.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The build seems to be broken:
>
> Sorry, that was my fault.

You were right that we needed to make the call-out to Lisp more robust,
so I pushed a change that checks whether the function is defined before
calling it.

After your faces.el fix and that additional check, Emacs now builds for
me again, both incremental and bootstrap.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> After your faces.el fix and that additional check, Emacs now builds for
> me again, both incremental and bootstrap.

Or...  did I speak too soon?  It builds fine on Debian and Windows
(incrementally and bootstrap), but I'm getting this on Macos:

Applications/Xcode.app/Contents/Developer/usr/bin/make -C ../admin/grammars all EMACS="../../src/bootstrap-emacs"
make[3]: Nothing to be done for `all'.
  GEN      loaddefs.el
/bin/sh: line 1: 51261 Killed: 9               EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp -l autoload --eval '(setq autoload-ensure-writable t)' --eval '(setq autoload-builtin-package-versions t)' --eval '(setq generated-autoload-file (expand-file-name (unmsys--file-name "loaddefs.el")))' -f batch-update-autoloads . ./calc ./calendar ./cedet ./cedet/ede ./cedet/semantic ./cedet/semantic/analyze ./cedet/semantic/bovine ./cedet/semantic/decorate ./cedet/semantic/symref ./cedet/semantic/wisent ./cedet/srecode ./emacs-lisp ./emulation ./erc ./eshell ./gnus ./image ./international ./language ./leim ./leim/ja-dic ./leim/quail ./mail ./mh-e ./net ./nxml ./org ./play ./progmodes ./textmodes ./url ./vc
make[2]: *** [loaddefs.el] Error 137
make[1]: *** [../lisp/loaddefs.el] Error 2
make: *** [src] Error 2


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Or...  did I speak too soon?  It builds fine on Debian and Windows
> (incrementally and bootstrap), but I'm getting this on Macos:

Ah, I'm seeing it elsewhere too when saying "touch lisp/*.el":

make[2]: *** [Makefile:279: ../lisp/cus-start.elc] Error 139
make[1]: *** [Makefile:765: ../lisp/cus-start.elc] Error 2
/bin/sh: line 2: 113032 Segmentation fault      (core dumped) EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)' -l bytecomp -f byte-compile-refresh-preloaded -f batch-byte-compile ../lisp/button.el
make[2]: *** [Makefile:279: ../lisp/button.elc] Error 139
make[1]: *** [Makefile:765: ../lisp/button.elc] Error 2
/bin/sh: line 2: 113024 Segmentation fault      (core dumped) EMACSLOADPATH= '../src/bootstrap-emacs' -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)' -l bytecomp -f byte-compile-refresh-preloaded -f batch-byte-compile ../lisp/abbrev.el

Still investigating...  perhaps I got the lifecycle of a variable wrong
or something?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Still investigating...  perhaps I got the lifecycle of a variable wrong
> or something?

It only happens with an -O2 build -- an -O0 build works fine.  Here's
what gdb says:

#0  0x00005555557103a7 in AREF (idx=23456123314108, array=0x7ffff1a398cd)
    at lisp.h:731
#1  HASH_KEY (idx=11728061657054, h=0x7ffff1a39848) at lisp.h:2374
#2  hash_lookup (h=0x7ffff1a39848, key=0x555555cae444, hash=hash@entry=0x0)
    at fns.c:4479
#3  0x000055555573f9f2 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:1415
#4  0x0000555555701d07 in Ffuncall (nargs=2, args=args@entry=0x7fffffffcec8)
    at eval.c:3055
#5  0x000055555573d1c8 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:632
#6  0x0000555555701d07 in Ffuncall (nargs=1, args=args@entry=0x7fffffffd790)
    at eval.c:3055
#7  0x000055555573d1c8 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:632
#8  0x000055555570502f in apply_lambda
    (fun=0x7ffff1a23485, args=<optimized out>, count=4) at eval.c:3188
#9  0x00005555557040b3 in eval_sub (form=<optimized out>) at eval.c:2591
#10 0x0000555555705cd9 in Feval (form=0x7ffff1fc6b23, lexical=<optimized out>)
    at eval.c:2343
#11 0x0000555555700da7 in internal_condition_case
    (bfun=bfun@entry=0x5555556872a0 <top_level_2>, handlers=handlers@entry=0x90, hfun=hfun@entry=0x55555568ccf0 <cmd_error>) at eval.c:1478
#12 0x0000555555687f26 in top_level_1 (ignore=ignore@entry=0x0)
    at keyboard.c:1111
#13 0x00005555557032a3 in internal_catch
    (tag=tag@entry=0xe610, func=func@entry=0x555555687f00 <top_level_1>, arg=arg@entry=0x0) at eval.c:1198
#14 0x0000555555687228 in command_loop () at lisp.h:1002
#15 0x000055555568c906 in recursive_edit_1 () at keyboard.c:720
#16 0x000055555568cc32 in Frecursive_edit () at keyboard.c:789
#17 0x00005555555a286f in main (argc=13, argv=<optimized out>) at emacs.c:2308


Which isn't very useful.

(gdb) xbacktrace 
"command-line-1" (0xffffcee0)
"command-line" (0xffffd7a8)
"normal-top-level" (0xffffdc70)

And this is with all the new file locking stuff disabled.  Very odd.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Hm!

I rewound back to checkout 90c89e8bdeca61aceae79e4c60a9a51800574914, and
then said

make bootstrap; touch lisp/*.el; make

and that segfaults.  So this build failure doesn't seem to be related to
the locking changes.  *phew* So I'm bisecting now back to the offending
commit.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I rewound back to checkout 90c89e8bdeca61aceae79e4c60a9a51800574914, and
> then said
>
> make bootstrap; touch lisp/*.el; make
>
> and that segfaults.  So this build failure doesn't seem to be related to
> the locking changes.  *phew* So I'm bisecting now back to the offending
> commit.

OK, I give up -- I rewound back to October 2020, and it still displays
this behaviour.  I'm guessing the problem arrived on master from a
branch that was merged, but I'm not proficient enough in git to poke
through that stuff.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: Segfault during loadup

[Lars Ingebrigtsen]

To reproduce:

make bootstrap; touch lisp/*.el; make

...
make[2]: *** [Makefile:279: ../lisp/cus-start.elc] Error 139
make[1]: *** [Makefile:765: ../lisp/cus-start.elc] Error 2
/bin/sh: line 2: 113032 Segmentation fault (core dumped)


I bisected this, and git bisect pointed me to:

commit 6cf62141c4467314f67c2ef75a4bf94d41ff050f
Author:     Paul Eggert <eggert@cs.ucla.edu>
AuthorDate: Sat Sep 5 12:13:32 2020 -0700

    Reinstall recent GC-related changes
    
    The report that they broke macOS was a false alarm, as the
    previous commit was also broken (Bug#43152#62).

Running under gdb after the "make" fails:

gdb ./bootstrap-emacs

run -batch --no-site-file --no-site-lisp --eval '(setq load-prefer-newer t)'       -l bytecomp -f byte-compile-refresh-preloaded   -f batch-byte-compile ../lisp/abbrev.elb

#0  0x00005555557103a7 in AREF (idx=23456123314108, array=0x7ffff1a398cd)
    at lisp.h:731
#1  HASH_KEY (idx=11728061657054, h=0x7ffff1a39848) at lisp.h:2374
#2  hash_lookup (h=0x7ffff1a39848, key=0x555555cae444, hash=hash@entry=0x0)
    at fns.c:4479
...

There seems to be a corruption in the hash tables somewhere.  It's
totally reproducible with the recipe, but I haven't found more
self-contained example to reproduce it.

This does not show up with -O0, but does show up with -O1 and -O2.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: Segfault during loadup

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 49261@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Thu, 08 Jul 2021 02:09:45 +0200
> 
> To reproduce:
> 
> make bootstrap; touch lisp/*.el; make
> 
> ...
> make[2]: *** [Makefile:279: ../lisp/cus-start.elc] Error 139
> make[1]: *** [Makefile:765: ../lisp/cus-start.elc] Error 2
> /bin/sh: line 2: 113032 Segmentation fault (core dumped)
> 
> 
> I bisected this, and git bisect pointed me to:
> 
> commit 6cf62141c4467314f67c2ef75a4bf94d41ff050f
> Author:     Paul Eggert <eggert@cs.ucla.edu>
> AuthorDate: Sat Sep 5 12:13:32 2020 -0700
> 
>     Reinstall recent GC-related changes
>     
>     The report that they broke macOS was a false alarm, as the
>     previous commit was also broken (Bug#43152#62).

So the last paragraph above is incorrect, and "the previous commit" is
not broken?  Then what about the information in

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#65
  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#68
  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#71

And the rest of the discussion about verify.h being the culprit?

bug#49261: Segfault during loadup

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So the last paragraph above is incorrect, and "the previous commit" is
> not broken?  Then what about the information in
>
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#65
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#68
>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=43152#71
>
> And the rest of the discussion about verify.h being the culprit?

Hm...  is that for Macos only, though?  I'm currently doing the testing
on Linux (Debian and Fedora, which both displays the same behaviour).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: Segfault during loadup

[Paul Eggert]

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

On 7/7/21 5:09 PM, Lars Ingebrigtsen wrote:
> There seems to be a corruption in the hash tables somewhere.  It's
> totally reproducible with the recipe

Thanks for the recipe; it let me reproduce the problem on Fedora 34 x86-64.

The problem comes from the fact that mark_maybe_pointer works 
differently on pdumper objects than it works on ordinary objects. On 
ordinary objects, roots can point anywhere into an object (because this 
sort of thing has happened on real machines), whereas on pdumper 
objects, roots had to point to the start of the object.

I worked around this particular problem changing mark_maybe_pointer so 
that pdumper roots can also be tagged (see first attached patch). 
However, I suspect this is not a complete fix, as it doesn't cover the 
case where a root points to some part of a pdumper object that is not at 
the object's start. I added a FIXME about this. Perhaps Daniel can take 
a look at it sometime. I think the remaining bug will be hit only rarely 
(if ever).

The second attached patch is in the same area, but is not part of the 
fix. It causes the GC to be a bit more accurate (i.e., less 
conservative) for roots, which can help avoid some leaks.

[-- Attachment #2: 0001-Fix-pdumper-related-GC-bug.patch --]
[-- Type: text/x-patch, Size: 1623 bytes --]

From 2f7afef5ffe023a7a12520201ab70643f826abfd Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Jul 2021 00:27:43 -0700
Subject: [PATCH 1/2] Fix pdumper-related GC bug
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/alloc.c (mark_maybe_pointer): Also mark pointers
to pdumper objects, even when the pointers are tagged.
Add a FIXME saying why this isn’t enough.
---
 src/alloc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/alloc.c b/src/alloc.c
index 76d8c7ddd1..752eaec135 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4755,6 +4755,17 @@ mark_maybe_pointer (void *p)
      definitely _don't_ have an object.  */
   if (pdumper_object_p (p))
     {
+      /* FIXME: This code assumes that every reachable pdumper object
+	 is addressed either by a pointer to the object start, or by
+	 the same pointer with an LSB-style tag.  This assumption
+	 fails if a pdumper object is reachable only via machine
+	 addresses of non-initial object components.  Although such
+	 addressing is rare in machine code generated by C compilers
+	 from Emacs source code, it can occur in some cases.  To fix
+	 this problem, the pdumper code should grok non-initial
+	 addresses, as the non-pdumper code does.  */
+      uintptr_t mask = VALMASK;
+      p = (void *) ((uintptr_t) p & mask);
       /* Don't use pdumper_object_p_precise here! It doesn't check the
          tag bits. OBJ here might be complete garbage, so we need to
          verify both the pointer and the tag.  */
-- 
2.31.1


[-- Attachment #3: 0002-Make-pdumper-marking-pickier.patch --]
[-- Type: text/x-patch, Size: 4037 bytes --]

From f6472cc8e2fdcfd7365240783f34e101fe44142b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Jul 2021 00:54:32 -0700
Subject: [PATCH 2/2] Make pdumper-marking pickier
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Prevent some false-positives in conservative GC marking.
This doesn’t fix any correctness bugs; it’s merely to
reclaim some memory instead of keeping it unnecessarily.
* src/alloc.c (mark_maybe_pointer): New arg SYMBOL_ONLY.
All callers changed.  Check that the pointer’s tag, if any,
matches the pdumper-reported type.
---
 src/alloc.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index 752eaec135..b3668d2131 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4740,7 +4740,7 @@ live_small_vector_p (struct mem_node *m, void *p)
    marked.  */
 
 static void
-mark_maybe_pointer (void *p)
+mark_maybe_pointer (void *p, bool symbol_only)
 {
   struct mem_node *m;
 
@@ -4765,15 +4765,21 @@ mark_maybe_pointer (void *p)
 	 this problem, the pdumper code should grok non-initial
 	 addresses, as the non-pdumper code does.  */
       uintptr_t mask = VALMASK;
-      p = (void *) ((uintptr_t) p & mask);
+      void *po = (void *) ((uintptr_t) p & mask);
+      char *cp = p;
+      char *cpo = po;
       /* Don't use pdumper_object_p_precise here! It doesn't check the
          tag bits. OBJ here might be complete garbage, so we need to
          verify both the pointer and the tag.  */
-      int type = pdumper_find_object_type (p);
-      if (pdumper_valid_object_type_p (type))
-        mark_object (type == Lisp_Symbol
-                     ? make_lisp_symbol (p)
-                     : make_lisp_ptr (p, type));
+      int type = pdumper_find_object_type (po);
+      if (pdumper_valid_object_type_p (type)
+	  && (!USE_LSB_TAG || p == po || cp - cpo == type))
+	{
+	  if (type == Lisp_Symbol)
+	    mark_object (make_lisp_symbol (po));
+	  else if (!symbol_only)
+	    mark_object (make_lisp_ptr (po, type));
+	}
       return;
     }
 
@@ -4791,6 +4797,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_CONS:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Cons *h = live_cons_holding (m, p);
 	    if (!h)
 	      return;
@@ -4800,6 +4808,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_STRING:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_String *h = live_string_holding (m, p);
 	    if (!h)
 	      return;
@@ -4818,6 +4828,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_FLOAT:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Float *h = live_float_holding (m, p);
 	    if (!h)
 	      return;
@@ -4827,6 +4839,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_VECTORLIKE:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Vector *h = live_large_vector_holding (m, p);
 	    if (!h)
 	      return;
@@ -4836,6 +4850,8 @@ mark_maybe_pointer (void *p)
 
 	case MEM_TYPE_VECTOR_BLOCK:
 	  {
+	    if (symbol_only)
+	      return;
 	    struct Lisp_Vector *h = live_small_vector_holding (m, p);
 	    if (!h)
 	      return;
@@ -4897,7 +4913,7 @@ mark_memory (void const *start, void const *end)
   for (pp = start; (void const *) pp < end; pp += GC_POINTER_ALIGNMENT)
     {
       void *p = *(void *const *) pp;
-      mark_maybe_pointer (p);
+      mark_maybe_pointer (p, false);
 
       /* Unmask any struct Lisp_Symbol pointer that make_lisp_symbol
 	 previously disguised by adding the address of 'lispsym'.
@@ -4906,7 +4922,7 @@ mark_memory (void const *start, void const *end)
 	 non-adjacent words and P might be the low-order word's value.  */
       intptr_t ip;
       INT_ADD_WRAPV ((intptr_t) p, (intptr_t) lispsym, &ip);
-      mark_maybe_pointer ((void *) ip);
+      mark_maybe_pointer ((void *) ip, true);
     }
 }
 
-- 
2.31.1

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Cc: 49261@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 11 Jul 2021 01:36:21 -0700
> 
> I worked around this particular problem changing mark_maybe_pointer so 
> that pdumper roots can also be tagged (see first attached patch). 
> However, I suspect this is not a complete fix, as it doesn't cover the 
> case where a root points to some part of a pdumper object that is not at 
> the object's start. I added a FIXME about this. Perhaps Daniel can take 
> a look at it sometime. I think the remaining bug will be hit only rarely 
> (if ever).
> 
> The second attached patch is in the same area, but is not part of the 
> fix. It causes the GC to be a bit more accurate (i.e., less 
> conservative) for roots, which can help avoid some leaks.

Thanks, but these changes don't cater to 32-bit builds --with-wide-int:

    CC       alloc.o
  In file included from alloc.c:33:
  alloc.c: In function 'mark_maybe_pointer':
  lisp.h:251:18: warning: unsigned conversion from 'long long int' to 'uintptr_t' {aka 'unsigned int'} changes value from '2305843009213693951' to '4294967295' [-Woverflow]
    251 | # define VALMASK (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)
	|                  ^
  alloc.c:4767:24: note: in expansion of macro 'VALMASK'
   4767 |       uintptr_t mask = VALMASK;
	|                        ^~~~~~~

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Date: Sun, 11 Jul 2021 13:21:16 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: larsi@gnus.org, 49261@debbugs.gnu.org
> 
> Thanks, but these changes don't cater to 32-bit builds --with-wide-int:
> 
>     CC       alloc.o
>   In file included from alloc.c:33:
>   alloc.c: In function 'mark_maybe_pointer':
>   lisp.h:251:18: warning: unsigned conversion from 'long long int' to 'uintptr_t' {aka 'unsigned int'} changes value from '2305843009213693951' to '4294967295' [-Woverflow]
>     251 | # define VALMASK (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)
> 	|                  ^
>   alloc.c:4767:24: note: in expansion of macro 'VALMASK'
>    4767 |       uintptr_t mask = VALMASK;
> 	|                        ^~~~~~~

I tried to fix this on master, please take a look.

bug#49261: Segfault during loadup

[Paul Eggert]

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

On 7/11/21 8:25 AM, Eli Zaretskii wrote:

>>    lisp.h:251:18: warning: unsigned conversion from 'long long int' to 'uintptr_t' {aka 'unsigned int'} changes value from '2305843009213693951' to '4294967295' [-Woverflow]
>>      251 | # define VALMASK (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)
>> 	|                  ^
>>    alloc.c:4767:24: note: in expansion of macro 'VALMASK'
>>     4767 |       uintptr_t mask = VALMASK;
>> 	|                        ^~~~~~~
> I tried to fix this on master, please take a look.

Yes that GCC warning was bogus, and your pacification of GCC is valid 
now that we no longer tag the MSB of pointers. Still, there should be a 
simpler way to pacify GCC so I installed a further fix that I hope does 
that (see first attached patch). This fix simply uses a cast (uintptr_t) 
VALMASK to pacify GCC; if GCC issues a bogus warning even for that cast, 
we could substitute (uintptr_t) (VALMASK & UINTPTR_MAX) though this is 
starting to get a little ridiculous.

The version of GCC that I tried (11.1.1 20210531 (Red Hat 11.1.1-3)) 
don't warn about the original code, so perhaps the bogus warning that 
you saw is a GCC bug that's been fixed in later GCC versions. However, 
GCC 11.1.1 does warn about some other stuff so I installed the remaining 
patches to pacify it. Some of these patches fix real (albeit unlikely) 
bugs in Emacs. Some work around what are evidently flaws in GCC 11.1.1. 
Oh well.

[-- Attachment #2: 0001-Pacify-gcc-Woverflow-more-nicely.patch --]
[-- Type: text/x-patch, Size: 1121 bytes --]

From da2f772fe575b20bff51b49aa5ded2bf15a2c89d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 11 Jul 2021 23:54:32 -0700
Subject: [PATCH 1/5] Pacify gcc -Woverflow more nicely

* src/alloc.c (mark_maybe_pointer): Simplify pacification
of gcc -Woverflow (unknown GCC version).
---
 src/alloc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/alloc.c b/src/alloc.c
index e3b038c51c..ee3fd64a00 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4764,12 +4764,7 @@ mark_maybe_pointer (void *p, bool symbol_only)
 	 from Emacs source code, it can occur in some cases.  To fix
 	 this problem, the pdumper code should grok non-initial
 	 addresses, as the non-pdumper code does.  */
-#ifdef WIDE_EMACS_INT
-      uintptr_t mask = ~((uintptr_t) 0);
-#else
-      uintptr_t mask = VALMASK;
-#endif
-      void *po = (void *) ((uintptr_t) p & mask);
+      void *po = (void *) ((uintptr_t) p & (uintptr_t) VALMASK);
       char *cp = p;
       char *cpo = po;
       /* Don't use pdumper_object_p_precise here! It doesn't check the
-- 
2.30.2


[-- Attachment #3: 0002-Pacify-gcc-11.1.1-Wanalyzer-null-argument.patch --]
[-- Type: text/x-patch, Size: 7349 bytes --]

From 2337869fbf8b967eb53ee57f978f3751987e43dc Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Jul 2021 00:00:20 -0700
Subject: [PATCH 2/5] Pacify gcc 11.1.1 -Wanalyzer-null-argument
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lib-src/etags.c (regexp): Omit member force_explicit_name,
since it’s always true.  All uses removed.  This lets us
remove calls to strlen (name) where GCC isn’t smart enough
to deduce that name must be nonnull.
* lib-src/movemail.c (main): Fix bug that could cause
link (tempname, NULL) to be called.
* src/emacs.c (argmatch): Break check into two ‘if’s,
since GCC doesn’t seem to be smart enough to check the single ‘if’.
* src/gtkutil.c (xg_update_menu_item): Fix bug where strcmp
could be given a NULL arg.
* src/xfont.c (xfont_list_family): Use nonnull value for dummy
initial value.
---
 lib-src/etags.c    | 49 ++++++++++++++++++----------------------------
 lib-src/movemail.c | 14 +++++--------
 src/emacs.c        |  4 +++-
 src/gtkutil.c      |  2 +-
 src/xfont.c        |  5 ++++-
 5 files changed, 32 insertions(+), 42 deletions(-)

diff --git a/lib-src/etags.c b/lib-src/etags.c
index c39c93db33..88b49f803e 100644
--- a/lib-src/etags.c
+++ b/lib-src/etags.c
@@ -340,7 +340,6 @@ #define xrnew(op, n, m) ((op) = xnrealloc (op, n, (m) * sizeof *(op)))
   struct re_pattern_buffer *pat; /* the compiled pattern */
   struct re_registers regs;	/* re registers */
   bool error_signaled;		/* already signaled for this regexp */
-  bool force_explicit_name;	/* do not allow implicit tag name */
   bool ignore_case;		/* ignore case when matching */
   bool multi_line;		/* do a multi-line match on the whole file */
 } regexp;
@@ -6910,7 +6909,6 @@ add_regex (char *regexp_pattern, language *lang)
   struct re_pattern_buffer *patbuf;
   regexp *rp;
   bool
-    force_explicit_name = true, /* do not use implicit tag names */
     ignore_case = false,	/* case is significant */
     multi_line = false,		/* matches are done one line at a time */
     single_line = false;	/* dot does not match newline */
@@ -6949,7 +6947,8 @@ add_regex (char *regexp_pattern, language *lang)
       case 'N':
 	if (modifiers == name)
 	  error ("forcing explicit tag name but no name, ignoring");
-	force_explicit_name = true;
+	/* This option has no effect and is present only for backward
+	   compatibility.  */
 	break;
       case 'i':
 	ignore_case = true;
@@ -7004,7 +7003,6 @@ add_regex (char *regexp_pattern, language *lang)
   p_head->pat = patbuf;
   p_head->name = savestr (name);
   p_head->error_signaled = false;
-  p_head->force_explicit_name = force_explicit_name;
   p_head->ignore_case = ignore_case;
   p_head->multi_line = multi_line;
 }
@@ -7144,20 +7142,15 @@ regex_tag_multiline (void)
 		name = NULL;
 	      else /* make a named tag */
 		name = substitute (buffer, rp->name, &rp->regs);
-	      if (rp->force_explicit_name)
-		{
-		  /* Force explicit tag name, if a name is there. */
-		  pfnote (name, true, buffer + linecharno,
-			  charno - linecharno + 1, lineno, linecharno);
-
-		  if (debug)
-		    fprintf (stderr, "%s on %s:%"PRIdMAX": %s\n",
-			     name ? name : "(unnamed)", curfdp->taggedfname,
-			     lineno, buffer + linecharno);
-		}
-	      else
-		make_tag (name, strlen (name), true, buffer + linecharno,
-			  charno - linecharno + 1, lineno, linecharno);
+
+	      /* Force explicit tag name, if a name is there. */
+	      pfnote (name, true, buffer + linecharno,
+		      charno - linecharno + 1, lineno, linecharno);
+
+	      if (debug)
+		fprintf (stderr, "%s on %s:%"PRIdMAX": %s\n",
+			 name ? name : "(unnamed)", curfdp->taggedfname,
+			 lineno, buffer + linecharno);
 	      break;
 	    }
 	}
@@ -7471,18 +7464,14 @@ readline (linebuffer *lbp, FILE *stream)
 		name = NULL;
 	      else /* make a named tag */
 		name = substitute (lbp->buffer, rp->name, &rp->regs);
-	      if (rp->force_explicit_name)
-		{
-		  /* Force explicit tag name, if a name is there. */
-		  pfnote (name, true, lbp->buffer, match, lineno, linecharno);
-		  if (debug)
-		    fprintf (stderr, "%s on %s:%"PRIdMAX": %s\n",
-			     name ? name : "(unnamed)", curfdp->taggedfname,
-			     lineno, lbp->buffer);
-		}
-	      else
-		make_tag (name, strlen (name), true,
-			  lbp->buffer, match, lineno, linecharno);
+
+	      /* Force explicit tag name, if a name is there. */
+	      pfnote (name, true, lbp->buffer, match, lineno, linecharno);
+
+	      if (debug)
+		fprintf (stderr, "%s on %s:%"PRIdMAX": %s\n",
+			 name ? name : "(unnamed)", curfdp->taggedfname,
+			 lineno, lbp->buffer);
 	      break;
 	    }
 	}
diff --git a/lib-src/movemail.c b/lib-src/movemail.c
index cfdebccb8d..e683da179d 100644
--- a/lib-src/movemail.c
+++ b/lib-src/movemail.c
@@ -270,6 +270,7 @@ main (int argc, char **argv)
 	 You might also wish to verify that your system is one which
 	 uses lock files for this purpose.  Some systems use other methods.  */
 
+      bool lockname_unlinked = false;
       inname_len = strlen (inname);
       lockname = xmalloc (inname_len + sizeof ".lock");
       strcpy (lockname, inname);
@@ -312,15 +313,10 @@ main (int argc, char **argv)
 	     Five minutes should be good enough to cope with crashes
 	     and wedgitude, and long enough to avoid being fooled
 	     by time differences between machines.  */
-	  if (stat (lockname, &st) >= 0)
-	    {
-	      time_t now = time (0);
-	      if (st.st_ctime < now - 300)
-		{
-		  unlink (lockname);
-		  lockname = 0;
-		}
-	    }
+	  if (!lockname_unlinked
+	      && stat (lockname, &st) == 0
+	      && st.st_ctime < time (0) - 300)
+	    lockname_unlinked = unlink (lockname) == 0 || errno == ENOENT;
 	}
 
       delete_lockname = lockname;
diff --git a/src/emacs.c b/src/emacs.c
index b7982ece64..866e43fda9 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -670,7 +670,9 @@ argmatch (char **argv, int argc, const char *sstr, const char *lstr,
     }
   arglen = (valptr != NULL && (p = strchr (arg, '=')) != NULL
 	    ? p - arg : strlen (arg));
-  if (lstr == 0 || arglen < minlen || strncmp (arg, lstr, arglen) != 0)
+  if (!lstr)
+    return 0;
+  if (arglen < minlen || strncmp (arg, lstr, arglen) != 0)
     return 0;
   else if (valptr == NULL)
     {
diff --git a/src/gtkutil.c b/src/gtkutil.c
index dee2a93089..313cfc82c2 100644
--- a/src/gtkutil.c
+++ b/src/gtkutil.c
@@ -3221,7 +3221,7 @@ xg_update_menu_item (widget_value *val,
       gtk_label_set_text (wkey, utf8_key);
     }
 
-  if (! old_label || strcmp (utf8_label, old_label) != 0)
+  if (utf8_label && (! old_label || strcmp (utf8_label, old_label) != 0))
     {
       label_changed = true;
       gtk_label_set_text (wlbl, utf8_label);
diff --git a/src/xfont.c b/src/xfont.c
index 0570ee96a9..81d356175a 100644
--- a/src/xfont.c
+++ b/src/xfont.c
@@ -596,7 +596,10 @@ xfont_list_family (struct frame *f)
   char **names;
   int num_fonts, i;
   Lisp_Object list;
-  char *last_family UNINIT;
+  char const *last_family;
+#if defined GCC_LINT || defined lint
+  last_family = "";
+#endif
   int last_len;
 
   block_input ();
-- 
2.30.2


[-- Attachment #4: 0003-Pacify-gcc-11.1.1-Wanalyzer-possible-null-dereferenc.patch --]
[-- Type: text/x-patch, Size: 5627 bytes --]

From 1a0fe2a5184cd4c57972994cf4b688042aecc534 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Jul 2021 00:06:34 -0700
Subject: [PATCH 3/5] Pacify gcc 11.1.1 -Wanalyzer-possible-null-dereference
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* oldXMenu/Create.c (XMenuCreate):
* oldXMenu/Internal.c (_XMRecomputePane, _XMRecomputeSelection):
* oldXMenu/XMakeAssoc.c (XMakeAssoc):
* test/src/emacs-module-resources/mod-test.c (Fmod_test_userptr_make):
Don’t assume that malloc and calloc succeed.
---
 oldXMenu/Create.c                          |  2 ++
 oldXMenu/Internal.c                        | 31 +++++++++-------------
 oldXMenu/XMakeAssoc.c                      |  2 ++
 test/src/emacs-module-resources/mod-test.c |  4 +++
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/oldXMenu/Create.c b/oldXMenu/Create.c
index 7eb17c508d..e209bbecee 100644
--- a/oldXMenu/Create.c
+++ b/oldXMenu/Create.c
@@ -598,6 +598,8 @@ XMenuCreate(Display *display, Window parent, register char const *def_env)
    * Create pane, active, and inactive GC's.
    */
   values = (XGCValues *)malloc(sizeof(XGCValues));
+  if (!values)
+    return NULL;
   valuemask = (GCForeground | GCBackground | GCFont | GCLineWidth);
 
   /*
diff --git a/oldXMenu/Internal.c b/oldXMenu/Internal.c
index f489e27bea..3e97f9ab3f 100644
--- a/oldXMenu/Internal.c
+++ b/oldXMenu/Internal.c
@@ -534,7 +534,6 @@ _XMRecomputePane(register Display *display, register XMenu *menu, register XMPan
     register int window_y;	/* Recomputed window Y coordinate. */
 
     unsigned long change_mask;	/* Value mask to reconfigure window. */
-    XWindowChanges *changes;	/* Values to use in configure window. */
 
     register Bool config_p = False;	/* Reconfigure pane window? */
 
@@ -612,21 +611,19 @@ _XMRecomputePane(register Display *display, register XMenu *menu, register XMPan
 	 * it for creation with the new configuration.
 	 */
 	if (p_ptr->window) {
+	    XWindowChanges changes;
 	    change_mask = (CWX | CWY | CWWidth | CWHeight);
-	    changes = (XWindowChanges *)malloc(sizeof(XWindowChanges));
-	    changes->x = p_ptr->window_x;
-	    changes->y = p_ptr->window_y;
-	    changes->width = p_ptr->window_w;
-	    changes->height = p_ptr->window_h;
+	    changes.x = p_ptr->window_x;
+	    changes.y = p_ptr->window_y;
+	    changes.width = p_ptr->window_w;
+	    changes.height = p_ptr->window_h;
 
 	    XConfigureWindow(
 			     display,
 			     p_ptr->window,
 			     change_mask,
-			     changes
+			     &changes
 			     );
-	    free(changes);
-
 	}
 	else {
 	    if (_XMWinQueAddPane(display, menu, p_ptr) == _FAILURE) {
@@ -681,7 +678,6 @@ _XMRecomputeSelection(register Display *display, register XMenu *menu, register
                        		/* Selection sequence number. */
 {
     register Bool config_s = False;	/* Reconfigure selection window? */
-    XWindowChanges *changes;		/* Values to change in configure. */
     unsigned long change_mask;		/* Value mask for XConfigureWindow. */
 
     /*
@@ -738,22 +734,19 @@ _XMRecomputeSelection(register Display *display, register XMenu *menu, register
 	 * for creation with the new configuration.
 	 */
 	if (s_ptr->window) {
-	    changes = (XWindowChanges *)malloc(sizeof(XWindowChanges));
+	    XWindowChanges changes;
 	    change_mask = (CWX | CWY | CWWidth | CWHeight);
-	    changes = (XWindowChanges *)malloc(sizeof(XWindowChanges));
-	    changes->x = s_ptr->window_x;
-	    changes->y = s_ptr->window_y;
-	    changes->width = s_ptr->window_w;
-	    changes->height = s_ptr->window_h;
+	    changes.x = s_ptr->window_x;
+	    changes.y = s_ptr->window_y;
+	    changes.width = s_ptr->window_w;
+	    changes.height = s_ptr->window_h;
 
 	    XConfigureWindow(
 			     display,
 			     s_ptr->window,
 			     change_mask,
-			     changes
+			     &changes
 			     );
-	    free(changes);
-
 	}
 	else {
 	    if (_XMWinQueAddSelection(display, menu, s_ptr) == _FAILURE) {
diff --git a/oldXMenu/XMakeAssoc.c b/oldXMenu/XMakeAssoc.c
index 9bbde2cf94..2530e8e507 100644
--- a/oldXMenu/XMakeAssoc.c
+++ b/oldXMenu/XMakeAssoc.c
@@ -69,6 +69,8 @@ XMakeAssoc(register Display *dpy, register XAssocTable *table, register XID x_id
 	/* before the current value of "Entry". */
 	/* Create a new XAssoc and load it with new provided data. */
 	new_entry = (XAssoc *) malloc(sizeof(XAssoc));
+	if (!new_entry)
+	  return; /* This obsolete API has no way to report failure!  */
 	new_entry->display = dpy;
 	new_entry->x_id = x_id;
 	new_entry->data = data;
diff --git a/test/src/emacs-module-resources/mod-test.c b/test/src/emacs-module-resources/mod-test.c
index ad59cfc18c..5720af8c60 100644
--- a/test/src/emacs-module-resources/mod-test.c
+++ b/test/src/emacs-module-resources/mod-test.c
@@ -288,6 +288,8 @@ Fmod_test_return_unibyte (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
   char large_unused_buffer[512];
 };
 
+static void signal_errno (emacs_env *, char const *);
+
 /* Return a new user-pointer to a super_struct, with amazing_int set
    to the passed parameter.  */
 static emacs_value
@@ -295,6 +297,8 @@ Fmod_test_userptr_make (emacs_env *env, ptrdiff_t nargs, emacs_value args[],
 			void *data)
 {
   struct super_struct *p = calloc (1, sizeof *p);
+  if (!p)
+    signal_errno (env, "calloc");
   p->amazing_int = env->extract_integer (env, args[0]);
   return env->make_user_ptr (env, free, p);
 }
-- 
2.30.2


[-- Attachment #5: 0004-Pacify-gcc-11.1.1-Wclobbered.patch --]
[-- Type: text/x-patch, Size: 1043 bytes --]

From c22cf4d02ff7ebd85839aac5336f6e279f32db54 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Jul 2021 00:07:38 -0700
Subject: [PATCH 4/5] Pacify gcc 11.1.1 -Wclobbered

* src/eval.c (Fprogn, internal_lisp_condition_case):
Add CACHEABLE to work around more instances of -Wclobbered bug.
---
 src/eval.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/eval.c b/src/eval.c
index 18faa0b9b1..b76ced79d6 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -462,7 +462,7 @@ DEFUN ("progn", Fprogn, Sprogn, 0, UNEVALLED, 0,
 usage: (progn BODY...)  */)
   (Lisp_Object body)
 {
-  Lisp_Object val = Qnil;
+  Lisp_Object CACHEABLE val = Qnil;
 
   while (CONSP (body))
     {
@@ -1429,7 +1429,7 @@ internal_lisp_condition_case (Lisp_Object var, Lisp_Object bodyform,
 	}
     }
 
-  Lisp_Object result = eval_sub (bodyform);
+  Lisp_Object CACHEABLE result = eval_sub (bodyform);
   handlerlist = oldhandlerlist;
   if (!NILP (success_handler))
     {
-- 
2.30.2


[-- Attachment #6: 0005-Port-test-module-to-glibc-2.33.patch --]
[-- Type: text/x-patch, Size: 1736 bytes --]

From a79c578f3d77101964b837e8fa8b8109f21c7a88 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Jul 2021 00:11:22 -0700
Subject: [PATCH 5/5] Port test module to glibc 2.33

* test/Makefile.in (REPLACE_FREE, FREE_SOURCE_0, FREE_SOURCE_1):
New macros.
($(test_module)): Improve accuracy of test as to whether free.c
should be compiled; glibc 2.33 does not need it compiled and the
compilation breaks if you try, if you build with
--enable-gcc-warnings.
---
 test/Makefile.in | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/test/Makefile.in b/test/Makefile.in
index c1518d3dcd..7047c24482 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -49,6 +49,8 @@ SEPCHAR =
 
 HAVE_NATIVE_COMP = @HAVE_NATIVE_COMP@
 
+REPLACE_FREE = @REPLACE_FREE@
+
 -include ${top_builddir}/src/verbose.mk
 
 # Load any GNU ELPA dependencies that are present, for optional tests.
@@ -274,6 +276,9 @@ MODULE_CFLAGS =
 test_module = $(test_module_dir)/mod-test${SO}
 src/emacs-module-tests.log src/emacs-module-tests.elc: $(test_module)
 
+FREE_SOURCE_0 =
+FREE_SOURCE_1 = $(srcdir)/../lib/free.c
+
 # In the compilation command, we can't use any object or archive file
 # as source because those are not compiled with -fPIC.  Therefore we
 # use only source files.
@@ -282,7 +287,7 @@ $(test_module): $(test_module:
 	$(AM_V_CCLD)$(CC) -shared $(CPPFLAGS) $(MODULE_CFLAGS) $(LDFLAGS) \
 	  -o $@ $< $(LIBGMP) \
 	  $(and $(GMP_H),$(srcdir)/../lib/mini-gmp-gnulib.c) \
-	  $(if $(OMIT_GNULIB_MODULE_free-posix),,$(srcdir)/../lib/free.c) \
+	  $(FREE_SOURCE_$(REPLACE_FREE)) \
 	  $(srcdir)/../lib/timespec.c $(srcdir)/../lib/gettime.c
 endif
 
-- 
2.30.2

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Cc: larsi@gnus.org, 49261@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 12 Jul 2021 00:16:07 -0700
> 
> >>    lisp.h:251:18: warning: unsigned conversion from 'long long int' to 'uintptr_t' {aka 'unsigned int'} changes value from '2305843009213693951' to '4294967295' [-Woverflow]
> >>      251 | # define VALMASK (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)
> >> 	|                  ^
> >>    alloc.c:4767:24: note: in expansion of macro 'VALMASK'
> >>     4767 |       uintptr_t mask = VALMASK;
> >> 	|                        ^~~~~~~
> > I tried to fix this on master, please take a look.
> 
> Yes that GCC warning was bogus, and your pacification of GCC is valid 

Hmm... is it really a bogus warning?  VALMASK is a 64-bit value, and
uintptr_t is 32-bit wide.

> now that we no longer tag the MSB of pointers. Still, there should be a 
> simpler way to pacify GCC so I installed a further fix that I hope does 
> that (see first attached patch). This fix simply uses a cast (uintptr_t) 
> VALMASK to pacify GCC; if GCC issues a bogus warning even for that cast, 
> we could substitute (uintptr_t) (VALMASK & UINTPTR_MAX) though this is 
> starting to get a little ridiculous.

Your fix compiles cleanly, thanks.

bug#49261: Segfault during loadup

[Paul Eggert]

On 7/12/21 5:07 AM, Eli Zaretskii wrote:

>> Yes that GCC warning was bogus, and your pacification of GCC is valid
> 
> Hmm... is it really a bogus warning?  VALMASK is a 64-bit value, and
> uintptr_t is 32-bit wide.

It's bogus in the sense that 'uintptr_t mask = VALMASK;' has 
well-defined behavior in C; there is no undefined behavior there, since 
VALMASK is an integer and uintptr_t is unsigned. And truncation is what 
is wanted here, so the warning is bogus.

bug#49261: Segfault during loadup

[Andreas Schwab]

The warning isn't bogus.  It points out a potential bug.  That's what
warnings are all about.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Cc: larsi@gnus.org, 49261@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 12 Jul 2021 07:50:41 -0700
> 
> On 7/12/21 5:07 AM, Eli Zaretskii wrote:
> 
> >> Yes that GCC warning was bogus, and your pacification of GCC is valid
> > 
> > Hmm... is it really a bogus warning?  VALMASK is a 64-bit value, and
> > uintptr_t is 32-bit wide.
> 
> It's bogus in the sense that 'uintptr_t mask = VALMASK;' has 
> well-defined behavior in C; there is no undefined behavior there, since 
> VALMASK is an integer and uintptr_t is unsigned. And truncation is what 
> is wanted here, so the warning is bogus.

Then what is the -Woverflow option for?  Can you show an example of
code which -Woverflow would flag that doesn't produce a bogus warning?

bug#49261: Segfault during loadup

[Paul Eggert]

On 7/12/21 10:54 AM, Eli Zaretskii wrote:
> Then what is the -Woverflow option for?  Can you show an example of
> code which -Woverflow would flag that doesn't produce a bogus warning?

Sure: the GCC documentation says -Woverflow is supposed to warn about 
"compile-time overflow in constant expressions". So GCC should (and 
does) warn about this top-level declaration:

int x = INT_MAX + 1;

However, there is no overflow here:

unsigned a = -1, b = INT_MIN, c = LLONG_MAX;

and these declarations have well-defined behavior in C, so -Woverflow 
should not issue warnings for them even though they are unsigned 
conversions that change numeric values.

It might be useful to some programmers to generate warnings about these 
unsigned conversions, but this should be a separate -W option not 
-Woverflow. There's no overflow here.

bug#49261: Segfault during loadup

[Andreas Schwab]

On Jul 13 2021, Paul Eggert wrote:

> However, there is no overflow here:
>
> unsigned a = -1, b = INT_MIN, c = LLONG_MAX;
>
> and these declarations have well-defined behavior in C,

This is a bogus argument.  You can write total bullshit by using only
well-defined C.  And assinging a wider constant to a narrow object,
losing bits, is likely hiding a bug.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#49261: Segfault during loadup

[Paul Eggert]

On 7/14/21 2:42 AM, Andreas Schwab wrote:
> On Jul 13 2021, Paul Eggert wrote:
>
>> However, there is no overflow here:
>>
>> unsigned a = -1, b = INT_MIN, c = LLONG_MAX;
>>
>> and these declarations have well-defined behavior in C,
> This is a bogus argument.

It's not bogus. I quoted the GCC documentation and noted that GCC's 
behavior does not match its documentation. At the very least -- even if 
you like GCC's behavior, which I don't -- the documentation should match 
the behavior.

> assinging a wider constant to a narrow object,
> losing bits, is likely hiding a bug.

If that's what you think -Woverflow should do, then GCC doesn't do that 
either. For example:

unsigned char a = -1, b = 255;

This generates no warning with -Woverflow, even though A's 
initialization assigns a wider constant to a narrow object, losing bits.

Without looking at GCC's source code, it's not clear what -Woverflow 
actually does. It is a bit of a mess. Certainly -Woverflow does not do 
what it's documented to do.

bug#49261: Segfault during loadup

[Andreas Schwab]

On Jul 14 2021, Paul Eggert wrote:

> This generates no warning with -Woverflow, even though A's initialization
> assigns a wider constant to a narrow object, losing bits.

This is always safe.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Cc: larsi@gnus.org, 49261@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 13 Jul 2021 18:12:00 -0500
> 
> On 7/12/21 10:54 AM, Eli Zaretskii wrote:
> > Then what is the -Woverflow option for?  Can you show an example of
> > code which -Woverflow would flag that doesn't produce a bogus warning?
> 
> Sure: the GCC documentation says -Woverflow is supposed to warn about 
> "compile-time overflow in constant expressions". So GCC should (and
> does) warn about this top-level declaration:
> 
> int x = INT_MAX + 1;
> 
> However, there is no overflow here:
> 
> unsigned a = -1, b = INT_MIN, c = LLONG_MAX;

You are saying that there's some fundamental difference between

  INT_MAX + 1

and

  (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)

?  Or between an expression 'x = FOO' and 'mask = BAR'?  I don't see
any fundamental difference.  IMO, the warning was valid, as the
assignment loses significant bits.

bug#49261: Segfault during loadup

[Paul Eggert]

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

On 7/14/21 7:36 AM, Eli Zaretskii wrote:
> You are saying that there's some fundamental difference between
>
>    INT_MAX + 1
>
> and
>
>    (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)

Yes there's a fundamental difference. INT_MAX + 1 has a signed integer 
overflow that violates the C standard. Obviously GCC should diagnose it.

The other expression conforms to the C standard and there is no error or 
overflow there. There's no reason -Woverflow should provoke a diagnostic 
for it.

> Or between an expression 'x = FOO' and 'mask = BAR'?
I don't know what x, mask, FOO, and BAR refer to.

> the warning was valid, as the
> assignment loses significant bits.
I originally wrote it as "uintptr_t mask = VALMASK;" because I would 
rather avoid C casts when possible (they're too powerful and allow too 
many bugs to go undetected). I dislike the workaround that I installed 
because of (a) its unnecessary cast and (b) the lack of clarity that 
it's intended that we want to discard any bits outside UINTPTR_MAX ((b) 
was a problem with my original code too).

To try to fix both (a) and (b) I installed the attached further patch. 
It is a bit more verbose than what C requires, but the verbosity should 
help explain that masking with UINTPTR_MAX is intended, and the 
verbosity shouldn't hurt efficiency.


[-- Attachment #2: 0001-Pacify-gcc-Woverflow-more-clearly.patch --]
[-- Type: text/x-patch, Size: 1077 bytes --]

From 0afbde4e68c1161a54f9593ecb5b66fe42aa0de4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 14 Jul 2021 17:10:06 -0500
Subject: [PATCH] Pacify gcc -Woverflow more clearly

* src/alloc.c (mark_maybe_pointer): Make it clearer that ANDing
with UINTPTR_MAX is intended.  Omit a now-unnecessary cast.
---
 src/alloc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/alloc.c b/src/alloc.c
index ee3fd64a00..8edcd06c84 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -4764,7 +4764,9 @@ mark_maybe_pointer (void *p, bool symbol_only)
 	 from Emacs source code, it can occur in some cases.  To fix
 	 this problem, the pdumper code should grok non-initial
 	 addresses, as the non-pdumper code does.  */
-      void *po = (void *) ((uintptr_t) p & (uintptr_t) VALMASK);
+      uintptr_t mask = VALMASK & UINTPTR_MAX;
+      uintptr_t masked_p = (uintptr_t) p & mask;
+      void *po = (void *) masked_p;
       char *cp = p;
       char *cpo = po;
       /* Don't use pdumper_object_p_precise here! It doesn't check the
-- 
2.25.1

bug#49261: Segfault during loadup

[Eli Zaretskii]

> Cc: larsi@gnus.org, 49261@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 14 Jul 2021 17:24:37 -0500
> 
> I originally wrote it as "uintptr_t mask = VALMASK;" because I would 
> rather avoid C casts when possible (they're too powerful and allow too 
> many bugs to go undetected). I dislike the workaround that I installed 
> because of (a) its unnecessary cast and (b) the lack of clarity that 
> it's intended that we want to discard any bits outside UINTPTR_MAX ((b) 
> was a problem with my original code too).
> 
> To try to fix both (a) and (b) I installed the attached further patch. 
> It is a bit more verbose than what C requires, but the verbosity should 
> help explain that masking with UINTPTR_MAX is intended, and the 
> verbosity shouldn't hurt efficiency.

Thanks, I think this new code is much cleaner and less mysterious.

bug#49261: Segfault during loadup

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I worked around this particular problem changing mark_maybe_pointer so
> that pdumper roots can also be tagged (see first attached
> patch).

Thanks; I can confirm that I can no longer reproduce the crash after
this patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Thu, 08 Jul 2021 00:22:15 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > I rewound back to checkout 90c89e8bdeca61aceae79e4c60a9a51800574914, and
> > then said
> >
> > make bootstrap; touch lisp/*.el; make
> >
> > and that segfaults.  So this build failure doesn't seem to be related to
> > the locking changes.  *phew* So I'm bisecting now back to the offending
> > commit.
> 
> OK, I give up -- I rewound back to October 2020, and it still displays
> this behaviour.  I'm guessing the problem arrived on master from a
> branch that was merged, but I'm not proficient enough in git to poke
> through that stuff.

Is this only on macOS?  If not, on which systems do you see this
crash?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 23:04:29 +0200
> 
> I rewound back to checkout 90c89e8bdeca61aceae79e4c60a9a51800574914, and
> then said
> 
> make bootstrap; touch lisp/*.el; make
> 
> and that segfaults.  So this build failure doesn't seem to be related to
> the locking changes.  *phew* So I'm bisecting now back to the offending
> commit.

To make sure this is indeed segfaults for that commit, I suggest to
clone the upstream repository into a separate directory and repeat the
experiment there.  There could be some left-overs of newer builds that
are not deleted/remade by a rebuild in a tree that is not 100% clean.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> To make sure this is indeed segfaults for that commit, I suggest to
> clone the upstream repository into a separate directory and repeat the
> experiment there.  There could be some left-overs of newer builds that
> are not deleted/remade by a rebuild in a tree that is not 100% clean.

I'm doing a "git clean -xf" between each build, which should remove
everything that's not from upstream, I think?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm doing a "git clean -xf" between each build, which should remove
> everything that's not from upstream, I think?

Just to ensure that there's nothing there, I did:

git clone git://git.savannah.gnu.org/emacs.git
cd emacs
git checkout 6cf62141c4467314f67c2ef75a4bf94d41ff050f
sh autogen.sh; ./configure; make -j16; touch lisp/*.el; make

This segfaults for me reliably on Debian and Fedora.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Thu, 08 Jul 2021 14:44:31 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > To make sure this is indeed segfaults for that commit, I suggest to
> > clone the upstream repository into a separate directory and repeat the
> > experiment there.  There could be some left-overs of newer builds that
> > are not deleted/remade by a rebuild in a tree that is not 100% clean.
> 
> I'm doing a "git clean -xf" between each build, which should remove
> everything that's not from upstream, I think?

Theoretically, yes.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 22:55:08 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Still investigating...  perhaps I got the lifecycle of a variable wrong
> > or something?
> 
> It only happens with an -O2 build -- an -O0 build works fine.  Here's
> what gdb says:
> 
> #0  0x00005555557103a7 in AREF (idx=23456123314108, array=0x7ffff1a398cd)
>     at lisp.h:731
> #1  HASH_KEY (idx=11728061657054, h=0x7ffff1a39848) at lisp.h:2374
> #2  hash_lookup (h=0x7ffff1a39848, key=0x555555cae444, hash=hash@entry=0x0)
>     at fns.c:4479
> #3  0x000055555573f9f2 in exec_byte_code
>     (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
>     at bytecode.c:1415
> #4  0x0000555555701d07 in Ffuncall (nargs=2, args=args@entry=0x7fffffffcec8)
>     at eval.c:3055
> #5  0x000055555573d1c8 in exec_byte_code
>     (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
>     at bytecode.c:632
> #6  0x0000555555701d07 in Ffuncall (nargs=1, args=args@entry=0x7fffffffd790)
>     at eval.c:3055
> #7  0x000055555573d1c8 in exec_byte_code
>     (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
>     at bytecode.c:632
> #8  0x000055555570502f in apply_lambda
>     (fun=0x7ffff1a23485, args=<optimized out>, count=4) at eval.c:3188
> #9  0x00005555557040b3 in eval_sub (form=<optimized out>) at eval.c:2591
> #10 0x0000555555705cd9 in Feval (form=0x7ffff1fc6b23, lexical=<optimized out>)
>     at eval.c:2343
> #11 0x0000555555700da7 in internal_condition_case
>     (bfun=bfun@entry=0x5555556872a0 <top_level_2>, handlers=handlers@entry=0x90, hfun=hfun@entry=0x55555568ccf0 <cmd_error>) at eval.c:1478
> #12 0x0000555555687f26 in top_level_1 (ignore=ignore@entry=0x0)
>     at keyboard.c:1111
> #13 0x00005555557032a3 in internal_catch
>     (tag=tag@entry=0xe610, func=func@entry=0x555555687f00 <top_level_1>, arg=arg@entry=0x0) at eval.c:1198
> #14 0x0000555555687228 in command_loop () at lisp.h:1002
> #15 0x000055555568c906 in recursive_edit_1 () at keyboard.c:720
> #16 0x000055555568cc32 in Frecursive_edit () at keyboard.c:789
> #17 0x00005555555a286f in main (argc=13, argv=<optimized out>) at emacs.c:2308
> 
> 
> Which isn't very useful.
> 
> (gdb) xbacktrace 
> "command-line-1" (0xffffcee0)
> "command-line" (0xffffd7a8)
> "normal-top-level" (0xffffdc70)

Can you show the form being evaluated in frame #10?  Like this:

  (gdb) fr 10
  (gdb) pp form

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Can you show the form being evaluated in frame #10?  Like this:
>
>   (gdb) fr 10
>   (gdb) pp form

Hm:

(gdb) fr 10
#10 0x0000555555706a59 in Feval (form=XIL(0x7ffff20f5023), 
    lexical=<optimized out>) at eval.c:2343
2343	  return unbind_to (count, eval_sub (form));
(gdb) pp form
#<INVALID_LISP_OBJECT 0x7ffff20f5023>

This is with an -O2 build; the bug also reproduces with an -O1 build;
I'll try that...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> This is with an -O2 build; the bug also reproduces with an -O1 build;
> I'll try that...

Here's the -O1 backtrace:

Thread 1 "bootstrap-emacs" received signal SIGSEGV, Segmentation fault.
0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
    at lisp.h:731
731	  return lisp_h_XLP (o);
(gdb) bt
#0  0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
    at lisp.h:731
#1  HASH_KEY (idx=3494065930, h=0x7ffff1bd8f98) at lisp.h:2374
#2  hash_lookup
    (h=0x7ffff1bd8f98, key=XIL(0x555555c76c64), hash=hash@entry=0x0)
    at fns.c:4479
#3  0x0000555555719cb9 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=args_template@entry=make_fixnum(257), nargs=nargs@entry=1, args=<optimized out>, args@entry=0x7fffffffd940) at bytecode.c:1415
#4  0x00005555556e643f in fetch_and_exec_byte_code
    (args=0x7fffffffd940, nargs=1, syms_left=make_fixnum(257), fun=XIL(0x7ffff1bc9895)) at lisp.h:731
#5  funcall_lambda
    (fun=XIL(0x7ffff1bc9895), nargs=nargs@entry=1, arg_vector=arg_vector@entry=0x7fffffffd940) at eval.c:3244
#6  0x00005555556e3bb4 in Ffuncall (nargs=2, args=args@entry=0x7fffffffd938)
    at eval.c:3043
#7  0x0000555555717cb6 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=args_template@entry=make_fixnum(0), nargs=nargs@entry=0, args=<optimized out>, args@entry=0x7fffffffe0e8) at bytecode.c:632
#8  0x00005555556e643f in fetch_and_exec_byte_code
    (args=0x7fffffffe0e8, nargs=0, syms_left=make_fixnum(0), fun=XIL(0x7ffff1bc79ed)) at lisp.h:731
#9  funcall_lambda
    (fun=XIL(0x7ffff1bc79ed), nargs=nargs@entry=0, arg_vector=arg_vector@entry=0x7fffffffe0e8) at eval.c:3244
#10 0x00005555556e3bb4 in Ffuncall (nargs=1, args=args@entry=0x7fffffffe0e0)
    at eval.c:3043
#11 0x0000555555717cb6 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=args_template@entry=make_fixnum(0), nargs=nargs@entry=0, args=<optimized out>, args@entry=0x7fffffffe4a0) at bytecode.c:632
#12 0x00005555556e643f in fetch_and_exec_byte_code
    (args=0x7fffffffe4a0, nargs=0, syms_left=make_fixnum(0), fun=XIL(0x7ffff1bc759d)) at lisp.h:731
#13 funcall_lambda
    (fun=fun@entry=XIL(0x7ffff1bc759d), nargs=nargs@entry=0, arg_vector=arg_vector@entry=0x7fffffffe4a0) at eval.c:3244
#14 0x00005555556e5aec in apply_lambda
    (fun=fun@entry=XIL(0x7ffff1bc759d), args=<optimized out>, count=count@entry=4) at eval.c:3188
#15 0x00005555556e5d64 in eval_sub (form=form@entry=XIL(0x7ffff20f4fe3))
    at eval.c:2561
#16 0x00005555556e76d7 in Feval
--Type <RET> for more, q to quit, c to continue without paging--
    (form=XIL(0x7ffff20f4fe3), lexical=lexical@entry=XIL(0)) at eval.c:2343
#17 0x0000555555670a48 in top_level_2 () at lisp.h:1002
#18 0x00005555556e2f8d in internal_condition_case
    (bfun=bfun@entry=0x555555670a33 <top_level_2>, handlers=handlers@entry=XIL(0x90), hfun=hfun@entry=0x555555673df4 <cmd_error>) at eval.c:1478
#19 0x0000555555670a03 in top_level_1 (ignore=ignore@entry=XIL(0))
    at keyboard.c:1111
#20 0x00005555556e5158 in internal_catch
    (tag=tag@entry=XIL(0xe610), func=func@entry=0x5555556709dd <top_level_1>, arg=arg@entry=XIL(0)) at eval.c:1198
#21 0x0000555555670986 in command_loop () at lisp.h:1002
#22 0x0000555555673a9c in recursive_edit_1 () at keyboard.c:720
#23 0x0000555555673d4d in Frecursive_edit () at keyboard.c:789
#24 0x000055555566fea2 in main (argc=13, argv=0x7fffffffe8b8) at emacs.c:2308

Lisp Backtrace:
"command-line-1" (0xffffd940)
"command-line" (0xffffe0e8)
"normal-top-level" (0xffffe4a0)
(gdb) fr 16
#16 0x00005555556e76d7 in Feval (form=XIL(0x7ffff20f4fe3), 
    lexical=lexical@entry=XIL(0)) at eval.c:2343
2343	  return unbind_to (count, eval_sub (form));
(gdb) pp form
#<INVALID_LISP_OBJECT 0x7ffff20f4fe3>
(gdb) 

Same invalid object.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Thu, 08 Jul 2021 14:42:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can you show the form being evaluated in frame #10?  Like this:
> >
> >   (gdb) fr 10
> >   (gdb) pp form
> 
> Hm:
> 
> (gdb) fr 10
> #10 0x0000555555706a59 in Feval (form=XIL(0x7ffff20f5023), 
>     lexical=<optimized out>) at eval.c:2343
> 2343	  return unbind_to (count, eval_sub (form));
> (gdb) pp form
> #<INVALID_LISP_OBJECT 0x7ffff20f5023>

Which means unfortunately that to see the form one needs to display
the form piecemeal, one member at a time, using the xtype and the
other x* commands...  Let me know if you need more detailed
instructions.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Which means unfortunately that to see the form one needs to display
> the form piecemeal, one member at a time, using the xtype and the
> other x* commands...  Let me know if you need more detailed
> instructions.

Yes, that'd be appreciated.

(gdb) print form
$1 = XIL(0x7ffff211c263)
(gdb) pr form
#<INVALID_LISP_OBJECT 0x7ffff211c263>
(gdb) xtype form
Lisp_Cons
(gdb) xcons form
$2 = (struct Lisp_Cons *) 0x7ffff211c260
{
  u = {
    s = {
      car = XIL(0x2aaa9c41a9e0),
      u = {
        cdr = XIL(0),
        chain = 0x0
      }
    },
    gcaligned = 0xe0
  }
}
(gdb) xcar form
$3 = 0x0
(gdb) xcdr form
$4 = 0x0

Which doesn't seem ... right?

Anyway, the build failures (on the old commit) aren't 100% reproducible
now.  When running the bootstrap under gdb, it now currently just hangs,
so I have to C-c to get the backtrace.  It does hang (always) at the
same place (see below).

So I'm still guessing that there's memory corruption introduced by the
patch, so actually looking at the backtrace may not be very
productive...

Reloading stale subdirs.el
Loading /tmp/emacs/lisp/subdirs.el (source)...
^C
Thread 1 "bootstrap-emacs" hit Breakpoint 1, terminate_due_to_signal (
    sig=sig@entry=2, backtrace_limit=backtrace_limit@entry=40) at emacs.c:378
378	  signal (sig, SIG_DFL);
(gdb) bt
#0  terminate_due_to_signal
    (sig=sig@entry=2, backtrace_limit=backtrace_limit@entry=40) at emacs.c:378
#1  0x0000555555599698 in handle_fatal_signal (sig=sig@entry=2)
    at sysdep.c:1768
#2  0x00005555555996ae in deliver_process_signal
    (handler=0x55555559968d <handle_fatal_signal>, sig=2) at sysdep.c:1726
#3  deliver_fatal_signal (sig=2) at sysdep.c:1774
#4  0x00007ffff59e3140 in <signal handler called> ()
    at /lib/x86_64-linux-gnu/libpthread.so.0
#5  hash_lookup
    (h=0x7ffff1fbe518, key=XIL(0x555555c54b94), hash=hash@entry=0x0)
    at lisp.h:718
#6  0x00005555557392a2 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:1415
#7  0x00005555556fdeb7 in Ffuncall (nargs=2, args=args@entry=0x7fffffffd6b8)
    at eval.c:2809
#8  0x0000555555736a78 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:632
#9  0x00005555556fdeb7 in Ffuncall (nargs=1, args=args@entry=0x7fffffffe040)
--Type <RET> for more, q to quit, c to continue without paging--
    at eval.c:2809
#10 0x0000555555736a78 in exec_byte_code
    (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=<optimized out>, nargs=<optimized out>, args=<optimized out>)
    at bytecode.c:632
#11 0x0000555555700e7f in apply_lambda
    (fun=XIL(0x7ffff1fc26f5), args=<optimized out>, count=4) at eval.c:2942
#12 0x00005555556fff03 in eval_sub (form=<optimized out>) at eval.c:2349
#13 0x0000555555701a29 in Feval
    (form=XIL(0x7ffff211c263), lexical=<optimized out>) at eval.c:2103


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Thu, 08 Jul 2021 15:34:54 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Which means unfortunately that to see the form one needs to display
> > the form piecemeal, one member at a time, using the xtype and the
> > other x* commands...  Let me know if you need more detailed
> > instructions.
> 
> Yes, that'd be appreciated.

Well, you did it (almost) correctly:

> (gdb) xtype form
> Lisp_Cons
> (gdb) xcons form
> $2 = (struct Lisp_Cons *) 0x7ffff211c260
> {
>   u = {
>     s = {
>       car = XIL(0x2aaa9c41a9e0),
>       u = {
>         cdr = XIL(0),
>         chain = 0x0
>       }
>     },
>     gcaligned = 0xe0
>   }
> }
> (gdb) xcar form
> $3 = 0x0
> (gdb) xcdr form
> $4 = 0x0

What you need to do is this:

  (gdb) xtype
  Lisp_Cons
  (gdb) xcons
  $2 = (struct Lisp_Cons *) 0x7ffff211c260
  {
    u = {
      s = {
	car = XIL(0x2aaa9c41a9e0),
	u = {
	  cdr = XIL(0),
	  chain = 0x0
	}
      },
      gcaligned = 0xe0
    }
  }
  (gdb) p $2->u.s.car
  (gdb) xtype

And after the second "xtype" continue with the x* command according to
the type.

Your mistake was to use an argument after xcons, xcar, and xcdr: these
should be invoked without an argument, immediately after printing a
Lisp_Object variable.  Like this:

  (gdb) p form
  (gdb) xcar
  (gdb) p form
  (gdb) xcdr

etc.

> Which doesn't seem ... right?

No, but the real form has a non-nil 'car', see above.  So that does
look valid, at least at this level.  The invalid part probably comes
when you recurse deeper into 'form'.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>   (gdb) p $2->u.s.car
>   (gdb) xtype
>
> And after the second "xtype" continue with the x* command according to
> the type.

Ah, thanks:

gdb) xtype
Lisp_Cons
(gdb) xcons
$2 = (struct Lisp_Cons *) 0x7ffff211c260
{
  u = {
    s = {
      car = XIL(0x2aaa9c41a9e0),
      u = {
        cdr = XIL(0),
        chain = 0x0
      }
    },
    gcaligned = 0xe0
  }
}
(gdb) p $2->u.s.car
$3 = XIL(0x2aaa9c41a9e0)
(gdb) xtype
Lisp_Symbol
(gdb) xsymbol
$4 = (struct Lisp_Symbol *) 0x7ffff1fc26c0
"normal-top-level"
(gdb) 

So it's segfaulting while evaling normal-top-level?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Sat, 10 Jul 2021 18:25:27 +0200
> 
> (gdb) p $2->u.s.car
> $3 = XIL(0x2aaa9c41a9e0)
> (gdb) xtype
> Lisp_Symbol
> (gdb) xsymbol
> $4 = (struct Lisp_Symbol *) 0x7ffff1fc26c0
> "normal-top-level"
> (gdb) 
> 
> So it's segfaulting while evaling normal-top-level?

Yes.  Which is consistent with the backtrace, so this didn't teach us
anything new.

It would be good to try to understand byte code of which function is
being run in frame #3 here:

> Thread 1 "bootstrap-emacs" received signal SIGSEGV, Segmentation fault.
> 0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
>     at lisp.h:731
> 731	  return lisp_h_XLP (o);
> (gdb) bt
> #0  0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
>     at lisp.h:731
> #1  HASH_KEY (idx=3494065930, h=0x7ffff1bd8f98) at lisp.h:2374
> #2  hash_lookup
>     (h=0x7ffff1bd8f98, key=XIL(0x555555c76c64), hash=hash@entry=0x0)
>     at fns.c:4479
> #3  0x0000555555719cb9 in exec_byte_code
>     (bytestr=<optimized out>, vector=<optimized out>, maxdepth=<optimized out>, args_template=args_template@entry=make_fixnum(257), nargs=nargs@entry=1, args=<optimized out>, args@entry=0x7fffffffd940) at bytecode.c:1415
> #4  0x00005555556e643f in fetch_and_exec_byte_code
>     (args=0x7fffffffd940, nargs=1, syms_left=make_fixnum(257), fun=XIL(0x7ffff1bc9895)) at lisp.h:731
> #5  funcall_lambda
>     (fun=XIL(0x7ffff1bc9895), nargs=nargs@entry=1, arg_vector=arg_vector@entry=0x7fffffffd940) at eval.c:3244

The problem is this is an optimized build and many variables are
"optimized out".

Line 1415 of bytecomp.c is here:

        CASE (Bswitch):
          {
            /* TODO: Perhaps introduce another byte-code for switch when the
	       number of cases is less, which uses a simple vector for linear
	       search as the jump table.  */
            Lisp_Object jmp_table = POP;
	    if (BYTE_CODE_SAFE && !HASH_TABLE_P (jmp_table))
              emacs_abort ();
            Lisp_Object v1 = POP;
            ptrdiff_t i;
            struct Lisp_Hash_Table *h = XHASH_TABLE (jmp_table);

            /* h->count is a faster approximation for HASH_TABLE_SIZE (h)
               here. */
            if (h->count <= 5 && !h->test.cmpfn)
              { /* Do a linear search if there are not many cases
                   FIXME: 5 is arbitrarily chosen.  */
		for (i = h->count; 0 <= --i; )
		  if (EQ (v1, HASH_KEY (h, i)))
		    break;
              }
            else
              i = hash_lookup (h, v1, NULL);  <<<<<<<<<<<<<<<<<


Stefan, what code in command-line-1 is likely to produce the Bswitch
byte-code?

Anyway, looking at frame 1, I guess the value of 'idx' is bogus, which
means something is wrong with the hash table.  So maybe we should
audit the part(s) of the offending commit that have something to do
with hash tables?  Also, what kind of hash-table are we looking up
there, is it possible to understand that by looking at the variables
involved in the above code?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It would be good to try to understand byte code of which function is
> being run in frame #3 here:
>
>> Thread 1 "bootstrap-emacs" received signal SIGSEGV, Segmentation fault.
>> 0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
>>     at lisp.h:731
>> 731	  return lisp_h_XLP (o);
>> (gdb) bt
>> #0  0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
>>     at lisp.h:731
>> #1  HASH_KEY (idx=3494065930, h=0x7ffff1bd8f98) at lisp.h:2374
>> #2  hash_lookup
>>     (h=0x7ffff1bd8f98, key=XIL(0x555555c76c64), hash=hash@entry=0x0)
>>     at fns.c:4479

Well, the segfault is in this:

  for (i = HASH_INDEX (h, start_of_bucket); 0 <= i; i = HASH_NEXT (h, i))
    {

and you can see that idx (i.e., i here) is 3494065930, which is a very
unusual value for idx -- it's usually below 2K.  So it can't really be
anything other than a memory corruption issue, I think?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  michael.albinus@gmx.de,
>   ncaprisunfan@gmail.com,  49261@debbugs.gnu.org, Paul Eggert
>  <eggert@cs.ucla.edu>
> Date: Sat, 10 Jul 2021 19:15:53 +0200
> 
> >> 731	  return lisp_h_XLP (o);
> >> (gdb) bt
> >> #0  0x00005555556f0549 in AREF (idx=6988131860, array=XIL(0x7ffff1bd901d))
> >>     at lisp.h:731
> >> #1  HASH_KEY (idx=3494065930, h=0x7ffff1bd8f98) at lisp.h:2374
> >> #2  hash_lookup
> >>     (h=0x7ffff1bd8f98, key=XIL(0x555555c76c64), hash=hash@entry=0x0)
> >>     at fns.c:4479
> 
> Well, the segfault is in this:
> 
>   for (i = HASH_INDEX (h, start_of_bucket); 0 <= i; i = HASH_NEXT (h, i))
>     {
> 
> and you can see that idx (i.e., i here) is 3494065930, which is a very
> unusual value for idx -- it's usually below 2K.  So it can't really be
> anything other than a memory corruption issue, I think?

Yes, but why? and in which hash-table?

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Michael Albinus <michael.albinus@gmx.de>,  ncaprisunfan@gmail.com,
>   49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 18:01:25 +0200
> 
> +(defun auto-save--transform-file-name (filename transforms
> +                                                prefix suffix)
> +  "Transform FILENAME according to TRANSFORMS.
> +See `auto-save-file-name-transforms' for the format of
> +TRANSFORMS.  PREFIX is prepended to the non-directory portion of
> +the resulting file name, and SUFFIX is appended."
> +  (let (result uniq)
> +    ;; Apply user-specified translations
> +    ;; to the file name.
> +    (while (and transforms (not result))
> +      (if (string-match (car (car transforms)) filename)
> +	  (setq result (replace-match (cadr (car transforms)) t nil
> +				      filename)
> +		uniq (car (cddr (car transforms)))))

If this is going to be called from C, it should probably use
save-match-data, because no one will expect that just modifying a file
from some Lisp program could clobber the match data.

Also, do we ever need this during loadup?  Because before files.el is
loaded by loadup, this function will not be available, so
unconditionally calling it from C without protection, not even
safe_call or somesuch, is not safe.

> +static Lisp_Object
> +make_lock_file_name (Lisp_Object fn)
> +{
> +  return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn));
> +}

I'd prefer not to have a function return an encoded file name, it's
unusual and unexpected.  It is better to leave that to the caller.
(And if you do that, the rationale for having a separate function for
this will all but disappear, I think.)

> -  fn = Fexpand_file_name (fn, Qnil);
> +  fn = make_lock_file_name (Fexpand_file_name (fn, Qnil));

In the original code, 'fn' was an un-encoded file name, but your
changes made it encoded.  Why not keep the code more similar by having
a separate variable with the encoded file name?  E.g., this would
avoid potential trouble here:

>    if (!NILP (subject_buf)
>        && NILP (Fverify_visited_file_modtime (subject_buf))
>        && !NILP (Ffile_exists_p (fn)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<

Ffile_exists_p was passed an un-encoded file name in the original
code.  It calls file handlers, and encodes local file names by itself,
so it is better to pass it un-encoded file names.

> -      && !(lfname && current_lock_owner (NULL, lfname) == -2))
> +      && !(create_lockfiles && current_lock_owner (NULL, lfname) == -2))
>      call1 (intern ("userlock--ask-user-about-supersession-threat"), fn);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Likewise here: Lisp function should generally be called with
un-encoded file names.

>  unlock_file_body (Lisp_Object fn)
>  {
>    char *lfname;
> -  USE_SAFE_ALLOCA;
> -
> -  Lisp_Object filename = Fexpand_file_name (fn, Qnil);
> -  fn = ENCODE_FILE (filename);
>  
> -  MAKE_LOCK_NAME (lfname, fn);
> +  Lisp_Object filename = make_lock_file_name (Fexpand_file_name (fn, Qnil));
> +  lfname = SSDATA (filename);
>  
>    int err = current_lock_owner (0, lfname);
>    if (err == -2 && unlink (lfname) != 0 && errno != ENOENT)
> @@ -736,7 +714,6 @@ unlock_file_body (Lisp_Object fn)
>    if (0 < err)
>      report_file_errno ("Unlocking file", filename, err);

Same problem here: 'filename' is now an encoded file name, so you call
report_file_errno with an encoded file name, which is a no-no.

Last, but not least: do we care that now locking a file will cons
strings, even with the default value of lock-file-name-transforms?
That sounds like we are punishing the vast majority of users for the
benefit of the few who need the opt-in behavior.  Should we perhaps
avoid consing strings, or maybe even calling Lisp altogether, under
the default value of that option?

Thanks.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> If this is going to be called from C, it should probably use
> save-match-data, because no one will expect that just modifying a file
> from some Lisp program could clobber the match data.

Right; now added.

> Also, do we ever need this during loadup?  Because before files.el is
> loaded by loadup, this function will not be available, so
> unconditionally calling it from C without protection, not even
> safe_call or somesuch, is not safe.

I'll try doing a "make bootstrap"...

>> +static Lisp_Object
>> +make_lock_file_name (Lisp_Object fn)
>> +{
>> +  return ENCODE_FILE (call1 (intern ("make-lock-file-name"), fn));
>> +}
>
> I'd prefer not to have a function return an encoded file name, it's
> unusual and unexpected.  It is better to leave that to the caller.
> (And if you do that, the rationale for having a separate function for
> this will all but disappear, I think.)

Right.  But it seemed like all the callers wanted an encoded file name
here, so it was marginally cleaner.

>> -  fn = Fexpand_file_name (fn, Qnil);
>> +  fn = make_lock_file_name (Fexpand_file_name (fn, Qnil));
>
> In the original code, 'fn' was an un-encoded file name, but your
> changes made it encoded.  Why not keep the code more similar by having
> a separate variable with the encoded file name?  E.g., this would
> avoid potential trouble here:

Yup; I've now rewritten this to not reuse variables in this way, because
it was pretty confusing.  (See the version of the patch I posted some
minutes ago.)

>>    if (!NILP (subject_buf)
>>        && NILP (Fverify_visited_file_modtime (subject_buf))
>>        && !NILP (Ffile_exists_p (fn)) <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
>
> Ffile_exists_p was passed an un-encoded file name in the original
> code.  It calls file handlers, and encodes local file names by itself,
> so it is better to pass it un-encoded file names.

[...]

> Same problem here: 'filename' is now an encoded file name, so you call
> report_file_errno with an encoded file name, which is a no-no.

Right; I'll fix up the un-encoded/encoded file name confusion here.

> Last, but not least: do we care that now locking a file will cons
> strings, even with the default value of lock-file-name-transforms?
> That sounds like we are punishing the vast majority of users for the
> benefit of the few who need the opt-in behavior.  Should we perhaps
> avoid consing strings, or maybe even calling Lisp altogether, under
> the default value of that option?

Hm...  I think for simplicity's sake, it makes sense to always call the
Lisp code.  Having two places where we insert ".#" into a file name just
seems error prone, long term.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Also, do we ever need this during loadup?  Because before files.el is
>> loaded by loadup, this function will not be available, so
>> unconditionally calling it from C without protection, not even
>> safe_call or somesuch, is not safe.
>
> I'll try doing a "make bootstrap"...

And that's finished now, and went without a hitch with the new patch.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 20:17:22 +0200
> 
> > Also, do we ever need this during loadup?  Because before files.el is
> > loaded by loadup, this function will not be available, so
> > unconditionally calling it from C without protection, not even
> > safe_call or somesuch, is not safe.
> 
> I'll try doing a "make bootstrap"...

Even if it currently works, it's unsafe, IMO, to not have any
protection there.  No one will remember that we rely on this not being
called early enough.

> > Last, but not least: do we care that now locking a file will cons
> > strings, even with the default value of lock-file-name-transforms?
> > That sounds like we are punishing the vast majority of users for the
> > benefit of the few who need the opt-in behavior.  Should we perhaps
> > avoid consing strings, or maybe even calling Lisp altogether, under
> > the default value of that option?
> 
> Hm...  I think for simplicity's sake, it makes sense to always call the
> Lisp code.  Having two places where we insert ".#" into a file name just
> seems error prone, long term.

My point is that this simplicity comes at a price.  We've been
consistently moving code from C primitives to Lisp, and by doing that
significantly increase the consing during even the simplest editing
operations.  All this consing adds up, with the result that Emacs
nowadays produces much more garbage, which then triggers frequent GC
cycles, which slow down Emacs.  It's a small wonder we see so many
people out there raising the GC threshold to dangerous levels.

So I'm asking whether the simplicity justifies the costs, here and
elsewhere.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Even if it currently works, it's unsafe, IMO, to not have any
> protection there.  No one will remember that we rely on this not being
> called early enough.

Right.  My reasoning was that it seemed unlikely to be a problem here,
since the same function also (possibly) calls the Lisp functions
userlock--ask-user-about-supersession-threat and ask-user-about-lock.
But those are only called in some cases, so we're calling out to Lisp
more now than before, and that may indeed be a problem.

I can add a check to whether the Lisp function is defined before calling
(and then not do locking if it isn't) to be more defensive here?

> My point is that this simplicity comes at a price.  We've been
> consistently moving code from C primitives to Lisp, and by doing that
> significantly increase the consing during even the simplest editing
> operations.  All this consing adds up, with the result that Emacs
> nowadays produces much more garbage, which then triggers frequent GC
> cycles, which slow down Emacs.  It's a small wonder we see so many
> people out there raising the GC threshold to dangerous levels.
>
> So I'm asking whether the simplicity justifies the costs, here and
> elsewhere.

I agree with you 100% that it's important to take this into
consideration when moving things from C to Lisp.  In this particular
case, I think the cost is justified, because this isn't a function
that's called in a loop, and the other things it does (calling
Fverify_visited_file_modtime and Ffile_exists_p, and actually creating
the lock file) totally swamps any extra string consing, I think.  (That
is, it won't take measurably longer.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I can add a check to whether the Lisp function is defined before calling
> (and then not do locking if it isn't) to be more defensive here?

Ah, I didn't notice that lock_file starts like this:

  /* Don't do locking while dumping Emacs.
     Uncompressing wtmp files uses call-process, which does not work
     in an uninitialized Emacs.  */
  if (will_dump_p ())
    return;

So I think we're basically safe...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 20:58:29 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Even if it currently works, it's unsafe, IMO, to not have any
> > protection there.  No one will remember that we rely on this not being
> > called early enough.
> 
> Right.  My reasoning was that it seemed unlikely to be a problem here,
> since the same function also (possibly) calls the Lisp functions
> userlock--ask-user-about-supersession-threat and ask-user-about-lock.
> But those are only called in some cases, so we're calling out to Lisp
> more now than before, and that may indeed be a problem.

Right, the call to userlock--ask-user-about-supersession-threat is
under some conditions that are rarely satisfied.

> I can add a check to whether the Lisp function is defined before calling
> (and then not do locking if it isn't) to be more defensive here?

I guess so.

> > So I'm asking whether the simplicity justifies the costs, here and
> > elsewhere.
> 
> I agree with you 100% that it's important to take this into
> consideration when moving things from C to Lisp.  In this particular
> case, I think the cost is justified, because this isn't a function
> that's called in a loop, and the other things it does (calling
> Fverify_visited_file_modtime and Ffile_exists_p, and actually creating
> the lock file) totally swamps any extra string consing, I think.  (That
> is, it won't take measurably longer.)

If it triggers GC, it _will_ take significantly longer.  But I won't
argue more about this, it's a lost battle anyway with current Emacs
development trends.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  ncaprisunfan@gmail.com,  49261@debbugs.gnu.org
> Date: Wed, 07 Jul 2021 20:17:22 +0200
> 
> Hm...  I think for simplicity's sake, it makes sense to always call the
> Lisp code.  Having two places where we insert ".#" into a file name just
> seems error prone, long term.

Btw, we could avoid having 2 places that produces the default
".#"+FILENAME lock file name by exposing to Lisp a C primitive which
does that.  So that problem can easily be solved.

bug#49261: 28.0.50; File Locking Breaks Presumptuous Toolchains

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Btw, we could avoid having 2 places that produces the default
> ".#"+FILENAME lock file name by exposing to Lisp a C primitive which
> does that.  So that problem can easily be solved.

Yes, that's definitely true.  We could even make it more general and
then use it, for instance, in the auto-save code path, too.

That is, with a signature of, say...

(file-name-surround file-name prefix &optional suffix)

(file-name-surround "/tmp/foo.txt" "#" "#")
=> "/tmp/#foo.txt#"

It'd be less consing than the current

(concat (file-name-directory filename) "#" (file-name-nondirectory filename) "#")

code we have.

A better name would be nice, though.  :-)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no