diff mbox

ARM FreeBSD fix bootstrap

Message ID 567AF5DB.70504@fgznet.ch
State New
Headers show

Commit Message

Andreas Tobler Dec. 23, 2015, 7:28 p.m. UTC 
On 23.12.15 11:22, Richard Earnshaw (lists) wrote:
> On 22/12/15 19:53, Andreas Tobler wrote:
>> Hi all,
>>
>> the commit for PR68617 broke boostrap on armv6*-*-freebsd*.
>>
>> We still have unaligned_access = 0 on armv6 here on FreeBSD.
>>
>> The commit from the above PR overrides my SUBTARGET_OVERRIDE_OPTIONS I
>> called in arm_option_override. And it sets the unaligned_access to 1.
>>
>> The attached patch fixes this, bootstrap ongoing but passed the breaking
>> stage where genmddeps bus errored.
>>
>> Is this patch ok for trunk once bootstrap completes?
>>
>> TIA,
>> Andreas
>>
>> 2015-12-22  Andreas Tobler  <andreast@gcc.gnu.org>
>>
>>      * config/arm/freebsd.h (SUBTARGET_OVERRIDE_OPTIONS): Adjust to
>>      check unaligned_access on the gcc_options set.
>>      * config/arm/arm.c (arm_option_override): Move
>>      SUBTARGET_OVERRIDE_OPTIONS from here to
>>      (arm_option_override_internal).
>>
>
> Moving this hunk to a different place potentially affects VXWORKS (the
> only other target that uses this hook).  I'd like to see confirmation
> from the VxWorks maintainers (Nathan?) that this doesn't cause any
> problems for them.  If it does, then I think you need to create a new
> subtarget hook (SUBTARGET_OVERRIDE_INTERNAL_OPTIONS?) and change FreeBSD
> to use that rather than the existing hook.

I noticed this morning that VxWorks might be affected. To be on the safe 
side I'd like to propose the attached version since it makes clear where 
the override belongs to and I don't think hijacking 
SUBTARGET_OVERRIDE_OPTIONS is a good idea here.
I need the override in the arm_option_override_internal function after 
the default has been set.

What do you think?

Thanks,

Andreas

2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>

	* config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
	SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
	unaligned_access on the gcc_options set.
	* config/arm/arm.c (arm_option_override_internal): Use
	SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.

Comments

Andreas Tobler Jan. 4, 2016, 9:45 p.m. UTC | #1 
Ping :)

TIA,
Andreas

On 23.12.15 20:28, Andreas Tobler wrote:
> On 23.12.15 11:22, Richard Earnshaw (lists) wrote:
>> On 22/12/15 19:53, Andreas Tobler wrote:
>>> Hi all,
>>>
>>> the commit for PR68617 broke boostrap on armv6*-*-freebsd*.
>>>
>>> We still have unaligned_access = 0 on armv6 here on FreeBSD.
>>>
>>> The commit from the above PR overrides my SUBTARGET_OVERRIDE_OPTIONS I
>>> called in arm_option_override. And it sets the unaligned_access to 1.
>>>
>>> The attached patch fixes this, bootstrap ongoing but passed the breaking
>>> stage where genmddeps bus errored.
>>>
>>> Is this patch ok for trunk once bootstrap completes?
>>>
>>> TIA,
>>> Andreas
>>>
>>> 2015-12-22  Andreas Tobler  <andreast@gcc.gnu.org>
>>>
>>>       * config/arm/freebsd.h (SUBTARGET_OVERRIDE_OPTIONS): Adjust to
>>>       check unaligned_access on the gcc_options set.
>>>       * config/arm/arm.c (arm_option_override): Move
>>>       SUBTARGET_OVERRIDE_OPTIONS from here to
>>>       (arm_option_override_internal).
>>>
>>
>> Moving this hunk to a different place potentially affects VXWORKS (the
>> only other target that uses this hook).  I'd like to see confirmation
>> from the VxWorks maintainers (Nathan?) that this doesn't cause any
>> problems for them.  If it does, then I think you need to create a new
>> subtarget hook (SUBTARGET_OVERRIDE_INTERNAL_OPTIONS?) and change FreeBSD
>> to use that rather than the existing hook.
>
> I noticed this morning that VxWorks might be affected. To be on the safe
> side I'd like to propose the attached version since it makes clear where
> the override belongs to and I don't think hijacking
> SUBTARGET_OVERRIDE_OPTIONS is a good idea here.
> I need the override in the arm_option_override_internal function after
> the default has been set.
>
> What do you think?
>
> Thanks,
>
> Andreas
>
> 2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>
>
> 	* config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
> 	SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
> 	unaligned_access on the gcc_options set.
> 	* config/arm/arm.c (arm_option_override_internal): Use
> 	SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.
Richard Earnshaw (lists) Jan. 5, 2016, 10:32 a.m. UTC | #2 
On 23/12/15 19:28, Andreas Tobler wrote:
> On 23.12.15 11:22, Richard Earnshaw (lists) wrote:
>> On 22/12/15 19:53, Andreas Tobler wrote:
>>> Hi all,
>>>
>>> the commit for PR68617 broke boostrap on armv6*-*-freebsd*.
>>>
>>> We still have unaligned_access = 0 on armv6 here on FreeBSD.
>>>
>>> The commit from the above PR overrides my SUBTARGET_OVERRIDE_OPTIONS I
>>> called in arm_option_override. And it sets the unaligned_access to 1.
>>>
>>> The attached patch fixes this, bootstrap ongoing but passed the breaking
>>> stage where genmddeps bus errored.
>>>
>>> Is this patch ok for trunk once bootstrap completes?
>>>
>>> TIA,
>>> Andreas
>>>
>>> 2015-12-22  Andreas Tobler  <andreast@gcc.gnu.org>
>>>
>>>      * config/arm/freebsd.h (SUBTARGET_OVERRIDE_OPTIONS): Adjust to
>>>      check unaligned_access on the gcc_options set.
>>>      * config/arm/arm.c (arm_option_override): Move
>>>      SUBTARGET_OVERRIDE_OPTIONS from here to
>>>      (arm_option_override_internal).
>>>
>>
>> Moving this hunk to a different place potentially affects VXWORKS (the
>> only other target that uses this hook).  I'd like to see confirmation
>> from the VxWorks maintainers (Nathan?) that this doesn't cause any
>> problems for them.  If it does, then I think you need to create a new
>> subtarget hook (SUBTARGET_OVERRIDE_INTERNAL_OPTIONS?) and change FreeBSD
>> to use that rather than the existing hook.
> 
> I noticed this morning that VxWorks might be affected. To be on the safe
> side I'd like to propose the attached version since it makes clear where
> the override belongs to and I don't think hijacking
> SUBTARGET_OVERRIDE_OPTIONS is a good idea here.
> I need the override in the arm_option_override_internal function after
> the default has been set.
> 
> What do you think?
> 
> Thanks,
> 
> Andreas
> 
> 2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>
> 
>     * config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
>     SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
>     unaligned_access on the gcc_options set.
>     * config/arm/arm.c (arm_option_override_internal): Use
>     SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.
> 
> 

This is OK.

One question, though, is whether you should explicitly override a user
request for unaligned accesses without at least warning that this is
being done.

R.

> 
> 
> 
> 
> 
> x_opts_unaligned2.diff
> 
> 
> Index: freebsd.h
> ===================================================================
> --- freebsd.h	(revision 231903)
> +++ freebsd.h	(working copy)
> @@ -120,10 +120,10 @@
>  #define SUBTARGET_CPU_DEFAULT   TARGET_CPU_arm9
>  #endif
>  
> -#define SUBTARGET_OVERRIDE_OPTIONS		\
> +#define SUBTARGET_OVERRIDE_INTERNAL_OPTIONS	\
>  do {						\
> -    if (unaligned_access)			\
> -	unaligned_access = 0;			\
> +    if (opts->x_unaligned_access)		\
> +	opts->x_unaligned_access = 0;		\
>  } while (0)
>  
>  #undef MAX_SYNC_LIBFUNC_SIZE
> Index: arm.c
> ===================================================================
> --- arm.c	(revision 231903)
> +++ arm.c	(working copy)
> @@ -2954,6 +2954,10 @@
>    /* Thumb2 inline assembly code should always use unified syntax.
>       This will apply to ARM and Thumb1 eventually.  */
>    opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags);
> +
> +#ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS
> +  SUBTARGET_OVERRIDE_INTERNAL_OPTIONS;
> +#endif
>  }
>  
>  /* Fix up any incompatible options that the user has specified.  */
>
Nathan Sidwell Jan. 5, 2016, 2:02 p.m. UTC | #3 
On 01/04/16 16:45, Andreas Tobler wrote:
> Ping :)

>> I noticed this morning that VxWorks might be affected. To be on the safe
>> side I'd like to propose the attached version since it makes clear where
>> the override belongs to and I don't think hijacking
>> SUBTARGET_OVERRIDE_OPTIONS is a good idea here.
>> I need the override in the arm_option_override_internal function after
>> the default has been set.
>

I  have no further comment, thanks for investigating the vxworks usage.

nathan
Andreas Tobler Jan. 5, 2016, 9:03 p.m. UTC | #4 
On 05.01.16 11:32, Richard Earnshaw (lists) wrote:
> On 23/12/15 19:28, Andreas Tobler wrote:

>> 2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>
>>
>>      * config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
>>      SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
>>      unaligned_access on the gcc_options set.
>>      * config/arm/arm.c (arm_option_override_internal): Use
>>      SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.
>>
>>
>
> This is OK.
>
> One question, though, is whether you should explicitly override a user
> request for unaligned accesses without at least warning that this is
> being done.


Like this?

config/arm/freebsd.h:

#define SUBTARGET_OVERRIDE_INTERNAL_OPTIONS		\
do {							\
     if (opts_set->x_unaligned_access == 1)		\
	warning (0, "target CPU does not support unaligned accesses");\
     if (opts->x_unaligned_access)			\
	opts->x_unaligned_access = 0;

Thank you for the feedback.

Andreas
Richard Earnshaw (lists) Jan. 6, 2016, 10:39 a.m. UTC | #5 
On 05/01/16 21:03, Andreas Tobler wrote:
> On 05.01.16 11:32, Richard Earnshaw (lists) wrote:
>> On 23/12/15 19:28, Andreas Tobler wrote:
> 
>>> 2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>
>>>
>>>      * config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
>>>      SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
>>>      unaligned_access on the gcc_options set.
>>>      * config/arm/arm.c (arm_option_override_internal): Use
>>>      SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.
>>>
>>>
>>
>> This is OK.
>>
>> One question, though, is whether you should explicitly override a user
>> request for unaligned accesses without at least warning that this is
>> being done.
> 
> 
> Like this?
> 
> config/arm/freebsd.h:
> 
> #define SUBTARGET_OVERRIDE_INTERNAL_OPTIONS        \
> do {                            \
>     if (opts_set->x_unaligned_access == 1)        \
>     warning (0, "target CPU does not support unaligned accesses");\
>     if (opts->x_unaligned_access)            \
>     opts->x_unaligned_access = 0;
> 

Something like that, but the problem is not that the CPU does not
support unaligned accesses, but that FreeBSD does not support CPUs doing
unaligned accesses.

Anyway, I'll pre-approve a patch adding a suitable warning here.

R.

> Thank you for the feedback.
> 
> Andreas   
>
Andreas Tobler Jan. 7, 2016, 8:37 p.m. UTC | #6 
On 06.01.16 11:39, Richard Earnshaw (lists) wrote:
> On 05/01/16 21:03, Andreas Tobler wrote:
>> On 05.01.16 11:32, Richard Earnshaw (lists) wrote:
>>> On 23/12/15 19:28, Andreas Tobler wrote:
>>
>>>> 2015-12-23  Andreas Tobler  <andreast@gcc.gnu.org>
>>>>
>>>>       * config/arm/freebsd.h: Rename SUBTARGET_OVERRIDE_OPTIONS to
>>>>       SUBTARGET_OVERRIDE_INTERNAL_OPTIONS. Adjust to check
>>>>       unaligned_access on the gcc_options set.
>>>>       * config/arm/arm.c (arm_option_override_internal): Use
>>>>       SUBTARGET_OVERRIDE_INTERNAL_OPTIONS.
>>>>
>>>>
>>>
>>> This is OK.
>>>
>>> One question, though, is whether you should explicitly override a user
>>> request for unaligned accesses without at least warning that this is
>>> being done.
>>
>>
>> Like this?
>>
>> config/arm/freebsd.h:
>>
>> #define SUBTARGET_OVERRIDE_INTERNAL_OPTIONS        \
>> do {                            \
>>      if (opts_set->x_unaligned_access == 1)        \
>>      warning (0, "target CPU does not support unaligned accesses");\
>>      if (opts->x_unaligned_access)            \
>>      opts->x_unaligned_access = 0;
>>
>
> Something like that, but the problem is not that the CPU does not
> support unaligned accesses, but that FreeBSD does not support CPUs doing
> unaligned accesses.
>
> Anyway, I'll pre-approve a patch adding a suitable warning here.

Done:
https://gcc.gnu.org/viewcvs/gcc?view=revision&revision=232141

Thank you,
Andreas
diff mbox

Patch

Index: freebsd.h
===================================================================
--- freebsd.h	(revision 231903)
+++ freebsd.h	(working copy)
@@ -120,10 +120,10 @@ 
 #define SUBTARGET_CPU_DEFAULT   TARGET_CPU_arm9
 #endif
 
-#define SUBTARGET_OVERRIDE_OPTIONS		\
+#define SUBTARGET_OVERRIDE_INTERNAL_OPTIONS	\
 do {						\
-    if (unaligned_access)			\
-	unaligned_access = 0;			\
+    if (opts->x_unaligned_access)		\
+	opts->x_unaligned_access = 0;		\
 } while (0)
 
 #undef MAX_SYNC_LIBFUNC_SIZE
Index: arm.c
===================================================================
--- arm.c	(revision 231903)
+++ arm.c	(working copy)
@@ -2954,6 +2954,10 @@ 
   /* Thumb2 inline assembly code should always use unified syntax.
      This will apply to ARM and Thumb1 eventually.  */
   opts->x_inline_asm_unified = TARGET_THUMB2_P (opts->x_target_flags);
+
+#ifdef SUBTARGET_OVERRIDE_INTERNAL_OPTIONS
+  SUBTARGET_OVERRIDE_INTERNAL_OPTIONS;
+#endif
 }
 
 /* Fix up any incompatible options that the user has specified.  */