ARM: Introduce ARM_DEFAULT_SHORT_ENUMS
diff mbox

Message ID 1491219150-25075-1-git-send-email-sebastian.huber@embedded-brains.de
State New
Headers show

Commit Message

Sebastian Huber April 3, 2017, 11:32 a.m. UTC
Allow targets to define the default for the short enums option.

gcc/

	* config/arm/arm.c: (ARM_DEFAULT_SHORT_ENUMS): Provide default
	definition.
	* config/arm/rtems.h (ARM_DEFAULT_SHORT_ENUMS) Define.
---
 gcc/config/arm/arm.c   | 6 +++++-
 gcc/config/arm/rtems.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Bernhard Reutner-Fischer April 4, 2017, 7:43 a.m. UTC | #1
On 3 April 2017 13:32:30 CEST, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
>Allow targets to define the default for the short enums option.

Does this work nowadays?
About 10 years ago I did this for some arm and had to force TREE_CODE at al to 16 bit manually, IIRC.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=34205
Sebastian Huber April 4, 2017, 7:48 a.m. UTC | #2
On 04/04/17 09:43, Bernhard Reutner-Fischer wrote:
> On 3 April 2017 13:32:30 CEST, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
>> Allow targets to define the default for the short enums option.
> Does this work nowadays?
> About 10 years ago I did this for some arm and had to force TREE_CODE at al to 16 bit manually, IIRC.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=34205

The short enums work fine on ARM. I want to get rid of them not due to 
compiler bugs.
Ramana Radhakrishnan April 4, 2017, 8:54 a.m. UTC | #3
On Tue, Apr 4, 2017 at 8:43 AM, Bernhard Reutner-Fischer
<rep.dot.nop@gmail.com> wrote:
> On 3 April 2017 13:32:30 CEST, Sebastian Huber <sebastian.huber@embedded-brains.de> wrote:
>>Allow targets to define the default for the short enums option.
>
> Does this work nowadays?
> About 10 years ago I did this for some arm and had to force TREE_CODE at al to 16 bit manually, IIRC.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=34205


I don't see why it won't work in cross-compilers and on Linux. the bug
report that you allude to seems to suggest that bootstrap was broken
with -fshort-enums i.e. -fshort-enums on a host. I don't see how
that's going to be possible without major work in the software but for
cross-compilers targeting embedded platforms I see no reason why
-fshort-enums won't work.


regards
Ramana
Ramana Radhakrishnan April 4, 2017, 9 a.m. UTC | #4
On Mon, Apr 3, 2017 at 12:32 PM, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> Allow targets to define the default for the short enums option.
>
> gcc/
>
>         * config/arm/arm.c: (ARM_DEFAULT_SHORT_ENUMS): Provide default
>         definition.
>         * config/arm/rtems.h (ARM_DEFAULT_SHORT_ENUMS) Define.
> ---
>  gcc/config/arm/arm.c   | 6 +++++-
>  gcc/config/arm/rtems.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index b24143e..33d3834 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -26547,11 +26547,15 @@ arm_promote_function_mode (const_tree type ATTRIBUTE_UNUSED,
>  }
>
>  /* AAPCS based ABIs use short enums by default.  */
> +#ifndef ARM_DEFAULT_SHORT_ENUMS
> +#define ARM_DEFAULT_SHORT_ENUMS \
> +  (TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX)
> +#endif

This belongs in arm.h rather than in arm.c

>
>  static bool
>  arm_default_short_enums (void)
>  {
> -  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
> +  return ARM_DEFAULT_SHORT_ENUMS;
>  }
>
>
> diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
> index 53cd987..b34bbe8 100644
> --- a/gcc/config/arm/rtems.h
> +++ b/gcc/config/arm/rtems.h
> @@ -27,3 +27,5 @@
>         builtin_assert ("system=rtems");        \
>         TARGET_BPABI_CPP_BUILTINS();            \
>      } while (0)
> +
> +#define ARM_DEFAULT_SHORT_ENUMS false


It's a change in ABI for the RTEMS platform but it certainly needs a
documentation update in the release notes . Also, is it necessary that
you need this in for GCC-7 or can you wait for stage-1 since we are in
regression fixes mode ?

Ramana

> --
> 1.8.4.5
>
Sebastian Huber April 4, 2017, 9:07 a.m. UTC | #5
On 04/04/17 11:00, Ramana Radhakrishnan wrote:
>> >  static bool
>> >  arm_default_short_enums (void)
>> >  {
>> >-  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
>> >+  return ARM_DEFAULT_SHORT_ENUMS;
>> >  }
>> >
>> >
>> >diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
>> >index 53cd987..b34bbe8 100644
>> >--- a/gcc/config/arm/rtems.h
>> >+++ b/gcc/config/arm/rtems.h
>> >@@ -27,3 +27,5 @@
>> >         builtin_assert ("system=rtems");        \
>> >         TARGET_BPABI_CPP_BUILTINS();            \
>> >      } while (0)
>> >+
>> >+#define ARM_DEFAULT_SHORT_ENUMS false
> It's a change in ABI for the RTEMS platform but it certainly needs a
> documentation update in the release notes . Also, is it necessary that
> you need this in for GCC-7 or can you wait for stage-1 since we are in
> regression fixes mode ?

For RTEMS, ABI changes are not really critical. I would like to get this 
into GCC 6.4. For GCC 7 its not urgent.
Ramana Radhakrishnan April 4, 2017, 10:41 a.m. UTC | #6
[dropping devel at rtems dot org as I don't want more bounces]

On Tue, Apr 4, 2017 at 10:07 AM, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> On 04/04/17 11:00, Ramana Radhakrishnan wrote:
>>>
>>> >  static bool
>>> >  arm_default_short_enums (void)
>>> >  {
>>> >-  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
>>> >+  return ARM_DEFAULT_SHORT_ENUMS;
>>> >  }
>>> >
>>> >
>>> >diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
>>> >index 53cd987..b34bbe8 100644
>>> >--- a/gcc/config/arm/rtems.h
>>> >+++ b/gcc/config/arm/rtems.h
>>> >@@ -27,3 +27,5 @@
>>> >         builtin_assert ("system=rtems");        \
>>> >         TARGET_BPABI_CPP_BUILTINS();            \
>>> >      } while (0)
>>> >+
>>> >+#define ARM_DEFAULT_SHORT_ENUMS false
>>
>> It's a change in ABI for the RTEMS platform but it certainly needs a
>> documentation update in the release notes . Also, is it necessary that
>> you need this in for GCC-7 or can you wait for stage-1 since we are in
>> regression fixes mode ?
>
>
> For RTEMS, ABI changes are not really critical. I would like to get this
> into GCC 6.4. For GCC 7 its not urgent.
>

The usual policy is not to have ABI changes within sub-releases of a
GCC release cycle. However if the RTEMs community is happy with it, I
have no particular objections.  I would however strongly suggest that
if you are fixing it for GCC 6.4 to then fix it for GCC-7 *and*
document it in the release notes.




regards
Ramana
Sebastian Huber April 4, 2017, 10:50 a.m. UTC | #7
On 04/04/17 12:41, Ramana Radhakrishnan wrote:
> [dropping devel at rtems dot org as I don't want more bounces]
>
> On Tue, Apr 4, 2017 at 10:07 AM, Sebastian Huber
> <sebastian.huber@embedded-brains.de>  wrote:
>> >On 04/04/17 11:00, Ramana Radhakrishnan wrote:
>>>> >>>
>>>>> >>> >  static bool
>>>>> >>> >  arm_default_short_enums (void)
>>>>> >>> >  {
>>>>> >>> >-  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
>>>>> >>> >+  return ARM_DEFAULT_SHORT_ENUMS;
>>>>> >>> >  }
>>>>> >>> >
>>>>> >>> >
>>>>> >>> >diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
>>>>> >>> >index 53cd987..b34bbe8 100644
>>>>> >>> >--- a/gcc/config/arm/rtems.h
>>>>> >>> >+++ b/gcc/config/arm/rtems.h
>>>>> >>> >@@ -27,3 +27,5 @@
>>>>> >>> >         builtin_assert ("system=rtems");        \
>>>>> >>> >         TARGET_BPABI_CPP_BUILTINS();            \
>>>>> >>> >      } while (0)
>>>>> >>> >+
>>>>> >>> >+#define ARM_DEFAULT_SHORT_ENUMS false
>>> >>
>>> >>It's a change in ABI for the RTEMS platform but it certainly needs a
>>> >>documentation update in the release notes . Also, is it necessary that
>>> >>you need this in for GCC-7 or can you wait for stage-1 since we are in
>>> >>regression fixes mode ?
>> >
>> >
>> >For RTEMS, ABI changes are not really critical. I would like to get this
>> >into GCC 6.4. For GCC 7 its not urgent.
>> >
> The usual policy is not to have ABI changes within sub-releases of a
> GCC release cycle. However if the RTEMs community is happy with it, I
> have no particular objections.  I would however strongly suggest that
> if you are fixing it for GCC 6.4 to then fix it for GCC-7*and*
> document it in the release notes.

For RTEMS ABI changes are not a big deal. Its more important that we 
don't carry GCC patches around and can use a particular FSF release 
unmodified. I will add this change to the GCC 6.4 and 7.1 release notes, 
if this is all right for Jakub.
Sebastian Huber April 4, 2017, 10:52 a.m. UTC | #8
On 04/04/17 12:50, Sebastian Huber wrote:
> On 04/04/17 12:41, Ramana Radhakrishnan wrote:
>> [dropping devel at rtems dot org as I don't want more bounces]
>>
>> On Tue, Apr 4, 2017 at 10:07 AM, Sebastian Huber
>> <sebastian.huber@embedded-brains.de> wrote:
>>> >On 04/04/17 11:00, Ramana Radhakrishnan wrote:
>>>>> >>>
>>>>>> >>> >  static bool
>>>>>> >>> >  arm_default_short_enums (void)
>>>>>> >>> >  {
>>>>>> >>> >-  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
>>>>>> >>> >+  return ARM_DEFAULT_SHORT_ENUMS;
>>>>>> >>> >  }
>>>>>> >>> >
>>>>>> >>> >
>>>>>> >>> >diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
>>>>>> >>> >index 53cd987..b34bbe8 100644
>>>>>> >>> >--- a/gcc/config/arm/rtems.h
>>>>>> >>> >+++ b/gcc/config/arm/rtems.h
>>>>>> >>> >@@ -27,3 +27,5 @@
>>>>>> >>> >         builtin_assert ("system=rtems");        \
>>>>>> >>> > TARGET_BPABI_CPP_BUILTINS();            \
>>>>>> >>> >      } while (0)
>>>>>> >>> >+
>>>>>> >>> >+#define ARM_DEFAULT_SHORT_ENUMS false
>>>> >>
>>>> >>It's a change in ABI for the RTEMS platform but it certainly needs a
>>>> >>documentation update in the release notes . Also, is it necessary 
>>>> that
>>>> >>you need this in for GCC-7 or can you wait for stage-1 since we 
>>>> are in
>>>> >>regression fixes mode ?
>>> >
>>> >
>>> >For RTEMS, ABI changes are not really critical. I would like to get 
>>> this
>>> >into GCC 6.4. For GCC 7 its not urgent.
>>> >
>> The usual policy is not to have ABI changes within sub-releases of a
>> GCC release cycle. However if the RTEMs community is happy with it, I
>> have no particular objections.  I would however strongly suggest that
>> if you are fixing it for GCC 6.4 to then fix it for GCC-7*and*
>> document it in the release notes.
>
> For RTEMS ABI changes are not a big deal. Its more important that we 
> don't carry GCC patches around and can use a particular FSF release 
> unmodified. I will add this change to the GCC 6.4 and 7.1 release 
> notes, if this is all right for Jakub. 

v2 of the patch is here:

https://gcc.gnu.org/ml/gcc-patches/2017-04/msg00138.html

Patch
diff mbox

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index b24143e..33d3834 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -26547,11 +26547,15 @@  arm_promote_function_mode (const_tree type ATTRIBUTE_UNUSED,
 }
 
 /* AAPCS based ABIs use short enums by default.  */
+#ifndef ARM_DEFAULT_SHORT_ENUMS
+#define ARM_DEFAULT_SHORT_ENUMS \
+  (TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX)
+#endif
 
 static bool
 arm_default_short_enums (void)
 {
-  return TARGET_AAPCS_BASED && arm_abi != ARM_ABI_AAPCS_LINUX;
+  return ARM_DEFAULT_SHORT_ENUMS;
 }
 
 
diff --git a/gcc/config/arm/rtems.h b/gcc/config/arm/rtems.h
index 53cd987..b34bbe8 100644
--- a/gcc/config/arm/rtems.h
+++ b/gcc/config/arm/rtems.h
@@ -27,3 +27,5 @@ 
 	builtin_assert ("system=rtems");	\
 	TARGET_BPABI_CPP_BUILTINS();    	\
     } while (0)
+
+#define ARM_DEFAULT_SHORT_ENUMS false