Matthew Malcomson <matthew.malcom...@arm.com> writes:
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index
> 5320e6c1e1e3c8d1482c20590049f763e11f8ff0..84050058be8eaa306b07655737e49ea8b6eb21a9
> 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -13709,6 +13709,53 @@ is greater or equal to this number, use callbacks
> instead of inline checks.
> E.g. to disable inline code use
> @option{--param asan-instrumentation-with-call-threshold=0}.
>
> +@item hwasan-instrument-stack
> +Enable hwasan instrumentation of statically sized stack-allocated variables.
> +This kind of instrumentation is enabled by default when using
> +@option{-fsanitize=hwaddress} and disabled by default when using
> +@option{-fsanitize=kernel-hwaddress}.
> +To disable stack instrumentation use
> +@option{--param hwasan-instrument-stack=0}, and to enable it use
> +@option{--param hwasan-instrument-stack=1}.
> +
> +@item hwasan-random-frame-tag
> +When using stack instrumentation, decide tags for stack variables using a
> +deterministic sequence beginning at a random tag for each frame. Usually
> tags
> +are chosen using the same sequence beginning from 1.
> +This is enabled by default for @option{-fsanitize=hwaddress} and unavailable
> +for @option{-fsanitize=kernel-hwaddress}.
> +To disable it use @option{--param hwasan-random-frame-tag=0}.
I think it would be worth clarifying this. I wasn't sure whether
“Usually tags are chosen…” was describing the “determinstic random
sequence” or whether it was describing the behaviour of
hwasan-random-frame-tag=0. If it's describing the behaviour of
hwasan-random-frame-tag=0, then I'm not sure “usually” applies,
given that hwasan-random-frame-tag=1 is the default.
> […]
> +@item -fsanitize=kernel-hwaddress
> +@opindex fsanitize=kernel-hwaddress
> +Enable Hardware-assisted AddressSanitizer for compilation of the Linux
> kernel.
> +Similar to @option{-fsanitize=kernel-address} but using an alternate
> +instrumentation method, and similar to @option{-fsanitize=hwaddress} but with
> +instrumentation differences necessary for compiling the Linux kernel.
> +These differences are to avoid hwasan library initialisation calls and to
initialization
> +account for the stack pointer having a different value in its top byte.
> +Note: This option has different defaults to the
> @option{-fsanitize=hwaddress}.
texinfo has:
@quotation Note
@end quotation
for this, but we don't seem to use it. Still, I think it would be better
to break the paragraph before Note: and use:
@emph{Note:}
which seems to be the preferred style in the GCC manual.
> +Instrumenting the stack and alloca calls are not on by default but is still
@code{alloca}
s/is still/are still/
> +possible by specifying it on the command line with
maybe s/it on the command line with/specifying the command-line options/?
Just a suggestion: would be happy with alternatives.
> +@option{--param hwasan-instrument-stack=1} and
> +@option{--param hwasan-instrument-allocas=1}. Using a random frame tag is not
and maybe add a “respectively” at the end of this sentence.
> +implemented for kernel instrumentation.
> +
> @item -fsanitize=pointer-compare
> @opindex fsanitize=pointer-compare
> Instrument comparison operation (<, <=, >, >=) with pointer operands.
> […]
> diff --git a/gcc/opts.c b/gcc/opts.c
> index
> ac9972d9c386247af3482e07a94c76da3e1abb4d..f3662062c421e1c58c3243109891900eb2dc84bc
> 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -823,6 +823,51 @@ control_options_for_live_patching (struct gcc_options
> *opts,
> /* --help option argument if set. */
> vec<const char *> help_option_arguments;
>
> +/* Return the string name describing the argument provided on the command
> line
> + which has set this particular flag. */
> +const char *
> +find_argument (struct gcc_options *opts, unsigned int flags)
> +{
I think either (a) the name and comment need to mention the
sanitiser more explicitly or (b) this should be a lambda function
in report_conflicting_sanitizer_options:
auto find_argument = [&](unsigned int flags)
{
…
};
(in which case the implementation and comments above are fine as-is).
> + for (int i = 0; sanitizer_opts[i].name != NULL; ++i)
> + {
> + /* Need to find the sanitizer_opts element which:
> + a) Could have set the flags requested.
> + b) Has been set on the command line.
> +
> + Can have (a) without (b) if the flag requested is e.g.
> + SANITIZE_ADDRESS, since both -fsanitize=address and
> + -fsanitize=kernel-address set this flag.
> +
> + Can have (b) without (a) by requesting more than one sanitizer on the
> + command line. */
> + if ((sanitizer_opts[i].flag & opts->x_flag_sanitize)
> + != sanitizer_opts[i].flag)
> + continue;
> + if ((sanitizer_opts[i].flag & flags) != flags)
> + continue;
> + return sanitizer_opts[i].name;
> + }
> + return NULL;
> +}
> +
> +
> +/* Report any conflicting sanitizer options requested. */
> +static void
> +report_conflicting_sanitizer_options (struct gcc_options *opts, location_t
> loc,
> + unsigned int left, unsigned int right)
The comment needs to describe at least the “LEFT” and “RIGHT” parameters
> +{
> + unsigned int left_seen = (opts->x_flag_sanitize & left);
> + unsigned int right_seen = (opts->x_flag_sanitize & right);
> + if (left_seen && right_seen)
> + {
> + const char* left_arg = find_argument (opts, left_seen);
> + const char* right_arg = find_argument (opts, right_seen);
> + gcc_assert (left_arg && right_arg);
> + error_at (loc,
> + "%<-fsanitize=%s%> is incompatible with %<-fsanitize=%s%>",
> + left_arg, right_arg);
> + }
> +}
>
> /* After all options at LOC have been read into OPTS and OPTS_SET,
> finalize settings of those options and diagnose incompatible
> @@ -1074,22 +1119,21 @@ finish_options (struct gcc_options *opts, struct
> gcc_options *opts_set,
> "%<-fsanitize=address%> or %<-fsanitize=kernel-address%>");
> }
>
> - /* Userspace and kernel ASan conflict with each other. */
> - if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS)
> - && (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS))
> - error_at (loc, "%qs is incompatible with %qs",
> - "-fsanitize=address", "-fsanitize=kernel-address");
> + /* Address sanitizers conflict with the thread sanitizer. */
> + report_conflicting_sanitizer_options (opts, loc, SANITIZE_THREAD,
> + SANITIZE_ADDRESS | SANITIZE_HWADDRESS);
> + /* The leak sanitizer conflicts with the thread sanitizer. */
> + report_conflicting_sanitizer_options (opts, loc, SANITIZE_LEAK,
> SANITIZE_THREAD);
Nit: long line.
> […]
> +-param=hwasan-instrument-allocas=
> +Common Joined UInteger Var(param_hwasan_protect_allocas) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan instrumentation of allocas/VLAs.
It would be good to keep the variable name in sync with the parameter,
so s/protect/instrument/ there too.
> […]
> @@ -6850,6 +6850,17 @@ DEFHOOK
> HOOK_VECTOR_END (mode_switching)
>
> #undef HOOK_PREFIX
> +#define HOOK_PREFIX "TARGET_MEMTAG_"
> +HOOK_VECTOR (TARGET_MEMTAG_, memtag)
> +
> +DEFHOOK
> +(can_tag_addresses,
> + "True if backend architecture naturally supports ignoring some region of\n\
> +pointers. This feature means that @option{-fsanitize=hwaddress} can work.",
“…if the backend architecture…”
LGTM otherwise, thanks.
Richard
