rnk added a comment.
I got some actual data. The way we package clang today, the extracted
installation is 134.83M, and lib/clang/7.0.0/lib/darwin/* is 13M, so it's a 10%
increase. It would be worth it to us to replace these with empty files if this
change does land, but again, I'd rather not
rnk added a subscriber: beanz.
rnk added a comment.
In https://reviews.llvm.org/D15225#1191304, @george.karpenkov wrote:
> @rnk As discussed, would it be acceptable for you to just have empty
> sanitizer runtime files in the resource directory?
I was talking to @beanz, and he suggested adding
george.karpenkov added a comment.
@rnk As discussed, would it be acceptable for you to just have empty sanitizer
runtime files in the resource directory?
Repository:
rL LLVM
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
george.karpenkov added a comment.
@rnk OK it's fine with me to revert it now and then we see what can be done.
Repository:
rL LLVM
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rnk added a comment.
In https://reviews.llvm.org/D15225#1183318, @george.karpenkov wrote:
> @rnk changing subjects: will you be at the LLVM social on Thursday? Could we
> discuss it there?
Yes, we can and should, but I'd like update chromium's copy of clang before
then.
Repository:
rL
george.karpenkov added a comment.
@rnk changing subjects: will you be at the LLVM social on Thursday? Could we
discuss it there?
Repository:
rL LLVM
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.karpenkov added a comment.
@rnk OK I see, we've accidentally posted comments nearly simultaneously.
Repository:
rL LLVM
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
george.karpenkov added a comment.
> it's just that the runtime library didn't happen to exist on the system
> performing compilation
But then conceptually, the compiler toolchain on the system performing
compilation is not fully supporting asan, right?
It works for producing asanified object
rnk added a comment.
In https://reviews.llvm.org/D15225#1183132, @george.karpenkov wrote:
> @rnk could you clarify how did it break the distributed asanified build? If
> the slave compiler supports asan it should have this runtime, right?
Most open source distributed build systems wrap only
george.karpenkov added a comment.
@rnk Regarding the error message, would it be possible to compromise on
something like "sanitizer runtime not available"?
I understand that the exact error message would be very useful for you, but
it's just confusing for a user getting a toolchain with Xcode,
george.karpenkov added a comment.
@rnk could you clarify how did it break the distributed asanified build? If the
slave compiler supports asan it should have this runtime, right?
Repository:
rL LLVM
https://reviews.llvm.org/D15225
___
rnk added subscribers: hans, thakis, samsonov, rnk.
rnk added a comment.
+@thakis @hans
I think this broke Chromium's distributed Mac ASan build.
I personally found the error message confusing for the reasons that @samsonov
mentioned back in 2015. It's not that ASan wasn't supported on the
This revision was automatically updated to reflect the committed changes.
Closed by commit rL337635: [Driver] Sanitizer support based on runtime library
presence (authored by george.karpenkov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D15225?vs=156603=156639#toc
delcypher added a comment.
@george.karpenkov Other than the comment that probably needs updating, LGTM.
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.h:425
+ /// \return Relative path to the filename for the library
+ /// containing the sanitizer {@code SanitizerName}.
You might want to update that comment to not mention
george.karpenkov updated this revision to Diff 156603.
https://reviews.llvm.org/D15225
Files:
clang/lib/Driver/ToolChains/Darwin.cpp
clang/lib/Driver/ToolChains/Darwin.h
clang/test/Driver/Inputs/resource_dir/lib/darwin/libclang_rt.asan_ios_dynamic.dylib
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |=
george.karpenkov added a comment.
@delcypher sorry i don't agree with the change requests.
@mracek any comments?
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |=
delcypher added inline comments.
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2298
SanitizerMask Res = ToolChain::getSupportedSanitizers();
- Res |= SanitizerKind::Address;
- Res |= SanitizerKind::Leak;
- Res |= SanitizerKind::Fuzzer;
- Res |=
delcypher requested changes to this revision.
delcypher added a comment.
This revision now requires changes to proceed.
Seems mostly fine apart from some minor nits.
If I'm honest I don't see any reason why this should be Darwin specific. Sure
the naming convention and location of the runtime
george.karpenkov added a comment.
@delcypher Could you take a look?
@kcc Any objections?
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
kubamracek added a comment.
This looks great to me, but someone else should review this as well.
https://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
george.karpenkov updated this revision to Diff 155785.
george.karpenkov edited the summary of this revision.
george.karpenkov edited reviewers, added: delcypher; removed: bob.wilson,
glider, t.p.northover, samsonov, beanz.
george.karpenkov added a comment.
Attempt #2: reduced version of this
beanz requested changes to this revision.
beanz added a comment.
This revision now requires changes to proceed.
It looks like @samsonov had quite a few valid review comments that are still
unresolved. I know he hasn't been active lately, but I think those points
deserve discussion.
-Chris
zaks.anna added a comment.
LGTM.
Any other comments to this?
http://reviews.llvm.org/D15225
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
ygribov added a subscriber: ygribov.
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s
kubabrecka added inline comments.
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s
ygribov added inline comments.
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s
zaks.anna added a comment.
> I don't know, is there a way to install runtime components for ASan if your
> distribution doesn't happen to have one (that must be tricky, as the version
> of ASan should match the version of the compiler).
Correct, there is no recommended way of installing the
zaks.anna added a comment.
> I see, so essentially you want to use a different approach for determining
> sanitizer availability (on OS X for now): if the library is present, then we
> support
> sanitizer, otherwise we don't: i.e. the binary distribution is the source of
> truth, not the
samsonov added inline comments.
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s
beanz added inline comments.
Comment at: test/Driver/fsanitize.c:221
@@ +220,3 @@
+// RUN: %clang -target x86_64-apple-darwin10
-resource-dir=%S/Inputs/resource_dir -fsanitize=memory -fsanitize=thread,memory
%s -### 2>&1 | FileCheck %s
kubabrecka added a comment.
> The one thing I'm not sure about on this is, do we really want this to be
> darwin-only? I kinda think it might be nice if we unified this behavior
> across all platforms.
> ... I wonder if this can't be better abstracted.
> I'd really like to see all operating
kubabrecka updated this revision to Diff 42041.
kubabrecka added a comment.
Applying clang-format.
http://reviews.llvm.org/D15225
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/ToolChain.h
lib/Driver/SanitizerArgs.cpp
lib/Driver/ToolChain.cpp
samsonov added inline comments.
Comment at: include/clang/Driver/ToolChain.h:410
@@ -407,1 +409,3 @@
+ /// (don't require runtime library).
+ virtual SanitizerMask getSanitizersRequiringTrap() const;
};
I don't think this is relevant to ToolChain at all.
beanz added a comment.
Mostly formatting comments. I think for the most part this is the right way to
go, but I don't really feel qualified to approve this. The one thing I'm not
sure about on this is, do we really want this to be darwin-only? I kinda think
it might be nice if we unified this
36 matches
Mail list logo
