On 3/19/20 10:26 PM, David Malcolm wrote:
On Thu, 2020-03-19 at 14:52 +0100, Martin Liška wrote:
Hi.
Hi Martin.
Hey.
The patch is about basic hint support for -fdump-foo options where
one can newly get something like:
$ ./xgcc -B. /tmp/foo.c -fdump-ipa-ynline -c
cc1: error: unrecognized command-line option ‘-fdump-ipa-ynline’; did you mean
‘-fdump-ipa-inline’?
$ ./xgcc -B. /tmp/foo.c -fdump-tree-switchlowe -c
cc1: error: unrecognized command-line option ‘-fdump-tree-switchlowe’; did you
mean ‘-fdump-tree-switchlower1’?
$ ./xgcc -B. /tmp/foo.c -fdump-rtl-sched -c
cc1: error: unrecognized command-line option ‘-fdump-rtl-sched’; did you mean
‘-fdump-rtl-sched1’?
I also considered the same support for --completion option but it's more
problematic
as the --completion is handled in driver. In driver we do not instantiate
pass_manager
where we have listed all passes (and their corresponding dump options).
This seems like a reasonable restriction; instantiating passes doesn't
seem like something we should be doing from within the driver (what
about things like dumps for arch-specific passes?)
These should work.
Thinking aloud: would it make sense to move --completion to the
compiler to get around this?
It's possible, the only challenge is to find a proper FE (cc1, cc1plus, ...).
Suggested options may differ based on that.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed in next stage1?
Thanks,
Martin
gcc/ChangeLog:
2020-03-19 Martin Liska <mli...@suse.cz>
* dumpfile.c (dump_switch_p): Change return type
and print option suggestion.
Strictly speaking this is gcc::dump_manager::dump_switch_p.
:)
[...]
-int
+void
gcc::dump_manager::
dump_switch_p (const char *arg)
{
@@ -1896,8 +1897,22 @@ dump_switch_p (const char *arg)
for (i = 0; i < m_extra_dump_files_in_use; i++)
any |= dump_switch_p_1 (arg, &m_extra_dump_files[i], true);
-
- return any;
+ if (!any)
+ {
+ char *s;
+ auto_vec<const char *> candidates;
+ for (size_t i = TDI_none + 1; i != TDI_end; i++)
+ candidates.safe_push (dump_files[i].swtch);
+ for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
+ candidates.safe_push (m_extra_dump_files[i].swtch);
If I'm reading this right, this covers the simplest cases, but misses
various valid options.
You did indeed describe it as "basic hint support", so fair enough, I
suppose.
Exactly. To be honest, the dump options are mainly for developers and
I don't want to spend much time on it.
What about the glob and "-all" variants?
Enhancement candidate.
Also, parse_dump_option does various things here.
Should this be integrated into option_proposer in opt-suggestions.c?
But maybe that's overkill if we're not going to do completion.
+ const char *hint = candidates_list_and_hint (arg, s, candidates);
+ if (hint)
+ error ("unrecognized command-line option %<-fdump-%s%>; "
+ "did you mean %<-fdump-%s%>?", arg, hint);
+ else
+ error ("unrecognized command-line option %<-fdump-%s%>", arg);
+ XDELETEVEC (s);
+ }
}
candidates_list_and_hint builds a list of space-separated candidates
and writes it back to s (for showing to the user). The above code
ignores this, which is probably good given that everything has an
implicit prefix of "-fdump-" which wouldn't be in those listed strings.
I don't like the way the patch is building that string and then doing
nothing with it. At first I thought it would be cleaner to convert
candidates_list_and_hint's "char *&str" param to a "char **out_str",
and support it being NULL, in which case not to build the candidatate
list.
But even simpler would be to use spellcheck.h's:
extern const char *
find_closest_string (const char *target,
const auto_vec<const char *> *candidates);
to get the hint without building the candidate list.
Why not just do that?
Yes, it works for me.
A random other thought: the spellcheck code makes use of overall
lengths to decide if a candidate is a meaningful suggestion, and I
wonder if it's an issue that we're implicitly stripping off and then
re-adding that "-fdump-" prefix. Perhaps not an issue.
[...]
diff --git a/gcc/testsuite/gcc.dg/spellcheck-options-22.c
b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
new file mode 100644
index 00000000000..b0ddae2e78e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+/* { dg-options "-fdump-ipa-ynline" } */
+/* { dg-error "unrecognized command-line option '-fdump-ipa-ynline'; did you mean
'-fdump-ipa-inline'?" "" { target *-*-* } 0 } */
There are various ways in which gcc::dump_manager::dump_switch_p's
"any" can be set, so it would be good to have test coverage of each of
them - but I'm not sure how feasible that is.
What about the "-all" variants?
Maybe that's overkill though.
I'm sending the basic version with change to find_closest_string.
I hope even this version is an improvement.
Martin
Hope this is constructive
Dave
>From 7c1e5cad3fc4a8ea129b180d9aa3b86f1b584221 Mon Sep 17 00:00:00 2001
From: Martin Liska <mli...@suse.cz>
Date: Thu, 19 Mar 2020 11:58:53 +0100
Subject: [PATCH] Provide hint for misspelled -fdump-foo options.
gcc/ChangeLog:
2020-03-19 Martin Liska <mli...@suse.cz>
* dumpfile.c (dump_switch_p): Change return type
and print option suggestion.
* dumpfile.h: Change return type.
* opts-global.c (handle_common_deferred_options):
Move error into dump_switch_p function.
gcc/testsuite/ChangeLog:
2020-03-19 Martin Liska <mli...@suse.cz>
* gcc.dg/spellcheck-options-22.c: New test.
---
gcc/dumpfile.c | 19 ++++++++++++++++---
gcc/dumpfile.h | 2 +-
gcc/opts-global.c | 3 +--
gcc/testsuite/gcc.dg/spellcheck-options-22.c | 3 +++
4 files changed, 21 insertions(+), 6 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/spellcheck-options-22.c
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 468ffab6cce..7920b0028ac 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -39,6 +39,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-pass.h" /* for "current_pass". */
#include "optinfo-emit-json.h"
#include "stringpool.h" /* for get_identifier. */
+#include "spellcheck.h"
/* If non-NULL, return one past-the-end of the matching SUBPART of
the WHOLE string. */
@@ -1874,7 +1875,7 @@ dump_switch_p_1 (const char *arg, struct dump_file_info *dfi, bool doglob)
return 1;
}
-int
+void
gcc::dump_manager::
dump_switch_p (const char *arg)
{
@@ -1896,8 +1897,20 @@ dump_switch_p (const char *arg)
for (i = 0; i < m_extra_dump_files_in_use; i++)
any |= dump_switch_p_1 (arg, &m_extra_dump_files[i], true);
-
- return any;
+ if (!any)
+ {
+ auto_vec<const char *> candidates;
+ for (size_t i = TDI_none + 1; i != TDI_end; i++)
+ candidates.safe_push (dump_files[i].swtch);
+ for (size_t i = 0; i < m_extra_dump_files_in_use; i++)
+ candidates.safe_push (m_extra_dump_files[i].swtch);
+ const char *hint = find_closest_string (arg, &candidates);
+ if (hint)
+ error ("unrecognized command-line option %<-fdump-%s%>; "
+ "did you mean %<-fdump-%s%>?", arg, hint);
+ else
+ error ("unrecognized command-line option %<-fdump-%s%>", arg);
+ }
}
/* Parse ARG as a -fopt-info switch and store flags, optgroup_flags
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 840ae4d55d5..00e175a4737 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -691,7 +691,7 @@ public:
char *
get_dump_file_name (struct dump_file_info *dfi, int part = -1) const;
- int
+ void
dump_switch_p (const char *arg);
/* Start a dump for PHASE. Store user-supplied dump flags in
diff --git a/gcc/opts-global.c b/gcc/opts-global.c
index c658805470e..b1a8429dc3c 100644
--- a/gcc/opts-global.c
+++ b/gcc/opts-global.c
@@ -385,8 +385,7 @@ handle_common_deferred_options (void)
break;
case OPT_fdump_:
- if (!g->get_dumps ()->dump_switch_p (opt->arg))
- error ("unrecognized command-line option %<-fdump-%s%>", opt->arg);
+ g->get_dumps ()->dump_switch_p (opt->arg);
break;
case OPT_fopt_info_:
diff --git a/gcc/testsuite/gcc.dg/spellcheck-options-22.c b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
new file mode 100644
index 00000000000..b0ddae2e78e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/spellcheck-options-22.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+/* { dg-options "-fdump-ipa-ynline" } */
+/* { dg-error "unrecognized command-line option '-fdump-ipa-ynline'; did you mean '-fdump-ipa-inline'?" "" { target *-*-* } 0 } */
--
2.25.1
