Re: [PATCH][gdb/testsuite] Fix gdb.base/endianity.exp with gcc-4.8

[Tom de Vries <tdevries@suse.de>]

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

On 12/14/20 5:55 PM, Simon Marchi wrote:
> On 2020-12-13 11:23 a.m., Tom de Vries wrote:
>> On 12/13/20 2:56 PM, Joel Brobecker wrote:
>>> Hi Tom,
>>>
>>>> When running test-case gdb.base/endianity.exp using gcc-4.8, we get:
>>>> ...
>>>> (gdb) x/x &o.v^M
>>>> 0x7fffffffd120: 0x00000004^M
>>>> (gdb) XFAIL: gdb.base/endianity.exp: x/x &o.v
>>>> x/xh &o.w^M
>>>> 0x7fffffffd124: 0x0003^M
>>>> (gdb) FAIL: gdb.base/endianity.exp: x/xh &o.w
>>>> ...
>>>>
>>>> The gcc 4.8 compiler does not support the scalar_storage_order attribute, so
>>>> the testcase is compiled without that attribute, and the expected results are
>>>> different.
>>>>
>>>> This is why there's the first XFAIL, and we could xfail the second FAIL for the
>>>> same reason.
>>>>
>>>> Instead, fix this by adapting the expected values based on whether the attribute
>>>> has been used in endianity.c.
>>>>
>>>> Also, remove hard-coding of the byte order in the expected memory printing.
>>>>
>>>> Tested on x86_64-linux, with gcc-4.8, gcc-7, and clang-10.
>>>>
>>>>
>>>> Any comments?
>>>
>>> For me, the whole point of this testcase is to test SSO, so if
>>> the compiler doesn't support it, the testcase loses its value
>>> entirely (to my eyes anyway). As a result of this, I dont' think
>>> bringing the extra complexity that you are suggesting is bringing
>>> any value -- I might argue that it's now hard to read the testcase
>>> an understand what we're trying to do (sorry!).
>>>
>>
>> Np, that's also good feedback, thanks.
>>
>>> In my opinion, rather than an XFAIL, we should just only do
>>> the second half of the testcase if the compiler supports it,
>>> than xfailing the tests. So I would do:
>>>
>>> if { ([test_compiler_info {gcc-[0-5]-*}] || ![test_compiler_info gcc*]) } {
>>>     # The rest of the testcase requires Scalar Storage Order support.
>>>     # This compiler does not support it, so skip the rest.
>>>     return
>>> }
>>>
>>
>> Ack, committed as below.
>>
>> Thanks,
>> - Tom
>>
> 
> Just a small nit, it would be nice to have an "unsupported" call in the
> if to record in the sum file that part of this file was skipped and why.
> 
> Also, I don't like the (pre-existing) check:
> 
>   if { ([test_compiler_info {gcc-[0-5]-*}] || ![test_compiler_info gcc*]) } {
> 
> That makes it so testing with any other compiler than gcc will result in
> this part being skipped.  What if clang gains support for this feature?
> 
> Would it make sense to have a small
> "supports_scalar_storage_order_attribute" util proc in lib/gdb.exp?

How about this?

Thanks,
- Tom


[-- Attachment #2: 0005-gdb-testsuite-Introduce-supports_scalar_storage_order_attribute.patch --]
[-- Type: text/x-patch, Size: 3876 bytes --]

[gdb/testsuite] Introduce supports_scalar_storage_order_attribute

Introduce support test procs:
- supports_scalar_storage_order_attribute, and
- supports_gnuc
and use them in test-case gdb.base/endianity.exp.

Tested on x86_64-linux with gcc-7.5.0, gcc-4.8.5, and clang 10.0.1.

gdb/testsuite/ChangeLog:

2020-12-19  Tom de Vries  <tdevries@suse.de>

	* lib/gdb.exp (supports_scalar_storage_order_attribute)
	(supports_gnuc): New proc.
	* gdb.base/endianity.exp: Define TEST_SSO.  Eliminate
	test_compiler_info calls.  Add unsupported message.
	* gdb.base/endianity.c: Use TEST_SSO.

---
 gdb/testsuite/gdb.base/endianity.c   |  2 +-
 gdb/testsuite/gdb.base/endianity.exp | 11 ++++++---
 gdb/testsuite/lib/gdb.exp            | 46 ++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/gdb.base/endianity.c b/gdb/testsuite/gdb.base/endianity.c
index ef3b6d4fdb..15cbdd9a26 100644
--- a/gdb/testsuite/gdb.base/endianity.c
+++ b/gdb/testsuite/gdb.base/endianity.c
@@ -26,7 +26,7 @@ struct otherendian
   __complex__ float cplx;
   double d;
 }
-#if defined __GNUC__ && (__GNUC__ >= 6)
+#if TEST_SSO
 #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
 __attribute__( ( scalar_storage_order( "big-endian" ) ) )
 #else
diff --git a/gdb/testsuite/gdb.base/endianity.exp b/gdb/testsuite/gdb.base/endianity.exp
index 4520799d04..d54c1b6973 100644
--- a/gdb/testsuite/gdb.base/endianity.exp
+++ b/gdb/testsuite/gdb.base/endianity.exp
@@ -15,7 +15,12 @@
 
 standard_testfile .c
 
-if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+set test_sso [expr \
+		  [supports_scalar_storage_order_attribute] \
+		  && [supports_gnuc]]
+
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  [list debug additional_flags=-DTEST_SSO=$test_sso]] } {
   return -1
 }
 
@@ -37,12 +42,12 @@ gdb_test "print o.d = -23.125" "= -23.125"
 gdb_test "print o" "= {v = 4, w = 3, x = 2, f = 1.5, cplx = 1.25 \\+ 7.25i, d = -23.125}" \
     "print o after assignment"
 
-if { ([test_compiler_info {gcc-[0-5]-*}] || ![test_compiler_info gcc*]) } {
+if { !$test_sso } {
     # The rest of the testcase requires Scalar Storage Order support.
     # This compiler does not support it, so skip the rest.
+    unsupported "No scalar storage order support"
     return -1
 }
 
 gdb_test "x/x &o.v" "0x04000000"
 gdb_test "x/xh &o.w" "0x0300"
-
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e812237d67..573d9cc802 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7669,5 +7669,51 @@ gdb_caching_proc have_fuse_ld_gold {
     return [gdb_simple_compile $me $src executable $flags]
 }
 
+# Return 1 if compiler supports scalar_storage_order attribute, otherwise
+# return 0.
+gdb_caching_proc supports_scalar_storage_order_attribute {
+    set me "supports_scalar_storage_order_attribute"
+    set src {
+	#include <string.h>
+	struct sle {
+	    int v;
+	} __attribute__((scalar_storage_order("little-endian")));
+	struct sbe {
+	    int v;
+	} __attribute__((scalar_storage_order("big-endian")));
+	struct sle sle;
+	struct sbe sbe;
+	int main () {
+	    sle.v = sbe.v = 0x11223344;
+	    int same = memcmp (&sle, &sbe, sizeof (int)) == 0;
+	    int sso = !same;
+	    return sso;
+	}
+    }
+    if { ![gdb_simple_compile $me $src executable ""] } {
+	return 0
+    }
+
+    set result [remote_exec host $obj]
+    set status [lindex $result 0]
+    set output [lindex $result 1]
+    if { $output != "" } {
+	return 0
+    }
+
+    return $status
+}
+
+# Return 1 if compiler supports __GNUC__, otherwise return 0.
+gdb_caching_proc supports_gnuc {
+    set me "supports_gnuc"
+    set src {
+	#ifndef __GNUC__
+	#error "No gnuc"
+	#endif
+    }
+    return [gdb_simple_compile $me $src object ""]
+}
+
 # Always load compatibility stuff.
 load_lib future.exp