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 <tbela...@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.
