Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jozef Lawrynowicz]

On 22/08/2017 11:57, Jozef Lawrynowicz wrote:

On 02/08/2017 17:45, Joseph Myers wrote:

On Wed, 2 Aug 2017, Jeff Law wrote:


I think Joseph's suggestion for looking at partial float handling may be
useful, though ia64's RFmode may be more interesting as it's not a
multiple of 8 in bitsize.  IF/KF modes on the ppc port have similar
properties.


The key issue those floating-point modes engage is the meaning of
precision.  IFmode and KFmode have precision set to 106 and 113 to
distinguish them.  But that's precision in the sense of number of 
mantissa

bits; as normally understood in GCC, precision should be the number of
significant bits, so 128 for both those modes (but having multiple
different binary floating-point modes with the same precision would
require changing how we deal with laying out long double, so the target
specifies a mode for floating-point types rather than leaving it to be
deduced from values such as LONG_DOUBLE_TYPE_SIZE).



Thanks for the advice, I'm looking into how the ppc KFmode behaves in
this situation now.

I also looked through the front end code a bit more, and the behaviour
of stor-layout.c:layout_type for RECORD_TYPE looks likes a smoking gun
to me.
For BOOLEAN_TYPE, INTEGER_TYPE, ENUMERAL_TYPE, REAL_TYPE,
FIXED_POINT_TYPE etc. layout_type sets:

TYPE_SIZE (type) = bitsize_int (GET_MODE_BITSIZE (TYPE_MODE (type)));

But for the individual field types in RECORD_TYPE, UNION_TYPE or
QUAL_UNION_TYPE, this same setting of TYPE_SIZE and friends is not
performed.
So a field in a RECORD_TYPE might be an INTEGER_TYPE, but TYPE_SIZE for
this INTEGER_TYPE will not be set as it would have been had the type not
been a field in a RECORD_TYPE.

So the fix to me looks to be to ensure that the code for the base types
(BOOLEAN_TYPE, INTEGER_TYPE etc.) from layout_type is also executed for
each type that happens to be a field in a RECORD/UNION/QUAL_UNION_TYPE.


Actually, the issue turned out to be that TYPE_SIZE is specifically set
in the first place.
When the internal data structures to handle __intN types are initialised
in tree.c, the compiler is also setting TYPE_SIZE.

For the other "standard" types, layout_type sets TYPE_SIZE. So rather
than specifically setting TYPE_SIZE in tree.c, I've removed this code so
TYPE_SIZE will get set like it does for every other type.

If the attached patch is acceptable, I would appreciate if someone could
commit it for me, as I do not have write access.

Thanks,
Jozef
From 5437c7ffa48f974c6960a1e308c4cdf0ea0a2648 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz 
Date: Thu, 24 Aug 2017 11:40:04 +
Subject: [PATCH] MSP430: Dont specifically set TYPE_SIZE for __intN types

2017-08-XX  Jozef Lawrynowicz   

PR target/78849
* gcc/tree.c (build_common_tree_nodes): Dont set TYPE_SIZE for __intN
types.

gcc/testsuite
2017-08-XX  Jozef Lawrynowicz   

PR target/78849
* gcc.target/msp430/msp430.exp: Remove -pedantic-errors from
DEFAULT_CFLAGS.
* gcc.target/msp430/pr78849.c: New test.
---
 gcc/testsuite/gcc.target/msp430/msp430.exp | 13 +---
 gcc/testsuite/gcc.target/msp430/pr78849.c  | 50 ++
 gcc/tree.c |  2 --
 3 files changed, 59 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/msp430/pr78849.c

diff --git a/gcc/testsuite/gcc.target/msp430/msp430.exp 
b/gcc/testsuite/gcc.target/msp430/msp430.exp
index e54d531..3be8711 100644
--- a/gcc/testsuite/gcc.target/msp430/msp430.exp
+++ b/gcc/testsuite/gcc.target/msp430/msp430.exp
@@ -24,10 +24,15 @@ if { ![istarget msp430-*-*] } then {
 # Load support procs.
 load_lib gcc-dg.exp
 
-# If a testcase doesn't have special options, use these.
+# The '-pedantic-errors' option in the global variable DEFAULT_CFLAGS that is
+# set by other drivers causes an error when the __int20 type is used, so remove
+# this option from DEFAULT_CFLAGS for the msp430 tests.
 global DEFAULT_CFLAGS
-if ![info exists DEFAULT_CFLAGS] then {
-set DEFAULT_CFLAGS ""
+if [info exists DEFAULT_CFLAGS] then {
+set MSP430_DEFAULT_CFLAGS \
+  [ string map { "-pedantic-errors" "" } $DEFAULT_CFLAGS ]
+} else {
+   set MSP430_DEFAULT_CFLAGS ""
 }
 
 # Initialize `dg'.
@@ -35,7 +40,7 @@ dg-init
 
 # Main loop.
 dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
-   "" $DEFAULT_CFLAGS
+   "" $MSP430_DEFAULT_CFLAGS
 
 # All done.
 dg-finish
diff --git a/gcc/testsuite/gcc.target/msp430/pr78849.c 
b/gcc/testsuite/gcc.target/msp430/pr78849.c
new file mode 100644
index 000..f70f0bb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/pr78849.c
@@ -0,0 +1,50 @@
+/* { dg-do compile } */
+/* { dg-final { scan-assembler ".size.*instance.*52" } } */
+
+struct t_inner
+{
+  __int20 a;
+  char val1;
+  __int20 b[3];
+  char val2;
+};
+
+struct t_full
+{
+  __int20 array[2];
+  char val1;

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jozef Lawrynowicz]

On 02/08/2017 17:45, Joseph Myers wrote:

On Wed, 2 Aug 2017, Jeff Law wrote:


I think Joseph's suggestion for looking at partial float handling may be
useful, though ia64's RFmode may be more interesting as it's not a
multiple of 8 in bitsize.  IF/KF modes on the ppc port have similar
properties.


The key issue those floating-point modes engage is the meaning of
precision.  IFmode and KFmode have precision set to 106 and 113 to
distinguish them.  But that's precision in the sense of number of mantissa
bits; as normally understood in GCC, precision should be the number of
significant bits, so 128 for both those modes (but having multiple
different binary floating-point modes with the same precision would
require changing how we deal with laying out long double, so the target
specifies a mode for floating-point types rather than leaving it to be
deduced from values such as LONG_DOUBLE_TYPE_SIZE).



Thanks for the advice, I'm looking into how the ppc KFmode behaves in
this situation now.

I also looked through the front end code a bit more, and the behaviour
of stor-layout.c:layout_type for RECORD_TYPE looks likes a smoking gun
to me.
For BOOLEAN_TYPE, INTEGER_TYPE, ENUMERAL_TYPE, REAL_TYPE,
FIXED_POINT_TYPE etc. layout_type sets:

TYPE_SIZE (type) = bitsize_int (GET_MODE_BITSIZE (TYPE_MODE (type)));

But for the individual field types in RECORD_TYPE, UNION_TYPE or
QUAL_UNION_TYPE, this same setting of TYPE_SIZE and friends is not
performed.
So a field in a RECORD_TYPE might be an INTEGER_TYPE, but TYPE_SIZE for
this INTEGER_TYPE will not be set as it would have been had the type not
been a field in a RECORD_TYPE.

So the fix to me looks to be to ensure that the code for the base types
(BOOLEAN_TYPE, INTEGER_TYPE etc.) from layout_type is also executed for
each type that happens to be a field in a RECORD/UNION/QUAL_UNION_TYPE.

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Joseph Myers]

On Wed, 2 Aug 2017, Jeff Law wrote:

> I think Joseph's suggestion for looking at partial float handling may be
> useful, though ia64's RFmode may be more interesting as it's not a
> multiple of 8 in bitsize.  IF/KF modes on the ppc port have similar
> properties.

The key issue those floating-point modes engage is the meaning of 
precision.  IFmode and KFmode have precision set to 106 and 113 to 
distinguish them.  But that's precision in the sense of number of mantissa 
bits; as normally understood in GCC, precision should be the number of 
significant bits, so 128 for both those modes (but having multiple 
different binary floating-point modes with the same precision would 
require changing how we deal with laying out long double, so the target 
specifies a mode for floating-point types rather than leaving it to be 
deduced from values such as LONG_DOUBLE_TYPE_SIZE).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jeff Law]

On 08/01/2017 10:26 AM, Jozef Lawrynowicz wrote:
> On 01/08/2017 00:08, Joseph Myers wrote:
>> On Wed, 26 Jul 2017, Jeff Law wrote:
>>
>>> TYPE_SIZE, according to my understanding, should be a tree for the size
>>> of the expression in bits.
>>>
>>> The problem is for msp430 that size varies depending on where it's used.
>>>   ie, in a register an object might have a bitsize of 20 bits, but in
>>> memory its size is 32 bits.
>>>
>>> I don't think that TYPE_SIZE has any concept that the use context is
>>> relevant to selecting the proper size.
>>
>> TYPE_SIZE_UNIT is unambiguously the memory size, including padding; it's
>> used for sizeof.  TYPE_SIZE may be less clear.  We've had issues before
>> with unions with x87 long double, which has 80-bit precision in registers
>> but is 12-byte or 16-byte in memory; that was wrong code (bug 71522)
>> rather than an ICE, but the long double case may be useful for comparison
>> of what various type properties are set to in such cases.
>>
> 
> Thanks for the responses.
> 
> Could it be possible to use "variable_size" to help the compiler choose
> which size to use for TYPE_SIZE? Although the problem I see with this
> right away is that variable_size is never executed on an INTEGER_CST,
> perhaps this is part of the problem?
I suspect variable_size is more for things like VLAs where the size of
an array may vary at runtime.

What we're dealing here is more that the size of an object varies
depending on if it's in a register or in memory.


> Zooming out a bit from TYPE_SIZE, the value that ends up being incorrect
> as a result of TYPE_SIZE is rli->bitpos, this value is used a lot in
> stor_layout.c:place_field, perhaps I need to look deeper in there.
I think Joseph's suggestion for looking at partial float handling may be
useful, though ia64's RFmode may be more interesting as it's not a
multiple of 8 in bitsize.  IF/KF modes on the ppc port have similar
properties.

If you could declare a type that corresponds to one of those modes, then
build an array of them and follow how that code works it might give you
some hints on how to fix intXX.
jeff

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jeff Law]

On 07/31/2017 05:08 PM, Joseph Myers wrote:
> On Wed, 26 Jul 2017, Jeff Law wrote:
> 
>> TYPE_SIZE, according to my understanding, should be a tree for the size
>> of the expression in bits.
>>
>> The problem is for msp430 that size varies depending on where it's used.
>>  ie, in a register an object might have a bitsize of 20 bits, but in
>> memory its size is 32 bits.
>>
>> I don't think that TYPE_SIZE has any concept that the use context is
>> relevant to selecting the proper size.
> 
> TYPE_SIZE_UNIT is unambiguously the memory size, including padding; it's 
> used for sizeof.  TYPE_SIZE may be less clear.  We've had issues before 
> with unions with x87 long double, which has 80-bit precision in registers 
> but is 12-byte or 16-byte in memory; that was wrong code (bug 71522) 
> rather than an ICE, but the long double case may be useful for comparison 
> of what various type properties are set to in such cases.
Thanks for the clarification.  I wandered around a bit before commenting
on Jozef's patch and didn't come across anything which made it
unambiguous.

And yes, x87 long doubles are a good example to compare/contrast against.

jeff

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jozef Lawrynowicz]

On 01/08/2017 00:08, Joseph Myers wrote:

On Wed, 26 Jul 2017, Jeff Law wrote:


TYPE_SIZE, according to my understanding, should be a tree for the size
of the expression in bits.

The problem is for msp430 that size varies depending on where it's used.
  ie, in a register an object might have a bitsize of 20 bits, but in
memory its size is 32 bits.

I don't think that TYPE_SIZE has any concept that the use context is
relevant to selecting the proper size.


TYPE_SIZE_UNIT is unambiguously the memory size, including padding; it's
used for sizeof.  TYPE_SIZE may be less clear.  We've had issues before
with unions with x87 long double, which has 80-bit precision in registers
but is 12-byte or 16-byte in memory; that was wrong code (bug 71522)
rather than an ICE, but the long double case may be useful for comparison
of what various type properties are set to in such cases.



Thanks for the responses.

Could it be possible to use "variable_size" to help the compiler choose
which size to use for TYPE_SIZE? Although the problem I see with this
right away is that variable_size is never executed on an INTEGER_CST,
perhaps this is part of the problem?

Zooming out a bit from TYPE_SIZE, the value that ends up being incorrect
as a result of TYPE_SIZE is rli->bitpos, this value is used a lot in
stor_layout.c:place_field, perhaps I need to look deeper in there.

For this specific bug, rli->bitpos is 20*ARRAY_SIZE, if I modify it in
a GDB session to be 32*ARRAY_SIZE, the case no longer ICEs.

Any suggestions on where I should be looking given this information?
Is this fix for this likely to be in the back-end?

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Joseph Myers]

On Wed, 26 Jul 2017, Jeff Law wrote:

> TYPE_SIZE, according to my understanding, should be a tree for the size
> of the expression in bits.
> 
> The problem is for msp430 that size varies depending on where it's used.
>  ie, in a register an object might have a bitsize of 20 bits, but in
> memory its size is 32 bits.
> 
> I don't think that TYPE_SIZE has any concept that the use context is
> relevant to selecting the proper size.

TYPE_SIZE_UNIT is unambiguously the memory size, including padding; it's 
used for sizeof.  TYPE_SIZE may be less clear.  We've had issues before 
with unions with x87 long double, which has 80-bit precision in registers 
but is 12-byte or 16-byte in memory; that was wrong code (bug 71522) 
rather than an ICE, but the long double case may be useful for comparison 
of what various type properties are set to in such cases.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jeff Law]

On 05/19/2017 07:35 AM, Jozef Lawrynowicz wrote:
> Original post: https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01030.html
> 
> The attached patch fixes an issue for the msp430 target where the TYPE_SIZE of
> the __int20 type was set using the precision (20 bits) instead of the 
> in-memory
> size (32 bits) of the type. This bug caused an ICE as reported in PR78849:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78849
> 
> The patch passed bootstrap and regression testing with no regressions
> on recent trunk (r247020) for x86_64-pc-linux-gnu.
> 
> The patch passed regression testing with "-mcpu=msp430x/-mlarge" for
> msp430-elf on the gcc-6-branch (r247086). Trunk doesn't build with C++
> support for msp430-elf which is why gcc-6-branch was used.
> 
> If the patch is acceptable I would appreciate if someone could commit it for 
> me
> as I don't have write access.
> 
> 
> 0001-Use-GET_MODE_BITSIZE-when-setting-TYPE_SIZE.patch
> 
> 
> From 81ee936dcdde4f4a7d4036479dbbff77da1e72bb Mon Sep 17 00:00:00 2001
> From: Jozef Lawrynowicz 
> Date: Wed, 12 Apr 2017 14:45:45 +
> Subject: [PATCH] Use GET_MODE_BITSIZE when setting TYPE_SIZE
> 
> 2017-05-XXJozef Lawrynowicz   
> 
>   gcc/
>   PR target/78849
>   * stor-layout.c (initialize_sizetypes): Use GET_MODE_BITSIZE when 
> setting TYPE_SIZE.
>   * tree.c (build_common_tree_nodes): Likewise.
> 
>   gcc/testsuite
>   PR target/78849
>   * gcc.target/msp430/pr78849.c: New test.
>   * gcc.target/msp430/msp430.exp: Remove -pedantic-errors option from 
> DEFAULT_CFLAGS.
TYPE_SIZE, according to my understanding, should be a tree for the size
of the expression in bits.

The problem is for msp430 that size varies depending on where it's used.
 ie, in a register an object might have a bitsize of 20 bits, but in
memory its size is 32 bits.

I don't think that TYPE_SIZE has any concept that the use context is
relevant to selecting the proper size.

I wonder if we would be better off keeping TYPE_SIZE as-is and instead
looking at the end use points where we might have that contextual
information.

Thoughts?
jeff

ping [PATCH] [MSP430] Fix PR78849: ICE on initialization of global struct containing __int20 array

[Jozef Lawrynowicz]

Original post: https://gcc.gnu.org/ml/gcc-patches/2017-04/msg01030.html

The attached patch fixes an issue for the msp430 target where the TYPE_SIZE of
the __int20 type was set using the precision (20 bits) instead of the in-memory
size (32 bits) of the type. This bug caused an ICE as reported in PR78849:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78849

The patch passed bootstrap and regression testing with no regressions
on recent trunk (r247020) for x86_64-pc-linux-gnu.

The patch passed regression testing with "-mcpu=msp430x/-mlarge" for
msp430-elf on the gcc-6-branch (r247086). Trunk doesn't build with C++
support for msp430-elf which is why gcc-6-branch was used.

If the patch is acceptable I would appreciate if someone could commit it for me
as I don't have write access.


0001-Use-GET_MODE_BITSIZE-when-setting-TYPE_SIZE.patch
Description: Binary data