bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#43725: 28.0.50; Include feature/native-comp into master


From: Eli Zaretskii
Subject: bug#43725: 28.0.50; Include feature/native-comp into master
Date: Sat, 13 Feb 2021 22:06:51 +0200

Andrea, can you please explain why there are so many changes in Lisp
files on the branch?

For example, what are changes like this one about:

  +(declare-function subr-native-lambda-list "data.c")

Or this:

  +   ((and (featurep 'nativecomp)
  +         (subrp def)
  +         (listp (subr-native-lambda-list def)))
  +    (subr-native-lambda-list def))

Or this:

  +  ;; Never native compile to allow cc-defs.el:2345 hack.
  +  (declare (speed -1))

This will not work on MS-Windows, you need to use path-separator to do
it portably:

  +    (when (featurep 'nativecomp)
  +      (defvar comp-eln-load-path)
  +      (let ((path-env (getenv "EMACSNATIVELOADPATH")))
  +        (when path-env
  +          (dolist (path (split-string path-env ":")) <<<<<<<<<<<<<
  +            (unless (string= "" path)
  +              (push path comp-eln-load-path)))))
  +      (push (concat user-emacs-directory "eln-cache/") comp-eln-load-path))

I'm not sure I understand this addition to epaths.nt:

  +/* Like PATH_LOADSEARCH, but contains the relative path from the
  +   installation directory.
  +*/
  +#define PATH_REL_LOADSEARCH ""

Can you explain how PATH_REL_LOADSEARCH is supposed to work, and for
what purpose it was introduced?

A few minor comments about the C code:

This is unsafe, because a fixnum can be larger than PTRDIFF_MAX:

  +static gcc_jit_lvalue *
  +emit_mvar_lval (Lisp_Object mvar)
  +{
  +  Lisp_Object mvar_slot = CALL1I (comp-mvar-slot, mvar);
  +
  +  if (EQ (mvar_slot, Qscratch))
  +    {
  +      if (!comp.scratch)
  +       comp.scratch = gcc_jit_function_new_local (comp.func,
  +                                                  NULL,
  +                                                  comp.lisp_obj_type,
  +                                                  "scratch");
  +      return comp.scratch;
  +    }
  +
  +  return comp.frame[XFIXNUM (mvar_slot)];  <<<<<<<<<<<<<<<<<<<<
  +}

Likewise, this is unsafe because a fixnum can be larger than INT_MAX:

  +  if (!FIXNUMP (idx))
  +    xsignal1 (Qnative_ice,
  +             build_string ("inconsistent data relocation container"));
  +  reloc.idx = gcc_jit_context_new_rvalue_from_int (comp.ctxt,
  +                                                  comp.ptrdiff_type,
  +                                                  XFIXNUM (idx)); <<<<<<<<

(There are several more calls with the same problem.)

Several comparisons like this one:

  +  if (val != (long) val)

are IMO better written as

  if (val > LONG_MAX || val < LONG_MIN)

Here, wouldn't it be better to have an assertion that there are no
more than 6 elements in the list:

  +  Lisp_Object arg[6];
  +
  +  Lisp_Object p = XCDR (insn);
  +  ptrdiff_t i = 0;
  +  FOR_EACH_TAIL (p)
  +    {
  +      if (i == sizeof (arg) / sizeof (Lisp_Object))
  +       break;
  +      arg[i++] = XCAR (p);
  +    }

If there are more than 6, we will be writing beyond the end of the
arg[] array.

This is nonportable:

  +  if (!noninteractive)
  +    {
  +      sigset_t blocked;
  +      /* Gcc doesn't like being interrupted at all.  */
  +      block_input ();
  +      sigemptyset (&blocked);
  +      sigaddset (&blocked, SIGALRM);
  +      sigaddset (&blocked, SIGINT);
  +#ifdef USABLE_SIGIO
  +      sigaddset (&blocked, SIGIO);
  +#endif
  +      pthread_sigmask (SIG_BLOCK, &blocked, &saved_sigset); <<<<<<<<<<<
  +      count = SPECPDL_INDEX ();
  +      record_unwind_protect_void (restore_sigmask);
  +    }

We shouldn't use pthread_sigmask unconditionally, we should use it
only on Posix platforms.  Can you explain why the signals here should
be blocked?  What happens if they aren't, and a signal arrives while
the compilation runs?  I'm asking because on MS-Windows blocking
signals with sigaddset/sigmask doesn't really work, so the question is
what if anything should be done here on Windows.

Here, 'i' could be ptrdiff_t, no need to use EMACS_INT:

  +  EMACS_INT d_vec_len = XFIXNUM (Flength (comp_u->data_vec));
  +  for (EMACS_INT i = 0; i < d_vec_len; i++)
  +    if (!EQ (data_relocs[i],  AREF (comp_u->data_vec, i)))
  +      return false;
  +
  +  d_vec_len = XFIXNUM (Flength (comp_u->data_impure_vec));
  +  for (EMACS_INT i = 0; i < d_vec_len; i++)

This should have a big FIXME, since it means the name of the DLL is in
two different places, and if we need to support a differently-named
DLL, we are in trouble:

  +#ifdef WINDOWSNT
  +  /* We may need to load libgccjit when dumping before term/w32-win.el
  +     defines `dynamic-library-alist`. This will fail if that variable
  +     is empty, so add libgccjit-0.dll to it.  */
  +  if (will_dump_p ())
  +    Vdynamic_library_alist = list1 (list2 (Qgccjit,
  +                                           build_string 
("libgccjit-0.dll")));

Is there a more elegant way of resolving this difficulty?

This lacks the ENCODE_FILE part:

  +           char *fname = SSDATA (concat2 (Vinvocation_directory,
  +                                          XCAR (comp_u->file)));
  +           FILE *file;
  +           if ((file = fopen (fname, "r")))

And why are you using fopen instead of emacs_fopen?





reply via email to

[Prev in Thread] Current Thread [Next in Thread]