[v2] testsuite: Only run test on target if VMA == LMA

Message ID 20220920132515.844262-1-torbjorn.svensson@foss.st.com
State Accepted, archived
Headers
Series [v2] testsuite: Only run test on target if VMA == LMA |

Checks

Context Check Description
snail/gcc-patches-check success Github commit url

Commit Message

Torbjorn SVENSSON Sept. 20, 2022, 1:25 p.m. UTC
  Checking that the triplet matches arm*-*-eabi (or msp430-*-*) is not
enough to know if the execution will enter an endless loop, or if it
will give a meaningful result. As the execution test only work when
VMA and LMA are equal, make sure that this condition is met.

2022-09-16  Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>

gcc/testsuite/ChangeLog:

	* lib/target-supports.exp (check_effective_target_vma_equals_lma): New.
        * c-c++-common/torture/attr-noinit-1.c: Requre VMA == LMA to run.
        * c-c++-common/torture/attr-noinit-2.c: Likewise.
        * c-c++-common/torture/attr-noinit-3.c: Likewise.
        * c-c++-common/torture/attr-persistent-1.c: Likewise.
        * c-c++-common/torture/attr-persistent-3.c: Likewise.

Co-Authored-By: Yvan ROUX  <yvan.roux@foss.st.com>
Signed-off-by: Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>
---
 .../c-c++-common/torture/attr-noinit-1.c      |  3 +-
 .../c-c++-common/torture/attr-noinit-2.c      |  3 +-
 .../c-c++-common/torture/attr-noinit-3.c      |  3 +-
 .../c-c++-common/torture/attr-persistent-1.c  |  3 +-
 .../c-c++-common/torture/attr-persistent-3.c  |  3 +-
 gcc/testsuite/lib/target-supports.exp         | 49 +++++++++++++++++++
 6 files changed, 59 insertions(+), 5 deletions(-)
  

Comments

Richard Sandiford Sept. 23, 2022, 5:34 p.m. UTC | #1
Torbjörn SVENSSON via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Checking that the triplet matches arm*-*-eabi (or msp430-*-*) is not
> enough to know if the execution will enter an endless loop, or if it
> will give a meaningful result. As the execution test only work when
> VMA and LMA are equal, make sure that this condition is met.
>
> 2022-09-16  Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>
>
> gcc/testsuite/ChangeLog:
>
> 	* lib/target-supports.exp (check_effective_target_vma_equals_lma): New.
>         * c-c++-common/torture/attr-noinit-1.c: Requre VMA == LMA to run.
>         * c-c++-common/torture/attr-noinit-2.c: Likewise.
>         * c-c++-common/torture/attr-noinit-3.c: Likewise.
>         * c-c++-common/torture/attr-persistent-1.c: Likewise.
>         * c-c++-common/torture/attr-persistent-3.c: Likewise.

Looks like a nice thing to have.

Could you add an entry to doc/sourcebuild.texi too?

A couple of comments below...

> Co-Authored-By: Yvan ROUX  <yvan.roux@foss.st.com>
> Signed-off-by: Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>
> ---
>  .../c-c++-common/torture/attr-noinit-1.c      |  3 +-
>  .../c-c++-common/torture/attr-noinit-2.c      |  3 +-
>  .../c-c++-common/torture/attr-noinit-3.c      |  3 +-
>  .../c-c++-common/torture/attr-persistent-1.c  |  3 +-
>  .../c-c++-common/torture/attr-persistent-3.c  |  3 +-
>  gcc/testsuite/lib/target-supports.exp         | 49 +++++++++++++++++++
>  6 files changed, 59 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
> index 877e7647ac9..f84eba0b649 100644
> --- a/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
> +++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
> @@ -1,4 +1,5 @@
> -/* { dg-do run } */
> +/* { dg-do link } */
> +/* { dg-do run { target { vma_equals_lma } } } */
>  /* { dg-require-effective-target noinit } */
>  /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
>  /* { dg-options "-save-temps" } */
> diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
> index befa2a0bd52..4528b9e3cfa 100644
> --- a/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
> +++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
> @@ -1,4 +1,5 @@
> -/* { dg-do run } */
> +/* { dg-do link } */
> +/* { dg-do run { target { vma_equals_lma } } } */
>  /* { dg-require-effective-target noinit } */
>  /* { dg-options "-fdata-sections -save-temps" } */
>  /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
> diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
> index 519e88a59a6..2f1745694c9 100644
> --- a/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
> +++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
> @@ -1,4 +1,5 @@
> -/* { dg-do run } */
> +/* { dg-do link } */
> +/* { dg-do run { target { vma_equals_lma } } } */
>  /* { dg-require-effective-target noinit } */
>  /* { dg-options "-flto -save-temps" } */
>  /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
> diff --git a/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c b/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
> index 72dc3c27192..b11a515cef8 100644
> --- a/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
> +++ b/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
> @@ -1,4 +1,5 @@
> -/* { dg-do run } */
> +/* { dg-do link } */
> +/* { dg-do run { target { vma_equals_lma } } } */
>  /* { dg-require-effective-target persistent } */
>  /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
>  /* { dg-options "-save-temps" } */
> diff --git a/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c b/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
> index 3e4fd28618d..068a72af5c8 100644
> --- a/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
> +++ b/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
> @@ -1,4 +1,5 @@
> -/* { dg-do run } */
> +/* { dg-do link } */
> +/* { dg-do run { target { vma_equals_lma } } } */
>  /* { dg-require-effective-target persistent } */
>  /* { dg-options "-flto -save-temps" } */
>  /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 703aba412a6..df8141a15d8 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -370,6 +370,55 @@ proc check_weak_override_available { } {
>      return [check_weak_available]
>  }
>  
> +# Return 1 if VMA is equal to LMA for the .data section, 0
> +# otherwise.  Cache the result.
> +
> +proc check_effective_target_vma_equals_lma { } {
> +    global tool
> +
> +    return [check_cached_effective_target vma_equals_lma {
> +	set src vma_equals_lma[pid].c
> +	set exe vma_equals_lma[pid].exe
> +	verbose "check_effective_target_vma_equals_lma  compiling testfile $src" 2
> +	set f [open $src "w"]
> +	puts $f "#ifdef __cplusplus\nextern \"C\"\n#endif\n"
> +	puts $f "int foo = 42; void main() {}"
> +	close $f
> +	set lines [${tool}_target_compile $src $exe executable ""]
> +	file delete $src
> +
> +	if [string match "" $lines] then {
> +	    # No error messages
> +
> +            set objdump_name [find_binutils_prog objdump]
> +            set output [remote_exec host "$objdump_name" "--section-headers --section=.data $exe"]
> +            set output [lindex $output 1]
> +
> +            remote_file build delete $exe
> +
> +            # Example output of objdump:
> +            #vma_equals_lma9059.exe:     file format elf32-littlearm
> +            #
> +            #Sections:
> +            #Idx Name          Size      VMA       LMA       File off  Algn
> +            #  6 .data         00000558  20000000  08002658  00020000  2**3
> +            #                  CONTENTS, ALLOC, LOAD, DATA
> +
> +            # Capture LMA and VMA columns for .data section
> +            if ![ regexp "\d*\d+\s+\.data\s+\d+\s+(\d+)\s+(\d+)" $output dummy vma lma ] {

Maybe my Tcl is getting rusty, but I'm surprised this quoting works.
I'd have expected single backslashes to be interpreted as C-style
sequences (for \n etc) and so be eaten before regexp sees them.
Quoting with {...} instead of "..." would avoid that.

> +                verbose "Could not parse objdump output" 2
> +                return 0
> +            } else {
> +                return [string equal $vma $lma]
> +            }
> +	} else {
> +            remote_file build delete $exe
> +            verbose "Could not determine if VMA is equal to LMA. Assuming not equal." 2
> +            return 0
> +	}

Would it be more conservative to return 1 on failure rather than 0?
That way, a faulty test would trigger XFAILs rather than UNSUPPORTEDs,
with XFAILs being more likely to get attention.

On the other hand, I suppose we don't lose much if these tests are
run on common targets only.  So either way is OK, just asking. ;-)

Thanks,
Richard

> +    }]
> +}
> +
>  # The "noinit" attribute is only supported by some targets.
>  # This proc returns 1 if it's supported, 0 if it's not.
  
Torbjorn SVENSSON Sept. 23, 2022, 6:37 p.m. UTC | #2
Hi Richard,

Thanks for your review.
Comments below.

On 2022-09-23 19:34, Richard Sandiford wrote:
> Torbjörn SVENSSON via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> Checking that the triplet matches arm*-*-eabi (or msp430-*-*) is not
>> enough to know if the execution will enter an endless loop, or if it
>> will give a meaningful result. As the execution test only work when
>> VMA and LMA are equal, make sure that this condition is met.
>>
>> 2022-09-16  Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 	* lib/target-supports.exp (check_effective_target_vma_equals_lma): New.
>>          * c-c++-common/torture/attr-noinit-1.c: Requre VMA == LMA to run.
>>          * c-c++-common/torture/attr-noinit-2.c: Likewise.
>>          * c-c++-common/torture/attr-noinit-3.c: Likewise.
>>          * c-c++-common/torture/attr-persistent-1.c: Likewise.
>>          * c-c++-common/torture/attr-persistent-3.c: Likewise.
> 
> Looks like a nice thing to have.
> 
> Could you add an entry to doc/sourcebuild.texi too?

I've added a note and will be shared in a v3.

>> +
>> +            # Example output of objdump:
>> +            #vma_equals_lma9059.exe:     file format elf32-littlearm
>> +            #
>> +            #Sections:
>> +            #Idx Name          Size      VMA       LMA       File off  Algn
>> +            #  6 .data         00000558  20000000  08002658  00020000  2**3
>> +            #                  CONTENTS, ALLOC, LOAD, DATA
>> +
>> +            # Capture LMA and VMA columns for .data section
>> +            if ![ regexp "\d*\d+\s+\.data\s+\d+\s+(\d+)\s+(\d+)" $output dummy vma lma ] {
> 
> Maybe my Tcl is getting rusty, but I'm surprised this quoting works.
> I'd have expected single backslashes to be interpreted as C-style
> sequences (for \n etc) and so be eaten before regexp sees them.
> Quoting with {...} instead of "..." would avoid that.

Good catch! I'm not fluent in Tcl and apparently, I was not testing this 
well enough before sending it to the list. I got the expected result for 
the test cases, but for the wrong reason. I've correct it for the v3.

>> +                verbose "Could not parse objdump output" 2
>> +                return 0
>> +            } else {
>> +                return [string equal $vma $lma]
>> +            }
>> +	} else {
>> +            remote_file build delete $exe
>> +            verbose "Could not determine if VMA is equal to LMA. Assuming not equal." 2
>> +            return 0
>> +	}
> 
> Would it be more conservative to return 1 on failure rather than 0?
> That way, a faulty test would trigger XFAILs rather than UNSUPPORTEDs,
> with XFAILs being more likely to get attention.

The main issue here is that for targets where VMA != LMA, executing the 
tests will fall into an endless recursion loop. Well, "endless" in the 
sense that the stack might be depleted or the test will simply timeout. 
The test cases are designed to assume that it's safe to call _start() 
from within main() to verify that the state of some variables tagged 
with certain attributes are correct after a "reset".

> On the other hand, I suppose we don't lose much if these tests are
> run on common targets only.  So either way is OK, just asking. ;-)

Do you think it's worth to have these tests reach the timeout to have 
them in the XFAIL list rather than in the UNSUPPORTED list?
Keep in mind that it's not just one test case, it's 5 test cases times 
the number of permutations of the CFLAGS...
It's also not expected that these test cases will be changed in a way 
that will make them work when VMA != LMA.

Kind regards,
Torbjörn
  
Richard Sandiford Sept. 30, 2022, 4:13 p.m. UTC | #3
Sorry for the slow reply.

Torbjorn SVENSSON <torbjorn.svensson@foss.st.com> writes:
> Hi Richard,
>
> Thanks for your review.
> Comments below.
>
> On 2022-09-23 19:34, Richard Sandiford wrote:
>> Torbjörn SVENSSON via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> Checking that the triplet matches arm*-*-eabi (or msp430-*-*) is not
>>> enough to know if the execution will enter an endless loop, or if it
>>> will give a meaningful result. As the execution test only work when
>>> VMA and LMA are equal, make sure that this condition is met.
>>>
>>> 2022-09-16  Torbjörn SVENSSON  <torbjorn.svensson@foss.st.com>
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* lib/target-supports.exp (check_effective_target_vma_equals_lma): New.
>>>          * c-c++-common/torture/attr-noinit-1.c: Requre VMA == LMA to run.
>>>          * c-c++-common/torture/attr-noinit-2.c: Likewise.
>>>          * c-c++-common/torture/attr-noinit-3.c: Likewise.
>>>          * c-c++-common/torture/attr-persistent-1.c: Likewise.
>>>          * c-c++-common/torture/attr-persistent-3.c: Likewise.
>> 
>> Looks like a nice thing to have.
>> 
>> Could you add an entry to doc/sourcebuild.texi too?
>
> I've added a note and will be shared in a v3.
>
>>> +
>>> +            # Example output of objdump:
>>> +            #vma_equals_lma9059.exe:     file format elf32-littlearm
>>> +            #
>>> +            #Sections:
>>> +            #Idx Name          Size      VMA       LMA       File off  Algn
>>> +            #  6 .data         00000558  20000000  08002658  00020000  2**3
>>> +            #                  CONTENTS, ALLOC, LOAD, DATA
>>> +
>>> +            # Capture LMA and VMA columns for .data section
>>> +            if ![ regexp "\d*\d+\s+\.data\s+\d+\s+(\d+)\s+(\d+)" $output dummy vma lma ] {
>> 
>> Maybe my Tcl is getting rusty, but I'm surprised this quoting works.
>> I'd have expected single backslashes to be interpreted as C-style
>> sequences (for \n etc) and so be eaten before regexp sees them.
>> Quoting with {...} instead of "..." would avoid that.
>
> Good catch! I'm not fluent in Tcl and apparently, I was not testing this 
> well enough before sending it to the list. I got the expected result for 
> the test cases, but for the wrong reason. I've correct it for the v3.
>
>>> +                verbose "Could not parse objdump output" 2
>>> +                return 0
>>> +            } else {
>>> +                return [string equal $vma $lma]
>>> +            }
>>> +	} else {
>>> +            remote_file build delete $exe
>>> +            verbose "Could not determine if VMA is equal to LMA. Assuming not equal." 2
>>> +            return 0
>>> +	}
>> 
>> Would it be more conservative to return 1 on failure rather than 0?
>> That way, a faulty test would trigger XFAILs rather than UNSUPPORTEDs,
>> with XFAILs being more likely to get attention.
>
> The main issue here is that for targets where VMA != LMA, executing the 
> tests will fall into an endless recursion loop. Well, "endless" in the 
> sense that the stack might be depleted or the test will simply timeout. 
> The test cases are designed to assume that it's safe to call _start() 
> from within main() to verify that the state of some variables tagged 
> with certain attributes are correct after a "reset".

Ah, OK.  In that case, I agree return 0 is the right choice.

>> On the other hand, I suppose we don't lose much if these tests are
>> run on common targets only.  So either way is OK, just asking. ;-)
>
> Do you think it's worth to have these tests reach the timeout to have 
> them in the XFAIL list rather than in the UNSUPPORTED list?
> Keep in mind that it's not just one test case, it's 5 test cases times 
> the number of permutations of the CFLAGS...
> It's also not expected that these test cases will be changed in a way 
> that will make them work when VMA != LMA.

Yeah, good points.  Let's stick with it as it is then.

Thanks,
Richard
  

Patch

diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
index 877e7647ac9..f84eba0b649 100644
--- a/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
+++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-1.c
@@ -1,4 +1,5 @@ 
-/* { dg-do run } */
+/* { dg-do link } */
+/* { dg-do run { target { vma_equals_lma } } } */
 /* { dg-require-effective-target noinit } */
 /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
 /* { dg-options "-save-temps" } */
diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
index befa2a0bd52..4528b9e3cfa 100644
--- a/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
+++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-2.c
@@ -1,4 +1,5 @@ 
-/* { dg-do run } */
+/* { dg-do link } */
+/* { dg-do run { target { vma_equals_lma } } } */
 /* { dg-require-effective-target noinit } */
 /* { dg-options "-fdata-sections -save-temps" } */
 /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
diff --git a/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c b/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
index 519e88a59a6..2f1745694c9 100644
--- a/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
+++ b/gcc/testsuite/c-c++-common/torture/attr-noinit-3.c
@@ -1,4 +1,5 @@ 
-/* { dg-do run } */
+/* { dg-do link } */
+/* { dg-do run { target { vma_equals_lma } } } */
 /* { dg-require-effective-target noinit } */
 /* { dg-options "-flto -save-temps" } */
 /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
diff --git a/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c b/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
index 72dc3c27192..b11a515cef8 100644
--- a/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
+++ b/gcc/testsuite/c-c++-common/torture/attr-persistent-1.c
@@ -1,4 +1,5 @@ 
-/* { dg-do run } */
+/* { dg-do link } */
+/* { dg-do run { target { vma_equals_lma } } } */
 /* { dg-require-effective-target persistent } */
 /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
 /* { dg-options "-save-temps" } */
diff --git a/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c b/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
index 3e4fd28618d..068a72af5c8 100644
--- a/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
+++ b/gcc/testsuite/c-c++-common/torture/attr-persistent-3.c
@@ -1,4 +1,5 @@ 
-/* { dg-do run } */
+/* { dg-do link } */
+/* { dg-do run { target { vma_equals_lma } } } */
 /* { dg-require-effective-target persistent } */
 /* { dg-options "-flto -save-temps" } */
 /* { dg-skip-if "data LMA != VMA" { msp430-*-* } { "-mlarge" } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 703aba412a6..df8141a15d8 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -370,6 +370,55 @@  proc check_weak_override_available { } {
     return [check_weak_available]
 }
 
+# Return 1 if VMA is equal to LMA for the .data section, 0
+# otherwise.  Cache the result.
+
+proc check_effective_target_vma_equals_lma { } {
+    global tool
+
+    return [check_cached_effective_target vma_equals_lma {
+	set src vma_equals_lma[pid].c
+	set exe vma_equals_lma[pid].exe
+	verbose "check_effective_target_vma_equals_lma  compiling testfile $src" 2
+	set f [open $src "w"]
+	puts $f "#ifdef __cplusplus\nextern \"C\"\n#endif\n"
+	puts $f "int foo = 42; void main() {}"
+	close $f
+	set lines [${tool}_target_compile $src $exe executable ""]
+	file delete $src
+
+	if [string match "" $lines] then {
+	    # No error messages
+
+            set objdump_name [find_binutils_prog objdump]
+            set output [remote_exec host "$objdump_name" "--section-headers --section=.data $exe"]
+            set output [lindex $output 1]
+
+            remote_file build delete $exe
+
+            # Example output of objdump:
+            #vma_equals_lma9059.exe:     file format elf32-littlearm
+            #
+            #Sections:
+            #Idx Name          Size      VMA       LMA       File off  Algn
+            #  6 .data         00000558  20000000  08002658  00020000  2**3
+            #                  CONTENTS, ALLOC, LOAD, DATA
+
+            # Capture LMA and VMA columns for .data section
+            if ![ regexp "\d*\d+\s+\.data\s+\d+\s+(\d+)\s+(\d+)" $output dummy vma lma ] {
+                verbose "Could not parse objdump output" 2
+                return 0
+            } else {
+                return [string equal $vma $lma]
+            }
+	} else {
+            remote_file build delete $exe
+            verbose "Could not determine if VMA is equal to LMA. Assuming not equal." 2
+            return 0
+	}
+    }]
+}
+
 # The "noinit" attribute is only supported by some targets.
 # This proc returns 1 if it's supported, 0 if it's not.