On Tue, Jan 26, 2016 at 01:54:52PM +0100, Bernd Schmidt wrote:
> On 01/25/2016 09:30 PM, Jakub Jelinek wrote:
>
> >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> I've been staring at it for a while, and on the whole I think I can make
> sense of this. However - it does not have test coverage. Can this be added?
I have trouble creating those. Because this needs to pass different
compile and different linker options, so I think only lto.exp framework
supports this, but then needs options (-fopenmp, -fopenacc, -fcilkplus,
-fsanitize=) that usually aren't unconditionally available, need some
library and/or target support, and thus such tests generally go into test
subdirectories where all that is tested.
> Also, is this a regression?
Andrew Pinski said so, but because further compiler emitted sanitizers have
been added over time into -fsanitize=undefined, it very likely is a
regression at least in regards to those sanitizers (compile with
-c -flto -fsanitize=undefined something that didn't result in any
sanitization in say 4.9, but now has some of the newer sanitizers, e.g.
__attribute__((nonnull)) char *foo (char *);
char *bar (char *p) { return foo (p); }
and then link without -fsanitize=undefined or with -fno-sanitize=undefined.
> > (parse_sanitizer_options): New function.
> > (common_handle_option): Use parse_sanitizer_options.
>
> I think this can go in, it just moves code around, right? That'll make
> followup patches smaller.
Yeah, committed.
>
> >--- gcc/lto-opts.c.jj 2016-01-23 00:13:00.897015402 +0100
> >+++ gcc/lto-opts.c 2016-01-25 14:06:31.834127398 +0100
> >@@ -199,9 +199,11 @@ lto_write_options (void)
> > /* Also drop all options that are handled by the driver as well,
> > which includes things like -o and -v or -fhelp for example.
> > We do not need those. The only exception is -foffload option, if we
> >- write it in offload_lto section. Also drop all diagnostic options. */
> >+ write it in offload_lto section. Also drop all diagnostic options,
> >+ and -fsanitize=. */
> > if ((cl_options[option->opt_index].flags & (CL_DRIVER|CL_WARNING))
> >- && (!lto_stream_offload_p || option->opt_index != OPT_foffload_))
> >+ && (!lto_stream_offload_p || option->opt_index != OPT_foffload_)
> >+ && option->opt_index != OPT_fsanitize_)
> > continue;
>
> This one puzzles me, doesn't it mean that no sanitizer options make it into
> the LTO stream, which would mean the new code in lto-wrapper doesn't
> trigger?
Sounds like an error in the comment change only to me, I meant to say that
the only exceptions are now
1) -foffload if we write it in offload_lto section
2) -fsanitize= (regardless where we write it)
The continue causes the given option not to be written into the LTO opts
subsection, which is the case here for CL_DRIVER/CL_WARNING options
(-fsanitize= is CL_DRIVER), with the exception of -foffload= under some
condition and with the exception of -fsanitize= (newly). I.e. the change
is that -fsanitize= used to be ignored, not writte into LTO opts, while with
the change it is written in there.
> >+/* Set *OPT to decoded -f{,no-}sanitize=shift. */
>
> I think I'd like some sort of comment about how this is an arbitrary choice,
Ok, will try to write some comment.
> just designed to enable DEF_SANITIZER_BUILTIN (IIUC). Also, why use shift
> and not just sanitize=undefined?
Because -fsanitize=undefined is a large collection of individual sanitizers,
and at least some of them affect also post-IPA code (e.g.
-fsanitize=unreachable). The goal is to pick one of the sanitizers that are
handled solely pre-IPA only, after that just are present in form of a
builtin call in the IL (and thus all that lto1 needs to do for that option
is initialize the builtins).
> >@@ -392,6 +415,24 @@ merge_and_complain (struct cl_decoded_op
> > break;
> > }
> > }
> >+
> >+ /* If FDECODED_OPTIONS requested any ubsan sanitization, pass through
>
> I think we want to add "and DECODED_OPTIONS doesn't", right?
Maybe. It also has to check the option->value though. The thing is,
because I didn't want to memmove options around when processing the first
TU, it will canonicalize all the -f{,no-}sanitize= options depending on if
in the end some ubsan sanitizers were enabled or not to
-f{,no-}sanitize=shift. And merge_and_complain thus should ignore
all the -fno-sanitize=shift options in DECODED_OPTIONS, if it finds any
-fsanitize=shift in DECODED_OPTIONS (optionally preceeded by some
-fno-sanitize=shift), then nothing needs to be added, otherwise
-fsanitize=shift is added.
Jakub
