[PATCH, testsuite] Fix no_trampolines test in check_effective_target_trampolines

[PATCH, testsuite] Fix no_trampolines test in check_effective_target_trampolines

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 3089 bytes --]

Hi,

Consider check_effective_target_trampolines:
...
# Return 1 if according to target_info struct and explicit target list 

# target is supposed to support trampolines. 


proc check_effective_target_trampolines { } {
     if [target_info exists no_trampolines] {
       return 0
     }
     if { [istarget avr-*-*]
          || [istarget msp430-*-*]
          || [istarget hppa2.0w-hp-hpux11.23]
          || [istarget hppa64-hp-hpux11.23] } {
         return 0;
     }
     return 1
}
...

If I disable the nvptx target test in check_effective_target_trampolines 
and run tests for target nvptx, then the proc returns true instead of 
false, and I get 'UNSUPPORTED -> FAIL' changes for tests that require 
the effective target.

This is due to the fact that 
https://github.com/MentorEmbedded/nvptx-tools/blob/master/nvptx-none-run.exp 
defines 'gcc,no_trampolines':
...
set_board_info gcc,no_trampolines 1
...
but check_effective_target_trampolines tests no_trampolines (without the 
'gcc,' in front):

The effective target trampolines was introduced in 2008, but we've been 
testing 'gcc,no_trampolines' in gcc_target_compile since at least 1997.
[ To complicate matters objc_target_compile tests for 
'objc,no_trampolines' to set -DNO_TRAMPOLINES, but AFAICT that macro is 
not used anywhere in the objc test suites, so I think that's dead code. ]

The original submission of the keyword is here ( 
https://gcc.gnu.org/ml/gcc-patches/2005-04/msg01925.html ) and uses 
'target_info exists no_trampolines'. But in a follow-up comment ( 
https://gcc.gnu.org/ml/gcc-patches/2005-04/msg01978.html ) there is the 
suggestion to add 'set_board_info gcc,no_trampolines 1' to the board 
file to trigger the test in check_effective_target_trampolines. So that 
sounds like the missing 'gcc,' is accidental rather than intentional.

In a further follow-up comment ( 
https://gcc.gnu.org/ml/gcc-patches/2005-04/msg02063.html ) Mike suggest 
to test for target triplets, which indeed was added in the next version. 
This probably explains why the missing 'gcc,' wasn't found in any 
further testing.

As things are now, boards for targets that are not listed in 
check_effective_target_trampolines but still want the no_trampolines 
behavior need to do:
- 'set_board_info gcc,no_trampolines 1' to trigger the test in
   gcc_target_compile and add -DNO_TRAMPOLINES to the flags
- 'set_board_info no_trampolines 1' to trigger the test in
   check_effective_target_trampolines

Given that:
- it's better to have to define one variable than two
- it looks like an accident that the 'gcc,' was dropped
- the one with the 'gcc,' prefix has been around longer, and is
   mentioned in dejagnu docs
I propose to test for 'gcc,no_trampolines' instead of 'no_trampolines' 
in check_effective_target_trampolines.

I don't think a backward compatibility test for 'no_trampolines' is 
required, because the affected boardfiles most likely already define both.

Tested by checking that the patch fixes the 'UNSUPPORTED -> FAIL' 
regression mentioned above for a single testcase.

OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-Fix-no_trampolines-test-in-check_effective_target_trampolines.patch --]
[-- Type: text/x-patch, Size: 849 bytes --]

Fix no_trampolines test in check_effective_target_trampolines

2017-06-08  Tom de Vries  <tom@codesourcery.com>

	* lib/target-supports.exp (check_effective_target_trampolines): Test for
	'gcc,no_trampolines' instead of 'no_trampolines'.

---
 gcc/testsuite/lib/target-supports.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 8b99f35..d0b35be 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -491,7 +491,7 @@ proc check_gc_sections_available { } {
 # target is supposed to support trampolines.
  
 proc check_effective_target_trampolines { } {
-    if [target_info exists no_trampolines] {
+    if [target_info exists gcc,no_trampolines] {
       return 0
     }
     if { [istarget avr-*-*]

Re: [PATCH, testsuite] Fix no_trampolines test in check_effective_target_trampolines

[Mike Stump]

On Jun 8, 2017, at 3:20 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:

> [ To complicate matters objc_target_compile tests for 'objc,no_trampolines' to set -DNO_TRAMPOLINES, but AFAICT that macro is not used anywhere in the objc test suites, so I think that's dead code. ]

Yes, Ok to remove the dead code as well.

> - it's better to have to define one variable than two
> - it looks like an accident that the 'gcc,' was dropped
> - the one with the 'gcc,' prefix has been around longer, and is
>  mentioned in dejagnu docs
> I propose to test for 'gcc,no_trampolines' instead of 'no_trampolines' in check_effective_target_trampolines.

> OK for trunk?

Ok.

I had hit this bug years ago, and was puzzled why people seemed to get it so wrong.  I took the easy way out and just defined the three of them.   :-(