public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>
To: "Torbjörn SVENSSON via Gcc-patches" <gcc-patches@gcc.gnu.org>,
	richard.sandiford@arm.com
Subject: Re: [PATCH v2] testsuite: Only run test on target if VMA == LMA
Date: Fri, 23 Sep 2022 20:37:26 +0200	[thread overview]
Message-ID: <6b9d664b-e7f4-c7c9-e167-137cfeca5924@foss.st.com> (raw)
In-Reply-To: <mpt1qs2xadu.fsf@arm.com>

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

  reply	other threads:[~2022-09-23 18:37 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20 13:25 Torbjörn SVENSSON
2022-09-23 17:34 ` Richard Sandiford
2022-09-23 18:37   ` Torbjorn SVENSSON [this message]
2022-09-30 16:13     ` Richard Sandiford

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6b9d664b-e7f4-c7c9-e167-137cfeca5924@foss.st.com \
    --to=torbjorn.svensson@foss.st.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=richard.sandiford@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).