PR52665 do not let .ident confuse assembler scan tests
diff mbox

Message ID 1466278273-7014-1-git-send-email-rep.dot.nop@gmail.com
State New
Headers show

Commit Message

Bernhard Reutner-Fischer June 18, 2016, 7:31 p.m. UTC
A branch with a name matching scan-assembler pattern triggers
inappropriate FAIL.

E.g. branch fixups-testsuite and
- gcc.target/i386/pr65871-?.c (scan-assembler-not "test")
- gcc.target/i386/pr41442.c (scan-assembler-times "test|cmp" 2)
etc.

This is a recurring problem as can be seen by some -fno-ident additions
by commits from e.g. Michael Meissner over the years: builtins-58.c,
powerpc/pr46728-?.c

The patch below adds -fno-ident if a testcase contains one of
scan-assembler, scan-assembler-not or scan-assembler-times.

Regression tested on x86_64-unknown-linux on a fixups-testsuite branch
where it fixes several false FAILs without regressions.

gcc/testsuite/ChangeLog

2016-06-18  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>

	PR testsuite/52665
	* lib/gcc-dg.exp (gcc-dg-test-1): Iterate over _required_options.
	* lib/target-supports.exp (scan-assembler_required_options,
	scan-assembler-not_required_options,
	scan-assembler-times_required_options): Add -fno-ident.
	* lib/scanasm.exp (scan-assembler-times): Fix error message.
	* c-c++-common/ident-0a.c: New test.
	* c-c++-common/ident-0b.c: New test.
	* c-c++-common/ident-1a.c: New test.
	* c-c++-common/ident-1b.c: New test.
	* c-c++-common/ident-2a.c: New test.
	* c-c++-common/ident-2b.c: New test.

Ok for trunk?

PS: proc force_conventional_output_for would be a bit misnomed by this,
not sure if it should be renamed to maybe set_required_options_for or
the like?

Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
Cc: Mike Stump <mikestump@comcast.net>
Cc: Rainer Orth <ro@CeBiTec.Uni-Bielefeld.DE>
Signed-off-by: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>
---
 gcc/testsuite/c-c++-common/ident-0a.c |  6 ++++++
 gcc/testsuite/c-c++-common/ident-0b.c | 10 ++++++++++
 gcc/testsuite/c-c++-common/ident-1a.c |  8 ++++++++
 gcc/testsuite/c-c++-common/ident-1b.c |  7 +++++++
 gcc/testsuite/c-c++-common/ident-2a.c |  6 ++++++
 gcc/testsuite/c-c++-common/ident-2b.c |  7 +++++++
 gcc/testsuite/lib/gcc-dg.exp          |  8 +++++---
 gcc/testsuite/lib/scanasm.exp         |  4 ++--
 gcc/testsuite/lib/target-supports.exp |  6 ++++++
 9 files changed, 57 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/ident-0a.c
 create mode 100644 gcc/testsuite/c-c++-common/ident-0b.c
 create mode 100644 gcc/testsuite/c-c++-common/ident-1a.c
 create mode 100644 gcc/testsuite/c-c++-common/ident-1b.c
 create mode 100644 gcc/testsuite/c-c++-common/ident-2a.c
 create mode 100644 gcc/testsuite/c-c++-common/ident-2b.c

Comments

Hans-Peter Nilsson June 18, 2016, 11:49 p.m. UTC | #1
On Sat, 18 Jun 2016, Bernhard Reutner-Fischer wrote:
> A branch with a name matching scan-assembler pattern triggers
> inappropriate FAIL.
>
> E.g. branch fixups-testsuite and
> - gcc.target/i386/pr65871-?.c (scan-assembler-not "test")
> - gcc.target/i386/pr41442.c (scan-assembler-times "test|cmp" 2)
> etc.
>
> This is a recurring problem as can be seen by some -fno-ident additions
> by commits from e.g. Michael Meissner over the years: builtins-58.c,
> powerpc/pr46728-?.c
>
> The patch below adds -fno-ident if a testcase contains one of
> scan-assembler, scan-assembler-not or scan-assembler-times.

This reminds me of a related but not opposing idea that I have
never acted on (besides bouncing it off a global maintainer
quite some time ago, with a positive response), to make common
identifier-like and filename-like strings invalid for use in
scan-assembler; there'd have to be a meta-character such as
whitespace.  It would help for "test" but admittedly not for
"test|cmp" given e.g. an objdir named "/tmp/mytest/gccmp/nop1".

Then there'd be an error thrown for such test-cases, alerting
the author (well, hopefully) that the test-case is in error, and
a comment in the scan-assembler code would make it prominent
that there'd have to be a whitespace or meta-like character
(i.e. [^-a-zA-Z_\.]) included in the string.  Yes, I know
whitespace can be included in filenames and that's regularly
done at least on some systems, but I bet you really get into
problems trying that for srcdir or objdir in a gcc build.

Sorry, still not going to act on it for now, but feel free if so
inclined.

brgds, H-P
Mike Stump June 19, 2016, 8:21 p.m. UTC | #2
On Jun 18, 2016, at 12:31 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> 
> A branch with a name matching scan-assembler pattern triggers
> inappropriate FAIL.

> The patch below adds -fno-ident if a testcase contains one of
> scan-assembler, scan-assembler-not or scan-assembler-times.

Kinda gross.  I'd like to consensus build a little, as I don't know that I have a better solution than the solution you propose to fix the issue.  I'd love it if one or more of Jacob, Law and Richard can chime in on direction here.  I'll have to think about this some more and see if I can come up with something that I like better.

If no one has a better solution, I'll approve the proposed solution.  Let's give people a little time to chime in.
Jeff Law June 20, 2016, 10:19 p.m. UTC | #3
On 06/18/2016 01:31 PM, Bernhard Reutner-Fischer wrote:
> A branch with a name matching scan-assembler pattern triggers
> inappropriate FAIL.
>
> E.g. branch fixups-testsuite and
> - gcc.target/i386/pr65871-?.c (scan-assembler-not "test")
> - gcc.target/i386/pr41442.c (scan-assembler-times "test|cmp" 2)
> etc.
>
> This is a recurring problem as can be seen by some -fno-ident additions
> by commits from e.g. Michael Meissner over the years: builtins-58.c,
> powerpc/pr46728-?.c
>
> The patch below adds -fno-ident if a testcase contains one of
> scan-assembler, scan-assembler-not or scan-assembler-times.
>
> Regression tested on x86_64-unknown-linux on a fixups-testsuite branch
> where it fixes several false FAILs without regressions.
>
> gcc/testsuite/ChangeLog
>
> 2016-06-18  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
>
> 	PR testsuite/52665
> 	* lib/gcc-dg.exp (gcc-dg-test-1): Iterate over _required_options.
> 	* lib/target-supports.exp (scan-assembler_required_options,
> 	scan-assembler-not_required_options,
> 	scan-assembler-times_required_options): Add -fno-ident.
> 	* lib/scanasm.exp (scan-assembler-times): Fix error message.
> 	* c-c++-common/ident-0a.c: New test.
> 	* c-c++-common/ident-0b.c: New test.
> 	* c-c++-common/ident-1a.c: New test.
> 	* c-c++-common/ident-1b.c: New test.
> 	* c-c++-common/ident-2a.c: New test.
> 	* c-c++-common/ident-2b.c: New test.
>
> Ok for trunk?
>
> PS: proc force_conventional_output_for would be a bit misnomed by this,
> not sure if it should be renamed to maybe set_required_options_for or
> the like?
OK.

Changing force_conventional_output to set_required_options_for is 
pre-approved as well.

jeff
Bernhard Reutner-Fischer Feb. 2, 2018, 1:25 p.m. UTC | #4
On 19 June 2016 at 22:21, Mike Stump <mikestump@comcast.net> wrote:
> On Jun 18, 2016, at 12:31 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>>
>> A branch with a name matching scan-assembler pattern triggers
>> inappropriate FAIL.
>
>> The patch below adds -fno-ident if a testcase contains one of
>> scan-assembler, scan-assembler-not or scan-assembler-times.
>
> Kinda gross.  I'd like to consensus build a little, as I don't know that I have a better solution than the solution you propose to fix the issue.  I'd love it if one or more of Jacob, Law and Richard can chime in on direction here.  I'll have to think about this some more and see if I can come up with something that I like better.
>
> If no one has a better solution, I'll approve the proposed solution.  Let's give people a little time to chime in.

Given the overwhelming silence this proposal has received, i take it
for granted that folks are thrilled and even up until now speechless
:)

So how should we proceed with -fno-ident.
And, btw, -fno-file to inhibit .file directives for the same reason,
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01785.html and all our
ugly filenames like gcc.target/powerpc/swps-p8-36.c which strive to
workaround the assembler-scans.

All those non-automatic hacks like naming swap() tests swps-,
uglifying scan-patterns (which are not terribly straight forward to
read anyway even without uglifying them) are error prone and costly.
IMHO we should make sure time is spent for useful stuff and not be
wasted to fight our very own testsuite.

-fno-ident ok for stage1?
What about -fno-file? Clever alternative suggestions? Don't care?

thanks,
Mike Stump Feb. 2, 2018, 6:56 p.m. UTC | #5
On Feb 2, 2018, at 5:25 AM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
> 
> Given the overwhelming silence this proposal has received, i take it
> for granted that folks are thrilled and even up until now speechless
> :)

> -fno-ident ok for stage1?
> What about -fno-file? Clever alternative suggestions? Don't care?

Sure.  We've had the bake time on the idea, no better solution has been proposed.  I think _solving_ the problem is a good thing, and that solution seems reasonable.
Jeff Law May 1, 2018, 5:47 p.m. UTC | #6
On 02/02/2018 06:25 AM, Bernhard Reutner-Fischer wrote:
> On 19 June 2016 at 22:21, Mike Stump <mikestump@comcast.net> wrote:
>> On Jun 18, 2016, at 12:31 PM, Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> wrote:
>>>
>>> A branch with a name matching scan-assembler pattern triggers
>>> inappropriate FAIL.
>>
>>> The patch below adds -fno-ident if a testcase contains one of
>>> scan-assembler, scan-assembler-not or scan-assembler-times.
>>
>> Kinda gross.  I'd like to consensus build a little, as I don't know that I have a better solution than the solution you propose to fix the issue.  I'd love it if one or more of Jacob, Law and Richard can chime in on direction here.  I'll have to think about this some more and see if I can come up with something that I like better.
>>
>> If no one has a better solution, I'll approve the proposed solution.  Let's give people a little time to chime in.
> 
> Given the overwhelming silence this proposal has received, i take it
> for granted that folks are thrilled and even up until now speechless
> :)
> 
> So how should we proceed with -fno-ident.
> And, btw, -fno-file to inhibit .file directives for the same reason,
> https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01785.html and all our
> ugly filenames like gcc.target/powerpc/swps-p8-36.c which strive to
> workaround the assembler-scans.
> 
> All those non-automatic hacks like naming swap() tests swps-,
> uglifying scan-patterns (which are not terribly straight forward to
> read anyway even without uglifying them) are error prone and costly.
> IMHO we should make sure time is spent for useful stuff and not be
> wasted to fight our very own testsuite.
> 
> -fno-ident ok for stage1?
> What about -fno-file? Clever alternative suggestions? Don't care?
I thought I ack'd this back in 2016? :-)

jeff
Bernhard Reutner-Fischer Sept. 5, 2018, 3:32 p.m. UTC | #7
On Tue, 21 Jun 2016 at 00:19, Jeff Law <law@redhat.com> wrote:
>
> On 06/18/2016 01:31 PM, Bernhard Reutner-Fischer wrote:
> > A branch with a name matching scan-assembler pattern triggers
> > inappropriate FAIL.
> >
> > E.g. branch fixups-testsuite and
> > - gcc.target/i386/pr65871-?.c (scan-assembler-not "test")
> > - gcc.target/i386/pr41442.c (scan-assembler-times "test|cmp" 2)
> > etc.
> >
> > This is a recurring problem as can be seen by some -fno-ident additions
> > by commits from e.g. Michael Meissner over the years: builtins-58.c,
> > powerpc/pr46728-?.c
> >
> > The patch below adds -fno-ident if a testcase contains one of
> > scan-assembler, scan-assembler-not or scan-assembler-times.
> >
> > Regression tested on x86_64-unknown-linux on a fixups-testsuite branch
> > where it fixes several false FAILs without regressions.
> >
> > gcc/testsuite/ChangeLog
> >
> > 2016-06-18  Bernhard Reutner-Fischer  <aldot@gcc.gnu.org>
> >
> >       PR testsuite/52665
> >       * lib/gcc-dg.exp (gcc-dg-test-1): Iterate over _required_options.
> >       * lib/target-supports.exp (scan-assembler_required_options,
> >       scan-assembler-not_required_options,
> >       scan-assembler-times_required_options): Add -fno-ident.
> >       * lib/scanasm.exp (scan-assembler-times): Fix error message.
> >       * c-c++-common/ident-0a.c: New test.
> >       * c-c++-common/ident-0b.c: New test.
> >       * c-c++-common/ident-1a.c: New test.
> >       * c-c++-common/ident-1b.c: New test.
> >       * c-c++-common/ident-2a.c: New test.
> >       * c-c++-common/ident-2b.c: New test.
> >
> > Ok for trunk?
> >
> > PS: proc force_conventional_output_for would be a bit misnomed by this,
> > not sure if it should be renamed to maybe set_required_options_for or
> > the like?
> OK.

Now applied without the rename to trunk as r264128.

thanks,

>
> Changing force_conventional_output to set_required_options_for is
> pre-approved as well.
>
> jeff
>

Patch
diff mbox

diff --git a/gcc/testsuite/c-c++-common/ident-0a.c b/gcc/testsuite/c-c++-common/ident-0a.c
new file mode 100644
index 0000000..900d206
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-0a.c
@@ -0,0 +1,6 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler-not turns off .ident  */
+/* { dg-do compile } */
+int i;
+
+/* { dg-final { scan-assembler-not "GCC: " } } */
diff --git a/gcc/testsuite/c-c++-common/ident-0b.c b/gcc/testsuite/c-c++-common/ident-0b.c
new file mode 100644
index 0000000..e08126d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-0b.c
@@ -0,0 +1,10 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler-not turns off .ident unless -fident in testcase */
+/* { dg-do compile } */
+/* { dg-options "-fident" } */
+int i;
+
+/* { dg-final { scan-assembler-not "GCC: " { xfail *-*-* } } } */
+/* The testsuite saw scan-assembler-not and turned off .ident so the above
+ * has to fail for proper operation since the testsuite itself forced
+ * -fident on again.  */
diff --git a/gcc/testsuite/c-c++-common/ident-1a.c b/gcc/testsuite/c-c++-common/ident-1a.c
new file mode 100644
index 0000000..867ee43
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-1a.c
@@ -0,0 +1,8 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler turns off .ident  */
+/* { dg-do compile } */
+int i;
+
+/* { dg-final { scan-assembler "GCC: " { xfail *-*-* } } } */
+/* The testsuite saw scan-assembler and turned off .ident so the above
+ * has to fail for proper operation.  */
diff --git a/gcc/testsuite/c-c++-common/ident-1b.c b/gcc/testsuite/c-c++-common/ident-1b.c
new file mode 100644
index 0000000..2431086
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-1b.c
@@ -0,0 +1,7 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler turns off .ident unless -fident in testcase */
+/* { dg-do compile } */
+/* { dg-options "-fident" } */
+int i;
+
+/* { dg-final { scan-assembler "GCC: " } } */
diff --git a/gcc/testsuite/c-c++-common/ident-2a.c b/gcc/testsuite/c-c++-common/ident-2a.c
new file mode 100644
index 0000000..131b867
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-2a.c
@@ -0,0 +1,6 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler-times turns off .ident  */
+/* { dg-do compile } */
+int i;
+
+/* { dg-final { scan-assembler-times "GCC: " 0 } } */ /* internal test, keep -times 0 ! */
diff --git a/gcc/testsuite/c-c++-common/ident-2b.c b/gcc/testsuite/c-c++-common/ident-2b.c
new file mode 100644
index 0000000..a21e25f
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ident-2b.c
@@ -0,0 +1,7 @@ 
+/* PR testsuite/52665
+ * Make sure scan-assembler-times turns off .ident unless -fident in testcase */
+/* { dg-do compile } */
+/* { dg-options "-fident" } */
+int ident;
+
+/* { dg-final { scan-assembler-times "GCC: " 1 } } */
diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
index 7039140..081b1a8 100644
--- a/gcc/testsuite/lib/gcc-dg.exp
+++ b/gcc/testsuite/lib/gcc-dg.exp
@@ -270,9 +270,11 @@  proc gcc-dg-test-1 { target_compile prog do_what extra_tool_flags } {
     foreach x [split $finalcode "\n"] {
 	set finalcmd [lindex $x 0]
 	if { [info procs ${finalcmd}_required_options] != "" } {
-	    set req [${finalcmd}_required_options]
-	    if { $req != "" && [lsearch -exact $extra_tool_flags $req] == -1 } {
-		lappend extra_tool_flags $req
+	    foreach req [${finalcmd}_required_options] {
+		if { $req != ""
+		     && [lsearch -exact $extra_tool_flags $req] == -1 } {
+		    lappend extra_tool_flags $req
+		}
 	    }
 	}
     }
diff --git a/gcc/testsuite/lib/scanasm.exp b/gcc/testsuite/lib/scanasm.exp
index 07b8f7d..a368474 100644
--- a/gcc/testsuite/lib/scanasm.exp
+++ b/gcc/testsuite/lib/scanasm.exp
@@ -212,11 +212,11 @@  proc scan-ada-spec-not { args } {
 # Call pass if pattern is present given number of times, otherwise fail.
 proc scan-assembler-times { args } {
     if { [llength $args] < 2 } {
-	error "scan-assembler: too few arguments"
+	error "scan-assembler-times: too few arguments"
         return
     }
     if { [llength $args] > 3 } {
-	error "scan-assembler: too many arguments"
+	error "scan-assembler-times: too many arguments"
 	return
     }
     if { [llength $args] >= 3 } {
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index ef5271a..d9ac988 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -6910,6 +6910,12 @@  proc force_conventional_output_for { test } {
     }
     proc ${test}_required_options {} {
 	global gcc_force_conventional_output
+	upvar 1 extra_tool_flags extra_tool_flags
+	if {[regexp -- "^scan-assembler" [info level 0]]
+	    && ![string match "*-fident*" $extra_tool_flags]} {
+	    # Do not let .ident confuse assembler scan tests
+	    return [list $gcc_force_conventional_output "-fno-ident"]
+	}
 	return $gcc_force_conventional_output
     }
 }