Re: [PATCH][ARM]Automatically add -mthumb for thumb-only target when mode isn't specified

[Maxim Kuvyrkov <maxim.kuvyrkov@linaro.org>]

> On Mar 5, 2015, at 9:14 AM, Terry Guo <flameroc@gmail.com> wrote:
> 
>> 
>> Thanks Terry (and everyone else) for explaining why we want to do this in the driver.  The substance of the patch looks good to me, and below are some comments and nit-picks.  (Also, I'm not an ARM maintainer, so this is a review, not an approval to commit).
>> 
>> Please make sure to update changelog before committing.
>> 
> 
> Thanks Maxim, your comments are great. I accepted all of them and
> commented two of them that I am not clear.
> 
> <<snapped>>
>>> +
>>> +struct arm_arch_core_flag
>>> +{
>>> +  const char *const name;
>>> +  const unsigned long flags;
>>> +};
>>> +
>>> +static const struct arm_arch_core_flag arm_arch_core_flags[] =
>>> +{
>>> +#undef ARM_CORE
>>> +#define ARM_CORE(NAME, X, IDENT, ARCH, FLAGS, COSTS) \
>>> +  {NAME, FLAGS | FL_FOR_ARCH##ARCH},
>>> +#include "arm-cores.def"
>>> +#undef ARM_CORE
>>> +#undef ARM_ARCH
>>> +#define ARM_ARCH(NAME, CORE, ARCH, FLAGS) \
>>> +  {NAME, FLAGS},
>>> +#include "arm-arches.def"
>>> +#undef ARM_ARCH
>>> +  {NULL, 0}
>>> +};
>> 
>> Did you consider implications from mixing ARCHes and CPUs in the same array?  It should not be a problem, but would you please double-check that cases like "-march=cortex-a15" are properly caught as errors elsewhere in the driver?
>> 
> 
> Not sure I follow you correctly here. This array is just used for my
> new arm_target_thumb_only function. It isn't used by any another code
> in gcc. So I don't think mixing them will break gcc option check
> mechanism. I tried below command and I can get error message:
> 
> $ ./install-native/bin/arm-none-eabi-gcc -march=cortex-a15 x.c -S -mthumb
> arm-none-eabi-gcc: error: unrecognized argument in option '-march=cortex-a15'

I'm just being paranoid here.  The above check you did is all I wanted.

> 
>>> #endif
>>> diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
>>> index 28ffe52..325a81c 100644
>>> --- a/gcc/config/arm/arm-protos.h
>>> +++ b/gcc/config/arm/arm-protos.h
>>> @@ -325,75 +325,6 @@ extern const char *arm_rewrite_selected_cpu (const char *name);
>>> 
>>> extern bool arm_is_constant_pool_ref (rtx);
>>> 
>>> -/* Flags used to identify the presence of processor capabilities.  */
>> 
>> You've lost this comment in the new file.  Was it intentional?
>> 
> 
> The line is used as first line in new file arm-flags.h.

Ack.

Thanks,

--
Maxim Kuvyrkov
www.linaro.org