diff mbox series

[stage1] Provide hint for misspelled -fdump-foo options.

Message ID 9861dde7-95e3-9f8c-e170-b5764eff3530@suse.cz
State New
Headers show
Series [stage1] Provide hint for misspelled -fdump-foo options. | expand

Commit Message

Martin Liška March 19, 2020, 1:52 p.m. UTC
Hi.

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).

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  <mliska@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  <mliska@suse.cz>

	* gcc.dg/spellcheck-options-22.c: New test.
---
  gcc/dumpfile.c                               | 21 +++++++++++++++++---
  gcc/dumpfile.h                               |  2 +-
  gcc/opts-global.c                            |  3 +--
  gcc/testsuite/gcc.dg/spellcheck-options-22.c |  3 +++
  4 files changed, 23 insertions(+), 6 deletions(-)
  create mode 100644 gcc/testsuite/gcc.dg/spellcheck-options-22.c

Comments

Jeff Law via Gcc-patches March 19, 2020, 9:26 p.m. UTC | #1
On Thu, 2020-03-19 at 14:52 +0100, Martin Liška wrote:

> Hi.

Hi Martin.

> 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?)

Thinking aloud: would it make sense to move --completion to the
compiler to get around this?

> 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  <mliska@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.

What about the glob and "-all" variants?
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?

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.


Hope this is constructive
Dave
Martin Liška March 20, 2020, 4:03 p.m. UTC | #2
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  <mliska@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
>
Jeff Law via Gcc-patches March 20, 2020, 4:15 p.m. UTC | #3
On Fri, 2020-03-20 at 17:03 +0100, Martin Liška wrote:
> On 3/19/20 10:26 PM, David Malcolm wrote:
> > On Thu, 2020-03-19 at 14:52 +0100, Martin Liška wrote:

[...]

> > 
> > > -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.

Fair enough.

[...]


> I'm sending the basic version with change to find_closest_string.
> I hope even this version is an improvement.
> 
> Martin

Thanks.  Looks good to me for stage 1.

Dave
diff mbox series

Patch

diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 468ffab6cce..e820f72edc3 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 "opts.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,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);
+      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);
+    }
 }
 
 /* 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 } */