diff mbox

vtables patch 1/3: allow empty array initializations

Message ID 5177BBB8.1070901@codesourcery.com
State New
Headers show

Commit Message

Bernd Schmidt April 24, 2013, 11:02 a.m. UTC
This is a patch series that changes the way vtables are constructed in
the C++ frontend (the first two are small preliminary patches, the meat
will be in 3/3). The problem I was trying to solve was that on the port
I was working on, function pointers are smaller than data pointers, and
size_t is smaller than a data pointer too. All three kinds of types are
used in vtables.

The C++ frontend assumes all three sizes are identical and constructs
references to them by just multiplying an index with a (constant) size.
(There is a target macro to add extra padding to the data members, but
it's unsuitable for the situation I described, and in any case wastes
too much space to be useful.) This patch series changes that so that
along with a vtable and its initializers, we build up a list of
structure fields and create a vtable type from these. We can then access
the various elements by using COMPONENT_REFs without having to know
exactly what the offsets are. Incidentally, libsupc++ already uses a
"struct vtable_prefix" to describe the layout.

I think this is a cleanup, but since the port won't be contributed I
could understand if the C++ maintainers don't want to risk this patch
set. If it doesn't go in, maybe it will be useful as a reference for
others when porting to a similar target.

There is one assumption here that needs to be pointed out, and one
little piece of ugliness. The assumption is that on current targets,
size_type_node is always the same size as const_ptr_type_node (i.e.
sizeof (void *) == sizeof (size_t)), and also that they have the same
alignment. This is required because the data fields of the vtable are
now created using a size_type_node. If someone knows of a target where
this isn't true, it would be helpful to know. Given the size_t/uintptr_t
testsuite patch I just submitted I'm thinking they don't exist, but I'm
kind of wondering about m32c, so Cc'ing DJ.

The small ugliness is that we must allow empty arrays in the middle of
structures since we must be able to take the address of such an array
field to get an object's vtable pointer. GCC seems to have no problems
with this concept in general, but since 4.5 (which I was working on) one
little problem has crept in: we crash in varasm.c when finding such an
array while initializing the vtable. This is addressed by the following
small preliminary patch which essentially just restores the previous code.

Bootstrapped and tested on x86_64-linux, ok?


Bernd

Comments

DJ Delorie April 24, 2013, 3:10 p.m. UTC | #1
> this isn't true, it would be helpful to know. Given the size_t/uintptr_t
> testsuite patch I just submitted I'm thinking they don't exist, but I'm
> kind of wondering about m32c, so Cc'ing DJ.

For m32c-elf with -mcpu=m32c, size_t is 16 bits but void* is 24 bits.
Bernd Schmidt April 24, 2013, 4:51 p.m. UTC | #2
On 04/24/2013 05:10 PM, DJ Delorie wrote:
> 
>> this isn't true, it would be helpful to know. Given the size_t/uintptr_t
>> testsuite patch I just submitted I'm thinking they don't exist, but I'm
>> kind of wondering about m32c, so Cc'ing DJ.
> 
> For m32c-elf with -mcpu=m32c, size_t is 16 bits but void* is 24 bits.

24 bits stored as three bytes, or four? How does this affect vtable
layout? I would have expected the C++ frontend and libsupc++ to
currently be inconsistent with each other given such a setup.


Bernd
DJ Delorie April 24, 2013, 7:14 p.m. UTC | #3
> 24 bits stored as three bytes, or four? How does this affect vtable
> layout? I would have expected the C++ frontend and libsupc++ to
> currently be inconsistent with each other given such a setup.

In memory, four, I think.  The address registers really are three
bytes though.  They're PSImode and gcc doesn't really have a good way
of using any specified PSImode precision.
Bernd Schmidt April 26, 2013, 11:05 a.m. UTC | #4
On 04/24/2013 09:14 PM, DJ Delorie wrote:
>> 24 bits stored as three bytes, or four? How does this affect vtable
>> layout? I would have expected the C++ frontend and libsupc++ to
>> currently be inconsistent with each other given such a setup.
> 
> In memory, four, I think.  The address registers really are three
> bytes though.  They're PSImode and gcc doesn't really have a good way
> of using any specified PSImode precision.

I took a look myself to find out the answer to the second part of my
question (I'll post a few patches to get m32c working on trunk later).
It turns out that as I expected, C++ support is somewhat broken on this
target. My vtables patch series fixes the following execution tests:

+PASS: g++.dg/abi/local1.C -std=c++98 execution test
+PASS: g++.dg/abi/local1.C -std=c++11 execution test
+PASS: g++.dg/ext/attr-alias-2.C -std=c++98 execution test
+PASS: g++.dg/ext/attr-alias-2.C -std=c++11 execution test
+PASS: g++.dg/lto/pr42987 cp_lto_pr42987_0.o-cp_lto_pr42987_1.o execute
 -flto -g
+PASS: g++.dg/lto/pr42987 cp_lto_pr42987_0.o-cp_lto_pr42987_1.o execute
 -flto -flto-partition=none -g
+PASS: g++.old-deja/g++.mike/thunk2.C -std=c++98 execution test
+PASS: g++.old-deja/g++.mike/thunk2.C -std=c++11 execution test
+PASS: g++.old-deja/g++.other/rtti3.C -std=gnu++98 execution test
+PASS: g++.old-deja/g++.other/rtti3.C -std=gnu++11 execution test
+PASS: g++.old-deja/g++.other/rtti4.C -std=gnu++98 execution test
+PASS: g++.old-deja/g++.other/rtti4.C -std=gnu++11 execution test

The downside is that it would be an ABI change for -mcpu=m32cm in terms
of generated code (to make it match what libsupc++ expects). Would you
consider that acceptable for this target, considering it is a bug fix
and the new layout is more space efficient?


Bernd
Mike Stump April 28, 2013, 7:36 p.m. UTC | #5
On Apr 26, 2013, at 4:05 AM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> On 04/24/2013 09:14 PM, DJ Delorie wrote:
>>> 24 bits stored as three bytes, or four? How does this affect vtable
>>> layout? I would have expected the C++ frontend and libsupc++ to
>>> currently be inconsistent with each other given such a setup.
>> 
>> In memory, four, I think.  The address registers really are three
>> bytes though.  They're PSImode and gcc doesn't really have a good way
>> of using any specified PSImode precision.

I have patches to let one specify a precision for partial int types, easy enough to do, and the rest of the compiler plays nicely for the most part with it...
DJ Delorie April 28, 2013, 9:13 p.m. UTC | #6
> I have patches to let one specify a precision for partial int types,
> easy enough to do, and the rest of the compiler plays nicely for the
> most part with it...

If you can make size_t truly be a 24-bit value, I'd be very happy :-)
Bernd Schmidt April 28, 2013, 9:47 p.m. UTC | #7
On 04/28/2013 11:13 PM, DJ Delorie wrote:
> 
>> I have patches to let one specify a precision for partial int types,
>> easy enough to do, and the rest of the compiler plays nicely for the
>> most part with it...
> 
> If you can make size_t truly be a 24-bit value, I'd be very happy :-)

This confuses me a little. Currently, size_t is 16 bits, correct? How
large is the address space really? Are pointers 24 bit flat objects, or
are the upper 16 bit some kind of segment selector?


Bernd
DJ Delorie April 28, 2013, 10 p.m. UTC | #8
For m32c chips, The address space is a flat 24-bit address space.
Address registers are 24 bits (i.e. they cannot hold an SImode) but
size_t is 16 bits originally because there aren't enough 24-bit math
ops and 32-bit math is too expensive.  I've tried to use PSImode for
size_t recently (different port) and it just doesn't work, partly
because size_t is defined by a *string* that must match a C type, and
partly because PSImode turns into BLKmode in many cases (not 2**N
sized).
diff mbox

Patch

commit 618d06f7d414842a934fb360fa98972478e13483
Author: Bernd Schmidt <bernds@codesourcery.com>
Date:   Tue Apr 23 15:19:07 2013 +0200

    Allow empty arrays to be initialized
    
    This undoes an earlier change in output_constructor_regular_field so that
    we no longer crash when a zero-size array is initialized.  This is in
    preparation for changes to the way C++ vtables are laid out.
    
    	* varasm.c (output_constructor_regular_field): Don't crash for
    	arrays with empty DECL_SIZE_UNIT.

diff --git a/gcc/varasm.c b/gcc/varasm.c
index 2532d80..830fdd0 100644
--- a/gcc/varasm.c
+++ b/gcc/varasm.c
@@ -4833,7 +4833,7 @@  output_constructor_regular_field (oc_local_state *local)
 	     better be last.  */
 	  gcc_assert (!fieldsize || !DECL_CHAIN (local->field));
 	}
-      else
+      else if (DECL_SIZE_UNIT (local->field))
 	fieldsize = tree_low_cst (DECL_SIZE_UNIT (local->field), 1);
     }
   else