Do not abort compilation when dump file is /dev/*

  The `configure` scripts generated with autoconf often tests compiler
features by setting output to `/dev/null`, which then sets the dump
folder as being /dev/* and the compilation halts with an error because
GCC cannot create files in /dev/. This is a problem when configure is
testing for compiler features because it cannot tell if the failure was
due to unsupported features or any other problem, and disable it even
if it is working.

As an example, running configure overriding CFLAGS="-fdump-ipa-clones"
will result in several compiler-features as being disabled because of
gcc halting with an error creating files in /dev/*.

This commit fixes this issue by checking if the dump folder is /dev/.
If yes, then it just informs the user and disables dumping, but does
not halt the compilation and the compiler retuns 0 to the shell.

gcc/ChangeLog
2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>

	* dumpfile.c (dump_open): Do not halt compilation when file
	matches /dev/*.

gcc/testsuite/ChangeLog
2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>

	* gcc.dg/devnull-dump.c: New.

Signed-off-by: Giuliano Belinassi <gbelinassi@suse.de>
---
 gcc/dumpfile.c                      | 17 ++++++++++++++++-
 gcc/testsuite/gcc.dg/devnull-dump.c |  7 +++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/devnull-dump.c
  

On Tue, 16 Nov 2021, Giuliano Belinassi wrote:

> The `configure` scripts generated with autoconf often tests compiler
> features by setting output to `/dev/null`, which then sets the dump
> folder as being /dev/* and the compilation halts with an error because
> GCC cannot create files in /dev/. This is a problem when configure is
> testing for compiler features because it cannot tell if the failure was
> due to unsupported features or any other problem, and disable it even
> if it is working.
> 
> As an example, running configure overriding CFLAGS="-fdump-ipa-clones"
> will result in several compiler-features as being disabled because of
> gcc halting with an error creating files in /dev/*.
> 
> This commit fixes this issue by checking if the dump folder is /dev/.
> If yes, then it just informs the user and disables dumping, but does
> not halt the compilation and the compiler retuns 0 to the shell.

I think matching '/dev/*' is broken.  Either failure to open a dump
file should be an error or not.  Btw, some configure tests check
for compiler output as well which still breaks in this case.

IMHO a more reasonable thing to do would be to not treat
-o /dev/null as a source for -dumpdir and friends.  Alex?
You did the last re-org, where'd we put such special casing?

Of course configure checks using -fdump-... with -o /dev/null are
a bit fragile.  They could use -fdump-...=FILENAME

Thnaks,
Richard.

> gcc/ChangeLog
> 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> 
> 	* dumpfile.c (dump_open): Do not halt compilation when file
> 	matches /dev/*.
> 
> gcc/testsuite/ChangeLog
> 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> 
> 	* gcc.dg/devnull-dump.c: New.
> 
> Signed-off-by: Giuliano Belinassi <gbelinassi@suse.de>
> ---
>  gcc/dumpfile.c                      | 17 ++++++++++++++++-
>  gcc/testsuite/gcc.dg/devnull-dump.c |  7 +++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/devnull-dump.c
> 
> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index 8169daf7f59..b1dbfb371af 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -378,7 +378,22 @@ dump_open (const char *filename, bool trunc)
>    FILE *stream = fopen (filename, trunc ? "w" : "a");
>  
>    if (!stream)
> -    error ("could not open dump file %qs: %m", filename);
> +    {
> +      /* Autoconf tests compiler functionalities by setting output to /dev/null.
> +	 In this case, if dumps are enabled, it will try to set the output
> +	 folder to /dev/*, which is of course invalid and the compiler will exit
> +	 with an error, resulting in configure script reporting the tested
> +	 feature as being unavailable. Here we test this case by checking if the
> +	 output file prefix has /dev/ and only inform the user in this case
> +	 rather than refusing to compile.  */
> +
> +      const char *const slash_dev = "/dev/";
> +      if (strncmp(slash_dev, filename, strlen(slash_dev)) == 0)
> +	inform (UNKNOWN_LOCATION,
> +		"could not open dump file %qs: %m. Dumps are disabled.", filename);
> +      else
> +	error ("could not open dump file %qs: %m", filename);
> +    }
>    return stream;
>  }
>  
> diff --git a/gcc/testsuite/gcc.dg/devnull-dump.c b/gcc/testsuite/gcc.dg/devnull-dump.c
> new file mode 100644
> index 00000000000..378e0901c28
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/devnull-dump.c
> @@ -0,0 +1,7 @@
> +/* { dg-do assemble } */
> +/* { dg-options "-fdump-ipa-clones -o /dev/null" } */
> +
> +int main()
> +{
> +  return 0;
> +}
>
  

On 11/16/21 17:52, Giuliano Belinassi via Gcc-patches wrote:
> |+ if (strncmp(slash_dev, filename, strlen(slash_dev)) == 0)|

Btw. you can use startswith function.

Cheers,
Martin
  

On Thu, 2021-11-18 at 10:43 +0100, Richard Biener wrote:
> On Tue, 16 Nov 2021, Giuliano Belinassi wrote:
> 
> > The `configure` scripts generated with autoconf often tests
> > compiler
> > features by setting output to `/dev/null`, which then sets the dump
> > folder as being /dev/* and the compilation halts with an error
> > because
> > GCC cannot create files in /dev/. This is a problem when configure
> > is
> > testing for compiler features because it cannot tell if the failure
> > was
> > due to unsupported features or any other problem, and disable it
> > even
> > if it is working.
> > 
> > As an example, running configure overriding CFLAGS="-fdump-ipa-
> > clones"
> > will result in several compiler-features as being disabled because
> > of
> > gcc halting with an error creating files in /dev/*.
> > 
> > This commit fixes this issue by checking if the dump folder is
> > /dev/.
> > If yes, then it just informs the user and disables dumping, but
> > does
> > not halt the compilation and the compiler retuns 0 to the shell.
> 
> I think matching '/dev/*' is broken.  Either failure to open a dump
> file should be an error or not.  Btw, some configure tests check
> for compiler output as well which still breaks in this case.

Hi,

IMHO gcc shouldn't fail when it cannot write the dump files because
compilation could still continue compiling even if the dumps cannot be
created. However, I chose to do not because the code may be issuing an
error for other reasons, and matching for /dev/ fixes the test case. 

> 
> IMHO a more reasonable thing to do would be to not treat
> -o /dev/null as a source for -dumpdir and friends.  Alex?
> You did the last re-org, where'd we put such special casing?
> 
> Of course configure checks using -fdump-... with -o /dev/null are
> a bit fragile.  They could use -fdump-...=FILENAME
> 

But that would also set all clones output as being equal to FILENAME
and don't create one dump for every compiled file, no?

Thanks,
Giuliano.

> Thnaks,
> Richard.
> 
> > gcc/ChangeLog
> > 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> > 
> >         * dumpfile.c (dump_open): Do not halt compilation when file
> >         matches /dev/*.
> > 
> > gcc/testsuite/ChangeLog
> > 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> > 
> >         * gcc.dg/devnull-dump.c: New.
> > 
> > Signed-off-by: Giuliano Belinassi <gbelinassi@suse.de>
> > ---
> >  gcc/dumpfile.c                      | 17 ++++++++++++++++-
> >  gcc/testsuite/gcc.dg/devnull-dump.c |  7 +++++++
> >  2 files changed, 23 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/devnull-dump.c
> > 
> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> > index 8169daf7f59..b1dbfb371af 100644
> > --- a/gcc/dumpfile.c
> > +++ b/gcc/dumpfile.c
> > @@ -378,7 +378,22 @@ dump_open (const char *filename, bool trunc)
> >    FILE *stream = fopen (filename, trunc ? "w" : "a");
> >  
> >    if (!stream)
> > -    error ("could not open dump file %qs: %m", filename);
> > +    {
> > +      /* Autoconf tests compiler functionalities by setting output
> > to /dev/null.
> > +        In this case, if dumps are enabled, it will try to set the
> > output
> > +        folder to /dev/*, which is of course invalid and the
> > compiler will exit
> > +        with an error, resulting in configure script reporting the
> > tested
> > +        feature as being unavailable. Here we test this case by
> > checking if the
> > +        output file prefix has /dev/ and only inform the user in
> > this case
> > +        rather than refusing to compile.  */
> > +
> > +      const char *const slash_dev = "/dev/";
> > +      if (strncmp(slash_dev, filename, strlen(slash_dev)) == 0)
> > +       inform (UNKNOWN_LOCATION,
> > +               "could not open dump file %qs: %m. Dumps are
> > disabled.", filename);
> > +      else
> > +       error ("could not open dump file %qs: %m", filename);
> > +    }
> >    return stream;
> >  }
> >  
> > diff --git a/gcc/testsuite/gcc.dg/devnull-dump.c
> > b/gcc/testsuite/gcc.dg/devnull-dump.c
> > new file mode 100644
> > index 00000000000..378e0901c28
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/devnull-dump.c
> > @@ -0,0 +1,7 @@
> > +/* { dg-do assemble } */
> > +/* { dg-options "-fdump-ipa-clones -o /dev/null" } */
> > +
> > +int main()
> > +{
> > +  return 0;
> > +}
> > 
>
  

On Thu, 18 Nov 2021, Giuliano Belinassi wrote:

> On Thu, 2021-11-18 at 10:43 +0100, Richard Biener wrote:
> > On Tue, 16 Nov 2021, Giuliano Belinassi wrote:
> > 
> > > The `configure` scripts generated with autoconf often tests
> > > compiler
> > > features by setting output to `/dev/null`, which then sets the dump
> > > folder as being /dev/* and the compilation halts with an error
> > > because
> > > GCC cannot create files in /dev/. This is a problem when configure
> > > is
> > > testing for compiler features because it cannot tell if the failure
> > > was
> > > due to unsupported features or any other problem, and disable it
> > > even
> > > if it is working.
> > > 
> > > As an example, running configure overriding CFLAGS="-fdump-ipa-
> > > clones"
> > > will result in several compiler-features as being disabled because
> > > of
> > > gcc halting with an error creating files in /dev/*.
> > > 
> > > This commit fixes this issue by checking if the dump folder is
> > > /dev/.
> > > If yes, then it just informs the user and disables dumping, but
> > > does
> > > not halt the compilation and the compiler retuns 0 to the shell.
> > 
> > I think matching '/dev/*' is broken.  Either failure to open a dump
> > file should be an error or not.  Btw, some configure tests check
> > for compiler output as well which still breaks in this case.
> 
> Hi,
> 
> IMHO gcc shouldn't fail when it cannot write the dump files because
> compilation could still continue compiling even if the dumps cannot be
> created. However, I chose to do not because the code may be issuing an
> error for other reasons, and matching for /dev/ fixes the test case. 
> 
> > 
> > IMHO a more reasonable thing to do would be to not treat
> > -o /dev/null as a source for -dumpdir and friends.  Alex?
> > You did the last re-org, where'd we put such special casing?
> > 
> > Of course configure checks using -fdump-... with -o /dev/null are
> > a bit fragile.  They could use -fdump-...=FILENAME
> > 
> 
> But that would also set all clones output as being equal to FILENAME
> and don't create one dump for every compiled file, no?

Yes.  But then whoever blindly adds -fdump- to CFLAGS over the
run of configure is really to blame.  Yes, I agree - "silent"
failure is bad, but that's also the fault of configure since it
cannot distinguish between real and accidential fails when you
supply wrong CFLAGS.

So I'm not sure if -fdump- should be special cased.  But for
sure -o /dev/null might not be a good source for our heuristic
way to determine the aux file prefix and directory.

Richard.

> Thanks,
> Giuliano.
> 
> > Thnaks,
> > Richard.
> > 
> > > gcc/ChangeLog
> > > 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> > > 
> > >         * dumpfile.c (dump_open): Do not halt compilation when file
> > >         matches /dev/*.
> > > 
> > > gcc/testsuite/ChangeLog
> > > 2021-11-16  Giuliano Belinassi  <gbelinassi@suse.de>
> > > 
> > >         * gcc.dg/devnull-dump.c: New.
> > > 
> > > Signed-off-by: Giuliano Belinassi <gbelinassi@suse.de>
> > > ---
> > >  gcc/dumpfile.c                      | 17 ++++++++++++++++-
> > >  gcc/testsuite/gcc.dg/devnull-dump.c |  7 +++++++
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > >  create mode 100644 gcc/testsuite/gcc.dg/devnull-dump.c
> > > 
> > > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> > > index 8169daf7f59..b1dbfb371af 100644
> > > --- a/gcc/dumpfile.c
> > > +++ b/gcc/dumpfile.c
> > > @@ -378,7 +378,22 @@ dump_open (const char *filename, bool trunc)
> > >    FILE *stream = fopen (filename, trunc ? "w" : "a");
> > >  
> > >    if (!stream)
> > > -    error ("could not open dump file %qs: %m", filename);
> > > +    {
> > > +      /* Autoconf tests compiler functionalities by setting output
> > > to /dev/null.
> > > +        In this case, if dumps are enabled, it will try to set the
> > > output
> > > +        folder to /dev/*, which is of course invalid and the
> > > compiler will exit
> > > +        with an error, resulting in configure script reporting the
> > > tested
> > > +        feature as being unavailable. Here we test this case by
> > > checking if the
> > > +        output file prefix has /dev/ and only inform the user in
> > > this case
> > > +        rather than refusing to compile.  */
> > > +
> > > +      const char *const slash_dev = "/dev/";
> > > +      if (strncmp(slash_dev, filename, strlen(slash_dev)) == 0)
> > > +       inform (UNKNOWN_LOCATION,
> > > +               "could not open dump file %qs: %m. Dumps are
> > > disabled.", filename);
> > > +      else
> > > +       error ("could not open dump file %qs: %m", filename);
> > > +    }
> > >    return stream;
> > >  }
> > >  
> > > diff --git a/gcc/testsuite/gcc.dg/devnull-dump.c
> > > b/gcc/testsuite/gcc.dg/devnull-dump.c
> > > new file mode 100644
> > > index 00000000000..378e0901c28
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/devnull-dump.c
> > > @@ -0,0 +1,7 @@
> > > +/* { dg-do assemble } */
> > > +/* { dg-options "-fdump-ipa-clones -o /dev/null" } */
> > > +
> > > +int main()
> > > +{
> > > +  return 0;
> > > +}
> > > 
> > 
> 
>
  

On Nov 18, 2021, Richard Biener <rguenther@suse.de> wrote:

> IMHO a more reasonable thing to do would be to not treat
> -o /dev/null as a source for -dumpdir and friends.  Alex?

+1

I think we already have some special-casing for /dev/null somewhere.

> You did the last re-org, where'd we put such special casing?

I think we're missing something like this, to avoid messing with dumpdir
with -o /dev/null.  We already use the same function when computing
outbase just below this.

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 506c2acc282d6..a986728fb91d6 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -5098,7 +5098,8 @@ process_command (unsigned int decoded_options_count,
 
   bool explicit_dumpdir = dumpdir;
 
-  if (!save_temps_overrides_dumpdir && explicit_dumpdir)
+  if ((!save_temps_overrides_dumpdir && explicit_dumpdir)
+      || (output_file != NULL && not_actual_file_p (output_file)))
     {
       /* Do nothing.  */
     }
  

On Fri, 19 Nov 2021, Alexandre Oliva wrote:

> On Nov 18, 2021, Richard Biener <rguenther@suse.de> wrote:
> 
> > IMHO a more reasonable thing to do would be to not treat
> > -o /dev/null as a source for -dumpdir and friends.  Alex?
> 
> +1
> 
> I think we already have some special-casing for /dev/null somewhere.

Grepping finds me the following in system.h which is already checked
for in gcc.c in a few places indeed.

/* Provide a default for the HOST_BIT_BUCKET.
   This suffices for POSIX-like hosts.  */

#ifndef HOST_BIT_BUCKET
#define HOST_BIT_BUCKET "/dev/null"
#endif


> > You did the last re-org, where'd we put such special casing?
> 
> I think we're missing something like this, to avoid messing with dumpdir
> with -o /dev/null.  We already use the same function when computing
> outbase just below this.

Ah yeah, not_actual_file_p should do the trick indeed.  Giuliano, can
you update the patch like below?  I think we should still adjust
documentation as you did.

> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 506c2acc282d6..a986728fb91d6 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -5098,7 +5098,8 @@ process_command (unsigned int decoded_options_count,
>  
>    bool explicit_dumpdir = dumpdir;
>  
> -  if (!save_temps_overrides_dumpdir && explicit_dumpdir)
> +  if ((!save_temps_overrides_dumpdir && explicit_dumpdir)
> +      || (output_file != NULL && not_actual_file_p (output_file)))
>      {
>        /* Do nothing.  */
>      }
>
  

On Fri, 19 Nov 2021 10:35:26 +0100 (CET)
Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> On Fri, 19 Nov 2021, Alexandre Oliva wrote:
> 
> > On Nov 18, 2021, Richard Biener <rguenther@suse.de> wrote:
> >   
> > > IMHO a more reasonable thing to do would be to not treat
> > > -o /dev/null as a source for -dumpdir and friends.  Alex?  
> > 
> > +1
> > 
> > I think we already have some special-casing for /dev/null somewhere.  
> 
> Grepping finds me the following in system.h which is already checked
> for in gcc.c in a few places indeed.
> 
> /* Provide a default for the HOST_BIT_BUCKET.
>    This suffices for POSIX-like hosts.  */
> 
> #ifndef HOST_BIT_BUCKET
> #define HOST_BIT_BUCKET "/dev/null"
> #endif
> 
> 
> > > You did the last re-org, where'd we put such special casing?  
> > 
> > I think we're missing something like this, to avoid messing with dumpdir
> > with -o /dev/null.  We already use the same function when computing
> > outbase just below this.  
> 
> Ah yeah, not_actual_file_p should do the trick indeed.  Giuliano, can
> you update the patch like below?  I think we should still adjust
> documentation as you did.

But that wouldn't cater for the general problem that the dumpdir is not
writable, no? Why not just simply check access W_OK of the dumpdir?

Otherwise a dumpdir /dev/full or anyother such path will cause the same
thing i guess.

thanks,
> 
> > diff --git a/gcc/gcc.c b/gcc/gcc.c
> > index 506c2acc282d6..a986728fb91d6 100644
> > --- a/gcc/gcc.c
> > +++ b/gcc/gcc.c
> > @@ -5098,7 +5098,8 @@ process_command (unsigned int decoded_options_count,
> >  
> >    bool explicit_dumpdir = dumpdir;
> >  
> > -  if (!save_temps_overrides_dumpdir && explicit_dumpdir)
> > +  if ((!save_temps_overrides_dumpdir && explicit_dumpdir)
> > +      || (output_file != NULL && not_actual_file_p (output_file)))
> >      {
> >        /* Do nothing.  */
> >      }
> >
  

On Fri, 19 Nov 2021, Bernhard Reutner-Fischer wrote:

> On Fri, 19 Nov 2021 10:35:26 +0100 (CET)
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> > On Fri, 19 Nov 2021, Alexandre Oliva wrote:
> > 
> > > On Nov 18, 2021, Richard Biener <rguenther@suse.de> wrote:
> > >   
> > > > IMHO a more reasonable thing to do would be to not treat
> > > > -o /dev/null as a source for -dumpdir and friends.  Alex?  
> > > 
> > > +1
> > > 
> > > I think we already have some special-casing for /dev/null somewhere.  
> > 
> > Grepping finds me the following in system.h which is already checked
> > for in gcc.c in a few places indeed.
> > 
> > /* Provide a default for the HOST_BIT_BUCKET.
> >    This suffices for POSIX-like hosts.  */
> > 
> > #ifndef HOST_BIT_BUCKET
> > #define HOST_BIT_BUCKET "/dev/null"
> > #endif
> > 
> > 
> > > > You did the last re-org, where'd we put such special casing?  
> > > 
> > > I think we're missing something like this, to avoid messing with dumpdir
> > > with -o /dev/null.  We already use the same function when computing
> > > outbase just below this.  
> > 
> > Ah yeah, not_actual_file_p should do the trick indeed.  Giuliano, can
> > you update the patch like below?  I think we should still adjust
> > documentation as you did.
> 
> But that wouldn't cater for the general problem that the dumpdir is not
> writable, no? Why not just simply check access W_OK of the dumpdir?
> 
> Otherwise a dumpdir /dev/full or anyother such path will cause the same
> thing i guess.

I think those cases are OK to diagnose.  Just choosing a
not_actual_file_p output as base to derive the dump directory is
bad.

Richard.