Re: [Patch 1/8, Arm, AArch64, GCC] Refactor mbranch-protection option parsing and make it common to AArch32 and AArch64 backends. [Was RE: [Patch 2/7, Arm, GCC] Add option -mbranch-protection.]

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

On 10/11/2021 13:55, Andrea Corallo via Gcc-patches wrote:
> Tejas Belagod via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> 
> [...]
> 
>> This change refactors all the mbranch-protection option parsing code and types
>> to make it common to both AArch32 and AArch64 backends.  This change also pulls
>> in some supporting types from AArch64 to make it common
>> (aarch_parse_opt_result).  The significant changes in this patch are the
>> movement of all branch protection parsing routines from aarch64.c to
>> aarch-common.c and supporting data types and static data structures.  This
>> patch also pre-declares variables and types required in the aarch32 back for
>> moved variables for function sign scope and key to prepare for the impending
>> series of patches that support parsing the feature mbranch-protection in the
>> aarch32 back end.
>>
>> 2021-10-25  Tejas Belagod  <tbelagod@arm.com>
>>
>> gcc/ChangeLog:
>>
>> 	* common/config/aarch64/aarch64-common.c: Include aarch-common.h.
>> 	(all_architectures): Fix comment.
>> 	(aarch64_parse_extension): Rename return type, enum value names.
>> 	* config/aarch64/aarch64-c.c (aarch64_update_cpp_builtins): Rename
>> 	factored out aarch_ra_sign_scope and aarch_ra_sign_key variables.
>> 	Also rename corresponding enum values.
>> 	* config/aarch64/aarch64-opts.h (aarch64_function_type): Factor out
>> 	aarch64_function_type and move it to common code as aarch_function_type
>> 	in aarch-common.h.
>> 	* config/aarch64/aarch64-protos.h: Include common types header, move out
>> 	types aarch64_parse_opt_result and aarch64_key_type to aarch-common.h
>> 	* config/aarch64/aarch64.c: Move mbranch-protection parsing types and
>> 	functions out into aarch-common.h and aarch-common.c.  Fix up all the name
>> 	changes resulting from the move.
>> 	* config/aarch64/aarch64.md: Fix up aarch64_ra_sign_key type name change
>> 	and enum value.
>> 	* config/aarch64/aarch64.opt: Include aarch-common.h to import type move.
>> 	Fix up name changes from factoring out common code and data.
>> 	* config/arm/aarch-common-protos.h: Export factored out routines to both
>> 	backends.
>> 	* config/arm/aarch-common.c: Include newly factored out types.  Move all
>> 	mbranch-protection code and data structures from aarch64.c.
>> 	* config/arm/aarch-common.h: New header that declares types shared between
>> 	aarch32 and aarch64 backends.
>> 	* config/arm/arm-protos.h: Declare types and variables that are made common
>> 	to aarch64 and aarch32 backends - aarch_ra_sign_key, aarch_ra_sign_scope and
>> 	aarch_enable_bti.
>>
>>
>> Tested the following configurations. OK for trunk?
>>
>> -mthumb/-march=armv8.1-m.main+pacbti/-mfloat-abi=soft
>> -marm/-march=armv7-a/-mfpu=vfpv3-d16/-mfloat-abi=softfp
>> mcmodel=small and tiny
>> aarch64-none-linux-gnu native test and bootstrap
>>
>> Thanks,
>> Tejas.
> 
> Hi Tejas,
> 
> going through the code I've spotted a couple of indentation nits that I
> guess are coming from the original source that was moved.
> 
>> diff --git a/gcc/config/arm/aarch-common.c b/gcc/config/arm/aarch-common.c
> 
> [...]
> 
>> +  /* Copy the last processed token into the argument to pass it back.
>> +    Used by option and attribute validation to print the offending token.  */
>> +  if (last_str)
>> +    {
>> +      if (str) strcpy (*last_str, str);
>> +      else *last_str = NULL;
> 
> I think we should have new lines after both if and else here.

Agreed.  This doesn't match the GNU style.

> 
>> +    }

There should also be a blank line here, before the next if clause.

>> +  if (res == AARCH_PARSE_OK)
>> +    {
>> +      /* If needed, alloc the accepted string then copy in const_str.
>> +	Used by override_option_after_change_1.  */
>> +      if (!accepted_branch_protection_string)
>> +	accepted_branch_protection_string = (char *) xmalloc (
>> +						      BRANCH_PROTECT_STR_MAX
>> +							+ 1);
> 						        ^^
>                                                          Indentation

It would be best to split this just before the '=', then then rest of 
the statement should fit on one line
> 
> 
>> +      strncpy (accepted_branch_protection_string, const_str,
>> +		BRANCH_PROTECT_STR_MAX + 1);
> 		^^
>                  Same
>> +      /* Forcibly null-terminate.  */
>> +      accepted_branch_protection_string[BRANCH_PROTECT_STR_MAX] = '\0';
>> +    }
>> +  return res;
>> +}
> 
> Thanks
> 
>    Andrea
> 

+++ b/gcc/config/arm/aarch-common.h
@@ -0,0 +1,73 @@
+/* Types shared between arm and aarch64.
+
+   Copyright (C) 1991-2021 Free Software Foundation, Inc.
+   Contributed by ARM Ltd.
+

I think this should be

/* Types shared between arm and aarch64.

    Copyright (C) 2009-2021 Free Software Foundation, Inc.
    Contributed by Arm Ltd.

Since all of the code has been derived from the aarch64 port.

-struct aarch64_branch_protect_type
-{
-  /* The type's name that the user passes to the branch-protection option
-    string.  */
-  const char* name;
-  /* Function to handle the protection type and set global variables.
-    First argument is the string token corresponding with this type and the
-    second argument is the next token in the option string.
-    Return values:
-    * AARCH64_PARSE_OK: Handling was sucessful.
-    * AARCH64_INVALID_ARG: The type is invalid in this context and the 
caller
-      should print an error.
-    * AARCH64_INVALID_FEATURE: The type is invalid and the handler 
prints its
-      own error.  */

The indentation of this comment looks wrong as well.

Finally,

+static enum aarch_parse_opt_result
+aarch_handle_pac_ret_protection (char* str ATTRIBUTE_UNUSED,
+				    char* rest ATTRIBUTE_UNUSED)
+{

This is C++ and we never use the arguments, even within ifdefs or 
macros, so better to write:

static enum aarch_parse_opt_result
aarch_handle_pac_ret_protection (char* /* str unused */,
				 char* /* rest unused */)
{

Similarly for the other parse helpers.

OK with those changes.

R.