[PATCH PR96230] driver: ICE in process_command, at gcc.c:5095

[Richard Biener]

On Thu, 23 Jul 2020, Alexandre Oliva wrote:

> On Jul 23, 2020, Richard Biener <rguenther@suse.de> wrote:
> 
> > On Thu, 23 Jul 2020, Zhanghaijian (A) wrote:
> >> # This option restores naming of aux and dump output files
> >> # after input files when multiple input files are named,
> >> # instead of getting them combined with the output name.
> 
> > Does it actually do what the above comment suggests?
> 
> It should.  When there are multiple inputs, it clearly does.  I guess
> it's just as clear I haven't tried it with a single input.
> 
> > I'd have naiively assumed that for gcc -S t.c -fdump-tree-optimized
> > -dumpbase "" I get a ".238t.optimized" file (thus empty dumpbase).
> 
> Uhh, I guess that would be a reasonable expectation, more so if combined
> with -dumpdir, but the documentation suggests -dumpbase "" is to restore
> pre-revamp behavior.  Should we change that?

No, after reading it it seems like a useful feature.

> Here's a patch that adds some more tests, fixes the misbehavior, and
> adds comments to conditions that led me down investigations that wasted
> a lot of time while looking into this problem and trying to make for
> more widespread changes.
> 
> Tested on x86_64-linux-gnu.  Ok to install?

OK.

Thanks,
Richard.

> 
> [PR96230] some -dumpbase-ext fixes
> 
> From: Alexandre Oliva <oliva@adacore.com>
> 
> The initial bug report was that compiling (-c) with -dumpbase ""
> -dumpbase-ext .<ext> crashes the driver.
> 
> The verification of -dumpbase-ext against -dumpbase doesn't cover the
> case in which -dumpbase activates backward-compatibility mode.
> 
> I added a test for that, and for -dumpbase-ext without -dumpbase,
> trying to make it work in a sensible way, as if applied to the default
> -dumpbase for each file.  It turned out that this made for too much
> complexity in dealing with suffixes derived from input filenames, so I
> gave that up and returned to discarding -dumpbase-ext as documented,
> ending up with a change identical to that in the original bug report.
> 
> I also thought I caught an off-by-one error in the initial
> verification, that caused dumpbase_ext to be discarded if it was
> identical to the specified dumpbase, but that turned out to be
> intentional as well, so I put in comments and a test to reflect it.
> 
> Finally, an earlier version of the newly-added tests used "$var.ext"
> in an expected output list, which showed me the handling of string
> expansion was incorrect.  Reworked the expr into an eval to make that
> work, and, absent any reliance on post-eval adjustments to so-expanded
> output names, I arranged for the adjustments to be skipped after eval.
> 
> 
> Co-Authored-By: "Zhanghaijian (A)" <z.zhanghaijian@huawei.com>
> for  gcc/ChangeLog
> 
> 	PR driver/96230
> 	* gcc.c (process_command): Adjust and document conditions to
> 	reset dumpbase_ext.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR driver/96230
> 	* gcc.misc-tests/outputs.exp: Add tests with -dumpbase-ext,
> 	with identical -dumpbase, with -dumpbase "", and without any
> 	-dumpbase.
> 	(outest): Fix "" expansion in expected outputs, skip
> 	adjustments.
> ---
>  gcc/gcc.c                                |   15 +++++++++++++--
>  gcc/testsuite/gcc.misc-tests/outputs.exp |   23 ++++++++++++++++++-----
>  2 files changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index c0eb3c1..10bc988 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -4907,6 +4907,9 @@ process_command (unsigned int decoded_options_count,
>        int lendb = strlen (dumpbase);
>        int lendbx = strlen (dumpbase_ext);
>  
> +      /* -dumpbase-ext must be a suffix proper; discard it if it
> +	  matches all of -dumpbase, as that would make for an empty
> +	  basename.  */
>        if (lendbx >= lendb
>  	  || strcmp (dumpbase + lendb - lendbx, dumpbase_ext) != 0)
>  	{
> @@ -5083,10 +5086,18 @@ process_command (unsigned int decoded_options_count,
>    /* Check that dumpbase_ext, if still present, still matches the end
>       of dumpbase, if present, and drop it otherwise.  We only retained
>       it above when dumpbase was absent to maybe use it to drop the
> -     extension from output_name before combining it with dumpdir.  */
> +     extension from output_name before combining it with dumpdir.  We
> +     won't deal with -dumpbase-ext when -dumpbase is not explicitly
> +     given, even if just to activate backward-compatible dumpbase:
> +     dropping it on the floor is correct, expected and documented
> +     behavior.  Attempting to deal with a -dumpbase-ext that might
> +     match the end of some input filename, or of the combination of
> +     the output basename with the suffix of the input filename,
> +     possible with an intermediate .gk extension for -fcompare-debug,
> +     is just calling for trouble.  */
>    if (dumpbase_ext)
>      {
> -      if (!dumpbase)
> +      if (!dumpbase || !*dumpbase)
>  	{
>  	  free (dumpbase_ext);
>  	  dumpbase_ext = NULL;
> diff --git a/gcc/testsuite/gcc.misc-tests/outputs.exp b/gcc/testsuite/gcc.misc-tests/outputs.exp
> index 0784a8e..1e3cd41 100644
> --- a/gcc/testsuite/gcc.misc-tests/outputs.exp
> +++ b/gcc/testsuite/gcc.misc-tests/outputs.exp
> @@ -137,11 +137,9 @@ proc outest { test sources opts dirs outputs } {
>  
>  	    if { [string match "\[\$\"\]" [string index $og 0]] } {
>  		global aout
> -		set og [expr $og]
> -		if { "$og" == "" } { continue }
> -	    }
> -
> -	    if { [string index $og 0] == "-" } then {
> +		eval set o $og
> +		if { "$o" == "" } { continue }
> +	    } elseif { [string index $og 0] == "-" } then {
>  		if { [string index $og 1] == "-" \
>  		     || [string index $og 1] == "." } then {
>  		    set o "$b-$b[string range $og 1 end]"
> @@ -723,6 +721,21 @@ outest "$b lto st mult namedb" $mult "-o dir/$b.exe -save-temps -O2 -flto -flto-
>  # !$skip_lto
>  }
>  
> +# PR96230 - -dumpbase "" with -dumpbase-ext, not linking
> +outest "$b single  -c  -o -db'' -dbx.c" $sing "-c -o dir/$b.o -dumpbase \"\" -dumpbase-ext .c -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-0.c.???r.final -0.su !!$gspd -0.dwo !0 .o} {}}
> +outest "$b mult  -c  -dd -db'' -dbx.c" $mult "-c -dumpdir dir/ -dumpbase \"\" -dumpbase-ext .c -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-1.c.???r.final -1.su !!$gspd -1.dwo !0 -2.c.???r.final -2.su !!$gspd -2.dwo} {-1.o -2.o}}
> +outest "$b single  -c  -o -db'' -dbx.x" $sing "-c -o dir/$b.o -dumpbase \"\" -dumpbase-ext .x -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-0.c.???r.final -0.su !!$gspd -0.dwo !0 .o} {}}
> +outest "$b mult  -c  -dd -db'' -dbx.x" $mult "-c -dumpdir dir/ -dumpbase \"\" -dumpbase-ext .x -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-1.c.???r.final -1.su !!$gspd -1.dwo !0 -2.c.???r.final -2.su !!$gspd -2.dwo} {-1.o -2.o}}
> +# Test -dumpbase-ext without an explicit -dumpbase too.
> +outest "$b single  -c  -o -dbx.c" $sing "-c -o dir/$b.o -dumpbase-ext .c -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{.c.???r.final .su !!$gspd .dwo !0 .o} {}}
> +outest "$b mult  -c  -dd -dbx.c" $mult "-c -dumpdir dir/ -dumpbase-ext .c -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-1.c.???r.final -1.su !!$gspd -1.dwo !0 -2.c.???r.final -2.su !!$gspd -2.dwo} {-1.o -2.o}}
> +outest "$b single  -c  -o -dbx.x" $sing "-c -o dir/$b.o -dumpbase-ext .x -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{.c.???r.final .su !!$gspd .dwo !0 .o} {}}
> +outest "$b mult  -c  -dd -dbx.x" $mult "-c -dumpdir dir/ -dumpbase-ext .x -fdump-rtl-final -fstack-usage $gsplit_dwarf" {dir/} {{-1.c.???r.final -1.su !!$gspd -1.dwo !0 -2.c.???r.final -2.su !!$gspd -2.dwo} {-1.o -2.o}}
> +outest "$b obj compare-debug save-temps -dbx.x" $sing "-c -fcompare-debug -save-temps -fdump-rtl-final -fstack-usage $gsplit_dwarf -fdump-final-insns -dumpbase-ext .x" {} {{-0.c.???r.final -0.su -0.i -0.c.gkd -0.s -0.gk.i -0.gk.c.???r.final -0.gk.c.gkd !!$gspd -0.dwo !0 -0.o}}
> +# -dumpbase-ext is dropped if identical to -dumpbase.
> +outest "$b asm db=dbext 1" $sing "-S -fdump-rtl-final -fstack-usage $gsplit_dwarf -dumpbase a -dumpbase-ext a" {} {{a.???r.final a.su -0.s}}
> +
> +
>  # Below are examples taken from the documentation.
>  # They are likely redundant with earlier test,
>  # but we want to make sure behavior matches the docs.
> 
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)