The --with-wide-int build and Lisp_Sub_Char_Table assertion

The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Eli Zaretskii]

This is a continuation of the discussion from several months ago,
which started here:

  http://lists.gnu.org/archive/html/emacs-devel/2014-07/msg00013.html

That discussion didn't reach any definite conclusion, AFAICT, and the
code was installed nonetheless.

Today, I tried building a 32-bit MinGW port --with-wide-int, and
immediately hit this static assertion:

  /* Make sure that sub char-table contents slot
     is aligned on a multiple of Lisp_Objects.  */
  verify ((offsetof (struct Lisp_Sub_Char_Table, contents)
	   - offsetof (struct Lisp_Sub_Char_Table, depth)) % word_size == 0);

In the 32-bit MinGW configuration with wide integers, we have:

  offsetof (struct Lisp_Sub_Char_Table, contents) = 16
  offsetof (struct Lisp_Sub_Char_Table, depth)    =  4
  word_size                                       =  8
  header_size                                     =  8
  offsetof(struct Lisp_Vector, contents)          =  8
  
and we immediately see that everything is fine, except the assertion
itself: it assumes that header_size and the offset of 'depth' have the
same value, which is false because header_size is the offset of
'contents' in a Lisp_Vector, not the size of 'struct vectorlike_header'
or the offset of 'depth' in a Lisp_Sub_Char_Table.

So I think the assertion should be changed to this:

  verify ((offsetof (struct Lisp_Sub_Char_Table, contents)
	   - header_size) % word_size == 0);

Since there are too many possible configurations and ABIs out there,
with and without wide-int, I'm asking here whether someone sees a
problem with the above modification of this assertion.

TIA

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Dmitry Antipov]

On 01/12/2015 07:29 PM, Eli Zaretskii wrote:

> In the 32-bit MinGW configuration with wide integers, we have:
>
>    offsetof (struct Lisp_Sub_Char_Table, contents) = 16
>    offsetof (struct Lisp_Sub_Char_Table, depth)    =  4
>    word_size                                       =  8

OK, but...

>    header_size                                     =  8
>    offsetof(struct Lisp_Vector, contents)          =  8

==> sizeof(ptrdiff_t) is 8 in a 32-bit configuration?  Looks pretty
strange.  Or, if sizeof(ptrdiff_t) is 4, what is sizeof(struct vectorlike_header)?
IIUC it should be 4, and so header_size should be 4 too...

Dmitry

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Eli Zaretskii]

> Date: Mon, 12 Jan 2015 20:03:29 +0300
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> On 01/12/2015 07:29 PM, Eli Zaretskii wrote:
> 
> > In the 32-bit MinGW configuration with wide integers, we have:
> >
> >    offsetof (struct Lisp_Sub_Char_Table, contents) = 16
> >    offsetof (struct Lisp_Sub_Char_Table, depth)    =  4
> >    word_size                                       =  8
> 
> OK, but...
> 
> >    header_size                                     =  8
> >    offsetof(struct Lisp_Vector, contents)          =  8
> 
> ==> sizeof(ptrdiff_t) is 8 in a 32-bit configuration?

No, it's 4, of course.

> Or, if sizeof(ptrdiff_t) is 4, what is sizeof(struct vectorlike_header)?

Also 4.

> IIUC it should be 4, and so header_size should be 4 too...

This is exactly the flaw: the assumption that if the size of 'struct
vectorlike_header' is 4, the offset of the next struct member is also
4.  In reality, a compiler can add padding, and evidently here it did,
probably to get that member aligned an a 8-byte boundary.

What I'm saying IOW is that instead of assuming the above, why not
simply use header_size as computed already, since that's what you use
in PSEUDOVECSIZE?

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Andreas Schwab]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 01/12/2015 07:29 PM, Eli Zaretskii wrote:
>
>> In the 32-bit MinGW configuration with wide integers, we have:
>>
>>    offsetof (struct Lisp_Sub_Char_Table, contents) = 16
>>    offsetof (struct Lisp_Sub_Char_Table, depth)    =  4
>>    word_size                                       =  8
>
> OK, but...
>
>>    header_size                                     =  8
>>    offsetof(struct Lisp_Vector, contents)          =  8
>
> ==> sizeof(ptrdiff_t) is 8 in a 32-bit configuration?  Looks pretty
> strange.  Or, if sizeof(ptrdiff_t) is 4, what is sizeof(struct vectorlike_header)?
> IIUC it should be 4, and so header_size should be 4 too...

header_size includes the padding between Lisp_Vector.header and
Lisp_Vector.contents.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Dmitry Antipov]

On 01/12/2015 08:26 PM, Eli Zaretskii wrote:

> This is exactly the flaw: the assumption that if the size of 'struct
> vectorlike_header' is 4, the offset of the next struct member is also
> 4.  In reality, a compiler can add padding, and evidently here it did,
> probably to get that member aligned an a 8-byte boundary.

For MinGW/32, 1) what happens if you use -m[no-]ms-bitfields?  (See
https://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html,
section 6.38.5 i386 Variable Attributes).  Or 2) what about using
__attribute__((packed))?  Stefan doesn't like it for an obvious reasons,
but may be just for this particular case...

Dmitry

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Eli Zaretskii]

> Date: Mon, 12 Jan 2015 21:32:32 +0300
> From: Dmitry Antipov <dmantipov@yandex.ru>
> CC: emacs-devel@gnu.org
> 
> On 01/12/2015 08:26 PM, Eli Zaretskii wrote:
> 
> > This is exactly the flaw: the assumption that if the size of 'struct
> > vectorlike_header' is 4, the offset of the next struct member is also
> > 4.  In reality, a compiler can add padding, and evidently here it did,
> > probably to get that member aligned an a 8-byte boundary.
> 
> For MinGW/32, 1) what happens if you use -m[no-]ms-bitfields?

With or without the "no" part?  -mms-bitfields is the default.
Anyway, I don't think this matters, since we don't have here
zero-width bitfields.

> Or 2) what about using __attribute__((packed))?  Stefan doesn't like
> it for an obvious reasons, but may be just for this particular
> case...

What for?  I already solved the problem in the most natural way, see
the modification of the 'verify' statement I posted.  Are you saying
it is incorrect?  If so, what is the problem with my suggestion?

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Paul Eggert]

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

On 01/12/2015 08:29 AM, Eli Zaretskii wrote:
>    verify ((offsetof (struct Lisp_Sub_Char_Table, contents)
> 	   - header_size) % word_size == 0);

Although that should work, even better let's change it to directly check 
both alignment and location.  I installed the attached patch to try to 
do that.


[-- Attachment #2: 0001-Port-to-32-bit-MingGW-with-wide-int.patch --]
[-- Type: text/x-patch, Size: 1951 bytes --]

From 3e65e299a1f7ea2634a1a47221564a33fb498c56 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 12 Jan 2015 11:26:06 -0800
Subject: [PATCH] Port to 32-bit MingGW --with-wide-int

Problem reported by Eli Zaretskii in:
http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00265.html
* lisp.h (struct Lisp_Sub_Char_Table): Check that offset matches
what we think it is, rather than checking only its alignment (and
doing so incorrectly on MinGW).
---
 src/ChangeLog | 9 +++++++++
 src/lisp.h    | 7 +++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 32f17e1..252dfd3 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,12 @@
+2015-01-12  Paul Eggert  <eggert@cs.ucla.edu>
+
+	Port to 32-bit MingGW --with-wide-int
+	Problem reported by Eli Zaretskii in:
+	http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00265.html
+	* lisp.h (struct Lisp_Sub_Char_Table): Check that offset matches
+	what we think it is, rather than checking only its alignment (and
+	doing so incorrectly on MinGW).
+
 2015-01-12  Dmitry Antipov  <dmantipov@yandex.ru>
 
 	* fileio.c (Ffile_name_as_directory, Fdirectory_file_name):
diff --git a/src/lisp.h b/src/lisp.h
index 9ed9375..6a39f08 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1689,10 +1689,9 @@ CHAR_TABLE_EXTRA_SLOTS (struct Lisp_Char_Table *ct)
 	  - CHAR_TABLE_STANDARD_SLOTS);
 }
 
-/* Make sure that sub char-table contents slot
-   is aligned on a multiple of Lisp_Objects.  */
-verify ((offsetof (struct Lisp_Sub_Char_Table, contents)
-	 - offsetof (struct Lisp_Sub_Char_Table, depth)) % word_size == 0);
+/* Make sure that sub char-table contents slot is where we think it is.  */
+verify (offsetof (struct Lisp_Sub_Char_Table, contents)
+	== offsetof (struct Lisp_Vector, contents[SUB_CHAR_TABLE_OFFSET]));
 
 /***********************************************************************
 			       Symbols
-- 
2.1.0

Re: The --with-wide-int build and Lisp_Sub_Char_Table assertion

[Eli Zaretskii]

> Date: Mon, 12 Jan 2015 11:30:05 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 01/12/2015 08:29 AM, Eli Zaretskii wrote:
> >    verify ((offsetof (struct Lisp_Sub_Char_Table, contents)
> > 	   - header_size) % word_size == 0);
> 
> Although that should work, even better let's change it to directly check 
> both alignment and location.  I installed the attached patch to try to 
> do that.

Thanks.