Re: [PATCHv2 2/7, GCC, ARM, V8M] Handling ARMv8-M Security Extension's cmse_nonsecure_entry attribute

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

Hi Andre,

On 25/10/16 17:28, Andre Vieira (lists) wrote:
> On 24/08/16 12:00, Andre Vieira (lists) wrote:
> > On 25/07/16 14:21, Andre Vieira (lists) wrote:
> > > This patch adds support for the ARMv8-M Security Extensions
> > > 'cmse_nonsecure_entry' attribute. In this patch we implement the
> > > attribute handling and diagnosis around the attribute. See Section 5.4
> > > of ARM®v8-M Security Extensions
> > > (http://infocenter.arm.com/help/topic/com.arm.doc.ecm0359818/index.html).
> > >
> > > *** gcc/ChangeLog ***
> > > 2016-07-25  Andre Vieira        <andre.simoesdiasvieira@arm.com>
> > >              Thomas Preud'homme  <thomas.preudhomme@arm.com>
> > >
> > >          * config/arm/arm.c (arm_handle_cmse_nonsecure_entry): New.
> > >          (arm_attribute_table): Added cmse_nonsecure_entry
> > >          (arm_compute_func_type): Handle cmse_nonsecure_entry.
> > >          (cmse_func_args_or_return_in_stack): New.
> > >          (arm_handle_cmse_nonsecure_entry): New.
> > >          * config/arm/arm.h (ARM_FT_CMSE_ENTRY): New macro define.
> > >          (IS_CMSE_ENTRY): Likewise.
> > >
> > > *** gcc/testsuite/ChangeLog ***
> > > 2016-07-25  Andre Vieira        <andre.simoesdiasvieira@arm.com>
> > >              Thomas Preud'homme  <thomas.preudhomme@arm.com>
> > >
> > >          * gcc.target/arm/cmse/cmse-3.c: New.
> > Added more documentation as requested.
> >
> > ----
> >
> > This patch adds support for the ARMv8-M Security Extensions
> > 'cmse_nonsecure_entry' attribute. In this patch we implement the
> > attribute handling and diagnosis around the attribute. See Section 5.4
> > of ARM®v8-M Security Extensions
> > (http://infocenter.arm.com/help/topic/com.arm.doc.ecm0359818/index.html).
> >
> > *** gcc/ChangeLog ***
> > 2016-07-xx  Andre Vieira        <andre.simoesdiasvieira@arm.com>
> >              Thomas Preud'homme  <thomas.preudhomme@arm.com>
> >
> >          * config/arm/arm.c (arm_handle_cmse_nonsecure_entry): New.
> >          (arm_attribute_table): Added cmse_nonsecure_entry
> >          (arm_compute_func_type): Handle cmse_nonsecure_entry.
> >          (cmse_func_args_or_return_in_stack): New.
> >          (arm_handle_cmse_nonsecure_entry): New.
> >          * config/arm/arm.h (ARM_FT_CMSE_ENTRY): New macro define.
> >          (IS_CMSE_ENTRY): Likewise.
> >          * doc/extend.texi (ARM ARMv8-M Security Extensions): New attribute.
> >
> > *** gcc/testsuite/ChangeLog ***
> > 2016-07-xx  Andre Vieira        <andre.simoesdiasvieira@arm.com>
> >              Thomas Preud'homme  <thomas.preudhomme@arm.com>
> >
> >          * gcc.target/arm/cmse/cmse-3.c: New.
> Hi,
>
> Rebased previous patch on top of trunk as requested. No changes to
> ChangeLog.
>
> Cheers,
> Andre


@@ -6661,6 +6668,110 @@ arm_handle_notshared_attribute (tree *node,
 }
 #endif

+/* This function returns true if a function with declaration FNDECL, name
+   NAME and type FNTYPE uses the stack to pass arguments or return variables
+   and false otherwise.  This is used for functions with the attributes
+   'cmse_nonsecure_call' or 'cmse_nonsecure_entry' and this function will issue
+   diagnostic messages if the stack is used.  */
+

I think NAME is the name of the attribute, not the function. The comment here is
misleading, otherwise the error message in this function doesn't make sense.
Please fix the description.

+static bool
+cmse_func_args_or_return_in_stack (tree fndecl, tree name, tree fntype)
+{
+  function_args_iterator args_iter;
+  CUMULATIVE_ARGS args_so_far_v;
+  cumulative_args_t args_so_far;
+  bool first_param = true;
+  tree arg_type, prev_arg_type = NULL_TREE, ret_type;
+
+  /* Error out if any argument is passed on the stack.  */
+  arm_init_cumulative_args (&args_so_far_v, fntype, NULL_RTX, fndecl);
+  args_so_far = pack_cumulative_args (&args_so_far_v);
+  FOREACH_FUNCTION_ARGS (fntype, arg_type, args_iter)
+    {
+      rtx arg_rtx;
+      machine_mode arg_mode = TYPE_MODE (arg_type);
+
+      prev_arg_type = arg_type;
+      if (VOID_TYPE_P (arg_type))
+    continue;
+
+      if (!first_param)
+    arm_function_arg_advance (args_so_far, arg_mode, arg_type, true);
+      arg_rtx = arm_function_arg (args_so_far, arg_mode, arg_type, true);
+      if (!arg_rtx
+      || arm_arg_partial_bytes (args_so_far, arg_mode, arg_type, true))
+    {
+      error ("%qE attribute not available to functions with arguments "
+         "passed on the stack", name);
+      return true;
+    }
+      first_param = false;
+    }
+
+  /* Error out for variadic functions since we cannot control how many
+     arguments will be passed and thus stack could be used. stdarg_p () is not
+     used for the checking to avoid browsing arguments twice.  */
+  if (prev_arg_type != NULL_TREE && !VOID_TYPE_P (prev_arg_type))
+    {
+      error ("%qE attribute not available to functions with variable number "
+         "of arguments", name);
+      return true;
+    }
+
+  /* Error out if return value is passed on the stack.  */
+  ret_type = TREE_TYPE (fntype);
+  if (arm_return_in_memory (ret_type, fntype))
+    {
+      error ("%qE attribute not available to functions that return value on "
+         "the stack", name);
+      return true;
+    }
+  return false;
+}
+
+/* Called upon detection of the use of the cmse_nonsecure_entry attribute, this
+   function will check whether the attribute is allowed here and will add the
+   attribute to the function declaration tree or otherwise issue a warning.  */
+
+static tree
+arm_handle_cmse_nonsecure_entry (tree *node, tree name,
+                 tree /* args */,
+                 int /* flags */,
+                 bool *no_add_attrs)
+{
+  tree fndecl;
+
+  if (!use_cmse)
+    {
+      *no_add_attrs = true;
+      return NULL_TREE;
+    }

Do you also want to warn the user here that the attribute will be ignored?
This looks ok to me otherwise.

Thanks,
Kyrill