-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[driver] Make --version show if assertions, etc. are enabled #87585
Conversation
It's useful to have some significant build options visible in the version when investigating problems with a specific compiler artifact. This makes it easy to see if assertions, expensive checks, sanitizers, etc. are enabled when checking a compiler version.
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang Author: Cassie Jones (porglezomp) ChangesIt's useful to have some significant build options visible in the version when investigating problems with a specific compiler artifact. This makes it easy to see if assertions, expensive checks, sanitizers, etc. are enabled when checking a compiler version. Example output:
Discussion thread: https://discourse.llvm.org/t/rfc-printing-more-build-config-in-clang-version/78112 Full diff: https://github.com/llvm/llvm-project/pull/87585.diff 2 Files Affected:
diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index 7a53764364ce4d..37180efb7ea67b 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -2002,6 +2002,44 @@ void Driver::PrintVersion(const Compilation &C, raw_ostream &OS) const {
// Print out the install directory.
OS << "InstalledDir: " << Dir << '\n';
+ // Print out build configuration options that impact the compiler's runtime
+ // behavior. Intended for identifying the source of issues when reproducing
+ // changes.
+ std::vector<std::string> BuildOptions = {
+#if !__OPTIMIZE__
+ "+unoptimized",
+#endif
+#ifndef NDEBUG
+ "+assertions",
+#endif
+#ifdef EXPENSIVE_CHECKS
+ "+expensive-checks",
+#endif
+#if __has_feature(address_sanitizer)
+ "+asan",
+#endif
+#if __has_feature(undefined_behavior_sanitizer)
+ "+ubsan",
+#endif
+#if __has_feature(memory_sanitizer)
+ "+msan",
+#endif
+#if __has_feature(dataflow_sanitizer)
+ "+dfsan",
+#endif
+ };
+ if (!BuildOptions.empty()) {
+ OS << "Build configuration: ";
+ bool FirstOption = true;
+ for (const auto &Option : BuildOptions) {
+ if (!FirstOption)
+ OS << ", ";
+ OS << Option;
+ FirstOption = false;
+ }
+ OS << '\n';
+ }
+
// If configuration files were used, print their paths.
for (auto ConfigFile : ConfigFiles)
OS << "Configuration file: " << ConfigFile << '\n';
diff --git a/clang/test/Driver/version-build-config.test b/clang/test/Driver/version-build-config.test
new file mode 100644
index 00000000000000..3d183f372908ea
--- /dev/null
+++ b/clang/test/Driver/version-build-config.test
@@ -0,0 +1,6 @@
+# REQUIRES: asserts
+# RUN: %clang --version 2>&1 | FileCheck %s
+
+# CHECK: clang version
+# When assertions are enabled, we should have a build configuration line that reflects that
+# CHECK: Build configuration: {{.*}}+assertions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added questions I'd like to see reviewers address here.
clang/lib/Driver/Driver.cpp
Outdated
#if !__OPTIMIZE__ | ||
"+unoptimized", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for reviewers: Is this a good enough mechanism for the "unoptimized" build case, or do we actually want to plumb the CMAKE_BUILD_TYPE
through? I like this because it doesn't expose extra information to the build, and I don't think distinguishing the different types of optimized builds is helpful.
Downside: doesn't work if you build clang with msvc. I don't know if that's an expected build configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building clang/LLVM with MSVC is a supported configuration and is tested with post-commit bots, so that is definitely an expected configuration. I'm not exactly sure what the best course of action is though. I don't think there's a universally supported preprocessor macro for whether or not optimizations are enabled.
CMAKE_BUILD_TYPE
I think would be the best bet. #72021 made it so that there are a fixed number of values (unless additional flags are set). That would also let the user differentiate between something like RELEASE
which is -O3
and MINSIZEREL
, which I believe is -Oz
. If there is a different value specified, just displaying something like custom I think should be fine.
I don't really have any opinions on this though and will leave to others to decide what the best course of action should be. Just trying to provide additional context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think +unoptimized
is good enough for me. CMAKE_BUILD_TYPE
doesn't give as much signal as it suggests, since toolchain files can stick in whatever CXX flags they want per build type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking about the fact that the build type doesn't directly correspond to optimized or not.
I should at least make it so it at least doesn't always display +unoptimized
on MSVC though, I can fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out llvm/Support/CommandLine
has a definition that's approximately correct for MSVC and uses __OPTIMIZE__
for gnu compatible compilers, I switched to using that.
clang/lib/Driver/Driver.cpp
Outdated
"+dfsan", | ||
#endif | ||
}; | ||
if (!BuildOptions.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want it to be hidden for the default config?
My opinion: yes—because this is helpful to llvm developers but not to compiler end-users.
Do we want a build option to force it hidden that vendors could use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making it opt-out, and having the release scripts disable it seems like a good idea to me. It's not actionable for end-users, but if one has to remember to turn it on (or even know it's an option), one might not see the benefit of it in day-to-day development.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this as LLVM_VERSION_PRINTER_SHOW_BUILD_CONFIG
which defaults on, is there anything I should do to update the release scripts here or is that for someone else to do?
clang/lib/Driver/Driver.cpp
Outdated
// Print out build configuration options that impact the compiler's runtime | ||
// behavior. Intended for identifying the source of issues when reproducing | ||
// changes. | ||
std::vector<std::string> BuildOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llvm::SmallVector<llvm::StringRef>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be const StringRef BuildOptions[] = {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems interleaveComma
doesn't work with plain arrays currently (because of using .begin()
instead of std::begin()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are all const char*, you can make this an ArrayRef<StringRef>
.
Hopefully that aligns all the comments in this thread :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this to a global variable to support making this a reusable API between tools, so I think ArrayRef<StringRef>
ends up being the appropriate type in the end.
Would it be worth doing this for the llvm tools like dsymutil and dwarfdump as well? They already print "Optimized build (with assertions)" but this PR covers a few more things, is more legible and it would be nice have more uniformity between the two. |
Allow us to reuse this logic in the feature printing.
✅ With the latest revision this PR passed the C/C++ code formatter. |
This reverts commit 1213ba2.
Moved the |
Some compilers released to end-users will want to force-disable printing the build config, since that's intended for compiler developers and not really end-users. Add that flag, defaulting to on, so compiler developers get the most benefit but release engineers can hide it.
The result of those refactors I now have a new build option to support hiding the version, should I be touching the |
cc @nico |
Co-authored-by: Jon Roelofs <jroelofs@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this LGTM!
llvm/lib/Support/CommandLine.cpp
Outdated
#if __has_feature(address_sanitizer) | ||
"+asan", | ||
#endif | ||
#if __has_feature(undefined_behavior_sanitizer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sort sanitizers alphabetically?
You can add thread_sanitizer and hwaddress_sanitizer
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on TSan.
I followed the documented list of __has_feature
that I could see, I don't see hwaddress_sanitizer
in the list here: https://clang.llvm.org/docs/LanguageExtensions.html#extensions-for-dynamic-analysis
I see it in the tests though, adding.
- Sort sanitizers by name - Add some missing sanitizers - Oops! Fix a lifetime issue with the compiler build config
It's useful to have some significant build options visible in the version when investigating problems with a specific compiler artifact. This makes it easy to see if assertions, expensive checks, sanitizers, etc. are enabled when checking a compiler version.
Example output:
Discussion thread: https://discourse.llvm.org/t/rfc-printing-more-build-config-in-clang-version/78112