[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda added a comment. Great, thanks... https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
arphaman added a comment. I was impatient, so I already started on a patch for diagtool. I'll post it soon. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda added a comment. I'd be happy to do that if it would help. If so, should I do it here create a new diff? Perhaps we might even make sense add the ability to pass in a message and find the matching name/index. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
arphaman added a comment. Right. I was aware of the `diagtool` before, but didn't really look into what it did. TIL! It would make sense to add this kind of mapping functionality to that tool then. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda added a comment. Not sure how to do this all in a script, but perhaps we could enhance diagtool to generate this mapping for you. It currently only lists warnings, but I don't think it would be difficult to extend it and generate a mapping you could use in your script. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
arphaman added a comment. My script relies on a hack to map the name of the diagnostic to the enum value. We need to come up with a better to map the diagnostic name to the enum value. I propose a new utility tool that would take the name of the diagnostic and map it back to the enum value. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda abandoned this revision. hintonda added a comment. Okay, sounds good. Look forward to seeing Alex's script... https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
rjmccall added a comment. To me, features that only serve to help compiler development need to meet a higher bar than this. This just seems really marginal. Like Alex said, you should be able to pretty easily write a debugger script that breaks when it sees a specific diagnostic, or a diagnostic at a specific line and column. That would be quite welcome in utils/. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda added a comment. It's just an effort to make the code a bit more accessible, especially for new users (or ones not used to running find/grep). Steve had suggested adding an option that took the entire message and matched it when it was produced. However, that won't work very well since the message isn't actually produced until just before it is printed, which means the assert/backtrace isn't in the correctly location if the it's delayed. That's why I chose to simply print just the enum name and index. Adding `__FILE__`/`__LINE__` info would help identify the exact location (could be multiple for the same error message), but due to the way the code is structured it isn't really doable. It might be kinda fun writing the script you suggest -- I'll look into it, but printing the enum in the first place for little or no cost seems a bit more elegant. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
rjmccall added a comment. This is a cute hack, but... I'm not sure I accept the premise that it's a noteworthy obstacle to Clang development to do two greps instead of one. And I don't think I've ever had to debug a diagnostic where __FILE__ and __LINE__ information would've been helpful. Also, you could easily write a script that could automatically annotate arbitrary text with this information retroactively by turning the diagnostic text into a bunch of regular expressions, and then you wouldn't even need to re-run the compiler. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda updated this revision to Diff 106094. hintonda added a comment. - Only maintain enum names in debug builds. Current cost of maintaining enum names is 176k. https://reviews.llvm.org/D35175 Files: include/clang/Basic/DiagnosticIDs.h include/clang/Basic/DiagnosticOptions.def include/clang/Driver/CC1Options.td lib/Basic/DiagnosticIDs.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/TextDiagnosticPrinter.cpp test/Frontend/diagnostics-option-diag-ids.cpp Index: test/Frontend/diagnostics-option-diag-ids.cpp === --- /dev/null +++ test/Frontend/diagnostics-option-diag-ids.cpp @@ -0,0 +1,15 @@ +// RUN: not %clang_cc1 -fdiagnostics-show-option -fdiagnostics-show-diag-ids %s 2> %t +// RUN: FileCheck < %t %s + +class xx { + xx(){} +}; + +void foo(sss) { +// CHECK: [[@LINE-1]]:10: error: unknown type name 'sss' [err_unknown_typename:{{[0-9]+}}] + int; +// CHECK: [[@LINE-1]]:3: warning: declaration does not declare anything [-Wmissing-declarations,ext_no_declarators:{{[0-9]+}}] + xx x; +// CHECK: [[@LINE-1]]:6: error: calling a private constructor of class 'xx' [err_access_ctor:{{[0-9]+}}] +// CHECK: [[@LINE-9]]:3: note: implicitly declared private here [note_access_natural:{{[0-9]+}}] +} Index: lib/Frontend/TextDiagnosticPrinter.cpp === --- lib/Frontend/TextDiagnosticPrinter.cpp +++ lib/Frontend/TextDiagnosticPrinter.cpp @@ -19,6 +19,7 @@ #include "llvm/ADT/SmallString.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/Support/Signals.h" #include using namespace clang; @@ -103,6 +104,12 @@ } } } + // If the user wants to see diagnostic ids, include it too. + if (DiagOpts.ShowDiagIDs) { +OS << (Started ? "," : " ["); +Started = true; +OS << DiagnosticIDs::getNameFromID(Info.getID()) << ":" << Info.getID(); + } if (Started) OS << ']'; } Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -1079,6 +1079,8 @@ << ShowCategory; } + Opts.ShowDiagIDs = Args.hasArg(OPT_fdiagnostics_show_diag_ids); + StringRef Format = Args.getLastArgValue(OPT_fdiagnostics_format, "clang"); if (Format == "clang") Index: lib/Basic/DiagnosticIDs.cpp === --- lib/Basic/DiagnosticIDs.cpp +++ lib/Basic/DiagnosticIDs.cpp @@ -228,7 +228,31 @@ return CategoryNameTable[CategoryID].getName(); } +StringRef DiagnosticIDs::getNameFromID(unsigned DiagID) { +#ifndef NDEBUG + static const char *StaticDiagName[] = { + #define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR, \ + SHOWINSYSHEADER, CATEGORY)\ +#ENUM, +#include "clang/Basic/DiagnosticCommonKinds.inc" +#include "clang/Basic/DiagnosticDriverKinds.inc" +#include "clang/Basic/DiagnosticFrontendKinds.inc" +#include "clang/Basic/DiagnosticSerializationKinds.inc" +#include "clang/Basic/DiagnosticLexKinds.inc" +#include "clang/Basic/DiagnosticParseKinds.inc" +#include "clang/Basic/DiagnosticASTKinds.inc" +#include "clang/Basic/DiagnosticCommentKinds.inc" +#include "clang/Basic/DiagnosticSemaKinds.inc" +#include "clang/Basic/DiagnosticAnalysisKinds.inc" +#undef DIAG + }; + if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID)) +return StringRef(StaticDiagName[Info - StaticDiagInfo]); +#endif + + return StringRef(); +} DiagnosticIDs::SFINAEResponse DiagnosticIDs::getDiagnosticSFINAEResponse(unsigned DiagID) { Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -380,6 +380,8 @@ HelpText<"Ignore unexpected diagnostic messages">; def Wno_rewrite_macros : Flag<["-"], "Wno-rewrite-macros">, HelpText<"Silence ObjC rewriting warnings">; +def fdiagnostics_show_diag_ids : Flag<["-"], "fdiagnostics-show-diag-ids">, + HelpText<"Show Diagnostic enum name and index">; //===--===// // Frontend Options Index: include/clang/Basic/DiagnosticOptions.def === --- include/clang/Basic/DiagnosticOptions.def +++ include/clang/Basic/DiagnosticOptions.def @@ -61,6 +61,7 @@ DIAGOPT(ShowNoteIncludeStack, 1, 0) /// Show include stacks for notes. VALUE_DIAGOPT(ShowCategories, 2, 0) /// Show categories: 0 -> none, 1 -> Number, /// 2 -> Full Name. +DIAGOPT(ShowDiagIDs, 1, 0) /// Show Diagnostic ID ENUM_DIAGOPT(Format, TextDiagnosticFormat, 2, Clang) /// Format for diagnostics: Index:
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
arphaman added a comment. > Currently looks like around 200k (4534 @ 33 byte avg length + ptr). If this > is too much, we could make it conditional based on NDEBUG or some other macro > at compile time. I think this is mostly useful during development, so some conditional mechanism would make sense IMO. I think that it makes sense to avoid the growth of our release binaries if such growth can be avoided. > This type of behavior (either an assert/bt or coupled with debugger) could be > useful as a quick and easy solution. However, capturing `__FILE__` and > `__LINE__` when a diagnostic is reported, would be my preference. However, > that change would be very invasive and should probably be handled by a source > to source transformation -- I did some of this by hand as a proof of concept, > but doing all of clang manually would take quite a while, not to mention > various tools that also use diagnostics. I can definitely see the usefulness of `__FILE__` and `__LINE__` markers. However, I think that should be a development only feature as well. I agree about the source to source transformation, a refactoring tool should handle it. > Agreed, but the problem with relying exclusively on coverage is that it can't > cover the various permutations, e.g., "%select{A|B|C}0". It's pretty > difficult to tell if A, B, and C were actually tested -- or if that makes a > difference. > > If we included enum name (and permutation) with `__FILE__` and `__LINE__` in > the output, then we could quickly analyze the test output and produce a > simple report. I think this would also be helpful in allowing test writers > to see exactly which diagnostic report was triggered for each test. That makes sense, thanks for pointing it out. I agree, It would be useful if we had such "coverage" reports for diagnostics. https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda updated this revision to Diff 105961. hintonda added a comment. - Make option cc1 only. Rename function, and add test. https://reviews.llvm.org/D35175 Files: include/clang/Basic/DiagnosticIDs.h include/clang/Basic/DiagnosticOptions.def include/clang/Driver/CC1Options.td lib/Basic/DiagnosticIDs.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/TextDiagnosticPrinter.cpp test/Frontend/diagnostics-option-diag-ids.cpp Index: test/Frontend/diagnostics-option-diag-ids.cpp === --- /dev/null +++ test/Frontend/diagnostics-option-diag-ids.cpp @@ -0,0 +1,15 @@ +// RUN: not %clang_cc1 -fdiagnostics-show-option -fdiagnostics-show-diag-ids %s 2> %t +// RUN: FileCheck < %t %s + +class xx { + xx(){} +}; + +void foo(sss) { +// CHECK: [[@LINE-1]]:10: error: unknown type name 'sss' [err_unknown_typename:{{[0-9]+}}] + int; +// CHECK: [[@LINE-1]]:3: warning: declaration does not declare anything [-Wmissing-declarations,ext_no_declarators:{{[0-9]+}}] + xx x; +// CHECK: [[@LINE-1]]:6: error: calling a private constructor of class 'xx' [err_access_ctor:{{[0-9]+}}] +// CHECK: [[@LINE-9]]:3: note: implicitly declared private here [note_access_natural:{{[0-9]+}}] +} Index: lib/Frontend/TextDiagnosticPrinter.cpp === --- lib/Frontend/TextDiagnosticPrinter.cpp +++ lib/Frontend/TextDiagnosticPrinter.cpp @@ -19,6 +19,7 @@ #include "llvm/ADT/SmallString.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/Support/Signals.h" #include using namespace clang; @@ -103,6 +104,12 @@ } } } + // If the user wants to see diagnostic ids, include it too. + if (DiagOpts.ShowDiagIDs) { +OS << (Started ? "," : " ["); +Started = true; +OS << DiagnosticIDs::getNameFromID(Info.getID()) << ":" << Info.getID(); + } if (Started) OS << ']'; } Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -1079,6 +1079,8 @@ << ShowCategory; } + Opts.ShowDiagIDs = Args.hasArg(OPT_fdiagnostics_show_diag_ids); + StringRef Format = Args.getLastArgValue(OPT_fdiagnostics_format, "clang"); if (Format == "clang") Index: lib/Basic/DiagnosticIDs.cpp === --- lib/Basic/DiagnosticIDs.cpp +++ lib/Basic/DiagnosticIDs.cpp @@ -37,6 +37,7 @@ }; struct StaticDiagInfoRec { + const char *IDName; uint16_t DiagID; unsigned DefaultSeverity : 3; unsigned Class : 3; @@ -74,8 +75,9 @@ #define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR, \ SHOWINSYSHEADER, CATEGORY)\ {\ -diag::ENUM, DEFAULT_SEVERITY, CLASS, DiagnosticIDs::SFINAE, NOWERROR, \ -SHOWINSYSHEADER, CATEGORY, GROUP, STR_SIZE(DESC, uint16_t), DESC \ +#ENUM, diag::ENUM, DEFAULT_SEVERITY, CLASS, DiagnosticIDs::SFINAE, \ +NOWERROR, SHOWINSYSHEADER, CATEGORY, GROUP, STR_SIZE(DESC, uint16_t), \ +DESC \ }\ , #include "clang/Basic/DiagnosticCommonKinds.inc" @@ -228,7 +230,11 @@ return CategoryNameTable[CategoryID].getName(); } - +StringRef DiagnosticIDs::getNameFromID(unsigned DiagID) { + if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID)) +return StringRef(Info->IDName); + return StringRef(); +} DiagnosticIDs::SFINAEResponse DiagnosticIDs::getDiagnosticSFINAEResponse(unsigned DiagID) { Index: include/clang/Driver/CC1Options.td === --- include/clang/Driver/CC1Options.td +++ include/clang/Driver/CC1Options.td @@ -380,6 +380,8 @@ HelpText<"Ignore unexpected diagnostic messages">; def Wno_rewrite_macros : Flag<["-"], "Wno-rewrite-macros">, HelpText<"Silence ObjC rewriting warnings">; +def fdiagnostics_show_diag_ids : Flag<["-"], "fdiagnostics-show-diag-ids">, + HelpText<"Show Diagnostic enum name and index">; //===--===// // Frontend Options Index: include/clang/Basic/DiagnosticOptions.def === --- include/clang/Basic/DiagnosticOptions.def +++ include/clang/Basic/DiagnosticOptions.def @@ -61,6 +61,7 @@ DIAGOPT(ShowNoteIncludeStack, 1, 0) /// Show include stacks for notes. VALUE_DIAGOPT(ShowCategories, 2, 0) /// Show categories: 0 -> none, 1 -> Number, /// 2 -> Full Name. +DIAGOPT(ShowDiagIDs, 1, 0) /// Show Diagnostic ID
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
arphaman added a comment. Thanks, that's pretty cool! How bigger did the clang binary get after you've added all these strings? I feel like this is more of a CC1 option as well. I have some feedback for your additional ideas: > add another option to pass in the index (or enum) to force an assert or > backtrace when a specific DiagID is seen. That sounds quite useful, but could it be something that's more suited for an external debugging script? I have a personal script that computes the enum value for a particular diagnostic, launches clang in lldb, sets a breakpoint for that particular diagnostic enum and runs clang. I could work on upstreaming it into clang/utils if people are interested. > capture FILE and LINE when a diagnostic is created and output it. This would > make it easier to find the specific instance, and verify all instances are > actually tested. Currently, it's almost impossible to determine if all > instances are actually tested. I reckon the first part (find the specific instance) could be useful, but I think that if you can force a backtrace on a specific DiagID then it becomes less useful. I disagree with the second part, can't you use our coverage bots and see if the all places where the diagnostic is emitted are covered to see if they are tested? It might be tedious to find these places, but maybe we can add a search for our coverage viewer so you quickly find the lines that have the name of diagnostic? https://reviews.llvm.org/D35175 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.
hintonda created this revision. This option helps locate the origin of a diagnostic message by outputing the enum name and index associated with a specific DiagID, allowing users to grep the code for the enum name directly without having to find it in the td files first. Additional ideas: 1. add another option to pass in the index (or enum) to force an assert or backtrace when a specific DiagID is seen. 2. capture __FILE__ and __LINE__ when a diagnostic is created and output it. This would make it easier to find the specific instance, and verify all instances are actually tested. Currently, it's almost impossible to determine if all instances are actually tested. 3. keep track of the permutations and make sure each one is tested. https://reviews.llvm.org/D35175 Files: include/clang/Basic/DiagnosticIDs.h include/clang/Basic/DiagnosticOptions.def include/clang/Driver/Options.td lib/Basic/DiagnosticIDs.cpp lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp lib/Frontend/TextDiagnosticPrinter.cpp Index: lib/Frontend/TextDiagnosticPrinter.cpp === --- lib/Frontend/TextDiagnosticPrinter.cpp +++ lib/Frontend/TextDiagnosticPrinter.cpp @@ -19,6 +19,7 @@ #include "llvm/ADT/SmallString.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/raw_ostream.h" +#include "llvm/Support/Signals.h" #include using namespace clang; @@ -103,6 +104,13 @@ } } } + // If the user wants to see diagnostic ids, include it too. + if (DiagOpts.ShowDiagIDs) { +OS << (Started ? "," : " ["); +Started = true; +OS << DiagnosticIDs::getDiagIDName(Info.getID()) << ":" << Info.getID(); + } + if (Started) OS << ']'; } Index: lib/Frontend/CompilerInvocation.cpp === --- lib/Frontend/CompilerInvocation.cpp +++ lib/Frontend/CompilerInvocation.cpp @@ -1079,6 +1079,8 @@ << ShowCategory; } + Opts.ShowDiagIDs = Args.hasArg(OPT_fdiagnostics_show_diag_ids); + StringRef Format = Args.getLastArgValue(OPT_fdiagnostics_format, "clang"); if (Format == "clang") Index: lib/Driver/ToolChains/Clang.cpp === --- lib/Driver/ToolChains/Clang.cpp +++ lib/Driver/ToolChains/Clang.cpp @@ -4046,6 +4046,11 @@ CmdArgs.push_back(A->getValue()); } + if (Args.hasFlag(options::OPT_fdiagnostics_show_diag_ids, + options::OPT_fno_diagnostics_show_diag_ids, false)) { +CmdArgs.push_back("-fdiagnostics-show-diag-ids"); + } + if (Args.hasFlag(options::OPT_fdiagnostics_show_hotness, options::OPT_fno_diagnostics_show_hotness, false)) CmdArgs.push_back("-fdiagnostics-show-hotness"); Index: lib/Basic/DiagnosticIDs.cpp === --- lib/Basic/DiagnosticIDs.cpp +++ lib/Basic/DiagnosticIDs.cpp @@ -37,6 +37,7 @@ }; struct StaticDiagInfoRec { + const char *IDName; uint16_t DiagID; unsigned DefaultSeverity : 3; unsigned Class : 3; @@ -74,8 +75,9 @@ #define DIAG(ENUM, CLASS, DEFAULT_SEVERITY, DESC, GROUP, SFINAE, NOWERROR, \ SHOWINSYSHEADER, CATEGORY)\ {\ -diag::ENUM, DEFAULT_SEVERITY, CLASS, DiagnosticIDs::SFINAE, NOWERROR, \ -SHOWINSYSHEADER, CATEGORY, GROUP, STR_SIZE(DESC, uint16_t), DESC \ +#ENUM, diag::ENUM, DEFAULT_SEVERITY, CLASS, DiagnosticIDs::SFINAE, \ +NOWERROR, SHOWINSYSHEADER, CATEGORY, GROUP, STR_SIZE(DESC, uint16_t), \ +DESC \ }\ , #include "clang/Basic/DiagnosticCommonKinds.inc" @@ -179,6 +181,12 @@ return 0; } +StringRef DiagnosticIDs::getDiagIDName(unsigned DiagID) { + if (const StaticDiagInfoRec *Info = GetDiagInfo(DiagID)) +return StringRef(Info->IDName); + return StringRef(); +} + namespace { // The diagnostic category names. struct StaticDiagCategoryRec { Index: include/clang/Driver/Options.td === --- include/clang/Driver/Options.td +++ include/clang/Driver/Options.td @@ -742,6 +742,10 @@ Group, Flags<[CC1Option]>, HelpText<"Display include stacks for diagnostic notes">; def fdiagnostics_format_EQ : Joined<["-"], "fdiagnostics-format=">, Group; def fdiagnostics_show_category_EQ : Joined<["-"], "fdiagnostics-show-category=">, Group; +def fdiagnostics_show_diag_ids : Flag<["-"], "fdiagnostics-show-diag-ids">, +Group, Flags<[CC1Option]>; +def fno_diagnostics_show_diag_ids : Flag<["-"], "fno-diagnostics-show-diag-ids">, +Group, Flags<[CC1Option]>; def fdiagnostics_show_template_tree : Flag<["-"],