This is an archive of the discontinued LLVM Phabricator instance.

clang-cl: Diagnose the usage of ASAN with a debug runtime library
ClosedPublic

Authored by ehsan on Oct 13 2014, 4:43 PM.

Details

Summary

AddressSanitizer currently doesn't support this configuration, and binaries
built with it will just get into an infinite loop during startup.

Diff Detail

Event Timeline

ehsan updated this revision to Diff 14844.Oct 13 2014, 4:43 PM
ehsan retitled this revision from to clang-cl: Diagnose the usage of ASAN with a debug runtime library.
ehsan updated this object.
ehsan edited the test plan for this revision. (Show Details)
ehsan added a reviewer: timurrrr.
ehsan added a subscriber: Unknown Object (MLST).
samsonov added inline comments.
lib/Driver/Tools.cpp
3590

Why not use err_drv_argument_not_allowed_with here and show that "-fsanitize=address not allowed with /MTd" or smth. like that?

ehsan added inline comments.Oct 13 2014, 5:15 PM
lib/Driver/Tools.cpp
3590

Well, technically there is nothing wrong with using these two arguments together. I tried to make it clear in the diagnostic that this is something that is just not supported yet. Do you think that's reasonable?

Then I'd vote for more generic driver error type: err_drv_argument_not_supported_with (with a slightly different wording than err_drv_argument_not_allowed_with).

I'd defer to Hans, he knows this code better.

I'm fine with the general direction.

include/clang/Basic/DiagnosticDriverKinds.td
89

The tool is called "AddressSanitizer", not "address sanitizer".

I'd prefer something line

"AddressSanitizer doesn't support linking with debug runtime libraries yet"
timurrrr added inline comments.Oct 14 2014, 3:56 AM
test/Driver/fsanitize.c
161

I think you can do something like

// RUN: ... -MDd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// RUN: ... -MTd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// RUN: ... -LDd ... | FileCheck %s -check-prefix=CHECK-ASAN-DEBUG-RTL
// CHECK-ASAN-DEBUG-RTL: <warning message>

// RUN: ...
// RUN: ...
// RUN: ...
// CHECK-ASAN-RELEASE-RTL-NOTE: <warning message>
ehsan updated this revision to Diff 14860.Oct 14 2014, 5:48 AM
ehsan edited edge metadata.

Addressed the review comments.

samsonov added inline comments.Oct 14 2014, 10:52 AM
include/clang/Basic/DiagnosticDriverKinds.td
88

Please put this note next to another notes defined in this file.

lib/Driver/Tools.cpp
3587

Generally we parse sanitizer arguments and print the error diagnostics in SanitizerArgs constructor (lib/Driver/SanitizerArgs.cpp). For example, that file is the place where we form error messages about incompatible sanitizer runtimes. I think, it makes sense to put these checks there as well to keep it consistent. You can also use "SanitizerArgs::lastArgumentForKind" method there.

hans edited edge metadata.Oct 14 2014, 11:15 AM

I'd defer to Hans, he knows this code better.

Seems like samsonov is on it.

ehsan updated this revision to Diff 14877.Oct 14 2014, 11:59 AM
ehsan edited edge metadata.

Addressed the review comments.

Ah, right.
Please stay in the loop for windows-specific stuff.
14 окт. 2014 г. 22:15 пользователь "Hans Wennborg" <hans@chromium.org>
написал:

I'd defer to Hans, he knows this code better.

Seems like samsonov is on it.

http://reviews.llvm.org/D5764

samsonov added inline comments.Oct 14 2014, 1:31 PM
lib/Driver/SanitizerArgs.cpp
178 ↗(On Diff #14877)

Can you use the following syntax here?

if (Arg *WindowsDebugRTArg = Args.getLastArg(options::OPT__SLASH_MTd, options::OPT__SLASH_MDd,
    options::OPT__SLASH_LDd)) {
  D.Diag(...) << ....;
}

Also, what is MSVC/clang-cl semantics if I pass "/MTd /MT"? Will it pick up the last flag (/MT), or try to link both runtimes?
In the first case we don't need to produce the error.

test/Driver/fsanitize.c
167

This line is enough here.

ehsan added inline comments.Oct 14 2014, 2:20 PM
lib/Driver/SanitizerArgs.cpp
178 ↗(On Diff #14877)

When passing "/MTd /MT", /MTd will be ignored. clang-cl honors that too. I should actually add a test case for that too.

ehsan updated this revision to Diff 14897.Oct 14 2014, 3:07 PM

Addressed the review comments.

rnk added a subscriber: rnk.Oct 14 2014, 3:40 PM
rnk added inline comments.
lib/Driver/SanitizerArgs.cpp
178 ↗(On Diff #14877)

MSVC generally implements "last flag wins" for conflicting flags, with some exceptions for things like /EH. They warn on it, but we don't because it's been the behavior in gcc for a long time.

samsonov accepted this revision.Oct 14 2014, 4:08 PM
samsonov edited edge metadata.

LGTM. Please run the patch (.cpp file change) through clang-format-diff before submitting.

This revision is now accepted and ready to land.Oct 14 2014, 4:08 PM
ehsan closed this revision.Oct 14 2014, 4:26 PM
ehsan updated this revision to Diff 14902.

Closed by commit rL219744 (authored by @ehsan).