From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Andrea Corallo via "Bug reports for GNU Emacs, the Swiss army knife of text editors" Newsgroups: gmane.emacs.bugs Subject: bug#43725: 28.0.50; Include feature/native-comp into master Date: Tue, 16 Feb 2021 21:13:07 +0000 Message-ID: References: <87wny8xwcc.fsf@gnus.org> <83im9sqk1b.fsf@gnu.org> <83v9drp8va.fsf@gnu.org> <831rdjd95w.fsf@gnu.org> Reply-To: Andrea Corallo Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="34175"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: larsi@gnus.org, monnier@iro.umontreal.ca, 43725@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Feb 16 22:14:11 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lC7fd-0008m9-QV for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 16 Feb 2021 22:14:09 +0100 Original-Received: from localhost ([::1]:59452 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lC7fc-0004xf-TU for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 16 Feb 2021 16:14:08 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:46448) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lC7fW-0004we-83 for bug-gnu-emacs@gnu.org; Tue, 16 Feb 2021 16:14:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:58481) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lC7fW-0002Eu-0b for bug-gnu-emacs@gnu.org; Tue, 16 Feb 2021 16:14:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1lC7fV-0006eX-Si for bug-gnu-emacs@gnu.org; Tue, 16 Feb 2021 16:14:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Andrea Corallo Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 16 Feb 2021 21:14:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 43725 X-GNU-PR-Package: emacs Original-Received: via spool by 43725-submit@debbugs.gnu.org id=B43725.161350999225508 (code B ref 43725); Tue, 16 Feb 2021 21:14:01 +0000 Original-Received: (at 43725) by debbugs.gnu.org; 16 Feb 2021 21:13:12 +0000 Original-Received: from localhost ([127.0.0.1]:41794 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lC7eh-0006dM-LF for submit@debbugs.gnu.org; Tue, 16 Feb 2021 16:13:11 -0500 Original-Received: from mx.sdf.org ([205.166.94.24]:51731) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1lC7ef-0006dD-K1 for 43725@debbugs.gnu.org; Tue, 16 Feb 2021 16:13:10 -0500 Original-Received: from mab (ma.sdf.org [205.166.94.33]) by mx.sdf.org (8.15.2/8.14.5) with ESMTPS id 11GLD7cc025090 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits) verified NO); Tue, 16 Feb 2021 21:13:08 GMT In-Reply-To: <831rdjd95w.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 13 Feb 2021 22:06:51 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:200158 Archived-At: Addressing some easy part of the review to close the day. Eli Zaretskii writes: [...] > 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)]; <<<<<<<<<<<<<<<<<<<< > +} Fixed by 543e6e664c > 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.) Should we never trust in C a value coming from a Lisp_Object even if is supposed to be constructed on purpose? > Several comparisons like this one: > > + if (val != (long) val) > > are IMO better written as > > if (val > LONG_MAX || val < LONG_MIN) Fixed by 72e4a22391 > 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); > + } This way we can have insns longer than 6 operands but we don't load them. These are tipically comment insn we use as a debug note therefore not relevant here (code generation). > 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. IIRC the compilation was crashing. Actually we should be able to get rid of this piece of code. ATM we always run in a non interactive (typically child) process compilations so this code is not exercised anymore. Removed by 21858596f0 > 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++) Fixed by 7b676861dd Thanks Andrea