Re: Emacs port to gcc -fcheck-pointer-bounds

[Paul Eggert <eggert@cs.ucla.edu>]

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

On 12/11/2017 07:26 AM, Eli Zaretskii wrote:
> So we will have two different data types implementing a Lisp object,
> one --with-wide-int and another without it?  That sounds suboptimal to
> me.

I figured out a simple fix to get it to work --with-wide-int (see first 
attached patch), so I hope this issue is moot now, and boldly installed 
the patches into master, along with the attached followup patches.

> Why not reserve the new representation to the -fcheck-pointer-bounds
> builds only?

Although we could change lisp.h to do that, I'd rather not. Having 
Lisp_Object be an opaque pointer type is a win even for non-developers 
who merely make minor changes to the C code and are not using 
-fcheck-pointer-bounds or --enable-check-lisp-object-type or anything 
like that. In practice it's error-prone when Lisp_Object is an integer 
type, and if we can easily avoid these errors we should.

Here's another way to think about it. Our current practice of defaulting 
to --enable-check-lisp-object-type for developers is an outgrowth of the 
fact that integer Lisp_Objects are so error-prone. Unfortunately, this 
practice is dicey in its own right, as it means developers are dealing 
with different object code than non-developers. I would favor going back 
to the old practice of disabling --enable-check-lisp-object-type by 
default, even for developers, once we've shaken out the change that I 
just installed. That way, developers and non-developers will default to 
more-similar machine code.

[-- Attachment #2: 0001-Port-fcheck-pointer-bounds-to-with-wide-int.patch --]
[-- Type: text/x-patch, Size: 1893 bytes --]

From e921f97df9a11fd6f43ee040ba97c686c3fa62ee Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 12 Dec 2017 12:59:27 -0800
Subject: [PATCH 1/2] Port --fcheck-pointer-bounds to --with-wide-int

* src/lisp (XSYMBOL) [__CHKP__ && !USE_LSB_TAG]:
Bypass pointer bounds checking here,
instead of failing the entire build.
(make_lisp_symbol): Improve comment.
---
 src/lisp.h | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/lisp.h b/src/lisp.h
index 56545b7094..eb31ba209a 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -902,12 +902,15 @@ INLINE struct Lisp_Symbol *
 {
 #if USE_LSB_TAG
   return lisp_h_XSYMBOL (a);
-#elif defined __CHKP__
-# error "pointer-checking not supported with wide integers"
 #else
   eassert (SYMBOLP (a));
   intptr_t i = (intptr_t) XUNTAG (a, Lisp_Symbol);
   void *p = (char *) lispsym + i;
+# ifdef __CHKP__
+  /* Bypass pointer checking.  Although this could be improved it is
+     probably not worth the trouble.  */
+  p = __builtin___bnd_set_ptr_bounds (p, sizeof (struct Lisp_Symbol));
+# endif
   return p;
 #endif
 }
@@ -916,9 +919,11 @@ INLINE Lisp_Object
 make_lisp_symbol (struct Lisp_Symbol *sym)
 {
 #ifdef __CHKP__
-  /* Although this should use '__builtin___bnd_narrow_ptr_bounds (sym,
-     sym, sizeof *sym)', that would run afoul of GCC bug 83251
-     <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83251>.  */
+  /* Although '__builtin___bnd_narrow_ptr_bounds (sym, sym, sizeof *sym)'
+     should be more efficient, it runs afoul of GCC bug 83251
+     <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83251>.
+     Also, attempting to call __builtin___bnd_chk_ptr_bounds (sym, sizeof *sym)
+     here seems to trigger a GCC bug, as yet undiagnosed.  */
   char *addr = __builtin___bnd_set_ptr_bounds (sym, sizeof *sym);
   char *symoffset = addr - (intptr_t) lispsym;
 #else
-- 
2.14.3


[-- Attachment #3: 0002-Fix-recently-introduced-cast-typo.patch --]
[-- Type: text/x-patch, Size: 966 bytes --]

From 8e78d49765993bbbb93d42b0530f5ffaa4e759f4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 12 Dec 2017 15:15:56 -0800
Subject: [PATCH 2/2] Fix recently-introduced cast typo

* src/alloc.c (SDATA_OF_STRING): Put cast in right spot.
This matters only if GC_CHECK_STRING_BYTES, which is sort
of a coals-to-Newcastle situation if pointer bounds checking
is also enabled.
---
 src/alloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/alloc.c b/src/alloc.c
index 9197ff12ef..fb0f948474 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -1727,7 +1727,7 @@ static EMACS_INT total_string_bytes;
    a pointer to the `u.data' member of its sdata structure; the
    structure starts at a constant offset in front of that.  */
 
-#define SDATA_OF_STRING(S) ptr_bounds_init ((sdata *) ((S)->u.s.data \
+#define SDATA_OF_STRING(S) ((sdata *) ptr_bounds_init ((S)->u.s.data \
 						       - SDATA_DATA_OFFSET))
 
 
-- 
2.14.3