This is an archive of the discontinued LLVM Phabricator instance.

[Clang][ASan] Introduce `-fsanitize-address-destructor-kind=` driver & frontend option.
ClosedPublic

Authored by delcypher on Feb 11 2021, 7:34 PM.

Details

Summary

The new -fsanitize-address-destructor-kind= option allows control over how module
destructors are emitted by ASan.

The new option is consumed by both the driver and the frontend and is propagated into
codegen options by the frontend.

Both the legacy and new pass manager code have been updated to consume the new option
from the codegen options.

It would be nice if the new utility functions (AsanDtorKindToString and
AsanDtorKindFromString) could live in LLVM instead of Clang so they could be
consumed by other language frontends. Unfortunately that doesn't work because
the clang driver doesn't link against the LLVM instrumentation library.

rdar://71609176

Diff Detail

Event Timeline

delcypher created this revision.Feb 11 2021, 7:34 PM
delcypher requested review of this revision.Feb 11 2021, 7:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2021, 7:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
delcypher updated this revision to Diff 323213.Feb 11 2021, 7:36 PM

clang-format fix

vitalybuka added inline comments.Feb 12 2021, 1:50 AM
clang/include/clang/Driver/Options.td
1504

What is the difference between them and why it's need to be configured from outside?

delcypher added inline comments.Feb 12 2021, 10:30 AM
clang/include/clang/Driver/Options.td
1504

Good questions.

What is the difference between them

I'll add some documentation to explain this. But the two different modes are introduced by https://reviews.llvm.org/D96571. Basically the default ("global") emits ASan module destructor calls via llvm.global_dtors which was the previous behaviour. The other mode "none" simply causes no ASan module destructors to be emitted. I plan to introduce another mode in a future patch.

why it's need to be configured from outside?

At the bare minimum this option needs to be controllable by the driver so that we can do the next patch (https://reviews.llvm.org/D96573) where based on target details and other flags we set the ASan destructor kind. This means that the frontend needs to be able to consume to option too.

It is technically not necessary for this new option to be a driver option. However, I made it a driver option too to match the existing ASan clang frontend options which are also driver options (e.g. -fsanitize-address-use-odr-indicator).

Add documentation for -fsanitize-address-destructor-kind= option.

delcypher added inline comments.Feb 12 2021, 12:58 PM
clang/include/clang/Driver/Options.td
1504

@vitalybuka To expand on my answer a little more. -fsanitize-address-destructor-kind= is needed because we need to able to control how ASan's module are emitted and unfortunately the information required to make the decision on which kind to emit is not available at the level of LLVM IR. The information we need to be able to make the decision is only available in the Clang driver. This is why this patch wires everything up so that the driver can change how ASan module destructors are emitted.

vitalybuka added inline comments.Feb 12 2021, 1:09 PM
clang/include/clang/Driver/Options.td
1504

@vitalybuka To expand on my answer a little more. -fsanitize-address-destructor-kind= is needed because we need to able to control how ASan's module are emitted and unfortunately the information required to make the decision on which kind to emit is not available at the level of LLVM IR. The information we need to be able to make the decision is only available in the Clang driver. This is why this patch wires everything up so that the driver can change how ASan module destructors are emitted.

I didn't check all patches yet, but we have -fsanitize=kernel-address and I noticed that your patches mentioned kernel. Could be possible just use -fsanitize=kernel-address and let ASan decide how to handle dtors for this platform?

@jansvoboda11 I noticed that the Options.td file is making use of some fancy mixins that mean the option gets automatically marshalled by the frontend into the codegen options :). I tried this out using the patch to this patch and all my tests seem to pass. Should I be using these mixins (I don't know how stable they are because this looks like a new feature). Am I using them properly? It seems that the driver still has to parse the option manually (which is actually what I want) but the frontend automagically parses the option and modifies the right codegen option for me.

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 57aa469ff211..0e8ea7debfca 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1481,11 +1481,17 @@ defm sanitize_address_use_odr_indicator : BoolOption<"f", "sanitize-address-use-
             " reports in partially sanitized programs at the cost of an increase in binary size">,
   NegFlag<SetFalse, [], "Disable ODR indicator globals">>,
   Group<f_clang_Group>;
-def sanitize_address_destructor_kind_EQ : Joined<["-"], "fsanitize-address-destructor-kind=">,
-  MetaVarName<"<kind>">,
-  Flags<[CC1Option]>,
-  HelpText<"Set destructor type used in ASan instrumentation">,
-  Group<f_clang_Group>;
+def sanitize_address_destructor_kind_EQ
+    : Joined<["-"], "fsanitize-address-destructor-kind=">,
+      MetaVarName<"<kind>">,
+      Flags<[CC1Option]>,
+      HelpText<"Set destructor type used in ASan instrumentation">,
+      Group<f_clang_Group>,
+      Values<"none,global">,
+      NormalizedValuesScope<"llvm::AsanDtorKind">,
+      NormalizedValues<["None", "Global"]>,
+      MarshallingInfoString<CodeGenOpts<"SanitizeAddressDtorKind">, "Global">,
+      AutoNormalizeEnum;
 // Note: This flag was introduced when it was necessary to distinguish between
 //       ABI for correct codegen.  This is no longer needed, but the flag is
 //       not removed since targeting either ABI will behave the same.
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index a6baf026a7e8..0a12705a9261 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -1511,19 +1511,6 @@ bool CompilerInvocation::ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args,
 
   Opts.EmitVersionIdentMetadata = Args.hasFlag(OPT_Qy, OPT_Qn, true);
 
-  if (LangOptsRef.Sanitize.has(SanitizerKind::Address)) {
-    if (Arg *A =
-            Args.getLastArg(options::OPT_sanitize_address_destructor_kind_EQ)) {
-      auto destructorKind = AsanDtorKindFromString(A->getValue());
-      if (destructorKind == llvm::AsanDtorKind::Invalid) {
-        Diags.Report(clang::diag::err_drv_unsupported_option_argument)
-            << A->getOption().getName() << A->getValue();
-      } else {
-        Opts.setSanitizeAddressDtorKind(destructorKind);
-      }
-    }
-  }
-
   return Success;
 }
 
diff --git a/clang/test/CodeGen/asan-destructor-kind.cpp b/clang/test/CodeGen/asan-destructor-kind.cpp
index 2bdb782db2b5..567482b72643 100644
--- a/clang/test/CodeGen/asan-destructor-kind.cpp
+++ b/clang/test/CodeGen/asan-destructor-kind.cpp
@@ -3,7 +3,7 @@
 // RUN:   -fsanitize-address-destructor-kind=bad_arg -emit-llvm -o - \
 // RUN:   -triple x86_64-apple-macosx10.15 %s 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CHECK-BAD-ARG
-// CHECK-BAD-ARG: unsupported argument 'bad_arg' to option 'fsanitize-address-destructor-kind='
+// CHECK-BAD-ARG: invalid value 'bad_arg' in '-fsanitize-address-destructor-kind=bad_arg'
 
 // Default is global dtor
 // RUN: %clang_cc1 -fsanitize=address -emit-llvm -o - -triple x86_64-apple-macosx10.15 \
delcypher added inline comments.Feb 12 2021, 4:09 PM
clang/include/clang/Driver/Options.td
1504

@vitalybuka Unfortunately using -fsanitize=kernel-address isn't a viable option. In a future patch I'll be adding another destructor kind where the decision to use it is also unfortunately not known at the LLVM IR level and is instead only known by Clang driver.

If you really don't want -fsanitize-address-destructor-kind= exposed as driver option (i.e. exposed to our users) we could make it a frontend (cc1) only option. However, that doesn't really seem to match the existing sanitizer frontend options which all seem to be driver options as well.

@jansvoboda11 I noticed that the Options.td file is making use of some fancy mixins that mean the option gets automatically marshalled by the frontend into the codegen options :). I tried this out using the patch to this patch and all my tests seem to pass. Should I be using these mixins (I don't know how stable they are because this looks like a new feature). Am I using them properly? It seems that the driver still has to parse the option manually (which is actually what I want) but the frontend automagically parses the option and modifies the right codegen option for me.

Hi, yes, this is a new feature. The option generation is not being enforced in any way yet, but the automatic marshalling should work. You can test it by running your clang -cc1 commands with the -round-trip-args flag (or clang with -Xclang -round-trip-args). With this option, the original cc1 command line is first parsed into a dummy CompilerInvocation, which is used to generate new command line. The new command line is what is then used to create the real CompilerInvocation.

As you've correctly discovered, this does not interact with the driver in any way.

I'll be sending an email to cfe-dev about this shortly. Your marshalling code seems correct to me. If something does not work, check out the patch with documentation D95790 and feel free to let me know.

vitalybuka resigned from this revision.Feb 17 2021, 2:11 PM

LGTM, but leaving for others

Rebase and fix merge conflict caused by f2f59d2a060788f17040ad924ee2d11da0e215c8

@jansvoboda11 I noticed that the Options.td file is making use of some fancy mixins that mean the option gets automatically marshalled by the frontend into the codegen options :). I tried this out using the patch to this patch and all my tests seem to pass. Should I be using these mixins (I don't know how stable they are because this looks like a new feature). Am I using them properly? It seems that the driver still has to parse the option manually (which is actually what I want) but the frontend automagically parses the option and modifies the right codegen option for me.

Hi, yes, this is a new feature. The option generation is not being enforced in any way yet, but the automatic marshalling should work. You can test it by running your clang -cc1 commands with the -round-trip-args flag (or clang with -Xclang -round-trip-args). With this option, the original cc1 command line is first parsed into a dummy CompilerInvocation, which is used to generate new command line. The new command line is what is then used to create the real CompilerInvocation.

As you've correctly discovered, this does not interact with the driver in any way.

I'll be sending an email to cfe-dev about this shortly. Your marshalling code seems correct to me. If something does not work, check out the patch with documentation D95790 and feel free to let me know.

@jansvoboda11 Thanks. If it's okay with you I'll make the change to use the new marshalling infrastructure in a separate patch (https://reviews.llvm.org/D97327) because I may need to back port this patch to an older LLVM branch.

delcypher marked 2 inline comments as done.Feb 23 2021, 12:27 PM

@jansvoboda11 Thanks. If it's okay with you I'll make the change to use the new marshalling infrastructure in a separate patch (https://reviews.llvm.org/D97327) because I may need to back port this patch to an older LLVM branch.

That's fine by me.

This revision is now accepted and ready to land.Feb 25 2021, 2:59 AM
arphaman accepted this revision.Feb 25 2021, 10:29 AM

Thanks, LGTM

This revision was landed with ongoing or failed builds.Feb 25 2021, 12:03 PM
This revision was automatically updated to reflect the committed changes.