Re: [PATCH v2 4/4] RISC-V: Implement target attribute

[Jeff Law <jeffreyalaw@gmail.com>]

On 10/9/23 22:13, Kito Cheng wrote:
> The target attribute which proposed in [1], target attribute allow user
> to specify a local setting per-function basis.
> 
> The syntax of target attribute is `__attribute__((target("<ATTR-STRING>")))`.
> 
> and the syntax of `<ATTR-STRING>` describes below:
> ```
> ATTR-STRING := ATTR-STRING ';' ATTR
>               | ATTR
> 
> ATTR        := ARCH-ATTR
>               | CPU-ATTR
>               | TUNE-ATTR
> 
> ARCH-ATTR   := 'arch=' EXTENSIONS-OR-FULLARCH
> 
> EXTENSIONS-OR-FULLARCH := <EXTENSIONS>
>                          | <FULLARCHSTR>
> 
> EXTENSIONS             := <EXTENSION> ',' <EXTENSIONS>
>                          | <EXTENSION>
> 
> FULLARCHSTR            := <full-arch-string>
> 
> EXTENSION              := <OP> <EXTENSION-NAME> <VERSION>
> 
> OP                     := '+'
> 
> VERSION                := [0-9]+ 'p' [0-9]+
>                          | [1-9][0-9]*
>                          |
> 
> EXTENSION-NAME         := Naming rule is defined in RISC-V ISA manual
> 
> CPU-ATTR    := 'cpu=' <valid-cpu-name>
> TUNE-ATTR   := 'tune=' <valid-tune-name>
> ```
> 
> [1] https://github.com/riscv-non-isa/riscv-c-api-doc/pull/35
> 
> gcc/ChangeLog:
> 
> 	* config.gcc (riscv): Add riscv-target-attr.o.
> 	* config/riscv/riscv-opts.h (TARGET_MIN_VLEN_OPTS): New.
> 	* config/riscv/riscv-protos.h (riscv_declare_function_size) New.
> 	(riscv_option_valid_attribute_p): New.
> 	(riscv_override_options_internal): New.
> 	(struct riscv_tune_info): New.
> 	(riscv_parse_tune): New.
> 	* config/riscv/riscv-target-attr.cc
> 	(class riscv_target_attr_parser): New.
> 	(struct riscv_attribute_info): New.
> 	(riscv_attributes): New.
> 	(riscv_target_attr_parser::parse_arch):
> 	(riscv_target_attr_parser::handle_arch):
> 	(riscv_target_attr_parser::handle_cpu):
> 	(riscv_target_attr_parser::handle_tune):
> 	(riscv_target_attr_parser::update_settings):
> 	(riscv_process_one_target_attr):
> 	(num_occurences_in_str):
> 	(riscv_process_target_attr):
> 	(riscv_option_valid_attribute_p):
> 	* config/riscv/riscv.cc: Include target-globals.h and
> 	riscv-subset.h.
> 	(struct riscv_tune_info): Move to riscv-protos.h.
> 	(get_tune_str):
> 	(riscv_parse_tune):
> 	(riscv_declare_function_size):
> 	(riscv_option_override): Build target_option_default_node and
> 	target_option_current_node.
> 	(riscv_save_restore_target_globals):
> 	(riscv_option_restore):
> 	(riscv_previous_fndecl):
> 	(riscv_set_current_function): Apply the target attribute.
> 	(TARGET_OPTION_RESTORE): Define.
> 	(TARGET_OPTION_VALID_ATTRIBUTE_P): Ditto.
> 	* config/riscv/riscv.h (SWITCHABLE_TARGET): Define to 1.
> 	(ASM_DECLARE_FUNCTION_SIZE) Define.
> 	* config/riscv/riscv.opt (mtune=): Add Save attribute.
> 	(mcpu=): Ditto.
> 	(mcmodel=): Ditto.
> 	* config/riscv/t-riscv: Add build rule for riscv-target-attr.o
> 	* doc/extend.texi: Add doc for target attribute.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/target-attr-01.c: New.
> 	* gcc.target/riscv/target-attr-02.c: Ditto.
> 	* gcc.target/riscv/target-attr-03.c: Ditto.
> 	* gcc.target/riscv/target-attr-04.c: Ditto.
> 	* gcc.target/riscv/target-attr-05.c: Ditto.
> 	* gcc.target/riscv/target-attr-06.c: Ditto.
> 	* gcc.target/riscv/target-attr-07.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-01.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-02.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-03.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-04.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-05.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-06.c: Ditto.
> 	* gcc.target/riscv/target-attr-bad-07.c: Ditto.
> 	* gcc.target/riscv/target-attr-warning-01.c: Ditto.
> 	* gcc.target/riscv/target-attr-warning-02.c: Ditto.
> 	* gcc.target/riscv/target-attr-warning-03.c: Ditto.
I probably would have split the save/restore state out as a separate 
patch, but it's not a big deal.  A bit surprised we didn't already have 
that (and thus SWITCHABLE_TARGET) enabled already.  But no worries about 
that.



And just so I can do the right thing WRT RISE, what's the state of an 
implementation for LLVM?




> diff --git a/gcc/config/riscv/riscv-target-attr.cc b/gcc/config/riscv/riscv-target-attr.cc
> new file mode 100644
> index 00000000000..33cff2c222f
> --- /dev/null
> +++ b/gcc/config/riscv/riscv-target-attr.cc

> +      /* Parsing the extension list like "+<ext>[,+<ext>]*".  */
> +      size_t len = strlen (str);
> +      char *str_to_check = (char *) alloca (len + 1);
If STR is under user control, then this could be a potential security 
issue if they were to provide a crazy long string (at least until we 
implement stack-clash protection -- next year).

In general we should avoid alloca unless we know the amount of memory 
consumed is relatively small and we have a high degree of confidence the 
code isn't going to be called inside a loop (and not inlined into a loop 
in the caller).  I might even go so far as to say no programmer should 
ever use alloca.

Clearly I spent way too much time in my life fighting security issues 
resulting from code mis-using alloca, even by folks who write pretty 
good code in general.




> +
> +/* Parse ARG_STR which contains the definition of one target attribute.
> +   Show appropriate errors if any or return true if the attribute is valid.  */
> +
> +static bool
> +riscv_process_one_target_attr (char *arg_str,
> +			       location_t loc,
> +			       riscv_target_attr_parser &attr_parser)
> +{
> +  size_t len = strlen (arg_str);
> +
> +  if (len == 0)
> +    {
> +      error_at (loc, "malformed %<target()%> attribute");
> +      return false;
> +    }
> +
> +  char *str_to_check = (char *) alloca (len + 1);
alloca again :-)

Generally OK.  I would suggest avoiding the allocas.  malloc is orders 
of magnitude slower, but nowhere near as bad from a security standpoint.

Assuming you're agreeable to adjusting the code to avoid alloca, we'll 
do a quick turnaround on the v3 -- I'll just audit the return paths to 
make sure we don't leak and we'll be good to go.

jeff