[clang][LLVM Demangler] Add a diagnostic that validates that all mang… #111391
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Viktoriia Bakalova (VitaNuo) Changes…led names produced by Introduce the above assertion behind the Full diff: https://github.com/llvm/llvm-project/pull/111391.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/CodeGenOptions.def b/clang/include/clang/Basic/CodeGenOptions.def
index eac831278ee20d..d3e150fe53f804 100644
--- a/clang/include/clang/Basic/CodeGenOptions.def
+++ b/clang/include/clang/Basic/CodeGenOptions.def
@@ -462,6 +462,9 @@ ENUM_CODEGENOPT(ZeroCallUsedRegs, llvm::ZeroCallUsedRegs::ZeroCallUsedRegsKind,
/// non-deleting destructors. (No effect on Microsoft ABI.)
CODEGENOPT(CtorDtorReturnThis, 1, 0)
+/// Whether to validate if a produced mangled name can be demangled with LLVM demangler.
+CODEGENOPT(NoDemanglingFailures, 1, 0)
+
/// FIXME: Make DebugOptions its own top-level .def file.
#include "DebugOptions.def"
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 607ff47a857b8f..a6a8cfe21676c0 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -1967,6 +1967,10 @@ def fclang_abi_compat_EQ : Joined<["-"], "fclang-abi-compat=">, Group<f_clang_Gr
Visibility<[ClangOption, CC1Option]>,
MetaVarName<"<version>">, Values<"<major>.<minor>,latest">,
HelpText<"Attempt to match the ABI of Clang <version>">;
+def fno_demangling_failures: Flag<["-"], "fno-demangling-failures">, Group<f_clang_Group>,
+ Visibility<[ClangOption, CC1Option]>,
+ HelpText<"Assert that clang can demangle all the mangled names it generates">,
+ MarshallingInfoFlag<CodeGenOpts<"NoDemanglingFailures">>;
def fclasspath_EQ : Joined<["-"], "fclasspath=">, Group<f_Group>;
def fcolor_diagnostics : Flag<["-"], "fcolor-diagnostics">, Group<f_Group>,
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 25c1c496a4f27f..6afbfa9ae45ed9 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -48,12 +48,14 @@
#include "clang/Basic/Version.h"
#include "clang/CodeGen/BackendUtil.h"
#include "clang/CodeGen/ConstantInitBuilder.h"
+#include "clang/Driver/Driver.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
#include "llvm/BinaryFormat/ELF.h"
+#include "llvm/Demangle/Demangle.h"
#include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
#include "llvm/IR/AttributeMask.h"
#include "llvm/IR/CallingConv.h"
@@ -75,6 +77,7 @@
#include "llvm/TargetParser/Triple.h"
#include "llvm/TargetParser/X86TargetParser.h"
#include "llvm/Transforms/Utils/BuildLibCalls.h"
+#include <cassert>
#include <optional>
using namespace clang;
@@ -2044,6 +2047,10 @@ StringRef CodeGenModule::getMangledName(GlobalDecl GD) {
GD.getWithKernelReferenceKind(KernelReferenceKind::Kernel),
ND));
+ if (getCodeGenOpts().NoDemanglingFailures)
+ assert((!llvm::isMangledName(MangledName) || llvm::demangle(MangledName) !=
+ MangledName) && "clang must demangle a mangled name it generates!");
+
auto Result = Manglings.insert(std::make_pair(MangledName, GD));
return MangledDeclNames[CanonicalGD] = Result.first->first();
}
diff --git a/clang/test/CodeGenCXX/assert-demangle.cpp b/clang/test/CodeGenCXX/assert-demangle.cpp
new file mode 100644
index 00000000000000..86a7686f72d929
--- /dev/null
+++ b/clang/test/CodeGenCXX/assert-demangle.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -emit-llvm -fno-demangling-failures -triple %itanium_abi_triple -o - %s | FileCheck %s
+
+// CHECK: @_ZN6foobar3barEv
+// CHECK: @_ZN6foobar1A3fooEi
+namespace foobar {
+struct A {
+ void foo (int) {
+ }
+};
+
+void bar() {
+ A().foo(0);
+}
+} // namespace foobar
diff --git a/llvm/include/llvm/Demangle/Demangle.h b/llvm/include/llvm/Demangle/Demangle.h
index fe129603c0785d..910f0410d62db3 100644
--- a/llvm/include/llvm/Demangle/Demangle.h
+++ b/llvm/include/llvm/Demangle/Demangle.h
@@ -67,6 +67,13 @@ char *dlangDemangle(std::string_view MangledName);
/// demangling occurred.
std::string demangle(std::string_view MangledName);
+/// Determines if the argument string is a valid mangled name known to the
+/// demangler.
+/// \param Name - reference to a string that is potentially a mangled name.
+/// \returns - true if the argument represents a valid mangled name, false
+/// otherwise.
+bool isMangledName(std::string_view Name);
+
bool nonMicrosoftDemangle(std::string_view MangledName, std::string &Result,
bool CanHaveLeadingDot = true,
bool ParseParams = true);
diff --git a/llvm/lib/Demangle/Demangle.cpp b/llvm/lib/Demangle/Demangle.cpp
index f0f7eacac98e64..6b40cbb56cf28d 100644
--- a/llvm/lib/Demangle/Demangle.cpp
+++ b/llvm/lib/Demangle/Demangle.cpp
@@ -47,6 +47,16 @@ static bool isRustEncoding(std::string_view S) { return starts_with(S, "_R"); }
static bool isDLangEncoding(std::string_view S) { return starts_with(S, "_D"); }
+static bool isMicrosoftEncoding(std::string_view S) {
+ return starts_with(S, '?');
+}
+
+bool llvm::isMangledName(std::string_view Name) {
+ return starts_with(Name, '.') || isItaniumEncoding(Name) ||
+ isRustEncoding(Name) || isDLangEncoding(Name) ||
+ isMicrosoftEncoding(Name);
+}
+
bool llvm::nonMicrosoftDemangle(std::string_view MangledName,
std::string &Result, bool CanHaveLeadingDot,
bool ParseParams) {
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
VitaNuo
force-pushed
the
assert_demangle
branch
from
9c16570 to
542558c
Compare
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Since this is being done as an assert, this is something that really only applies during debug mode. I dont think this compiler flag makes sense because of it.
IMO, this should either be a preprocessor macro to enable (so it is a build config), or a diagnostic that happens all the time (perhaps whose default changes?).
The reason I am adding a flag on top of an assert is that there are unfortunately still many mangled names produced by |
|
Sorry for jumping in late. I think Erich is on point that having a flag that controls an assertion is a bit of a red flag as we are mixing build configuration and runtime configuration. It is at least unusual and may cause confusion. After thinking about this a bit more, should we maybe go all-in on one of the approaches?
I would probably vouch for the second approach. The only downside that I see is that we have to expose the warning flag to users (right?) and this is something that should be internal (-cc1). What do people think of that proposal? |
I agree completely. I have a preference for the second thing as well. We can have this be a warning-as-default-error type thing, which allows us to disable this with a flag, but is still an error. I MIGHT suggest using the functionality to see if that diagnostic is on before doing the test (due to how much additional work there is here), but IMO, that is a much better way forward. |
👍 I guess that's a good signal that this is the way to go. AFAIK, @VitaNuo has already started exploring this direction.
@erichkeane how do you feel about having it enabled given that this does not really block users, but merely points at demangling issues in LLVM's demangler (that they don't necessarily even use). It feels like this should be internal, but then we won't get any bug reports. I feel like we should at least fix the majority of the issues before enabling it (hence the rough roadmap above). |
I definitely have some heartburn about the enabling-of-flag by default, I would expect these diagnostics to be something actionable by our users, but I also definitely see the wish to get as many people testing this to improve the llvm demangler as possible. I pinged a few others to comment here, but I'd have to be reasonably sure this is a 'no one will ever see this' kind of thing, and if they DO, have a good idea of what to do. I'm on the fence, but if we were to, I have some thoughts: |
|
Totally agree to 2 to 4. Regarding the testing and => default enablement, I think we'll have to first see how many failures we'll see on LLVM + Google internal codebase. It's hard to tell at this point how many of those there will be, and so how feasible it is (or how soon it is feasible) to enable the warning by default. |
I am not super comfortable with enabling the diagnostic by default because it's not something the user can do anything about aside from disable the diagnostic (which means they'll report one issue and probably never report another again because they disabled the warning due to chattiness) and I don't think we want to train users to ignore diagnostics or force them to turn off low-value ones. However, what about this as a compromise: we enable the diagnostic without a diagnostic group (so it cannot be disabled) in all builds up to the release candidate, and then we fully disable it for the actual releases? This way, early adopters can help us find issues, but we're not inflicting any pain on the general public once we release. (FWIW, I think this is something that should have an RFC for wider community buy-in given the effects.) |
…led names produced by `clang` can be demangled by LLVM demangler. Introduce the above warning behind the `-fdemangling-failures` flag, since: a. the diagnostic cannot be fixed by an application developer. b. the diagnostic is expected to be quite chatty.
VitaNuo
force-pushed
the
assert_demangle
branch
from
542558c to
54ee2e5
Compare
We would need a way to enable or disable this diagnostic internally, ideally via a runtime switch, so that we don't need to ship a new compiler every time a user hits this warning. I think the same also holds for users that use RC: we want them to report the bug, but they also need to make progress on their testing of RC/whatever else they're doing with it. I think the way out of it is to enable this diagnostic only after we make sure it is not chatty. So we do a gradual rollout:
Hopefully by the last step, we will be discovering almost all names that we cannot demangle during Clang development and users will almost never see it. +1 to having this discussion in the RFC |
Sounds good, I'll try to summarize this discussion as an RFC.
It might be a good idea to do this first, in order to collect more data for an RFC and the corresponding discussion. |
VitaNuo
changed the title
…res` flag to `clang` driver.
llvmbot
added
the
clang:driver
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
Close for me, jsut a couple of small requests.
Thanks for your patience on this response, I've been at LLVMDev all week.
| CODEGENOPT(CtorDtorReturnThis, 1, 0) | ||
|
|
||
| /// Whether to issue a diagnostic if a produced mangled name can not be demangled with the LLVM demangler. | ||
| CODEGENOPT(DemanglingFailures, 1, 0) |
There was a problem hiding this comment.
| CODEGENOPT(DemanglingFailures, 1, 0) | |
| CODEGENOPT(DiagDemanglerFailures, 1, 0) |
There was a problem hiding this comment.
Thanks, mentioning diagnostics makes sense.
|
|
||
| def warn_name_cannot_be_demangled : Warning< | ||
| "cannot demangle the name '%0'">, | ||
| InGroup<CXX20Compat>; |
There was a problem hiding this comment.
Group is wrong here, we likely want to give it its own group for the diagnostic.
There was a problem hiding this comment.
Added a new Demangler group.
| Visibility<[ClangOption, CC1Option]>, | ||
| MetaVarName<"<version>">, Values<"<major>.<minor>,latest">, | ||
| HelpText<"Attempt to match the ABI of Clang <version>">; | ||
| def fdemangling_failures: Flag<["-"], "fdemangling-failures">, Group<f_clang_Group>, |
There was a problem hiding this comment.
| def fdemangling_failures: Flag<["-"], "fdemangling-failures">, Group<f_clang_Group>, | |
| def fdiag-demangler_failures: Flag<["-"], "fdiag-demangler-failures">, Group<f_clang_Group>, |
Same as above... I'm not attached to the demangling vs demangler, but mentioning explicitly that this is for 'diag' everywhere is necessary for readability.
There was a problem hiding this comment.
Thank you, good point. I've looked through the existing flags, there are multiple ones that start with fdiagnostics_XXX, so I have renamed the flag to fdiagnostics_demangler_failures.
Let me know if this works.
… controls the demangler diagnostic. Reduce the new include count in CodeGenModule.cpp. Add a new diagnostic group for demangler failures.
Thanks Erich for the approval. |
|
The RFC is published https://discourse.llvm.org/t/rfc-clang-diagnostic-for-demangling-failures/82835. |
Thanks! I thought i commented elsewhere, but I think Else, the patch is good delta what happens with the RFC. Please make sure to wait until Aaron calls the RFC. |
Thanks! I think it was
Ofc. I am also OOO next week. |
|
After some internal discussions, we have decided to put this work on hold. In case someone in the community would like to pick up the effort, this PR has the code for the diagnostic. I will also add a comment to the codegen module mentioning the invariant that should hold in the future. |
5 participants
…led names produced by
clangcan be demangled by LLVM demangler.Introduce the above diagnostic behind the
-fno-demangling-failuresflag to prevent unintended breakages.