Emacs port to gcc -fcheck-pointer-bounds

Emacs port to gcc -fcheck-pointer-bounds

[Paul Eggert]

In <https://debbugs.gnu.org/29600> I published patches to port Emacs to the 
-fcheck-pointer-bounds option of GCC, so that I can debug Emacs with hardware 
pointer bounds checking on platforms that support it (such as the Kaby Lake chip 
in my year-old laptop running Ubuntu 17.10). This entails changing the 
fundamental Emacs internal word from an integer to a pointer of the same width - 
which is not as big a deal as one might think, as the commonly-used EMACS_INT 
type does not change and Emacs users and Emacs Lisp programmers should not 
notice any change.

I would like to install these patches on 'master' soon, and am mentioning this 
on emacs-devel to give a heads-up to the few but hardy volunteers who work on 
the low-level part of the Emacs implementation.

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

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 6 Dec 2017 23:34:54 -0800
> Cc: Pip Cet <pipcet@gmail.com>
> 
> In <https://debbugs.gnu.org/29600> I published patches to port Emacs to the 
> -fcheck-pointer-bounds option of GCC, so that I can debug Emacs with hardware 
> pointer bounds checking on platforms that support it (such as the Kaby Lake chip 
> in my year-old laptop running Ubuntu 17.10). This entails changing the 
> fundamental Emacs internal word from an integer to a pointer of the same width - 
> which is not as big a deal as one might think, as the commonly-used EMACS_INT 
> type does not change and Emacs users and Emacs Lisp programmers should not 
> notice any change.

Thanks.

It's a large patch, so I think the discussion could benefit from an
overview of the main points of the implementation.

In particular, how would this work in a build --with-wide-int?

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

[Paul Eggert]

On 12/08/2017 05:45 AM, Eli Zaretskii wrote:
> It's a large patch, so I think the discussion could benefit from an
> overview of the main points of the implementation.
>
> In particular, how would this work in a build --with-wide-int?

The short answer is, "It doesn't"; this is enforced by an '# error 
"pointer-checking not supported with wide integers"' in src/lisp.h. The 
longer answer is that I could add support for it, but I doubt whether 
it's worth the trouble.

The basic idea of -fcheck-pointer-bounds is that you cannot lie to GCC 
that a pointer is an integer: if a value is intended to be a pointer you 
must declare it to be a pointer. You are allowed to calculate an 
out-of-range pointer so long as you don't use it, and you are allowed to 
cast it to some other type or to void *. The hardware/software 
combination checks the bounds some (but not all, alas) pointer 
dereferencing operations. The idea is to catch the most common cases 
that might be victimized.

The -mmpx implementation does not change the size of pointers: they are 
still 8 bytes on x86-64 and are still 4 bytes on x86. Instead, the 
hardware keeps a cache that maps addresses of pointers to the pointers' 
bounds. The compiler generates code that deduces a pointer's bounds from 
the cache when the program loads a pointer from memory. It's the 
compiler's responsibility to keep track of the bounds of pointers that 
it keeps in registers, so that when it stores these pointers the cache 
can be updated. The reason for all this complexity is to support 
programs where only some modules have been built with 
-fcheck-pointer-bounds, and where pointers are passed back and forth 
between modules.

The -mmpx implementation is jury-rigged, and I do not recommend it for 
production: it is so slow and buggy that not a lot of people use it, and 
quite possibly it will be withdrawn from GCC some day. However, for 
debugging Emacs it is useful and I found a real bug in emacs-26 with it. 
Unlike -fsanitize=address, -fcheck-pointer-bounds works with a dumped 
Emacs. (Well, it works halfway: since the cache doesn't survive 
undumping, the dumped Emacs cannot check pointers created before Emacs 
was dumped.) In contrast, -fsanitize=address makes a dumped Emacs crash 
immediately.

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

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Fri, 8 Dec 2017 14:06:25 -0800
> Cc: pipcet@gmail.com, Emacs-devel@gnu.org
> 
> On 12/08/2017 05:45 AM, Eli Zaretskii wrote:
> > It's a large patch, so I think the discussion could benefit from an
> > overview of the main points of the implementation.
> >
> > In particular, how would this work in a build --with-wide-int?
> 
> The short answer is, "It doesn't"

I suspected that much.  Too bad, but not a catastrophe.

What about the more general question I asked: can you say a few words
about the idea of your implementation of the support for
"-fcheck-pointer-bounds"?

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

[Paul Eggert]

On 12/09/2017 12:33 AM, Eli Zaretskii wrote:
> can you say a few words
> about the idea of your implementation of the support for
> "-fcheck-pointer-bounds"?


Sure. Three basic points.

1. The Emacs C code should store pointer values only in objects declared to be 
of type pointer. Otherwise, every time Emacs converted an integer to a pointer, 
machine code generated by -fcheck-pointer-bounds would disable bounds checking 
for that pointer (which would defeat the point of bounds checking).

This is what the first patch does. I like this patch anyway, since it cleans up 
the Emacs internals a bit and it doesn't significantly affect performance in the 
typical case where -fcheck-pointer-bounds is not used.

This first patch does not mean Emacs can't cast integers to pointers; that's OK. 
Emacs just can't cast pointers to integers and back again and then dereference 
the result and expect pointer-bounds checking to catch errors there.

2. With the 1st patch installed, building with -fcheck-pointer-bounds makes 
Emacs crash due to some false alarms. A typical example is that Emacs takes two 
individually valid but unrelated pointers P and Q, computes Q-P, and then later 
dereferences by computing P[Q - P], which crashes because Q-P falls outside P's 
bounds. The 2nd patch inserts the minimal changes to Emacs to avoid these 
crashes, by widening P's bounds in such cases.

3. The downside of the 2nd patch is that pointer bounds are often made too wide, 
so bounds checking won't catch some errors that it could easily catch. To fix 
some of this, the 3rd patch tightens pointer bounds when that is easy. This 
patch does not attempt to tighten bounds in all cases, as that would involve too 
many changes to the code and would make bounds-checking even slower than it is. 
It merely tightens bounds in a few strategic places, mostly in allocators, so 
that bounds errors are likely to be caught. It's a cost/benefit guesswork where 
I've tried to minimize development and runtime cost while maximizing 
error-catching benefit.

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

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: pipcet@gmail.com, Emacs-devel@gnu.org
> Date: Sat, 9 Dec 2017 23:10:43 -0800
> 
> 1. The Emacs C code should store pointer values only in objects declared to be 
> of type pointer. Otherwise, every time Emacs converted an integer to a pointer, 
> machine code generated by -fcheck-pointer-bounds would disable bounds checking 
> for that pointer (which would defeat the point of bounds checking).
> 
> This is what the first patch does. I like this patch anyway, since it cleans up 
> the Emacs internals a bit and it doesn't significantly affect performance in the 
> typical case where -fcheck-pointer-bounds is not used.

But if we install this patch regardless of -fcheck-pointer-bounds,
then the --with-wide-int build will cease working, won't it?

Thanks for the other explanations.

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

[Paul Eggert]

Eli Zaretskii wrote:
> But if we install this patch regardless of -fcheck-pointer-bounds,
> then the --with-wide-int build will cease working, won't it?

No, --with-wide-int still works fine, because it causes LISP_WORDS_ARE_POINTERS 
to be false, so src/lisp.h falls back on using EMACS_INT for the basic Lisp 
type, just as before.

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

[Eli Zaretskii]

> Cc: pipcet@gmail.com, Emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 10 Dec 2017 23:54:59 -0800
> 
> Eli Zaretskii wrote:
> > But if we install this patch regardless of -fcheck-pointer-bounds,
> > then the --with-wide-int build will cease working, won't it?
> 
> No, --with-wide-int still works fine, because it causes LISP_WORDS_ARE_POINTERS 
> to be false, so src/lisp.h falls back on using EMACS_INT for the basic Lisp 
> type, just as before.

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.

Why not reserve the new representation to the -fcheck-pointer-bounds
builds only?  Such builds will anyway be used only by Emacs developers
(not even by all of them), and for those who do a different
representation won't be a significant obstacle.

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

[Paul Eggert]

[-- 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

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

[Eli Zaretskii]

> Cc: pipcet@gmail.com, Emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Tue, 12 Dec 2017 15:35:01 -0800
> 
> 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.

But using a (fake) pointer is only marginally safer than using an
integer, isn't it?

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

[Paul Eggert]

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

On 12/13/2017 08:20 AM, Eli Zaretskii wrote:
> using a (fake) pointer is only marginally safer than using an
> integer, isn't it?

The fake pointer catches (at compile-time) common faults like the one 
the attached patch fixes, where an int was passed where a Lisp_Object 
was expected. These are the most important faults that 
--enable-check-lisp-object-type catches.

We could say that the fake pointer is only marginally safer, in the 
sense that --enable-check-lisp-object-type is only marginally safer than 
--disable-check-lisp-object-type. However, this marginal safety is 
useful; and once you have the fake pointer, 
--enable-check-lisp-object-type doesn't buy much extra safety that is 
useful.


[-- Attachment #2: 0001-Fix-type-typo-on-Solaris.patch --]
[-- Type: text/x-patch, Size: 794 bytes --]

From c662e2d4fc3678d1ea6eda16541b82bc88f0890b Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 1 Dec 2016 22:45:07 -0800
Subject: [PATCH] Fix type typo on Solaris

* src/sysdep.c (system_process_attributes) [SOLARIS2 && HAVE_PROCFS]:
Fix type mismatch, caught by --enable-check-lisp-object-type.
---
 src/sysdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sysdep.c b/src/sysdep.c
index 892e97626b..257634292b 100644
--- a/src/sysdep.c
+++ b/src/sysdep.c
@@ -3371,7 +3371,7 @@ system_process_attributes (Lisp_Object pid)
     nread = 0;
   else
     {
-      record_unwind_protect (close_file_unwind, fd);
+      record_unwind_protect_int (close_file_unwind, fd);
       nread = emacs_read (fd, &pinfo, sizeof pinfo);
     }
 
-- 
2.14.3

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

[Stefan Monnier]

>> using a (fake) pointer is only marginally safer than using an
>> integer, isn't it?
> The fake pointer catches (at compile-time) common faults like the one the
> attached patch fixes, where an int was passed where a Lisp_Object was
> expected. These are the most important faults
> that --enable-check-lisp-object-type catches.

Indeed, it doesn't catch things like `x + n` since adding a constant to
a pointer is also a valid operation, but it does catch the vast majority
of problems.

> and once you have the fake pointer, --enable-check-lisp-object-type
> doesn't buy much extra safety that is useful.

It does give us some extra checking, but not very much, indeed.
Maybe we can turn it into a no-op.


        Stefan

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

[Paul Eggert]

On 12/13/2017 11:17 AM, Stefan Monnier wrote:
>> The fake pointer catches (at compile-time) common faults like the one 
>> the
>> attached patch fixes, where an int was passed where a Lisp_Object was 
>> expected. These are the most important faults that 
>> --enable-check-lisp-object-type catches. 
> ... it doesn't catch things like `x + n` since adding a constant to a 
> pointer is also a valid operation

Actually it catches even (x + n), because Lisp_Object is 'union Lisp_X 
*', and the union type is deliberately incomplete so the C compiler does 
not know its size and cannot multiply n by sizeof (union Lisp_X). The C 
Standard requires a diagnostic for (x + n) and practice compilers 
invariably issue at least a warning.

There are some things it doesn't catch. Most of these (e.g., 
'Lisp_Object x = 0;', or 'Lisp Object x = FOO, y = BAR; return x == y;') 
are harmless annoyances. The only worrisome thing not caught is 
converting between void * and Lisp_Object, e.g., 'Lisp_Object z = malloc 
(n);'. However, to my mind it's overkill to 
--enable-check-lisp-object-type by default just to catch this, as void * 
is dangerous with every C pointer type and there's little extra harm to 
making it dangerous with Lisp_Object too.

> Maybe we can turn it into a no-op.

Yes, that's my thought too.

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

[Paul Eggert]

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

Stefan Monnier wrote:
>> and once you have the fake pointer, --enable-check-lisp-object-type
>> doesn't buy much extra safety that is useful.
> It does give us some extra checking, but not very much, indeed.
> Maybe we can turn it into a no-op.

To get the ball rolling on that, I installed the attached into master. This 
doesn't turn --enable-check-lisp-object-type into a no-op; it merely goes back 
to disabling it by default. We can remove it later if in practice it's not that 
helpful to enable it.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Default-CHECK_LISP_OBJECT_TYPE-to-no.patch --]
[-- Type: text/x-patch; name="0001-Default-CHECK_LISP_OBJECT_TYPE-to-no.patch", Size: 2138 bytes --]

From 5959b48ece0abe4639667c023da6363859088676 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 17 Dec 2017 18:43:14 -0800
Subject: [PATCH] Default CHECK_LISP_OBJECT_TYPE to "no"

* configure.ac: Go back to not defining CHECK_LISP_OBJECT_TYPE by
default for developer builds, since it is no longer that useful.
We can make it a no-op entirely later, if in practice it's not
that helpful to enable it.
---
 configure.ac | 7 +++----
 etc/NEWS     | 7 +++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index 562b19a..ec1418b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -900,10 +900,9 @@ AC_DEFUN
 
 AC_ARG_ENABLE([check-lisp-object-type],
   [AS_HELP_STRING([--enable-check-lisp-object-type],
-     [Enable compile-time checks for the Lisp_Object data type,
-      which can catch some bugs during development.
-      The default is "no" if --enable-gcc-warnings is "no".])])
-if test "${enable_check_lisp_object_type-$gl_gcc_warnings}" != "no"; then
+     [Enable compile time checks for the Lisp_Object data type,
+      which can catch some bugs during development.])])
+if test "$enable_check_lisp_object_type" = yes; then
   AC_DEFINE([CHECK_LISP_OBJECT_TYPE], 1,
     [Define to enable compile-time checks for the Lisp_Object data type.])
 fi
diff --git a/etc/NEWS b/etc/NEWS
index 1382f96..1ab1930 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -38,6 +38,13 @@ interpreter or modules that it uses.  If your platform supports it you
 can enable it when configuring, e.g., './configure CFLAGS="-g3 -O2
 -mmpx -fcheck-pointer-bounds"' on Intel MPX platforms.
 
+** Emacs now normally uses a pointer type instead of an integer type
+for the fundamental word in the Emacs Lisp interpreter, to help
+catch typos and support -fcheck-pointer-bounds.  The 'configure'
+option --enable-check-lisp-object-type is therefore no longer as
+useful and so is no longer enabled by default in developer builds,
+to reduce differences between developer and production builds.
+
 \f
 * Startup Changes in Emacs 27.1
 
-- 
2.7.4

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

[Pip Cet]

How does this work? Is sizeof(void *) == 24? (I don't have hardware
that supports this, unfortunately, and no software emulation appears
to be available).

I would suggest an explicit __bnd_check_pointer_bounds in
make_lisp_symbol until the GCC issue is fixed, as an out-of-bounds
symbol index seems a very real possibility for a bug. (And maybe
lisp.h should include ptr-bounds.h, as we'll probably need it in the
allocation functions there?)

The rest of the patch looks good to me, though it's unfortunate that
NIL_IS_ZERO's definition is becoming less futureproof (it's a minor
detail, but switching to a macro there might be better).

On Thu, Dec 7, 2017 at 7:34 AM, Paul Eggert <eggert@cs.ucla.edu> wrote:
> In <https://debbugs.gnu.org/29600> I published patches to port Emacs to the
> -fcheck-pointer-bounds option of GCC, so that I can debug Emacs with
> hardware pointer bounds checking on platforms that support it (such as the
> Kaby Lake chip in my year-old laptop running Ubuntu 17.10). This entails
> changing the fundamental Emacs internal word from an integer to a pointer of
> the same width - which is not as big a deal as one might think, as the
> commonly-used EMACS_INT type does not change and Emacs users and Emacs Lisp
> programmers should not notice any change.
>
> I would like to install these patches on 'master' soon, and am mentioning
> this on emacs-devel to give a heads-up to the few but hardy volunteers who
> work on the low-level part of the Emacs implementation.

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

[Paul Eggert]

On 12/08/2017 08:13 AM, Pip Cet wrote:
> I would suggest an explicit __bnd_check_pointer_bounds in
> make_lisp_symbol until the GCC issue is fixed, as an out-of-bounds
> symbol index seems a very real possibility for a bug.

Thanks, that's a good suggestion, and I'll give it a whirl. I did try it 
on an earlier iteration and don't recall why I yanked it.