Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 7:51 PM, Jason Merrill wrote: > On 03/15/2016 08:25 PM, Joseph Myers wrote: >> >> On Tue, 15 Mar 2016, H.J. Lu wrote: >> >>> On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers >>> wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: > On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers > wrote: >> >> I'm not sure if the zero-size arrays (a GNU extension) are considered >> to >> make a struct non-empty, but in any case I think the tests should >> cover >> such arrays as elements of structs. > > > There are couple tests for structs with members of array > of empty types. testsuite/g++.dg/abi/empty14.h has My concern is the other way round - structs with elements such as "int a[0];", an array [0] of a nonempty type. My reading of the subobject definition is that such an array should not cause the struct to be considered nonempty (it doesn't result in any int subobjects). >>> >>> >>> This is a test for struct with zero-size array, which isn't treated >>> as empty type. C++ and C are compatible in its passing. >> >> >> Where is the current definition of empty types you're proposing for use in >> GCC? Is the behavior of this case clear from that definition? > > > "An empty type is a type where it and all of its subobjects (recursively) > are of structure, union, or array type. No memory slot nor register should > be used to pass or return an object of empty type." > > It seems to me that such a struct should be considered an empty type under > this definition, since a zero-length array has no subobjects. > Since zero-size array is GCC extension, we can change it. Do we want to change its passing for C? -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Mar 16, 2016 at 2:45 AM, Bernhard Reutner-Fischer
wrote:
> On March 16, 2016 3:17:20 AM GMT+01:00, "H.J. Lu" wrote:
>
>>> Where is the current definition of empty types you're proposing for
>>use in
>>> GCC? Is the behavior of this case clear from that definition?
>>
>>https://gcc.gnu.org/ml/gcc/2016-03/msg00071.html
>>
>>Jason's patch follows it. Here is a test for struct with zero-size
>>array of empty type, which is treated as empty type.
>
> index 000..489eb3a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/abi/empty19.C
> @@ -0,0 +1,17 @@
> +// PR c++/60336
> +// { dg-do run }
> +// { dg-options "-Wabi=9 -x c" }
> +// { dg-additional-sources "empty14a.c" }
>
> 14a ? Not 19a ?
> Thanks
>
>
Here is the updated patch.
--
H.J.
From d7da4b56dddbd75da163b9fd3cc9ff4241be6ca9 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Tue, 15 Mar 2016 19:14:30 -0700
Subject: [PATCH] Add a test for struct with zero-size array of empty type
---
gcc/testsuite/g++.dg/abi/empty19.C | 17 +
gcc/testsuite/g++.dg/abi/empty19.h | 10 ++
gcc/testsuite/g++.dg/abi/empty19a.c | 6 ++
3 files changed, 33 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/abi/empty19.C
create mode 100644 gcc/testsuite/g++.dg/abi/empty19.h
create mode 100644 gcc/testsuite/g++.dg/abi/empty19a.c
diff --git a/gcc/testsuite/g++.dg/abi/empty19.C b/gcc/testsuite/g++.dg/abi/empty19.C
new file mode 100644
index 000..e3e855a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19.C
@@ -0,0 +1,17 @@
+// PR c++/60336
+// { dg-do run }
+// { dg-options "-Wabi=9 -x c" }
+// { dg-additional-sources "empty19a.c" }
+// { dg-prune-output "command line option" }
+
+#include "empty19.h"
+extern "C" void fun(struct dummy, struct foo);
+
+int main()
+{
+ struct dummy d;
+ struct foo f = { -1, -2, -3, -4, -5 };
+
+ fun(d, f); // { dg-warning "empty" }
+ return 0;
+}
diff --git a/gcc/testsuite/g++.dg/abi/empty19.h b/gcc/testsuite/g++.dg/abi/empty19.h
new file mode 100644
index 000..616b87b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19.h
@@ -0,0 +1,10 @@
+struct dummy0 { };
+struct dummy { struct dummy0 d[0]; };
+struct foo
+{
+ int i1;
+ int i2;
+ int i3;
+ int i4;
+ int i5;
+};
diff --git a/gcc/testsuite/g++.dg/abi/empty19a.c b/gcc/testsuite/g++.dg/abi/empty19a.c
new file mode 100644
index 000..767b1eb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19a.c
@@ -0,0 +1,6 @@
+#include "empty19.h"
+void fun(struct dummy d, struct foo f)
+{
+ if (f.i1 != -1)
+__builtin_abort();
+}
--
2.5.0
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 12:32 PM, Jason Merrill wrote:
> On 03/15/2016 12:00 PM, H.J. Lu wrote:
>>
>> On Tue, Mar 15, 2016 at 8:35 AM, Jason Merrill wrote:
>>>
>>> I'm concerned about how this patch changes both target-independent code
>>> and
>>> target-specific code, with a passing remark that other targets might need
>>> to
>>> make similar changes. I'm also concerned about the effect of this on
>>> other
>>> languages that might not want the same change. So, here's an alternative
>>> patch that implements the change in the front end (and includes your
>>> testcases, thanks!).
>>>
>>> Thoughts?
>>
>>
>> On x86-64, I got
>>
>>
>> /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:273:23:
>> error: empty class ‘std::__facet_shims::other_abi {aka
>> std::integral_constant}’ parameter passing ABI changes in
>> -fabi-version=10 (GCC 6) [-Werror=abi]
>> __collate_transform(other_abi{}, _M_get(), st, lo, hi);
>
>
> Right, need to remove the -Werror=abi bit from the patch until Jonathan
> updates libstdc++.
>
> Jason
>
I got
FAIL: g++.dg/abi/pr60336-1.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type
FAIL: g++.dg/abi/pr60336-5.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type
FAIL: g++.dg/abi/pr60336-6.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type
FAIL: g++.dg/abi/pr60336-7.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type
FAIL: g++.dg/abi/pr60336-9.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type
FAIL: g++.dg/abi/pr68355.C scan-assembler jmp[\t
]+[^$]*?_Z3xxx17integral_constantIbLb1EE
They are expected since get_ref_base_and_extent needs to be
changed to set bitsize to 0 for empty types so that when
ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to
get 0 as the maximum size on empty type. Otherwise, find_tail_calls
won't perform tail call optimization for functions with empty type
parameters.
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Mar 16, 2016 at 9:58 AM, Jason Merrill wrote: > On 03/16/2016 08:38 AM, H.J. Lu wrote: >> >> FAIL: g++.dg/abi/pr60336-1.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx9true_type >> FAIL: g++.dg/abi/pr60336-5.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx9true_type >> FAIL: g++.dg/abi/pr60336-6.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx9true_type >> FAIL: g++.dg/abi/pr60336-7.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx9true_type >> FAIL: g++.dg/abi/pr60336-9.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx9true_type >> FAIL: g++.dg/abi/pr68355.C scan-assembler jmp[\t >> ]+[^$]*?_Z3xxx17integral_constantIbLb1EE > > > These pass for me on x86_64, but I do see calls with -m32. > >> They are expected since get_ref_base_and_extent needs to be >> changed to set bitsize to 0 for empty types so that when >> ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to >> get 0 as the maximum size on empty type. Otherwise, find_tail_calls >> won't perform tail call optimization for functions with empty type >> parameters. > > > That isn't why the optimization isn't happening in pr68355 with -m32; the > .optimized dump has > > xxx (D.2289); [tail call] > > Rather, the failure seems to happen in load_register_parameter, at > >> /* Check for overlap with already clobbered argument area, >> providing that this has non-zero size. */ >> if (is_sibcall >> && (size == 0 >> || mem_overlaps_already_clobbered_arg_p >>(XEXP (args[i].value, 0), >> size))) >> *sibcall_failure = 1; > > > The code seems to contradict the comment, and seems to have been broken by > r162402. Applying this additional patch fixes those tests. > I am running the full test now. -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 03/16/2016 03:39 PM, H.J. Lu wrote: On Wed, Mar 16, 2016 at 10:02 AM, H.J. Lu wrote: On Wed, Mar 16, 2016 at 9:58 AM, Jason Merrill wrote: On 03/16/2016 08:38 AM, H.J. Lu wrote: FAIL: g++.dg/abi/pr60336-1.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-5.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-6.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-7.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-9.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr68355.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx17integral_constantIbLb1EE These pass for me on x86_64, but I do see calls with -m32. They are expected since get_ref_base_and_extent needs to be changed to set bitsize to 0 for empty types so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to get 0 as the maximum size on empty type. Otherwise, find_tail_calls won't perform tail call optimization for functions with empty type parameters. That isn't why the optimization isn't happening in pr68355 with -m32; the .optimized dump has xxx (D.2289); [tail call] Rather, the failure seems to happen in load_register_parameter, at /* Check for overlap with already clobbered argument area, providing that this has non-zero size. */ if (is_sibcall && (size == 0 || mem_overlaps_already_clobbered_arg_p (XEXP (args[i].value, 0), size))) *sibcall_failure = 1; The code seems to contradict the comment, and seems to have been broken by r162402. Applying this additional patch fixes those tests. I am running the full test now. On x86-64, I got export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/ubsan/object-size-9.c:11:13: runtime error: load of address 0x00600ffa with insufficient space for an object of type 'char' 0x00600ffa: note: pointer points here PASS: gcc.dg/ubsan/object-size-9.c -O2 execution test FAIL: gcc.dg/ubsan/object-size-9.c -O2 output pattern test Output was: That looks like a dejagnu glitch; the output you quote seems to match the expected output from the test. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 03/16/2016 07:55 AM, H.J. Lu wrote: On Tue, Mar 15, 2016 at 7:51 PM, Jason Merrill wrote: On 03/15/2016 08:25 PM, Joseph Myers wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers wrote: I'm not sure if the zero-size arrays (a GNU extension) are considered to make a struct non-empty, but in any case I think the tests should cover such arrays as elements of structs. There are couple tests for structs with members of array of empty types. testsuite/g++.dg/abi/empty14.h has My concern is the other way round - structs with elements such as "int a[0];", an array [0] of a nonempty type. My reading of the subobject definition is that such an array should not cause the struct to be considered nonempty (it doesn't result in any int subobjects). This is a test for struct with zero-size array, which isn't treated as empty type. C++ and C are compatible in its passing. Where is the current definition of empty types you're proposing for use in GCC? Is the behavior of this case clear from that definition? "An empty type is a type where it and all of its subobjects (recursively) are of structure, union, or array type. No memory slot nor register should be used to pass or return an object of empty type." It seems to me that such a struct should be considered an empty type under this definition, since a zero-length array has no subobjects. Since zero-size array is GCC extension, we can change it. Do we want to change its passing for C? I would think so; it seems to follow clearly from this definition. I have trouble imagining that anyone would ever pass an object containing a zero-length array by value, so it shouldn't matter much either way, but I consistency is good. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Mar 16, 2016 at 7:33 AM, Jason Merrill wrote: > On 03/16/2016 07:55 AM, H.J. Lu wrote: >> >> On Tue, Mar 15, 2016 at 7:51 PM, Jason Merrill wrote: >>> >>> On 03/15/2016 08:25 PM, Joseph Myers wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: > On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers > wrote: >> >> >> On Tue, 15 Mar 2016, H.J. Lu wrote: >> >>> On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers >>> >>> wrote: I'm not sure if the zero-size arrays (a GNU extension) are considered to make a struct non-empty, but in any case I think the tests should cover such arrays as elements of structs. >>> >>> >>> >>> There are couple tests for structs with members of array >>> of empty types. testsuite/g++.dg/abi/empty14.h has >> >> >> >> My concern is the other way round - structs with elements such as >> "int a[0];", an array [0] of a nonempty type. My reading of the >> subobject >> definition is that such an array should not cause the struct to be >> considered nonempty (it doesn't result in any int subobjects). > > > > This is a test for struct with zero-size array, which isn't treated > as empty type. C++ and C are compatible in its passing. Where is the current definition of empty types you're proposing for use in GCC? Is the behavior of this case clear from that definition? >>> >>> >>> >>> "An empty type is a type where it and all of its subobjects (recursively) >>> are of structure, union, or array type. No memory slot nor register >>> should >>> be used to pass or return an object of empty type." >>> >>> It seems to me that such a struct should be considered an empty type >>> under >>> this definition, since a zero-length array has no subobjects. >> >> >> Since zero-size array is GCC extension, we can change it. Do we >> want to change its passing for C? > > > I would think so; it seems to follow clearly from this definition. I have > trouble imagining that anyone would ever pass an object containing a > zero-length array by value, so it shouldn't matter much either way, but I > consistency is good. > This requires change in both C and C++ frontends. -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 03/16/2016 08:38 AM, H.J. Lu wrote: FAIL: g++.dg/abi/pr60336-1.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-5.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-6.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-7.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr60336-9.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx9true_type FAIL: g++.dg/abi/pr68355.C scan-assembler jmp[\t ]+[^$]*?_Z3xxx17integral_constantIbLb1EE These pass for me on x86_64, but I do see calls with -m32. They are expected since get_ref_base_and_extent needs to be changed to set bitsize to 0 for empty types so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to get 0 as the maximum size on empty type. Otherwise, find_tail_calls won't perform tail call optimization for functions with empty type parameters. That isn't why the optimization isn't happening in pr68355 with -m32; the .optimized dump has xxx (D.2289); [tail call] Rather, the failure seems to happen in load_register_parameter, at /* Check for overlap with already clobbered argument area, providing that this has non-zero size. */ if (is_sibcall && (size == 0 || mem_overlaps_already_clobbered_arg_p (XEXP (args[i].value, 0), size))) *sibcall_failure = 1; The code seems to contradict the comment, and seems to have been broken by r162402. Applying this additional patch fixes those tests. commit b9e170023d97cef94f9b88ded1dfd3b4cf993294 Author: Jason Merrill Date: Wed Mar 16 12:57:37 2016 -0400 * calls.c (load_register_parameters): Fix zero size sibcall logic. diff --git a/gcc/calls.c b/gcc/calls.c index 7b28f43..6415e08 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -2083,9 +2083,9 @@ load_register_parameters (struct arg_data *args, int num_actuals, /* Check for overlap with already clobbered argument area, providing that this has non-zero size. */ if (is_sibcall - && (size == 0 - || mem_overlaps_already_clobbered_arg_p - (XEXP (args[i].value, 0), size))) + && size != 0 + && (mem_overlaps_already_clobbered_arg_p + (XEXP (args[i].value, 0), size))) *sibcall_failure = 1; if (size % UNITS_PER_WORD == 0
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Mar 16, 2016 at 10:02 AM, H.J. Lu wrote: > On Wed, Mar 16, 2016 at 9:58 AM, Jason Merrill wrote: >> On 03/16/2016 08:38 AM, H.J. Lu wrote: >>> >>> FAIL: g++.dg/abi/pr60336-1.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx9true_type >>> FAIL: g++.dg/abi/pr60336-5.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx9true_type >>> FAIL: g++.dg/abi/pr60336-6.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx9true_type >>> FAIL: g++.dg/abi/pr60336-7.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx9true_type >>> FAIL: g++.dg/abi/pr60336-9.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx9true_type >>> FAIL: g++.dg/abi/pr68355.C scan-assembler jmp[\t >>> ]+[^$]*?_Z3xxx17integral_constantIbLb1EE >> >> >> These pass for me on x86_64, but I do see calls with -m32. >> >>> They are expected since get_ref_base_and_extent needs to be >>> changed to set bitsize to 0 for empty types so that when >>> ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to >>> get 0 as the maximum size on empty type. Otherwise, find_tail_calls >>> won't perform tail call optimization for functions with empty type >>> parameters. >> >> >> That isn't why the optimization isn't happening in pr68355 with -m32; the >> .optimized dump has >> >> xxx (D.2289); [tail call] >> >> Rather, the failure seems to happen in load_register_parameter, at >> >>> /* Check for overlap with already clobbered argument area, >>> providing that this has non-zero size. */ >>> if (is_sibcall >>> && (size == 0 >>> || mem_overlaps_already_clobbered_arg_p >>>(XEXP (args[i].value, 0), >>> size))) >>> *sibcall_failure = 1; >> >> >> The code seems to contradict the comment, and seems to have been broken by >> r162402. Applying this additional patch fixes those tests. >> > > I am running the full test now. On x86-64, I got export/gnu/import/git/sources/gcc/gcc/testsuite/gcc.dg/ubsan/object-size-9.c:11:13: runtime error: load of address 0x00600ffa with insufficient space for an object of type 'char' 0x00600ffa: note: pointer points here PASS: gcc.dg/ubsan/object-size-9.c -O2 execution test FAIL: gcc.dg/ubsan/object-size-9.c -O2 output pattern test Output was: -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On March 16, 2016 3:17:20 AM GMT+01:00, "H.J. Lu" wrote:
>> Where is the current definition of empty types you're proposing for
>use in
>> GCC? Is the behavior of this case clear from that definition?
>
>https://gcc.gnu.org/ml/gcc/2016-03/msg00071.html
>
>Jason's patch follows it. Here is a test for struct with zero-size
>array of empty type, which is treated as empty type.
index 000..489eb3a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19.C
@@ -0,0 +1,17 @@
+// PR c++/60336
+// { dg-do run }
+// { dg-options "-Wabi=9 -x c" }
+// { dg-additional-sources "empty14a.c" }
14a ? Not 19a ?
Thanks
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 03/15/2016 08:25 PM, Joseph Myers wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers wrote: On Tue, 15 Mar 2016, H.J. Lu wrote: On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers wrote: I'm not sure if the zero-size arrays (a GNU extension) are considered to make a struct non-empty, but in any case I think the tests should cover such arrays as elements of structs. There are couple tests for structs with members of array of empty types. testsuite/g++.dg/abi/empty14.h has My concern is the other way round - structs with elements such as "int a[0];", an array [0] of a nonempty type. My reading of the subobject definition is that such an array should not cause the struct to be considered nonempty (it doesn't result in any int subobjects). This is a test for struct with zero-size array, which isn't treated as empty type. C++ and C are compatible in its passing. Where is the current definition of empty types you're proposing for use in GCC? Is the behavior of this case clear from that definition? "An empty type is a type where it and all of its subobjects (recursively) are of structure, union, or array type. No memory slot nor register should be used to pass or return an object of empty type." It seems to me that such a struct should be considered an empty type under this definition, since a zero-length array has no subobjects. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 5:25 PM, Joseph Myers wrote:
> On Tue, 15 Mar 2016, H.J. Lu wrote:
>
>> On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers
>> wrote:
>> > On Tue, 15 Mar 2016, H.J. Lu wrote:
>> >
>> >> On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers
>> >> wrote:
>> >> > I'm not sure if the zero-size arrays (a GNU extension) are considered to
>> >> > make a struct non-empty, but in any case I think the tests should cover
>> >> > such arrays as elements of structs.
>> >>
>> >> There are couple tests for structs with members of array
>> >> of empty types. testsuite/g++.dg/abi/empty14.h has
>> >
>> > My concern is the other way round - structs with elements such as
>> > "int a[0];", an array [0] of a nonempty type. My reading of the subobject
>> > definition is that such an array should not cause the struct to be
>> > considered nonempty (it doesn't result in any int subobjects).
>>
>> This is a test for struct with zero-size array, which isn't treated
>> as empty type. C++ and C are compatible in its passing.
>
> Where is the current definition of empty types you're proposing for use in
> GCC? Is the behavior of this case clear from that definition?
https://gcc.gnu.org/ml/gcc/2016-03/msg00071.html
Jason's patch follows it. Here is a test for struct with zero-size
array of empty type, which is treated as empty type.
--
H.J.
From 222c8fcf6518b8689ead18516ce49ba71a1c0a49 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Tue, 15 Mar 2016 19:14:30 -0700
Subject: [PATCH] Add a test for struct with zero-size array of empty type
---
gcc/testsuite/g++.dg/abi/empty19.C | 17 +
gcc/testsuite/g++.dg/abi/empty19.h | 10 ++
gcc/testsuite/g++.dg/abi/empty19a.c | 6 ++
3 files changed, 33 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/abi/empty19.C
create mode 100644 gcc/testsuite/g++.dg/abi/empty19.h
create mode 100644 gcc/testsuite/g++.dg/abi/empty19a.c
diff --git a/gcc/testsuite/g++.dg/abi/empty19.C b/gcc/testsuite/g++.dg/abi/empty19.C
new file mode 100644
index 000..489eb3a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19.C
@@ -0,0 +1,17 @@
+// PR c++/60336
+// { dg-do run }
+// { dg-options "-Wabi=9 -x c" }
+// { dg-additional-sources "empty14a.c" }
+// { dg-prune-output "command line option" }
+
+#include "empty19.h"
+extern "C" void fun(struct dummy, struct foo);
+
+int main()
+{
+ struct dummy d;
+ struct foo f = { -1, -2, -3, -4, -5 };
+
+ fun(d, f); // { dg-warning "empty" }
+ return 0;
+}
diff --git a/gcc/testsuite/g++.dg/abi/empty19.h b/gcc/testsuite/g++.dg/abi/empty19.h
new file mode 100644
index 000..616b87b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19.h
@@ -0,0 +1,10 @@
+struct dummy0 { };
+struct dummy { struct dummy0 d[0]; };
+struct foo
+{
+ int i1;
+ int i2;
+ int i3;
+ int i4;
+ int i5;
+};
diff --git a/gcc/testsuite/g++.dg/abi/empty19a.c b/gcc/testsuite/g++.dg/abi/empty19a.c
new file mode 100644
index 000..767b1eb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty19a.c
@@ -0,0 +1,6 @@
+#include "empty19.h"
+void fun(struct dummy d, struct foo f)
+{
+ if (f.i1 != -1)
+__builtin_abort();
+}
--
2.5.0
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, 15 Mar 2016, H.J. Lu wrote: > On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers wrote: > > On Tue, 15 Mar 2016, H.J. Lu wrote: > > > >> On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers > >> wrote: > >> > I'm not sure if the zero-size arrays (a GNU extension) are considered to > >> > make a struct non-empty, but in any case I think the tests should cover > >> > such arrays as elements of structs. > >> > >> There are couple tests for structs with members of array > >> of empty types. testsuite/g++.dg/abi/empty14.h has > > > > My concern is the other way round - structs with elements such as > > "int a[0];", an array [0] of a nonempty type. My reading of the subobject > > definition is that such an array should not cause the struct to be > > considered nonempty (it doesn't result in any int subobjects). > > This is a test for struct with zero-size array, which isn't treated > as empty type. C++ and C are compatible in its passing. Where is the current definition of empty types you're proposing for use in GCC? Is the behavior of this case clear from that definition? -- Joseph S. Myers jos...@codesourcery.com
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 3:34 PM, Joseph Myers wrote:
> On Tue, 15 Mar 2016, H.J. Lu wrote:
>
>> On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers
>> wrote:
>> > I'm not sure if the zero-size arrays (a GNU extension) are considered to
>> > make a struct non-empty, but in any case I think the tests should cover
>> > such arrays as elements of structs.
>>
>> There are couple tests for structs with members of array
>> of empty types. testsuite/g++.dg/abi/empty14.h has
>
> My concern is the other way round - structs with elements such as
> "int a[0];", an array [0] of a nonempty type. My reading of the subobject
> definition is that such an array should not cause the struct to be
> considered nonempty (it doesn't result in any int subobjects).
This is a test for struct with zero-size array, which isn't treated
as empty type. C++ and C are compatible in its passing.
--
H.J.
From 549583547f8dfb284b6ae083031757371907671f Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Tue, 15 Mar 2016 17:20:08 -0700
Subject: [PATCH] Add a test for struct with zero-size array
---
gcc/testsuite/g++.dg/abi/empty18.C | 17 +
gcc/testsuite/g++.dg/abi/empty18.h | 9 +
gcc/testsuite/g++.dg/abi/empty18a.c | 6 ++
3 files changed, 32 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/abi/empty18.C
create mode 100644 gcc/testsuite/g++.dg/abi/empty18.h
create mode 100644 gcc/testsuite/g++.dg/abi/empty18a.c
diff --git a/gcc/testsuite/g++.dg/abi/empty18.C b/gcc/testsuite/g++.dg/abi/empty18.C
new file mode 100644
index 000..cf850ce
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty18.C
@@ -0,0 +1,17 @@
+// PR c++/60336
+// { dg-do run }
+// { dg-options "-Wabi=9 -x c" }
+// { dg-additional-sources "empty18a.c" }
+// { dg-prune-output "command line option" }
+
+#include "empty18.h"
+extern "C" void fun(struct dummy, struct foo);
+
+int main()
+{
+ struct dummy d;
+ struct foo f = { -1, -2, -3, -4, -5 };
+
+ fun(d, f);
+ return 0;
+}
diff --git a/gcc/testsuite/g++.dg/abi/empty18.h b/gcc/testsuite/g++.dg/abi/empty18.h
new file mode 100644
index 000..86e7ecd
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty18.h
@@ -0,0 +1,9 @@
+struct dummy { int d[0]; };
+struct foo
+{
+ int i1;
+ int i2;
+ int i3;
+ int i4;
+ int i5;
+};
diff --git a/gcc/testsuite/g++.dg/abi/empty18a.c b/gcc/testsuite/g++.dg/abi/empty18a.c
new file mode 100644
index 000..902860b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/abi/empty18a.c
@@ -0,0 +1,6 @@
+#include "empty18.h"
+void fun(struct dummy d, struct foo f)
+{
+ if (f.i1 != -1)
+__builtin_abort();
+}
--
2.5.0
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, 15 Mar 2016, H.J. Lu wrote: > On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers wrote: > > I'm not sure if the zero-size arrays (a GNU extension) are considered to > > make a struct non-empty, but in any case I think the tests should cover > > such arrays as elements of structs. > > There are couple tests for structs with members of array > of empty types. testsuite/g++.dg/abi/empty14.h has My concern is the other way round - structs with elements such as "int a[0];", an array [0] of a nonempty type. My reading of the subobject definition is that such an array should not cause the struct to be considered nonempty (it doesn't result in any int subobjects). -- Joseph S. Myers jos...@codesourcery.com
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 2:39 PM, Joseph Myers wrote:
> I'm not sure if the zero-size arrays (a GNU extension) are considered to
> make a struct non-empty, but in any case I think the tests should cover
> such arrays as elements of structs.
There are couple tests for structs with members of array
of empty types. testsuite/g++.dg/abi/empty14.h has
struct dummy0 { };
struct dummy { struct dummy0 d[140]; };
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
I'm not sure if the zero-size arrays (a GNU extension) are considered to make a struct non-empty, but in any case I think the tests should cover such arrays as elements of structs. -- Joseph S. Myers jos...@codesourcery.com
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 03/15/2016 12:00 PM, H.J. Lu wrote:
On Tue, Mar 15, 2016 at 8:35 AM, Jason Merrill wrote:
I'm concerned about how this patch changes both target-independent code and
target-specific code, with a passing remark that other targets might need to
make similar changes. I'm also concerned about the effect of this on other
languages that might not want the same change. So, here's an alternative
patch that implements the change in the front end (and includes your
testcases, thanks!).
Thoughts?
On x86-64, I got
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:273:23:
error: empty class ‘std::__facet_shims::other_abi {aka
std::integral_constant}’ parameter passing ABI changes in
-fabi-version=10 (GCC 6) [-Werror=abi]
__collate_transform(other_abi{}, _M_get(), st, lo, hi);
Right, need to remove the -Werror=abi bit from the patch until Jonathan
updates libstdc++.
Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Mar 15, 2016 at 8:35 AM, Jason Merrill wrote:
> I'm concerned about how this patch changes both target-independent code and
> target-specific code, with a passing remark that other targets might need to
> make similar changes. I'm also concerned about the effect of this on other
> languages that might not want the same change. So, here's an alternative
> patch that implements the change in the front end (and includes your
> testcases, thanks!).
>
> Thoughts?
On x86-64, I got
libtool: compile:
/export/build/gnu/gcc-x32/build-x86_64-linux/./gcc/xgcc -shared-libgcc
-B/export/build/gnu/gcc-x32/build-x86_64-linux/./gcc -nostdinc++
-L/export/build/gnu/gcc-x32/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/src
-L/export/build/gnu/gcc-x32/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/src/.libs
-L/export/build/gnu/gcc-x32/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/libsupc++/.libs
-B/usr/gcc-6.0.0-x32/x86_64-pc-linux-gnu/bin/
-B/usr/gcc-6.0.0-x32/x86_64-pc-linux-gnu/lib/ -isystem
/usr/gcc-6.0.0-x32/x86_64-pc-linux-gnu/include -isystem
/usr/gcc-6.0.0-x32/x86_64-pc-linux-gnu/sys-include
-I/export/gnu/import/git/sources/gcc/libstdc++-v3/../libgcc
-I/export/build/gnu/gcc-x32/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu
-I/export/build/gnu/gcc-x32/build-x86_64-linux/x86_64-pc-linux-gnu/libstdc++-v3/include
-I/export/gnu/import/git/sources/gcc/libstdc++-v3/libsupc++
-std=gnu++11 -D_GLIBCXX_SHARED -fno-implicit-templates -Wall -Wextra
-Wwrite-strings -Wcast-qual -Werror=abi -Wabi=9
-fdiagnostics-show-location=once -ffunction-sections -fdata-sections
-frandom-seed=cow-shim_facets.lo -g -O2 -D_GNU_SOURCE -c
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cow-shim_facets.cc
-fPIC -DPIC -D_GLIBCXX_SHARED -o cow-shim_facets.o
In file included from
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cow-shim_facets.cc:35:0:
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:
In instantiation of
‘std::__facet_shims::{anonymous}::numpunct_shim<_CharT>::numpunct_shim(const
facet*, std::__facet_shims::{anonymous}::numpunct_shim<_CharT>::__cache_type*)
[with _CharT = char; std::__facet_shims::facet = std::locale::facet;
std::__facet_shims::{anonymous}::numpunct_shim<_CharT>::__cache_type =
std::__numpunct_cache]’:
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:461:20:
required from here
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:238:25:
error: empty class ‘std::__facet_shims::other_abi {aka
std::integral_constant}’ parameter passing ABI changes in
-fabi-version=10 (GCC 6) [-Werror=abi]
__numpunct_fill_cache(other_abi{}, f, c);
~^~~
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:
In instantiation of ‘int
std::__facet_shims::{anonymous}::collate_shim<_CharT>::do_compare(const
_CharT*, const _CharT*, const _CharT*, const _CharT*) const [with
_CharT = char]’:
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:462:20:
required from here
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:265:28:
error: empty class ‘std::__facet_shims::other_abi {aka
std::integral_constant}’ parameter passing ABI changes in
-fabi-version=10 (GCC 6) [-Werror=abi]
return __collate_compare(other_abi{}, _M_get(),
~^~~
lo1, hi1, lo2, hi2);
~~~
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:
In instantiation of
‘std::__facet_shims::{anonymous}::collate_shim<_CharT>::string_type
std::__facet_shims::{anonymous}::collate_shim<_CharT>::do_transform(const
_CharT*, const _CharT*) const [with _CharT = char;
std::__facet_shims::{anonymous}::collate_shim<_CharT>::string_type =
std::basic_string]’:
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:462:20:
required from here
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:273:23:
error: empty class ‘std::__facet_shims::other_abi {aka
std::integral_constant}’ parameter passing ABI changes in
-fabi-version=10 (GCC 6) [-Werror=abi]
__collate_transform(other_abi{}, _M_get(), st, lo, hi);
~~~^~~
/export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:
In instantiation of
‘std::__facet_shims::{anonymous}::moneypunct_shim<_CharT,
_Intl>::moneypunct_shim(const facet*,
std::__facet_shims::{anonymous}::moneypunct_shim<_CharT,
_Intl>::__cache_type*) [with _CharT = char; bool _Intl = true;
std::__facet_shims::facet = std::locale::facet;
std::__facet_shims::{anonymous}::moneypunct_shim<_CharT,
_Intl>::__cache_type = std::__moneypunct_cache]’:
...
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
I'm concerned about how this patch changes both target-independent code
and target-specific code, with a passing remark that other targets might
need to make similar changes. I'm also concerned about the effect of
this on other languages that might not want the same change. So, here's
an alternative patch that implements the change in the front end (and
includes your testcases, thanks!).
Thoughts?
commit 96d0f7ffec807b5a6b71dd2fc2f6745f441fabe0
Author: Jason Merrill
Date: Fri Mar 11 13:39:52 2016 -0500
* class.c (is_really_empty_class): An unnamed bit-field doesn't
make a class non-empty.
diff --git a/gcc/cp/class.c b/gcc/cp/class.c
index f6ad696..1027dad 100644
--- a/gcc/cp/class.c
+++ b/gcc/cp/class.c
@@ -8361,6 +8361,8 @@ is_really_empty_class (tree type)
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
+ /* An unnamed bit-field is not a data member. */
+ && (DECL_NAME (field) || !DECL_C_BIT_FIELD (field))
&& !is_really_empty_class (TREE_TYPE (field)))
return false;
return true;
commit 4a683e9e5e3b3ee824dbf86dd2ad7508ea4fdc3f
Author: Jason Merrill
Date: Fri Mar 11 13:40:02 2016 -0500
Pass empty class parameters like C.
* call.c (pass_as_empty_struct, empty_class_arg)
(warn_empty_class_abi): New.
(type_passed_as, build_x_va_arg): Use pass_as_empty_struct.
(build_call_a): Use empty_class_arg, warn_empty_class_abi.
* cp-tree.h (CPTI_EMPTY_STRUCT, empty_struct_type): New.
* decl.c (cxx_init_decl_processing): Create empty_struct_type.
(store_parm_decls): Use warn_empty_class_abi.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 3ad3bd5..d7cfb99 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -214,6 +214,8 @@ static void add_candidates (tree, tree, const vec *, tree, tree,
tsubst_flags_t);
static conversion *merge_conversion_sequences (conversion *, conversion *);
static tree build_temp (tree, tree, int, diagnostic_t *, tsubst_flags_t);
+static bool pass_as_empty_struct (tree type);
+static tree empty_class_arg (tree);
/* Returns nonzero iff the destructor name specified in NAME matches BASETYPE.
NAME can take many forms... */
@@ -341,7 +343,6 @@ build_call_a (tree function, int n, tree *argarray)
tree decl;
tree result_type;
tree fntype;
- int i;
function = build_addr_func (function, tf_warning_or_error);
@@ -379,16 +380,24 @@ build_call_a (tree function, int n, tree *argarray)
/* Don't pass empty class objects by value. This is useful
for tags in STL, which are used to control overload resolution.
We don't need to handle other cases of copying empty classes. */
+ bool warned = false;
+ if (decl && !TREE_PUBLIC (decl))
+/* Don't warn about the ABI of a function local to this TU. */
+warned = true;
if (! decl || ! DECL_BUILT_IN (decl))
-for (i = 0; i < n; i++)
+for (int i = 0; i < n; i++)
{
tree arg = CALL_EXPR_ARG (function, i);
- if (is_empty_class (TREE_TYPE (arg))
- && ! TREE_ADDRESSABLE (TREE_TYPE (arg)))
+ tree type = TREE_TYPE (arg);
+ if (is_really_empty_class (type)
+ && ! TREE_ADDRESSABLE (type))
{
- tree t = build0 (EMPTY_CLASS_EXPR, TREE_TYPE (arg));
- arg = build2 (COMPOUND_EXPR, TREE_TYPE (t), arg, t);
- CALL_EXPR_ARG (function, i) = arg;
+ location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+ CALL_EXPR_ARG (function, i) = empty_class_arg (arg);
+ /* Warn about ABI changes for a non-final argument. */
+ if (!warned && i < n-1
+ && warn_empty_class_abi (arg, loc))
+ warned = true;
}
}
@@ -6871,6 +6880,14 @@ build_x_va_arg (source_location loc, tree expr, tree type)
expr = build_va_arg (loc, expr, ref);
return convert_from_reference (expr);
}
+ else if (is_really_empty_class (type) && !TREE_ADDRESSABLE (type))
+{
+ /* Do the reverse of empty_class_arg. */
+ tree etype = pass_as_empty_struct (type) ? empty_struct_type : type;
+ expr = build_va_arg (loc, expr, etype);
+ tree ec = build0 (EMPTY_CLASS_EXPR, type);
+ return build2 (COMPOUND_EXPR, type, expr, ec);
+}
return build_va_arg (loc, expr, type);
}
@@ -6967,6 +6984,65 @@ convert_default_arg (tree type, tree arg, tree fn, int parmnum,
return arg;
}
+/* Return true iff TYPE should be passed and returned as a size 0 type rather
+ than its normal size, for compatibility with C. */
+
+static bool
+pass_as_empty_struct (tree type)
+{
+ return (abi_version_at_least (10)
+ && type != error_mark_node
+ && COMPLETE_TYPE_P (type)
+ && !TREE_ADDRESSABLE (type)
+ && is_really_empty_class (type));
+}
+
+/* Adjust the value VAL of empty class type TYPE for argument passing.
+ Keep this synced with build_x_va_arg. */
+
+static tree
+empty_class_arg (tree val)
+{
+ /* Don't pass empty class objects by value. This is useful
+ for tags
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Mar 2, 2016 at 8:25 AM, Ulrich Weigand wrote:
> H.J. Lu wrote:
>
>> + if (type && type_is_empty_type_p (type))
>> +{
>> + if (warn_empty_type_p)
>> + warn_empty_type ();
>> + return NULL;
>> +}
>
> If I understand the problem correctly, for C code empty types
> already were handled correctly, the problem occured just for
> C++ code (since the size of an empty size is != 0 there).
>
> However, it seems this warning check would now appear also
> for C code, even though there really shouldn't be any ABI
> changes there. Would it not be better to only generate the
> warning for C++ (maybe check whether the type size is nonzero)?
>
Very good point. Here is the updated patch to return false from
type_is_empty_type_p for type of zero size.
--
H.J.
From d800ae5689948887bcaff21f7b85a5d5193123b0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Sun, 15 Nov 2015 13:19:05 -0800
Subject: [PATCH 1/3] Properly pass empty type for i386 targets
According to x86 psABIs, no memory slot nor register should be used to
pass or return an object of empty type whose address isn't needed.
Middle-end and x86 backend are updated to ignore empty types, whose
address aren't needed, for parameter passing and function value return.
Other targets need similar changes if they want to follow x86 psABIs.
get_ref_base_and_extent is changed to set bitsize to 0 for empty types
so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to
get 0 as the maximum size on empty type. Otherwise, find_tail_calls
won't perform tail call optimization for functions with empty type
parameters, as shown in g++.dg/pr68355.C.
This ABI change is enabled only if the ABI level is at least 10, which
is updated in GCC 6.
gcc/
PR c++/60336
PR middle-end/67239
PR target/68355
* calls.c (initialize_argument_information): Warn empty type
if they are used in a variable argument list or aren't the last
arguments. Replace targetm.calls.function_arg,
targetm.calls.function_incoming_arg and
targetm.calls.function_arg_advance with function_arg,
function_incoming_arg and function_arg_advance.
(expand_call): Replace targetm.calls.function_arg,
targetm.calls.function_incoming_arg and
targetm.calls.function_arg_advance with function_arg,
function_incoming_arg and function_arg_advance.
(emit_library_call_value_1): Likewise.
(store_one_arg): Use 0 for empty type size. Don't
push 0 size argument onto stack.
(must_pass_in_stack_var_size_or_pad): Return false for empty
type.
* dse.c (get_call_args): Replace targetm.calls.function_arg
and targetm.calls.function_arg_advance with function_arg and
function_arg_advance.
* explow.c (hard_function_value): Use 0 for empty type size.
* expr.c (block_move_libcall_safe_for_call_parm): Replace
targetm.calls.function_arg and targetm.calls.function_arg_advance
with function_arg and function_arg_advance.
(copy_blkmode_to_reg): Use 0 for empty type size.
* function.c (aggregate_value_p): Replace
targetm.calls.return_in_memory with return_in_memory.
(assign_parm_data_all): Add warn_empty_type.
(assign_parms_augmented_arg_list): Set warn_empty_type if
empty types are used in a variable argument list or aren't
the last arguments.
(assign_parm_find_entry_rtl): Warn empty type if warn_empty_type
is set. Replace
targetm.calls.function_incoming_arg with function_incoming_arg.
(assign_parms): Only warn empty type once. Replace
targetm.calls.function_arg_advance with function_arg_advance.
(gimplify_parameters): Replace targetm.calls.function_arg_advance
with function_arg_advance.
(locate_and_pad_parm): Use 0 for empty type size.
(warn_empty_type): New function.
(function_arg_advance): New wrapper function.
(function_arg): Likewise.
(function_incoming_arg): Likewise.
(return_in_memory): Likewise.
* target.h (function_arg_advance): New prototype.
(function_arg): Likewise.
(function_incoming_arg): Likewise.
(return_in_memory): Likewise.
* targhooks.c (std_gimplify_va_arg_expr): Use 0 for empty type
size.
* tree-dfa.c (get_ref_base_and_extent): Likewise.
* tree.c (is_empty_type): New functiom.
(type_is_empty_type_p): Likewise.
* tree.h (type_is_empty_type_p): New prototype.
* var-tracking.c (prepare_call_arguments): Replace
targetm.calls.function_arg and targetm.calls.function_arg_advance
with function_arg and function_arg_advance.
* config/i386/i386.c (ix86_gimplify_va_arg): Use 0 for empty
type size.
gcc/testsuite/
PR c++/60336
PR middle-end/67239
PR target/68355
* g++.dg/abi/empty12.C: New test.
* g++.dg/abi/empty12.h: Likewise.
* g++.dg/abi/empty12a.c: Likewise.
* g++.dg/abi/empty13.C: Likewise.
* g++.dg/abi/empty13.h: Likewise.
* g++.dg/abi/empty13a.c: Likewise.
* g++.dg/abi/empty14.C: Likewise.
* g++.dg/abi/empty14.h: Likewise.
* g++.dg/abi/empty14a.c: Likewise.
* g++.dg/abi/empty15.C: Likewise.
* g++.dg/abi/empty15.h: Likewise.
* g++.dg/abi/empty15a.c: Likewise.
* g++.dg/abi/empty16.C: Likewise.
* g++.dg/abi/empty16.h: Li
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
H.J. Lu wrote:
> + if (type && type_is_empty_type_p (type))
> +{
> + if (warn_empty_type_p)
> + warn_empty_type ();
> + return NULL;
> +}
If I understand the problem correctly, for C code empty types
already were handled correctly, the problem occured just for
C++ code (since the size of an empty size is != 0 there).
However, it seems this warning check would now appear also
for C code, even though there really shouldn't be any ABI
changes there. Would it not be better to only generate the
warning for C++ (maybe check whether the type size is nonzero)?
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
ulrich.weig...@de.ibm.com
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Mon, Feb 29, 2016 at 5:02 PM, Jason Merrill wrote:
> On 01/27/2016 10:39 AM, H.J. Lu wrote:
>>
>> Here is the updated patch with new testcases. OK for trunk?
>
>
> This is not a complete patch.
>
> Please update type_is_empty_record_p to use the definition from the recent
> discussion:
Here are 3 patches. If all targets should follow x86 backends
on passing empty types, only first 2 patches are needed. Otherwise
the 3rd patch is needed to limit this to x86 targets.
>> An empty type is a type where it and all of its subobjects (recursively)
>> are of class, structure, union, or array type.
>
>
> This shouldn't need to rely on the front-end setting a flag. Be sure to
> ignore unnamed bit-fields, as they are not subobjects.
Done.
> I would also suggest having this function abort if the type is
> TREE_ADDRESSABLE.
>
I have
bool
type_is_empty_type_p (const_tree type)
{
...
if (TREE_ADDRESSABLE (type))
return false;
...
}
since any type may be passed to it.
Tested on x86-64 with
RUNTESTFLAGS="--target_board='unix{-mx32,-m32,}'"
OK for trunk?
Thanks.
--
H.J.
From 49bf71d9e6983b20150061ccd7332d4b73309cad Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Sun, 15 Nov 2015 13:19:05 -0800
Subject: [PATCH 1/3] Properly pass empty type for i386 targets
According to x86 psABIs, no memory slot nor register should be used to
pass or return an object of empty type whose address isn't needed.
Middle-end and x86 backend are updated to ignore empty types, whose
address aren't needed, for parameter passing and function value return.
Other targets need similar changes if they want to follow x86 psABIs.
get_ref_base_and_extent is changed to set bitsize to 0 for empty types
so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to
get 0 as the maximum size on empty type. Otherwise, find_tail_calls
won't perform tail call optimization for functions with empty type
parameters, as shown in g++.dg/pr68355.C.
This ABI change is enabled only if the ABI level is at least 10, which
is updated in GCC 6.
gcc/
PR c++/60336
PR middle-end/67239
PR target/68355
* calls.c (initialize_argument_information): Warn empty type
if they are used in a variable argument list or aren't the last
arguments. Replace targetm.calls.function_arg,
targetm.calls.function_incoming_arg and
targetm.calls.function_arg_advance with function_arg,
function_incoming_arg and function_arg_advance.
(expand_call): Replace targetm.calls.function_arg,
targetm.calls.function_incoming_arg and
targetm.calls.function_arg_advance with function_arg,
function_incoming_arg and function_arg_advance.
(emit_library_call_value_1): Likewise.
(store_one_arg): Use 0 for empty type size. Don't
push 0 size argument onto stack.
(must_pass_in_stack_var_size_or_pad): Return false for empty
type.
* dse.c (get_call_args): Replace targetm.calls.function_arg
and targetm.calls.function_arg_advance with function_arg and
function_arg_advance.
* explow.c (hard_function_value): Use 0 for empty type size.
* expr.c (block_move_libcall_safe_for_call_parm): Replace
targetm.calls.function_arg and targetm.calls.function_arg_advance
with function_arg and function_arg_advance.
(copy_blkmode_to_reg): Use 0 for empty type size.
* function.c (aggregate_value_p): Replace
targetm.calls.return_in_memory with return_in_memory.
(assign_parm_data_all): Add warn_empty_type.
(assign_parms_augmented_arg_list): Set warn_empty_type if
empty types are used in a variable argument list or aren't
the last arguments.
(assign_parm_find_entry_rtl): Warn empty type if warn_empty_type
is set. Replace
targetm.calls.function_incoming_arg with function_incoming_arg.
(assign_parms): Only warn empty type once. Replace
targetm.calls.function_arg_advance with function_arg_advance.
(gimplify_parameters): Replace targetm.calls.function_arg_advance
with function_arg_advance.
(locate_and_pad_parm): Use 0 for empty type size.
(warn_empty_type): New function.
(function_arg_advance): New wrapper function.
(function_arg): Likewise.
(function_incoming_arg): Likewise.
(return_in_memory): Likewise.
* target.h (function_arg_advance): New prototype.
(function_arg): Likewise.
(function_incoming_arg): Likewise.
(return_in_memory): Likewise.
* targhooks.c (std_gimplify_va_arg_expr): Use 0 for empty type
size.
* tree-dfa.c (get_ref_base_and_extent): Likewise.
* tree.c (is_empty_type): New functiom.
(type_is_empty_type_p): Likewise.
* tree.h (type_is_empty_type_p): New prototype.
* var-tracking.c (prepare_call_arguments): Replace
targetm.calls.function_arg and targetm.calls.function_arg_advance
with function_arg and function_arg_advance.
* config/i386/i386.c (ix86_gimplify_va_arg): Use 0 for empty
type size.
gcc/testsuite/
PR c++/60336
PR middle-end/67239
PR target/68355
* g++.dg/abi/empty12.C: New test.
* g++.dg/abi/empty12.h: Likewise.
* g++.dg/abi/empty12a.c: Likewise.
* g++.dg/abi/empty13.C: Likewise.
* g++.dg/abi/empty13.h: Likewise.
* g
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 01/27/2016 10:39 AM, H.J. Lu wrote: Here is the updated patch with new testcases. OK for trunk? This is not a complete patch. Please update type_is_empty_record_p to use the definition from the recent discussion: An empty type is a type where it and all of its subobjects (recursively) are of class, structure, union, or array type. This shouldn't need to rely on the front-end setting a flag. Be sure to ignore unnamed bit-fields, as they are not subobjects. I would also suggest having this function abort if the type is TREE_ADDRESSABLE. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Jan 27, 2016 at 5:46 AM, H.J. Lu wrote:
> On Wed, Jan 27, 2016 at 1:03 AM, Marc Glisse wrote:
>> On Wed, 27 Jan 2016, Jakub Jelinek wrote:
>>
>>> On Wed, Jan 27, 2016 at 09:10:14AM +0100, Marc Glisse wrote:
>
> Revised:
>
> /* Returns true if TYPE is POD of one-byte or less in size for the
> purpose
> of layout and an empty class or an class with empty classes. */
>
> static bool
> is_empty_record (tree type)
> {
> if (type == error_mark_node)
> return false;
>
> if (!CLASS_TYPE_P (type))
> return false;
>
> if (CLASSTYPE_NON_LAYOUT_POD_P (type))
> return false;
>
> gcc_assert (COMPLETE_TYPE_P (type));
>
> if (CLASSTYPE_EMPTY_P (type))
> return true;
>
> if (int_size_in_bytes (type) > 1)
> return false;
That's completely arbitrary :-(
>>>
>>>
>>> Yeah. Because (adapted to be compilable with C):
>>> struct A1 {}; struct A2 {};
>>> struct B1 { struct A1 a; struct A2 b; }; struct B2 { struct A1 a; struct
>>> A2 b; };
>>> struct C1 { struct B1 a; struct B2 b; }; struct C2 { struct B1 a; struct
>>> B2 b; };
>>> struct D1 { struct C1 a; struct C2 b; }; struct D2 { struct C1 a; struct
>>> C2 b; };
>>> struct E1 { struct D1 a; struct D2 b; }; struct E2 { struct D1 a; struct
>>> D2 b; };
>>> struct F1 { struct E1 a; struct E2 b; }; struct F2 { struct E1 a; struct
>>> E2 b; };
>>> struct G1 { struct F1 a; struct F2 b; }; struct G2 { struct F1 a; struct
>>> F2 b; };
>>> struct H1 { struct G1 a; struct G2 b; }; struct H2 { struct G1 a; struct
>>> G2 b; };
>>> struct I1 { struct H1 a; struct H2 b; }; struct I2 { struct H1 a; struct
>>> H2 b; };
>>> struct J1 { struct I1 a; struct I2 b; }; struct J2 { struct I1 a; struct
>>> I2 b; };
>>> struct K1 { struct J1 a; struct J2 b; };
>>> int v;
>>> __attribute__((noinline, noclone))
>>> struct K1 foo (int a, struct K1 x, int b)
>>> {
>>> v = a + b;
>>> return x;
>>> }
>>> struct K1 k, m;
>>> void
>>> bar (void)
>>> {
>>> m = foo (1, k, 2);
>>> }
>>> then would have a different calling convention between C and C++,
>>> so where is the argument that we change anything just to make the two
>>> compatible? Though, of course, those two will never be compatible,
>>> it is enough to add struct L1 { int a; struct K1 b; int c; }; and
>>> that structure has 1024+8 bytes in C++ and 8 bytes in C.
>>
>>
>> I don't know how empty classes are used in C in practice, but it could make
>> sense to have ABI compatibility as long as no empty struct is used as a
>> member of another struct (I also suggested an attribute to let C++ use the
>> same layout as C here: PR63579). But then the usual definition of empty
>> would be sufficient.
>>
>>> As clang generates different code for the above between C and C++, it
>>> clearly special cases for some reason just the most common case.
>>> IMHO it is not worth to change GCC ABI...
>>
>>
>> I was interested in this change because it improves C++, C compatibility was
>> a convenient excuse ;-)
>>
>
> I opened a clang bug:
>
> https://llvm.org/bugs/show_bug.cgi?id=26337
>
> I propose the following definitions:
>
> i. An empty record is:
>1. A class without member. Or
>2. An array of empty records. Or
>3. A class with empty records.
> ii. An empty record type for parameter passing is POD for the purpose of
> layout
> and
>1. A class without member. Or
>2. A class with empty records.
>
> /* An empty record is:
>1. A class without member. Or
>2. An array of empty records. Or
>3. A class with empty records. */
>
> /* Returns true if TYPE is an empty record or an array of empty records. */
>
> static bool
> is_empty_record_or_array_of_empty_record (tree type)
> {
> if (CLASS_TYPE_P (type))
> {
> if (CLASSTYPE_EMPTY_P (type))
> return true;
>
> tree field;
>
> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> if (TREE_CODE (field) == FIELD_DECL
> && !DECL_ARTIFICIAL (field)
> && !is_empty_record_or_array_of_empty_record (TREE_TYPE (field)))
> return false;
> return true;
> }
> else if (TREE_CODE (type) == ARRAY_TYPE)
> return is_empty_record_or_array_of_empty_record (TREE_TYPE (type));
> return false;
> }
>
> /* Returns true if TYPE is POD for the purpose of layout and
>1. A class without member. Or
>2. A class with empty records. */
>
> static bool
> is_empty_record_for_parm (tree type)
> {
> if (type == error_mark_node)
> return false;
>
> if (!CLASS_TYPE_P (type))
> return false;
>
> if (CLASSTYPE_NON_LAYOUT_POD_P (type))
> return false;
>
> gcc_assert (COMPLETE_TYPE_P (type));
>
> if (CLASSTYPE_EMPTY_P (type))
> return true;
>
> tree field;
>
> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> if (TREE_CODE (field) == FIELD_DECL
> && !DECL_ARTIFICIAL (field)
> && !is_empty_record_or_arr
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Jan 27, 2016 at 1:03 AM, Marc Glisse wrote:
> On Wed, 27 Jan 2016, Jakub Jelinek wrote:
>
>> On Wed, Jan 27, 2016 at 09:10:14AM +0100, Marc Glisse wrote:
Revised:
/* Returns true if TYPE is POD of one-byte or less in size for the
purpose
of layout and an empty class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
if (int_size_in_bytes (type) > 1)
return false;
>>>
>>>
>>> That's completely arbitrary :-(
>>
>>
>> Yeah. Because (adapted to be compilable with C):
>> struct A1 {}; struct A2 {};
>> struct B1 { struct A1 a; struct A2 b; }; struct B2 { struct A1 a; struct
>> A2 b; };
>> struct C1 { struct B1 a; struct B2 b; }; struct C2 { struct B1 a; struct
>> B2 b; };
>> struct D1 { struct C1 a; struct C2 b; }; struct D2 { struct C1 a; struct
>> C2 b; };
>> struct E1 { struct D1 a; struct D2 b; }; struct E2 { struct D1 a; struct
>> D2 b; };
>> struct F1 { struct E1 a; struct E2 b; }; struct F2 { struct E1 a; struct
>> E2 b; };
>> struct G1 { struct F1 a; struct F2 b; }; struct G2 { struct F1 a; struct
>> F2 b; };
>> struct H1 { struct G1 a; struct G2 b; }; struct H2 { struct G1 a; struct
>> G2 b; };
>> struct I1 { struct H1 a; struct H2 b; }; struct I2 { struct H1 a; struct
>> H2 b; };
>> struct J1 { struct I1 a; struct I2 b; }; struct J2 { struct I1 a; struct
>> I2 b; };
>> struct K1 { struct J1 a; struct J2 b; };
>> int v;
>> __attribute__((noinline, noclone))
>> struct K1 foo (int a, struct K1 x, int b)
>> {
>> v = a + b;
>> return x;
>> }
>> struct K1 k, m;
>> void
>> bar (void)
>> {
>> m = foo (1, k, 2);
>> }
>> then would have a different calling convention between C and C++,
>> so where is the argument that we change anything just to make the two
>> compatible? Though, of course, those two will never be compatible,
>> it is enough to add struct L1 { int a; struct K1 b; int c; }; and
>> that structure has 1024+8 bytes in C++ and 8 bytes in C.
>
>
> I don't know how empty classes are used in C in practice, but it could make
> sense to have ABI compatibility as long as no empty struct is used as a
> member of another struct (I also suggested an attribute to let C++ use the
> same layout as C here: PR63579). But then the usual definition of empty
> would be sufficient.
>
>> As clang generates different code for the above between C and C++, it
>> clearly special cases for some reason just the most common case.
>> IMHO it is not worth to change GCC ABI...
>
>
> I was interested in this change because it improves C++, C compatibility was
> a convenient excuse ;-)
>
I opened a clang bug:
https://llvm.org/bugs/show_bug.cgi?id=26337
I propose the following definitions:
i. An empty record is:
1. A class without member. Or
2. An array of empty records. Or
3. A class with empty records.
ii. An empty record type for parameter passing is POD for the purpose of layout
and
1. A class without member. Or
2. A class with empty records.
/* An empty record is:
1. A class without member. Or
2. An array of empty records. Or
3. A class with empty records. */
/* Returns true if TYPE is an empty record or an array of empty records. */
static bool
is_empty_record_or_array_of_empty_record (tree type)
{
if (CLASS_TYPE_P (type))
{
if (CLASSTYPE_EMPTY_P (type))
return true;
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record_or_array_of_empty_record (TREE_TYPE (field)))
return false;
return true;
}
else if (TREE_CODE (type) == ARRAY_TYPE)
return is_empty_record_or_array_of_empty_record (TREE_TYPE (type));
return false;
}
/* Returns true if TYPE is POD for the purpose of layout and
1. A class without member. Or
2. A class with empty records. */
static bool
is_empty_record_for_parm (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record_or_array_of_empty_record (TREE_TYPE (field)))
return false;
return true;
}
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, 27 Jan 2016, Jakub Jelinek wrote:
On Wed, Jan 27, 2016 at 09:10:14AM +0100, Marc Glisse wrote:
Revised:
/* Returns true if TYPE is POD of one-byte or less in size for the purpose
of layout and an empty class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
if (int_size_in_bytes (type) > 1)
return false;
That's completely arbitrary :-(
Yeah. Because (adapted to be compilable with C):
struct A1 {}; struct A2 {};
struct B1 { struct A1 a; struct A2 b; }; struct B2 { struct A1 a; struct A2 b;
};
struct C1 { struct B1 a; struct B2 b; }; struct C2 { struct B1 a; struct B2 b;
};
struct D1 { struct C1 a; struct C2 b; }; struct D2 { struct C1 a; struct C2 b;
};
struct E1 { struct D1 a; struct D2 b; }; struct E2 { struct D1 a; struct D2 b;
};
struct F1 { struct E1 a; struct E2 b; }; struct F2 { struct E1 a; struct E2 b;
};
struct G1 { struct F1 a; struct F2 b; }; struct G2 { struct F1 a; struct F2 b;
};
struct H1 { struct G1 a; struct G2 b; }; struct H2 { struct G1 a; struct G2 b;
};
struct I1 { struct H1 a; struct H2 b; }; struct I2 { struct H1 a; struct H2 b;
};
struct J1 { struct I1 a; struct I2 b; }; struct J2 { struct I1 a; struct I2 b;
};
struct K1 { struct J1 a; struct J2 b; };
int v;
__attribute__((noinline, noclone))
struct K1 foo (int a, struct K1 x, int b)
{
v = a + b;
return x;
}
struct K1 k, m;
void
bar (void)
{
m = foo (1, k, 2);
}
then would have a different calling convention between C and C++,
so where is the argument that we change anything just to make the two
compatible? Though, of course, those two will never be compatible,
it is enough to add struct L1 { int a; struct K1 b; int c; }; and
that structure has 1024+8 bytes in C++ and 8 bytes in C.
I don't know how empty classes are used in C in practice, but it could
make sense to have ABI compatibility as long as no empty struct is used as
a member of another struct (I also suggested an attribute to let
C++ use the same layout as C here: PR63579). But then the usual definition
of empty would be sufficient.
As clang generates different code for the above between C and C++, it
clearly special cases for some reason just the most common case.
IMHO it is not worth to change GCC ABI...
I was interested in this change because it improves C++, C compatibility
was a convenient excuse ;-)
--
Marc Glisse
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Jan 27, 2016 at 09:10:14AM +0100, Marc Glisse wrote:
> >Revised:
> >
> >/* Returns true if TYPE is POD of one-byte or less in size for the purpose
> > of layout and an empty class or an class with empty classes. */
> >
> >static bool
> >is_empty_record (tree type)
> >{
> > if (type == error_mark_node)
> > return false;
> >
> > if (!CLASS_TYPE_P (type))
> > return false;
> >
> > if (CLASSTYPE_NON_LAYOUT_POD_P (type))
> > return false;
> >
> > gcc_assert (COMPLETE_TYPE_P (type));
> >
> > if (CLASSTYPE_EMPTY_P (type))
> > return true;
> >
> > if (int_size_in_bytes (type) > 1)
> > return false;
>
> That's completely arbitrary :-(
Yeah. Because (adapted to be compilable with C):
struct A1 {}; struct A2 {};
struct B1 { struct A1 a; struct A2 b; }; struct B2 { struct A1 a; struct A2 b;
};
struct C1 { struct B1 a; struct B2 b; }; struct C2 { struct B1 a; struct B2 b;
};
struct D1 { struct C1 a; struct C2 b; }; struct D2 { struct C1 a; struct C2 b;
};
struct E1 { struct D1 a; struct D2 b; }; struct E2 { struct D1 a; struct D2 b;
};
struct F1 { struct E1 a; struct E2 b; }; struct F2 { struct E1 a; struct E2 b;
};
struct G1 { struct F1 a; struct F2 b; }; struct G2 { struct F1 a; struct F2 b;
};
struct H1 { struct G1 a; struct G2 b; }; struct H2 { struct G1 a; struct G2 b;
};
struct I1 { struct H1 a; struct H2 b; }; struct I2 { struct H1 a; struct H2 b;
};
struct J1 { struct I1 a; struct I2 b; }; struct J2 { struct I1 a; struct I2 b;
};
struct K1 { struct J1 a; struct J2 b; };
int v;
__attribute__((noinline, noclone))
struct K1 foo (int a, struct K1 x, int b)
{
v = a + b;
return x;
}
struct K1 k, m;
void
bar (void)
{
m = foo (1, k, 2);
}
then would have a different calling convention between C and C++,
so where is the argument that we change anything just to make the two
compatible? Though, of course, those two will never be compatible,
it is enough to add struct L1 { int a; struct K1 b; int c; }; and
that structure has 1024+8 bytes in C++ and 8 bytes in C.
As clang generates different code for the above between C and C++, it
clearly special cases for some reason just the most common case.
IMHO it is not worth to change GCC ABI...
Jakub
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, 26 Jan 2016, H.J. Lu wrote:
On Tue, Jan 26, 2016 at 1:40 PM, Jakub Jelinek wrote:
On Tue, Jan 26, 2016 at 01:21:52PM -0800, H.J. Lu wrote:
Like this:
/* Returns true if TYPE is POD for the purpose of layout and an empty
class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record (TREE_TYPE (field)))
return false;
return true;
}
So you say that K1 in e.g.:
struct A1 {}; struct A2 {};
struct B1 { A1 a; A2 b; }; struct B2 { A1 a; A2 b; };
struct C1 { B1 a; B2 b; }; struct C2 { B1 a; B2 b; };
struct D1 { C1 a; C2 b; }; struct D2 { C1 a; C2 b; };
struct E1 { D1 a; D2 b; }; struct E2 { D1 a; D2 b; };
struct F1 { E1 a; E2 b; }; struct F2 { E1 a; E2 b; };
struct G1 { F1 a; F2 b; }; struct G2 { F1 a; F2 b; };
struct H1 { G1 a; G2 b; }; struct H2 { G1 a; G2 b; };
struct I1 { H1 a; H2 b; }; struct I2 { H1 a; H2 b; };
struct J1 { I1 a; I2 b; }; struct J2 { I1 a; I2 b; };
struct K1 { J1 a; J2 b; };
int v;
__attribute__((noinline, noclone))
K1 foo (int a, K1 x, int b)
{
v = a + b;
return x;
}
K1 k, m;
void
bar (void)
{
m = foo (1, k, 2);
}
is empty class? What does clang do with this?
Jakub
Revised:
/* Returns true if TYPE is POD of one-byte or less in size for the purpose
of layout and an empty class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
if (int_size_in_bytes (type) > 1)
return false;
That's completely arbitrary :-(
If we are defining a new ABI, I preferred your previous version (at least
the idea, I didn't check the code). If we are trying to follow clang, then
we need either a clear English definition from a clang developer or at
least someone should check what conditions they use in the code. Trying to
infer it from a couple examples sounds too fragile for an ABI decision.
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record (TREE_TYPE (field)))
return false;
return true;
}
--
Marc Glisse
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Jan 26, 2016 at 1:40 PM, Jakub Jelinek wrote:
> On Tue, Jan 26, 2016 at 01:21:52PM -0800, H.J. Lu wrote:
>> Like this:
>>
>> /* Returns true if TYPE is POD for the purpose of layout and an empty
>>class or an class with empty classes. */
>>
>> static bool
>> is_empty_record (tree type)
>> {
>> if (type == error_mark_node)
>> return false;
>>
>> if (!CLASS_TYPE_P (type))
>> return false;
>>
>> if (CLASSTYPE_NON_LAYOUT_POD_P (type))
>> return false;
>>
>> gcc_assert (COMPLETE_TYPE_P (type));
>>
>> if (CLASSTYPE_EMPTY_P (type))
>> return true;
>>
>> tree field;
>>
>> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>> if (TREE_CODE (field) == FIELD_DECL
>> && !DECL_ARTIFICIAL (field)
>> && !is_empty_record (TREE_TYPE (field)))
>> return false;
>>
>> return true;
>> }
>
> So you say that K1 in e.g.:
>
> struct A1 {}; struct A2 {};
> struct B1 { A1 a; A2 b; }; struct B2 { A1 a; A2 b; };
> struct C1 { B1 a; B2 b; }; struct C2 { B1 a; B2 b; };
> struct D1 { C1 a; C2 b; }; struct D2 { C1 a; C2 b; };
> struct E1 { D1 a; D2 b; }; struct E2 { D1 a; D2 b; };
> struct F1 { E1 a; E2 b; }; struct F2 { E1 a; E2 b; };
> struct G1 { F1 a; F2 b; }; struct G2 { F1 a; F2 b; };
> struct H1 { G1 a; G2 b; }; struct H2 { G1 a; G2 b; };
> struct I1 { H1 a; H2 b; }; struct I2 { H1 a; H2 b; };
> struct J1 { I1 a; I2 b; }; struct J2 { I1 a; I2 b; };
> struct K1 { J1 a; J2 b; };
> int v;
> __attribute__((noinline, noclone))
> K1 foo (int a, K1 x, int b)
> {
> v = a + b;
> return x;
> }
> K1 k, m;
> void
> bar (void)
> {
> m = foo (1, k, 2);
> }
>
> is empty class? What does clang do with this?
>
> Jakub
Revised:
/* Returns true if TYPE is POD of one-byte or less in size for the purpose
of layout and an empty class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
if (int_size_in_bytes (type) > 1)
return false;
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record (TREE_TYPE (field)))
return false;
return true;
}
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Jan 26, 2016 at 01:21:52PM -0800, H.J. Lu wrote:
> Like this:
>
> /* Returns true if TYPE is POD for the purpose of layout and an empty
>class or an class with empty classes. */
>
> static bool
> is_empty_record (tree type)
> {
> if (type == error_mark_node)
> return false;
>
> if (!CLASS_TYPE_P (type))
> return false;
>
> if (CLASSTYPE_NON_LAYOUT_POD_P (type))
> return false;
>
> gcc_assert (COMPLETE_TYPE_P (type));
>
> if (CLASSTYPE_EMPTY_P (type))
> return true;
>
> tree field;
>
> for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
> if (TREE_CODE (field) == FIELD_DECL
> && !DECL_ARTIFICIAL (field)
> && !is_empty_record (TREE_TYPE (field)))
> return false;
>
> return true;
> }
So you say that K1 in e.g.:
struct A1 {}; struct A2 {};
struct B1 { A1 a; A2 b; }; struct B2 { A1 a; A2 b; };
struct C1 { B1 a; B2 b; }; struct C2 { B1 a; B2 b; };
struct D1 { C1 a; C2 b; }; struct D2 { C1 a; C2 b; };
struct E1 { D1 a; D2 b; }; struct E2 { D1 a; D2 b; };
struct F1 { E1 a; E2 b; }; struct F2 { E1 a; E2 b; };
struct G1 { F1 a; F2 b; }; struct G2 { F1 a; F2 b; };
struct H1 { G1 a; G2 b; }; struct H2 { G1 a; G2 b; };
struct I1 { H1 a; H2 b; }; struct I2 { H1 a; H2 b; };
struct J1 { I1 a; I2 b; }; struct J2 { I1 a; I2 b; };
struct K1 { J1 a; J2 b; };
int v;
__attribute__((noinline, noclone))
K1 foo (int a, K1 x, int b)
{
v = a + b;
return x;
}
K1 k, m;
void
bar (void)
{
m = foo (1, k, 2);
}
is empty class? What does clang do with this?
Jakub
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Jan 26, 2016 at 12:44 PM, Marc Glisse wrote:
> On Tue, 26 Jan 2016, H.J. Lu wrote:
>
>> On Tue, Jan 26, 2016 at 12:23 PM, Marc Glisse
>> wrote:
>>>
>>> On Tue, 26 Jan 2016, H.J. Lu wrote:
>>>
On Tue, Jan 26, 2016 at 11:27 AM, Jason Merrill
wrote:
>
>
> On 12/14/2015 05:08 PM, H.J. Lu wrote:
>>
>>
>>
>> + if (abi_version_at_least (10))
>> +TYPE_EMPTY_RECORD (t) = is_really_empty_class (t);
>
>
>
>
> This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to
> change how classes with just a vptr are passed.
>
> Otherwise, it looks OK to me.
Is true_type an empty class here? is_empty_class returns false
on this:
>>>
>>>
>>>
>>> It isn't empty in the usual C++ sense (we can't apply the empty base
>>> optimization to something that derives from it, for instance), or the one
>>> described in the itanium ABI (the relevant one here I guess). On the
>>> other
>>> hand, it is rather useless to pass it by value, so a different notion of
>>
>>
>> llvm/clang treats it as empty class and I think it should be treated
>> as "empty" class.
>
>
> Is it still empty if there are several empty members? Is there a clear
> definition somewhere of what empty means? I guess it makes sense to
> recursively allow "empty" members for this purpose.
Like this:
/* Returns true if TYPE is POD for the purpose of layout and an empty
class or an class with empty classes. */
static bool
is_empty_record (tree type)
{
if (type == error_mark_node)
return false;
if (!CLASS_TYPE_P (type))
return false;
if (CLASSTYPE_NON_LAYOUT_POD_P (type))
return false;
gcc_assert (COMPLETE_TYPE_P (type));
if (CLASSTYPE_EMPTY_P (type))
return true;
tree field;
for (field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
if (TREE_CODE (field) == FIELD_DECL
&& !DECL_ARTIFICIAL (field)
&& !is_empty_record (TREE_TYPE (field)))
return false;
return true;
}
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, 26 Jan 2016, H.J. Lu wrote:
On Tue, Jan 26, 2016 at 12:23 PM, Marc Glisse wrote:
On Tue, 26 Jan 2016, H.J. Lu wrote:
On Tue, Jan 26, 2016 at 11:27 AM, Jason Merrill wrote:
On 12/14/2015 05:08 PM, H.J. Lu wrote:
+ if (abi_version_at_least (10))
+TYPE_EMPTY_RECORD (t) = is_really_empty_class (t);
This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to
change how classes with just a vptr are passed.
Otherwise, it looks OK to me.
Is true_type an empty class here? is_empty_class returns false
on this:
It isn't empty in the usual C++ sense (we can't apply the empty base
optimization to something that derives from it, for instance), or the one
described in the itanium ABI (the relevant one here I guess). On the other
hand, it is rather useless to pass it by value, so a different notion of
llvm/clang treats it as empty class and I think it should be treated
as "empty" class.
Is it still empty if there are several empty members? Is there a clear
definition somewhere of what empty means? I guess it makes sense to
recursively allow "empty" members for this purpose.
empty might have been useful when the ABI was defined...
I proposed to update x86-64 psABI:
https://groups.google.com/forum/#!topic/x86-64-abi/VTE-LJ9VnDk
Does the full document have a definition of empty anywhere?
[hjl@gnu-skl-1 gcc]$ cat x.cc
struct dummy { };
struct true_type { struct dummy i; };
extern true_type y;
extern void xxx (true_type c);
void
yyy (void)
{
xxx (y);
}
[hjl@gnu-skl-1 gcc]$
--
Marc Glisse
--
Marc Glisse
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Jan 26, 2016 at 12:23 PM, Marc Glisse wrote:
> On Tue, 26 Jan 2016, H.J. Lu wrote:
>
>> On Tue, Jan 26, 2016 at 11:27 AM, Jason Merrill wrote:
>>>
>>> On 12/14/2015 05:08 PM, H.J. Lu wrote:
+ if (abi_version_at_least (10))
+TYPE_EMPTY_RECORD (t) = is_really_empty_class (t);
>>>
>>>
>>>
>>> This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to
>>> change how classes with just a vptr are passed.
>>>
>>> Otherwise, it looks OK to me.
>>
>>
>> Is true_type an empty class here? is_empty_class returns false
>> on this:
>
>
> It isn't empty in the usual C++ sense (we can't apply the empty base
> optimization to something that derives from it, for instance), or the one
> described in the itanium ABI (the relevant one here I guess). On the other
> hand, it is rather useless to pass it by value, so a different notion of
llvm/clang treats it as empty class and I think it should be treated
as "empty" class.
> empty might have been useful when the ABI was defined...
I proposed to update x86-64 psABI:
https://groups.google.com/forum/#!topic/x86-64-abi/VTE-LJ9VnDk
>
>> [hjl@gnu-skl-1 gcc]$ cat x.cc
>> struct dummy { };
>> struct true_type { struct dummy i; };
>>
>> extern true_type y;
>> extern void xxx (true_type c);
>>
>> void
>> yyy (void)
>> {
>> xxx (y);
>> }
>> [hjl@gnu-skl-1 gcc]$
>
>
> --
> Marc Glisse
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, 26 Jan 2016, H.J. Lu wrote:
On Tue, Jan 26, 2016 at 11:27 AM, Jason Merrill wrote:
On 12/14/2015 05:08 PM, H.J. Lu wrote:
+ if (abi_version_at_least (10))
+TYPE_EMPTY_RECORD (t) = is_really_empty_class (t);
This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to
change how classes with just a vptr are passed.
Otherwise, it looks OK to me.
Is true_type an empty class here? is_empty_class returns false
on this:
It isn't empty in the usual C++ sense (we can't apply the empty base
optimization to something that derives from it, for instance), or the one
described in the itanium ABI (the relevant one here I guess). On the other
hand, it is rather useless to pass it by value, so a different notion of
empty might have been useful when the ABI was defined...
[hjl@gnu-skl-1 gcc]$ cat x.cc
struct dummy { };
struct true_type { struct dummy i; };
extern true_type y;
extern void xxx (true_type c);
void
yyy (void)
{
xxx (y);
}
[hjl@gnu-skl-1 gcc]$
--
Marc Glisse
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Jan 26, 2016 at 11:27 AM, Jason Merrill wrote:
> On 12/14/2015 05:08 PM, H.J. Lu wrote:
>>
>> + if (abi_version_at_least (10))
>> +TYPE_EMPTY_RECORD (t) = is_really_empty_class (t);
>
>
> This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to
> change how classes with just a vptr are passed.
>
> Otherwise, it looks OK to me.
Is true_type an empty class here? is_empty_class returns false
on this:
[hjl@gnu-skl-1 gcc]$ cat x.cc
struct dummy { };
struct true_type { struct dummy i; };
extern true_type y;
extern void xxx (true_type c);
void
yyy (void)
{
xxx (y);
}
[hjl@gnu-skl-1 gcc]$
--
H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 12/14/2015 05:08 PM, H.J. Lu wrote: + if (abi_version_at_least (10)) +TYPE_EMPTY_RECORD (t) = is_really_empty_class (t); This should use is_empty_class or CLASSTYPE_EMPTY_P. We don't want to change how classes with just a vptr are passed. Otherwise, it looks OK to me. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Mon, Dec 14, 2015 at 12:43 PM, Jason Merrill wrote: > On 12/14/2015 03:39 PM, H.J. Lu wrote: >> >> On Mon, Dec 14, 2015 at 12:16 PM, Jason Merrill wrote: >>> >>> On 12/12/2015 01:42 PM, Marc Glisse wrote: On Sat, 12 Dec 2015, Jakub Jelinek wrote: > On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: >> >> >> On 12/11/2015 06:52 PM, H.J. Lu wrote: >>> >>> >>> On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener >>> wrote: On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: > > > On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: >> >> >> >> Empty C++ class is a corner case which isn't covered in psABI nor >> C++ ABI. >> There is no mention of "empty record" in GCC documentation. But >> there are >> plenty of "empty class" in gcc/cp. This change affects all >> targets. C++ ABI >> should specify how it should be passed. About this patch, aren't we supposed to enable new C++ ABIs with -fabi-version=42 (or whatever the next number is)? >>> >>> >>> >>> Yes, the patch should definitely make this conditional on >>> abi_version_at_least. >>> > There is a C++ ABI mailinglist, where you could discuss this issue: > http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. >>> >>> >>> >>> It is agreed that GCC is wrong on this: >>> >>> >>> >>> http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html >>> >> >> Yes, I think this is just a (nasty) bug on some GCC targets. > > > > Well, the argument in that thread is weird, because C and C++ empty > structs > are different, so it isn't surprising they are passed differently. > C++ makes those sizeof == 1, while C has them sizeof == 0. Maybe it isn't surprising, but it isn't particularly helpful either. It increases the number of places where the 2 are incompatible. (I personally don't care about empty C structs) >>> >>> >>> >>> Yep. The C standard doesn't have empty structs; it's a GNU extension. >>> But >>> in any case argument passing can be compatible between C and C++, so it >>> really should be. >>> >>> >> >> Before I make any changes, I'd like to ask if we should make >> argument passing can be compatible between C and C++ for >> all targets GCC support or just x86. > > > All. Here is the patch to guard this ABI change with the ABI level 10, which is updated in GCC 6. OK for master if there is no regression on x86? The patch for non-x86 targets is at https://gcc.gnu.org/ml/gcc-patches/2015-12/msg01063.html -- H.J. From fccd449a091589fedaf6ee4998271a16d93147fc Mon Sep 17 00:00:00 2001 From: "H.J. Lu" Date: Sun, 15 Nov 2015 13:19:05 -0800 Subject: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class Empty record should be returned and passed the same way in C and C++. This patch overloads a bit, side_effects_flag, in tree_base for C++ empty class. Middle-end and x86 backend are updated to ignore empty records for parameter passing and function value return. Other targets may need similar changes. get_ref_base_and_extent is changed to set bitsize to 0 for empty records so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to get 0 as the maximum size on empty record. Otherwise, find_tail_calls won't perform tail call optimization for functions with empty record parameters, as shown in g++.dg/pr60336-1.C and g++.dg/pr60336-2.C. This ABI change is enabled only if the ABI level is at least 10, which is updated in GCC 6. gcc/ PR c++/60336 PR middle-end/67239 PR target/68355 * calls.c (initialize_argument_information): Warn empty record if they are used in a variable argument list or aren't the last arguments. Replace targetm.calls.function_arg, targetm.calls.function_incoming_arg and targetm.calls.function_arg_advance with function_arg, function_incoming_arg and function_arg_advance. (expand_call): Replace targetm.calls.function_arg, targetm.calls.function_incoming_arg and targetm.calls.function_arg_advance with function_arg, function_incoming_arg and function_arg_advance. (emit_library_call_value_1): Likewise. (store_one_arg): Use 0 for empty record size. Don't push 0 size argument onto stack. (must_pass_in_stack_var_size_or_pad): Return false for empty record. * dse.c (get_call_args): Replace targetm.calls.function_arg and targetm.calls.function_arg_advance with function_arg and function_arg_advance. * expr.c (block_move_libcall_safe_for_call_parm): Likewise. * function.c (aggregate_value_p): Replace targetm.calls.return_in_memory with return_in_memory. (assign_parm_data_all): Add warn_empty_record. (assign_par
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 12/14/2015 03:39 PM, H.J. Lu wrote: On Mon, Dec 14, 2015 at 12:16 PM, Jason Merrill wrote: On 12/12/2015 01:42 PM, Marc Glisse wrote: On Sat, 12 Dec 2015, Jakub Jelinek wrote: On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: On 12/11/2015 06:52 PM, H.J. Lu wrote: On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener wrote: On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. About this patch, aren't we supposed to enable new C++ ABIs with -fabi-version=42 (or whatever the next number is)? Yes, the patch should definitely make this conditional on abi_version_at_least. There is a C++ ABI mailinglist, where you could discuss this issue: http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. It is agreed that GCC is wrong on this: http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html Yes, I think this is just a (nasty) bug on some GCC targets. Well, the argument in that thread is weird, because C and C++ empty structs are different, so it isn't surprising they are passed differently. C++ makes those sizeof == 1, while C has them sizeof == 0. Maybe it isn't surprising, but it isn't particularly helpful either. It increases the number of places where the 2 are incompatible. (I personally don't care about empty C structs) Yep. The C standard doesn't have empty structs; it's a GNU extension. But in any case argument passing can be compatible between C and C++, so it really should be. Before I make any changes, I'd like to ask if we should make argument passing can be compatible between C and C++ for all targets GCC support or just x86. All. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Mon, Dec 14, 2015 at 12:16 PM, Jason Merrill wrote: > On 12/12/2015 01:42 PM, Marc Glisse wrote: >> >> On Sat, 12 Dec 2015, Jakub Jelinek wrote: >> >>> On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: On 12/11/2015 06:52 PM, H.J. Lu wrote: > > On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener > wrote: >> >> On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf >> wrote: >>> >>> On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. >> >> >> >> About this patch, aren't we supposed to enable new C++ ABIs with >> -fabi-version=42 (or whatever the next number is)? > > > Yes, the patch should definitely make this conditional on > abi_version_at_least. > >>> There is a C++ ABI mailinglist, where you could discuss this issue: >>> http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev >> >> >> Yep. As long as the ABI doesn't state how to pass those I'd rather >> _not_ change GCCs way. > > > It is agreed that GCC is wrong on this: > > > http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html > Yes, I think this is just a (nasty) bug on some GCC targets. >>> >>> >>> Well, the argument in that thread is weird, because C and C++ empty >>> structs >>> are different, so it isn't surprising they are passed differently. >>> C++ makes those sizeof == 1, while C has them sizeof == 0. >> >> >> Maybe it isn't surprising, but it isn't particularly helpful either. It >> increases the number of places where the 2 are incompatible. >> (I personally don't care about empty C structs) > > > Yep. The C standard doesn't have empty structs; it's a GNU extension. But > in any case argument passing can be compatible between C and C++, so it > really should be. > > Before I make any changes, I'd like to ask if we should make argument passing can be compatible between C and C++ for all targets GCC support or just x86. -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 12/12/2015 01:42 PM, Marc Glisse wrote: On Sat, 12 Dec 2015, Jakub Jelinek wrote: On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: On 12/11/2015 06:52 PM, H.J. Lu wrote: On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener wrote: On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. About this patch, aren't we supposed to enable new C++ ABIs with -fabi-version=42 (or whatever the next number is)? Yes, the patch should definitely make this conditional on abi_version_at_least. There is a C++ ABI mailinglist, where you could discuss this issue: http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. It is agreed that GCC is wrong on this: http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html Yes, I think this is just a (nasty) bug on some GCC targets. Well, the argument in that thread is weird, because C and C++ empty structs are different, so it isn't surprising they are passed differently. C++ makes those sizeof == 1, while C has them sizeof == 0. Maybe it isn't surprising, but it isn't particularly helpful either. It increases the number of places where the 2 are incompatible. (I personally don't care about empty C structs) Yep. The C standard doesn't have empty structs; it's a GNU extension. But in any case argument passing can be compatible between C and C++, so it really should be. Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Sat, 12 Dec 2015, Jakub Jelinek wrote: On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: On 12/11/2015 06:52 PM, H.J. Lu wrote: On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener wrote: On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. About this patch, aren't we supposed to enable new C++ ABIs with -fabi-version=42 (or whatever the next number is)? There is a C++ ABI mailinglist, where you could discuss this issue: http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. It is agreed that GCC is wrong on this: http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html Yes, I think this is just a (nasty) bug on some GCC targets. Well, the argument in that thread is weird, because C and C++ empty structs are different, so it isn't surprising they are passed differently. C++ makes those sizeof == 1, while C has them sizeof == 0. Maybe it isn't surprising, but it isn't particularly helpful either. It increases the number of places where the 2 are incompatible. (I personally don't care about empty C structs) So I rather think clang should change rather than GCC. Passing empty arguments (with trivial copy constructor, etc) is a complete waste. Not passing them is strictly superior. The only reason to prefer passing them is that that's what gcc does and changing would break the ABI. I could understand clang being unhappy if they have to both break ABI compatibility with their older versions and move towards an inferior ABI... (arguably they could have copied the de facto gcc ABI to begin with) They might be willing to do it on linux-x86, but not on ios/mac where they dominate and compatibility with their own earlier versions matters more than compatibility with gcc? -- Marc Glisse
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Sat, Dec 12, 2015 at 7:27 AM, Jakub Jelinek wrote: > On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: >> On 12/11/2015 06:52 PM, H.J. Lu wrote: >> >On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener >> > wrote: >> >>On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf >> >> wrote: >> >>>On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: >> >> Empty C++ class is a corner case which isn't covered in psABI nor C++ >> ABI. >> There is no mention of "empty record" in GCC documentation. But there >> are >> plenty of "empty class" in gcc/cp. This change affects all targets. >> C++ ABI >> should specify how it should be passed. >> >>> >> >>>There is a C++ ABI mailinglist, where you could discuss this issue: >> >>>http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev >> >> >> >>Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ >> >>change >> >>GCCs way. >> > >> >It is agreed that GCC is wrong on this: >> > >> >http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html >> >> Yes, I think this is just a (nasty) bug on some GCC targets. > > Well, the argument in that thread is weird, because C and C++ empty structs > are different, so it isn't surprising they are passed differently. > C++ makes those sizeof == 1, while C has them sizeof == 0. > So I rather think clang should change rather than GCC. > But according to x86-64 psABI, sizeof == 1 should be passed in register, not on stack. This leads to weird GCC bugs like: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67239 -- H.J.
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Sat, Dec 12, 2015 at 09:51:23AM -0500, Jason Merrill wrote: > On 12/11/2015 06:52 PM, H.J. Lu wrote: > >On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener > > wrote: > >>On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf > >> wrote: > >>>On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: > > Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. > There is no mention of "empty record" in GCC documentation. But there are > plenty of "empty class" in gcc/cp. This change affects all targets. C++ > ABI > should specify how it should be passed. > >>> > >>>There is a C++ ABI mailinglist, where you could discuss this issue: > >>>http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev > >> > >>Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ > >>change > >>GCCs way. > > > >It is agreed that GCC is wrong on this: > > > >http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html > > Yes, I think this is just a (nasty) bug on some GCC targets. Well, the argument in that thread is weird, because C and C++ empty structs are different, so it isn't surprising they are passed differently. C++ makes those sizeof == 1, while C has them sizeof == 0. So I rather think clang should change rather than GCC. Jakub
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 12/11/2015 06:52 PM, H.J. Lu wrote: On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener wrote: On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. There is no mention of "empty record" in GCC documentation. But there are plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI should specify how it should be passed. There is a C++ ABI mailinglist, where you could discuss this issue: http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. It is agreed that GCC is wrong on this: http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html Yes, I think this is just a (nasty) bug on some GCC targets. Here is the updated patch. I updated -WpsABI to warn empty record which are passed in a variable argument list or aren't the last arguments. They are triggered in: /export/build/gnu/gcc-x32/build-x86_64-linux/prev-x86_64-pc-linux-gnu/libstdc++-v3/include/bits/hashtable.h:1507:7: note: the ABI of passing empty record has changed in GCC 6 > Oof. Well, at least it's all C++11 stuff, and GCC 5 still defaulted to C++98... Jason
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Thu, Dec 10, 2015 at 3:24 AM, Richard Biener wrote: > On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf > wrote: >> On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: >>> >>> Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. >>> There is no mention of "empty record" in GCC documentation. But there are >>> plenty of "empty class" in gcc/cp. This change affects all targets. C++ >>> ABI >>> should specify how it should be passed. >> >> There is a C++ ABI mailinglist, where you could discuss this issue: >> http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev > > Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ > change > GCCs way. It is agreed that GCC is wrong on this: http://sourcerytools.com/pipermail/cxx-abi-dev/2015-December/002876.html Here is the updated patch. I updated -WpsABI to warn empty record which are passed in a variable argument list or aren't the last arguments. They are triggered in: /export/build/gnu/gcc-x32/build-x86_64-linux/prev-x86_64-pc-linux-gnu/libstdc++-v3/include/bits/hashtable.h:1507:7: note: the ABI of passing empty record has changed in GCC 6 /export/build/gnu/gcc-x32/build-x86_64-linux/prev-x86_64-pc-linux-gnu/libstdc++-v3/include/bits/hashtable_policy.h:901:67: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:238:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:266:26: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:273:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:289:61: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:296:11: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:304:11: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:312:11: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:320:11: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:328:11: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:341:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:375:19: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:375:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:390:19: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:390:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:415:16: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:425:12: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:442:29: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:449:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:457:4: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:500:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:529:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:547:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:569:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:617:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/git/sources/gcc/libstdc++-v3/src/c++11/cxx11-shim_facets.cc:637:5: note: the ABI of passing empty record has changed in GCC 6 /export/gnu/import/
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Dec 9, 2015 at 10:31 PM, Markus Trippelsdorf wrote: > On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: >> >> Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. >> There is no mention of "empty record" in GCC documentation. But there are >> plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI >> should specify how it should be passed. > > There is a C++ ABI mailinglist, where you could discuss this issue: > http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev Yep. As long as the ABI doesn't state how to pass those I'd rather _not_ change GCCs way. Richard. > -- > Markus
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On 2015.12.09 at 10:53 -0800, H.J. Lu wrote: > > Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI. > There is no mention of "empty record" in GCC documentation. But there are > plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI > should specify how it should be passed. There is a C++ ABI mailinglist, where you could discuss this issue: http://sourcerytools.com/cgi-bin/mailman/listinfo/cxx-abi-dev -- Markus
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Dec 9, 2015 at 10:53 AM, H.J. Lu wrote:
> On Wed, Dec 9, 2015 at 6:05 AM, Richard Biener
> wrote:
>> On Tue, Dec 8, 2015 at 5:22 PM, H.J. Lu wrote:
>>> On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu wrote:
On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener
wrote:
> On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu wrote:
>> On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill wrote:
>>> On 11/20/2015 01:52 PM, H.J. Lu wrote:
On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener
wrote:
>
> On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu wrote:
>>
>> Empty record should be returned and passed the same way in C and C++.
>> This patch adds LANG_HOOKS_EMPTY_RECORD_P for C++ empty class, which
>> defaults to return false. For C++, LANG_HOOKS_EMPTY_RECORD_P is
>> defined
>> to is_really_empty_class, which returns true for C++ empty classes.
>> For
>> LTO, we stream out a bit to indicate if a record is empty and we
>> store
>> it in TYPE_LANG_FLAG_0 when streaming in. get_ref_base_and_extent is
>> changed to set bitsize to 0 for empty records. Middle-end and x86
>> backend are updated to ignore empty records for parameter passing and
>> function value return. Other targets may need similar changes.
>
>
> Please avoid a new langhook for this and instead claim a bit in
> tree_type_common
> like for example restrict_flag (double-check it is unused for
> non-pointers).
There is no bit in tree_type_common I can overload. restrict_flag is
checked for non-pointers to issue an error when it is used on
non-pointers:
/export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/template/qualttp20.C:19:38:
error: ‘__restrict__’ qualifiers cannot be applied to ‘AS::L’
typedef typename T::L __restrict__ r;// { dg-error "'__restrict__'
qualifiers cannot" "" }
>>>
>>>
>>> The C++ front end only needs to check TYPE_RESTRICT for this purpose on
>>> front-end-specific type codes like TEMPLATE_TYPE_PARM; cp_type_quals
>>> could
>>> handle that specifically if you change TYPE_RESTRICT to only apply to
>>> pointers.
>>>
>>
>> restrict_flag is also checked in this case:
>>
>> [hjl@gnu-6 gcc]$ cat x.i
>> struct dummy { };
>>
>> struct dummy
>> foo (struct dummy __restrict__ i)
>> {
>> return i;
>> }
>> [hjl@gnu-6 gcc]$ gcc -S x.i -Wall
>> x.i:4:13: error: invalid use of ‘restrict’
>> foo (struct dummy __restrict__ i)
>> ^
>> x.i:4:13: error: invalid use of ‘restrict’
>> [hjl@gnu-6 gcc]$
>>
>> restrict_flag can't also be used to indicate `i' is an empty record.
>
> I'm sure this error can be done during parsing w/o relying on
> TYPE_RESTRICT.
>
> But well, use any other free bit (but do not enlarge
> tree_type_common). Eventually
> you can free up a bit by putting sth into type_lang_specific currently
> using bits
> in tree_type_common.
There are no bits in tree_type_common I can move. Instead,
this patch overloads side_effects_flag in tree_base. Tested on
Linux/x86-64. OK for trunk?
>>
>> I'm fine with using side_effects_flag for this.
>>
>> I miss an explanation of where this detail of the ABI is documented and
>> wonder
>> if the term "empty record" is also used in that document and how it is
>> documented.
>>
>> Thus
>>
>> +/* Nonzero in a type considered an empty record. */
>> +#define TYPE_EMPTY_RECORD(NODE) \
>> + (TYPE_CHECK (NODE)->base.side_effects_flag)
>>
>> should refer to the ABI where is defined what an "empty record" is and how
>> it is handled by the backend(s).
>
> Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI.
> There is no mention of "empty record" in GCC documentation. But there are
> plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI
> should specify how it should be passed.
>
>> +/* Return true if type T is an empty record. */
>> +
>> +static inline bool
>> +type_is_empty_record_p (const_tree t)
>> +{
>> + return TYPE_EMPTY_RECORD (TYPE_MAIN_VARIANT (t));
>>
>> the type checker should probably check the bit is consistent across
>> variants so it can be tested on any of them.
>
> TYPE_EMPTY_RECORD is only relevant for parameter passing. For
>
> struct foo;
> typedef foo bar;
>
> TYPE_EMPTY_RECORD has no impact. Since call only uses
> the main variant type, checking TYPE_MAIN_VARIANT is sufficient.
>
>> You fail to adjust other targets gimplification hooks which suggests
>> this is a detail of the x86 psABI and not the C++ ABI? If so I miss
>> a -Wpsabi warning for this change.
>
> My change is generic. The only x86 specific change is
>
> diff --git a/gcc/config/i386/i38
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Wed, Dec 9, 2015 at 6:05 AM, Richard Biener
wrote:
> On Tue, Dec 8, 2015 at 5:22 PM, H.J. Lu wrote:
>> On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu wrote:
>>> On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener
>>> wrote:
On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu wrote:
> On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill wrote:
>> On 11/20/2015 01:52 PM, H.J. Lu wrote:
>>>
>>> On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener
>>> wrote:
On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu wrote:
>
> Empty record should be returned and passed the same way in C and C++.
> This patch adds LANG_HOOKS_EMPTY_RECORD_P for C++ empty class, which
> defaults to return false. For C++, LANG_HOOKS_EMPTY_RECORD_P is
> defined
> to is_really_empty_class, which returns true for C++ empty classes.
> For
> LTO, we stream out a bit to indicate if a record is empty and we store
> it in TYPE_LANG_FLAG_0 when streaming in. get_ref_base_and_extent is
> changed to set bitsize to 0 for empty records. Middle-end and x86
> backend are updated to ignore empty records for parameter passing and
> function value return. Other targets may need similar changes.
Please avoid a new langhook for this and instead claim a bit in
tree_type_common
like for example restrict_flag (double-check it is unused for
non-pointers).
>>>
>>>
>>> There is no bit in tree_type_common I can overload. restrict_flag is
>>> checked for non-pointers to issue an error when it is used on
>>> non-pointers:
>>>
>>>
>>> /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/template/qualttp20.C:19:38:
>>> error: ‘__restrict__’ qualifiers cannot be applied to ‘AS::L’
>>> typedef typename T::L __restrict__ r;// { dg-error "'__restrict__'
>>> qualifiers cannot" "" }
>>
>>
>> The C++ front end only needs to check TYPE_RESTRICT for this purpose on
>> front-end-specific type codes like TEMPLATE_TYPE_PARM; cp_type_quals
>> could
>> handle that specifically if you change TYPE_RESTRICT to only apply to
>> pointers.
>>
>
> restrict_flag is also checked in this case:
>
> [hjl@gnu-6 gcc]$ cat x.i
> struct dummy { };
>
> struct dummy
> foo (struct dummy __restrict__ i)
> {
> return i;
> }
> [hjl@gnu-6 gcc]$ gcc -S x.i -Wall
> x.i:4:13: error: invalid use of ‘restrict’
> foo (struct dummy __restrict__ i)
> ^
> x.i:4:13: error: invalid use of ‘restrict’
> [hjl@gnu-6 gcc]$
>
> restrict_flag can't also be used to indicate `i' is an empty record.
I'm sure this error can be done during parsing w/o relying on
TYPE_RESTRICT.
But well, use any other free bit (but do not enlarge
tree_type_common). Eventually
you can free up a bit by putting sth into type_lang_specific currently
using bits
in tree_type_common.
>>>
>>> There are no bits in tree_type_common I can move. Instead,
>>> this patch overloads side_effects_flag in tree_base. Tested on
>>> Linux/x86-64. OK for trunk?
>
> I'm fine with using side_effects_flag for this.
>
> I miss an explanation of where this detail of the ABI is documented and wonder
> if the term "empty record" is also used in that document and how it is
> documented.
>
> Thus
>
> +/* Nonzero in a type considered an empty record. */
> +#define TYPE_EMPTY_RECORD(NODE) \
> + (TYPE_CHECK (NODE)->base.side_effects_flag)
>
> should refer to the ABI where is defined what an "empty record" is and how
> it is handled by the backend(s).
Empty C++ class is a corner case which isn't covered in psABI nor C++ ABI.
There is no mention of "empty record" in GCC documentation. But there are
plenty of "empty class" in gcc/cp. This change affects all targets. C++ ABI
should specify how it should be passed.
> +/* Return true if type T is an empty record. */
> +
> +static inline bool
> +type_is_empty_record_p (const_tree t)
> +{
> + return TYPE_EMPTY_RECORD (TYPE_MAIN_VARIANT (t));
>
> the type checker should probably check the bit is consistent across
> variants so it can be tested on any of them.
TYPE_EMPTY_RECORD is only relevant for parameter passing. For
struct foo;
typedef foo bar;
TYPE_EMPTY_RECORD has no impact. Since call only uses
the main variant type, checking TYPE_MAIN_VARIANT is sufficient.
> You fail to adjust other targets gimplification hooks which suggests
> this is a detail of the x86 psABI and not the C++ ABI? If so I miss
> a -Wpsabi warning for this change.
My change is generic. The only x86 specific change is
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index d30fbff..308d9a4e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -10292,7 +10292,8 @@ ix86_gimplify_va_arg (tree valist, tree type, gimple_seq
*pre_p,
Re: PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Tue, Dec 8, 2015 at 5:22 PM, H.J. Lu wrote:
> On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu wrote:
>> On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener
>> wrote:
>>> On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu wrote:
On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill wrote:
> On 11/20/2015 01:52 PM, H.J. Lu wrote:
>>
>> On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener
>> wrote:
>>>
>>> On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu wrote:
Empty record should be returned and passed the same way in C and C++.
This patch adds LANG_HOOKS_EMPTY_RECORD_P for C++ empty class, which
defaults to return false. For C++, LANG_HOOKS_EMPTY_RECORD_P is
defined
to is_really_empty_class, which returns true for C++ empty classes.
For
LTO, we stream out a bit to indicate if a record is empty and we store
it in TYPE_LANG_FLAG_0 when streaming in. get_ref_base_and_extent is
changed to set bitsize to 0 for empty records. Middle-end and x86
backend are updated to ignore empty records for parameter passing and
function value return. Other targets may need similar changes.
>>>
>>>
>>> Please avoid a new langhook for this and instead claim a bit in
>>> tree_type_common
>>> like for example restrict_flag (double-check it is unused for
>>> non-pointers).
>>
>>
>> There is no bit in tree_type_common I can overload. restrict_flag is
>> checked for non-pointers to issue an error when it is used on
>> non-pointers:
>>
>>
>> /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/template/qualttp20.C:19:38:
>> error: ‘__restrict__’ qualifiers cannot be applied to ‘AS::L’
>> typedef typename T::L __restrict__ r;// { dg-error "'__restrict__'
>> qualifiers cannot" "" }
>
>
> The C++ front end only needs to check TYPE_RESTRICT for this purpose on
> front-end-specific type codes like TEMPLATE_TYPE_PARM; cp_type_quals could
> handle that specifically if you change TYPE_RESTRICT to only apply to
> pointers.
>
restrict_flag is also checked in this case:
[hjl@gnu-6 gcc]$ cat x.i
struct dummy { };
struct dummy
foo (struct dummy __restrict__ i)
{
return i;
}
[hjl@gnu-6 gcc]$ gcc -S x.i -Wall
x.i:4:13: error: invalid use of ‘restrict’
foo (struct dummy __restrict__ i)
^
x.i:4:13: error: invalid use of ‘restrict’
[hjl@gnu-6 gcc]$
restrict_flag can't also be used to indicate `i' is an empty record.
>>>
>>> I'm sure this error can be done during parsing w/o relying on TYPE_RESTRICT.
>>>
>>> But well, use any other free bit (but do not enlarge
>>> tree_type_common). Eventually
>>> you can free up a bit by putting sth into type_lang_specific currently
>>> using bits
>>> in tree_type_common.
>>
>> There are no bits in tree_type_common I can move. Instead,
>> this patch overloads side_effects_flag in tree_base. Tested on
>> Linux/x86-64. OK for trunk?
I'm fine with using side_effects_flag for this.
I miss an explanation of where this detail of the ABI is documented and wonder
if the term "empty record" is also used in that document and how it is
documented.
Thus
+/* Nonzero in a type considered an empty record. */
+#define TYPE_EMPTY_RECORD(NODE) \
+ (TYPE_CHECK (NODE)->base.side_effects_flag)
should refer to the ABI where is defined what an "empty record" is and how
it is handled by the backend(s).
+/* Return true if type T is an empty record. */
+
+static inline bool
+type_is_empty_record_p (const_tree t)
+{
+ return TYPE_EMPTY_RECORD (TYPE_MAIN_VARIANT (t));
the type checker should probably check the bit is consistent across
variants so it can be tested on any of them.
You fail to adjust other targets gimplification hooks which suggests
this is a detail of the x86 psABI and not the C++ ABI? If so I miss
a -Wpsabi warning for this change.
Did you investigate the effect of this patch on a larger code base?
More specifically does it only affect variadic functions?
An entry for changes.html is warranted as well.
Thanks,
Richard.
>> Thanks.
>>
>> --
>> H.J.
>
>
>
> --
> H.J.
PING^1: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
On Mon, Nov 23, 2015 at 12:53 PM, H.J. Lu wrote:
> On Mon, Nov 23, 2015 at 1:57 AM, Richard Biener
> wrote:
>> On Sat, Nov 21, 2015 at 12:46 AM, H.J. Lu wrote:
>>> On Fri, Nov 20, 2015 at 2:17 PM, Jason Merrill wrote:
On 11/20/2015 01:52 PM, H.J. Lu wrote:
>
> On Tue, Nov 17, 2015 at 4:22 AM, Richard Biener
> wrote:
>>
>> On Tue, Nov 17, 2015 at 12:01 PM, H.J. Lu wrote:
>>>
>>> Empty record should be returned and passed the same way in C and C++.
>>> This patch adds LANG_HOOKS_EMPTY_RECORD_P for C++ empty class, which
>>> defaults to return false. For C++, LANG_HOOKS_EMPTY_RECORD_P is defined
>>> to is_really_empty_class, which returns true for C++ empty classes. For
>>> LTO, we stream out a bit to indicate if a record is empty and we store
>>> it in TYPE_LANG_FLAG_0 when streaming in. get_ref_base_and_extent is
>>> changed to set bitsize to 0 for empty records. Middle-end and x86
>>> backend are updated to ignore empty records for parameter passing and
>>> function value return. Other targets may need similar changes.
>>
>>
>> Please avoid a new langhook for this and instead claim a bit in
>> tree_type_common
>> like for example restrict_flag (double-check it is unused for
>> non-pointers).
>
>
> There is no bit in tree_type_common I can overload. restrict_flag is
> checked for non-pointers to issue an error when it is used on
> non-pointers:
>
>
> /export/gnu/import/git/sources/gcc/gcc/testsuite/g++.dg/template/qualttp20.C:19:38:
> error: ‘__restrict__’ qualifiers cannot be applied to ‘AS::L’
> typedef typename T::L __restrict__ r;// { dg-error "'__restrict__'
> qualifiers cannot" "" }
The C++ front end only needs to check TYPE_RESTRICT for this purpose on
front-end-specific type codes like TEMPLATE_TYPE_PARM; cp_type_quals could
handle that specifically if you change TYPE_RESTRICT to only apply to
pointers.
>>>
>>> restrict_flag is also checked in this case:
>>>
>>> [hjl@gnu-6 gcc]$ cat x.i
>>> struct dummy { };
>>>
>>> struct dummy
>>> foo (struct dummy __restrict__ i)
>>> {
>>> return i;
>>> }
>>> [hjl@gnu-6 gcc]$ gcc -S x.i -Wall
>>> x.i:4:13: error: invalid use of ‘restrict’
>>> foo (struct dummy __restrict__ i)
>>> ^
>>> x.i:4:13: error: invalid use of ‘restrict’
>>> [hjl@gnu-6 gcc]$
>>>
>>> restrict_flag can't also be used to indicate `i' is an empty record.
>>
>> I'm sure this error can be done during parsing w/o relying on TYPE_RESTRICT.
>>
>> But well, use any other free bit (but do not enlarge
>> tree_type_common). Eventually
>> you can free up a bit by putting sth into type_lang_specific currently
>> using bits
>> in tree_type_common.
>
> There are no bits in tree_type_common I can move. Instead,
> this patch overloads side_effects_flag in tree_base. Tested on
> Linux/x86-64. OK for trunk?
>
> Thanks.
>
> --
> H.J.
--
H.J.
From 1e3e6ed42ed86b74d01bd3a6b869c15dba964c21 Mon Sep 17 00:00:00 2001
From: "H.J. Lu"
Date: Sun, 15 Nov 2015 13:19:05 -0800
Subject: [PATCH] Add TYPE_EMPTY_RECORD for C++ empty class
Empty record should be returned and passed the same way in C and C++.
This patch overloads a bit, side_effects_flag, in tree_base for C++
empty class. Middle-end and x86 backend are updated to ignore empty
records for parameter passing and function value return. Other targets
may need similar changes.
get_ref_base_and_extent is changed to set bitsize to 0 for empty records
so that when ref_maybe_used_by_call_p_1 calls get_ref_base_and_extent to
get 0 as the maximum size on empty record. Otherwise, find_tail_calls
won't perform tail call optimization for functions with empty record
parameters, as shown in g++.dg/pr60336-1.C and g++.dg/pr60336-2.C.
gcc/
PR c++/60336
PR middle-end/67239
PR target/68355
* calls.c (initialize_argument_information): Replace
targetm.calls.function_arg, targetm.calls.function_incoming_arg
and targetm.calls.function_arg_advance with function_arg,
function_incoming_arg and function_arg_advance.
(expand_call): Likewise.
(emit_library_call_value_1): Likewise.
(store_one_arg): Use 0 for empty record size. Don't
push 0 size argument onto stack.
(must_pass_in_stack_var_size_or_pad): Return false for empty
record.
* dse.c (get_call_args): Replace targetm.calls.function_arg
and targetm.calls.function_arg_advance with function_arg and
function_arg_advance.
* expr.c (block_move_libcall_safe_for_call_parm): Likewise.
* function.c (aggregate_value_p): Replace
targetm.calls.return_in_memory with return_in_memory.
(assign_parm_find_entry_rtl): Replace
targetm.calls.function_incoming_arg with function_incoming_arg.
(assign_parms): Replace targetm.calls.function_arg_advance with
function_arg_advance.
(gimplify_parameters): Replace targetm.calls.function_arg_advance
with function_arg_advance.
(locate_and_pad_parm): Use 0 for empty record size.
(f
