[cfe-commits] [PATCH] Print warning instead of error if optional ASan features are listed w/o -fsanitize=address

[Alexey Samsonov]

On Fri, Jan 25, 2013 at 1:12 AM, David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Jan 21, 2013 at 12:35 PM, Alexey Samsonov <samsonov at google.com>
> wrote:
> >
> > On Mon, Jan 21, 2013 at 9:56 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> On Mon, Jan 21, 2013 at 8:28 AM, Alexey Samsonov <samsonov at google.com>
> >> wrote:
> >> > Hi rsmith, kcc,
> >> >
> >> > This patch changes the behavior of Clang driver for command line like
> >> > "clang++ -fsanitize=init-order a.cc": now it ignores
> >> > -fsanitize=init-order argument and
> >> > warns that it's unused w/o "-fsanitize=address" (instead of reporting
> an
> >> > error).
> >>
> >> So if I understand correctly, the same warning is produced for all
> >> these variants:
> >> -fsanitize=init-order
> >> -fno-sanitize=address -fsanitize=init-order
> >> -fsanitize=address,init-order -fno-sanitize=address
> >
> >
> > Yes.
> >
> >>
> >>
> >> Could we specifically special case for situations like the last one?
> >> (or at least the last two) & make those silent (no warning) while
> >> possibly still producing an error for the first? Maybe that's
> >> difficult with our options parsing & not worth the hassle but I
> >> figured I'd check/ask.
> >
> >
> > Hm, interesting. We can somehow hack sanitizer option parsing to
> > deal with this and clearly distinguish the first and the last case, but
> > we deviation from current behavior ("-fsanitize" enables some set of
> > sanitizers,
> > "-fno-sanitize'" disables some set) may make things less clear.
> > E.g. if "-fsanitize=address,init-order -fno-sanitize=address" is "no
> > sanitizing",
>
> (excuse the delay - dropped this draft somewhere along the way)
>
> Sorry, I didn't mean to suggest a change in behavior - perhaps I
> misunderstood your warning/error.
>
> What I was assuming was that -fsanitize=init-order requires
> -fsanitize=address, right?


Right.


> In that case, -fsanitize=address,init-order
> -fno-sanitize=address would be good if it was silent (essentially:
> disabling -fsanitize-address would disable any
> subfeatures/dependencies of it)
>

Yeah, I find this pretty reasonable. I've updated the patch
to silently accept this.



>
> > then what should "-fsanitize=address,init-order -fno-sanitize=address
> > -fsanitize=address"
> > expand into?
>
> Fair question - I suppose it'd still be best to have that turn on both
> address and init-order.
>
> (but my points here are fairly hypothetical, I assume it's not worth
> going to this level of detail for this issue - but I don't know the
> use cases well enough to judge that)
>
> >
> >>
> >>
> >> > As a rationale, consider the case when one wants to optionally build a
> >> > large project
> >> > with ASan, but disable instrumentation for specific files. Previously,
> >> > adding
> >> > "-fno-sanitize=address" to a file-specific compile options would
> produce
> >> > an error when building the whole project with
> >> > "-fsanitize=address,init-order",
> >> > so we'd have to go and fix all "-fno-sanitize=..." occurences to
> contain
> >> > all possible features of ASan.
> >> >
> >> > http://llvm-reviews.chandlerc.com/D315
> >> >
> >> > Files:
> >> >   lib/Driver/Tools.cpp
> >> >   lib/Driver/SanitizerArgs.h
> >> >   test/Driver/fsanitize.c
> >> >   include/clang/Basic/DiagnosticGroups.td
> >> >   include/clang/Basic/DiagnosticDriverKinds.td
> >> >
> >> > Index: lib/Driver/Tools.cpp
> >> > ===================================================================
> >> > --- lib/Driver/Tools.cpp
> >> > +++ lib/Driver/Tools.cpp
> >> > @@ -1475,10 +1475,10 @@
> >> >
> >> >    // If -fsanitize contains extra features of ASan, it should also
> >> >    // explicitly contain -fsanitize=address.
> >> > -  if (NeedsAsan && ((Kind & Address) == 0))
> >> > -    D.Diag(diag::err_drv_argument_only_allowed_with)
> >> > -      << lastArgumentForKind(D, Args, NeedsAsanRt)
> >> > -      << "-fsanitize=address";
> >> > +  if (((Kind & Address) == 0) && ((Kind & AddressFull) != 0))
> >> > +    D.Diag(diag::warn_drv_unused_sanitizer)
> >> > +     << lastArgumentForKind(D, Args, AddressFull)
> >> > +     << "-fsanitize=address";
> >> >
> >> >    // Parse -f(no-)sanitize-blacklist options.
> >> >    if (Arg *BLArg = Args.getLastArg(options::OPT_fsanitize_blacklist,
> >> > Index: lib/Driver/SanitizerArgs.h
> >> > ===================================================================
> >> > --- lib/Driver/SanitizerArgs.h
> >> > +++ lib/Driver/SanitizerArgs.h
> >> > @@ -33,7 +33,7 @@
> >> >  #define SANITIZER(NAME, ID) ID = 1 << SO_##ID,
> >> >  #define SANITIZER_GROUP(NAME, ID, ALIAS) ID = ALIAS,
> >> >  #include "clang/Basic/Sanitizers.def"
> >> > -    NeedsAsanRt = AddressFull,
> >> > +    NeedsAsanRt = Address,
> >> >      NeedsTsanRt = Thread,
> >> >      NeedsMsanRt = Memory,
> >> >      NeedsUbsanRt = (Undefined & ~Bounds) | Integer
> >> > Index: test/Driver/fsanitize.c
> >> > ===================================================================
> >> > --- test/Driver/fsanitize.c
> >> > +++ test/Driver/fsanitize.c
> >> > @@ -31,7 +31,10 @@
> >> >  // CHECK-ASAN-TSAN: '-faddress-sanitizer' not allowed with
> >> > '-fthread-sanitizer'
> >> >
> >> >  // RUN: %clang -target x86_64-linux-gnu -fsanitize=init-order %s -###
> >> > 2>&1 | FileCheck %s --check-prefix=CHECK-ONLY-EXTRA-ASAN
> >> > -// CHECK-ONLY-EXTRA-ASAN: argument '-fsanitize=init-order' only
> allowed
> >> > with '-fsanitize=address'
> >> > +// CHECK-ONLY-EXTRA-ASAN: '-fsanitize=init-order' is ignored in
> absence
> >> > of '-fsanitize=address'
> >> > +
> >> > +// RUN: %clang -target x86_64-linux-gnu -Wno-unused-sanitize-argument
> >> > -fsanitize=init-order %s -### 2>&1 | FileCheck %s
> >> > --check-prefix=CHECK-WNO-UNUSED-SANITIZE-ARGUMENT
> >> > +// CHECK-WNO-UNUSED-SANITIZE-ARGUMENT-NOT: '-fsanitize=init-order' is
> >> > ignored in absence of '-fsanitize=address'
> >> >
> >> >  // RUN: %clang -target x86_64-linux-gnu
> -fsanitize-memory-track-origins
> >> > -pie %s -### 2>&1 | FileCheck %s
> --check-prefix=CHECK-ONLY-TRACK-ORIGINS
> >> >  // CHECK-ONLY-TRACK-ORIGINS: warning: argument unused during
> >> > compilation: '-fsanitize-memory-track-origins'
> >> > Index: include/clang/Basic/DiagnosticGroups.td
> >> > ===================================================================
> >> > --- include/clang/Basic/DiagnosticGroups.td
> >> > +++ include/clang/Basic/DiagnosticGroups.td
> >> > @@ -270,7 +270,9 @@
> >> >  def UnnamedTypeTemplateArgs : DiagGroup<"unnamed-type-template-args",
> >> >
> >> > [CXX98CompatUnnamedTypeTemplateArgs]>;
> >> >  def UnusedArgument : DiagGroup<"unused-argument">;
> >> > -def UnusedCommandLineArgument :
> >> > DiagGroup<"unused-command-line-argument">;
> >> > +def UnusedSanitizeArgument : DiagGroup<"unused-sanitize-argument">;
> >> > +def UnusedCommandLineArgument :
> >> > DiagGroup<"unused-command-line-argument",
> >> > +                                          [UnusedSanitizeArgument]>;
> >> >  def UnusedComparison : DiagGroup<"unused-comparison">;
> >> >  def UnusedExceptionParameter :
> DiagGroup<"unused-exception-parameter">;
> >> >  def UnneededInternalDecl :
> DiagGroup<"unneeded-internal-declaration">;
> >> > Index: include/clang/Basic/DiagnosticDriverKinds.td
> >> > ===================================================================
> >> > --- include/clang/Basic/DiagnosticDriverKinds.td
> >> > +++ include/clang/Basic/DiagnosticDriverKinds.td
> >> > @@ -123,6 +123,8 @@
> >> >  def warn_drv_empty_joined_argument : Warning<
> >> >    "joined argument expects additional value: '%0'">,
> >> >    InGroup<UnusedCommandLineArgument>;
> >> > +def warn_drv_unused_sanitizer : Warning<"'%0' is ignored in absence
> of
> >> > '%1'">,
> >> > +  InGroup<UnusedSanitizeArgument>;
> >> >  def warn_drv_clang_unsupported : Warning<
> >> >    "the clang compiler does not support '%0'">;
> >> >  def warn_drv_deprecated_arg : Warning<
> >> >
> >> > _______________________________________________
> >> > cfe-commits mailing list
> >> > cfe-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >
> >
> >
> >
> >
> > --
> > Alexey Samsonov, MSK
>



-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130125/a991fdf3/attachment.html>