From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9249 invoked by alias); 19 Apr 2017 18:03:55 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 9237 invoked by uid 89); 19 Apr 2017 18:03:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=340, tiny X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Apr 2017 18:03:53 +0000 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3JI3pvV120696 for ; Wed, 19 Apr 2017 14:03:53 -0400 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 29x77d9pvd-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 19 Apr 2017 14:03:52 -0400 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 19 Apr 2017 19:03:47 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (9.149.109.194) by e06smtp13.uk.ibm.com (192.168.101.143) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 19 Apr 2017 19:03:46 +0100 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v3JI3j4f14811410; Wed, 19 Apr 2017 18:03:45 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 55F6C4203F; Wed, 19 Apr 2017 19:02:45 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2EC4D4204C; Wed, 19 Apr 2017 19:02:45 +0100 (BST) Received: from oc1027705133.ibm.com (unknown [9.152.212.162]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTPS; Wed, 19 Apr 2017 19:02:45 +0100 (BST) From: Andreas Arnez To: Simon Marchi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 6/9] Fix handling of DWARF register pieces on big-endian targets References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-7-git-send-email-arnez@linux.vnet.ibm.com> <9a0ec20dcede60f09ee38c46b2f2ae9f@polymtl.ca> Date: Wed, 19 Apr 2017 18:03:00 -0000 In-Reply-To: <9a0ec20dcede60f09ee38c46b2f2ae9f@polymtl.ca> (Simon Marchi's message of "Fri, 14 Apr 2017 10:10:59 -0400") User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 x-cbid: 17041918-0012-0000-0000-000005100277 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17041918-0013-0000-0000-0000181D7BAB Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-19_15:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1704190151 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00572.txt.bz2 On Fri, Apr 14 2017, Simon Marchi wrote: > On 2017-04-07 13:38, Andreas Arnez wrote: [...] >> diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c >> index 1f89a08..d13c0c5 100644 >> --- a/gdb/dwarf2loc.c >> +++ b/gdb/dwarf2loc.c [...] >> @@ -1805,13 +1813,9 @@ read_pieced_value (struct value *v) >> if (this_size_bits > type_len - offset) >> this_size_bits = type_len - offset; >> >> - this_size = (this_size_bits + source_offset_bits % 8 + 7) / 8; >> + this_size = bits_to_bytes (source_offset_bits, this_size_bits); >> + buffer.reserve (this_size); >> source_offset = source_offset_bits / 8; >> - if (buffer_size < this_size) >> - { >> - buffer_size = this_size; >> - buffer.reserve (buffer_size); >> - } > > The removal of buffer_size and this "if" (and equivalent in > write_pieced_value) seems to be unrelated to the problem you are fixing, > but rather a simplification that could be done anyway. If that's the > case, you should bundle it with the similar changes from the previous > patch, in a patch that only does some refactoring but not behavior > changes. This particular simplification is triggered by the fact that we'd otherwise have to duplicate this code, now that the buffer reservation is potentially repeated. So it is in fact related to the fix. But you're right that it could be done anyway. Thus I'll extract a tiny patch just with that change and insert it before this one. [...] >> @@ -1999,20 +1997,24 @@ write_pieced_value (struct value *to, struct >> value *from) >> struct frame_info *frame = frame_find_by_id (c->frame_id); >> struct gdbarch *arch = get_frame_arch (frame); >> int gdb_regnum = dwarf_reg_to_regnum_or_error (arch, p->v.regno); >> - int reg_offset = dest_offset; >> + ULONGEST reg_bits = 8 * register_size (arch, gdb_regnum); >> >> if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG >> - && this_size <= register_size (arch, gdb_regnum)) >> + && p->size <= reg_bits) >> { >> /* Big-endian, and we want less than full size. */ >> - reg_offset = register_size (arch, gdb_regnum) - this_size; >> + dest_offset_bits += reg_bits - p->size; >> } >> + this_size = bits_to_bytes (dest_offset_bits, this_size_bits); >> + buffer.reserve (this_size); >> >> - if (need_bitwise) >> + if (dest_offset_bits % 8 != 0 || this_size_bits % 8 != 0) > > I thought the "need_bitwise" variable name helped to understand. To > compensate, could you add a comment explaining the "why" of that if? That's what the next comment is intended for. I can enhance it, though: /* Data is copied non-byte-aligned into the register. Need some bits from original register value. */ > >> { >> + /* Need some bits from original register value. */ >> int optim, unavail; >> [...] >> diff --git a/gdb/testsuite/gdb.dwarf2/var-access.exp >> b/gdb/testsuite/gdb.dwarf2/var-access.exp >> index c6abc87..4787dfb 100644 >> --- a/gdb/testsuite/gdb.dwarf2/var-access.exp >> +++ b/gdb/testsuite/gdb.dwarf2/var-access.exp [...] >> @@ -206,6 +218,15 @@ if ![runto_main] { >> return -1 >> } >> >> +# Determine endianness. >> +set endian "little" >> +gdb_test_multiple "show endian" "determine endianness" { >> + -re ".* (big|little) endian.*$gdb_prompt $" { >> + set endian $expect_out(1,string) >> + pass "endianness: $endian" >> + } >> +} > > "pass" should be passed the test name, which should be stable as much as > possible. The typical way to do it would be: > > # Determine endianness. > set endian "little" > set test "determine endianness" > gdb_test_multiple "show endian" $test { > -re ".* (big|little) endian.*$gdb_prompt $" { > set endian $expect_out(1,string) > pass $test > } > } The endianness determination should be extracted into a library function anyway, since it exists many times already. I'll do that in V2. > >> + >> # Byte-aligned memory pieces. >> gdb_test "print/d s1" " = \\{a = 2, b = 3, c = 0, d = 1\\}" \ >> "s1 == re-ordered buf" >> @@ -244,3 +265,14 @@ gdb_test_no_output "set var t1.y = 1234" >> gdb_test "print t1" " = \\{u = , x = -7, y = 1234, z = >> 340\\}" \ >> "verify t1" >> >> +switch $endian { big {set val 0x7ffc} little {set val 0x3ffe00} } > > I had to draw this in order to understand, so it might be useful to > others. > > For little endian (assuming the register is 32 bits): > > 3 f f e 0 0 > xxxx xxxx 0011 1111 1111 1110 0000 0000 > ^^ ^^^^ ^^^^ > | ^^ ^^^^ ^^^^ ^^^ ` x > | `- y > `- part of z > > > For big endian (assuming the register is 32 bits): > > 0 0 7 f f c > xxxx xxxx 0000 0000 0111 1111 1111 1100 > ^^^^ ^^^^ ^ ^^ > `- x ^^^ ^^^^ ^^^^ ^^ `- part of z > `- y Very nice :-) > > Not having worked with a big endian machine before, I'm still confused in > how bits are aligned in the register and which bit means what. Note that your little-endian picture shows bytes in reversed addressing order, i.e., highest-addressed byte left, and lowest-addressed byte right. This is OK, and the Intel ISA does it in the same way, but I find this aspect of the little-endian notation confusing as well ;-) > >> +gdb_test_no_output "set var \$[lindex $regname 0] = $val" \ >> + "init t2, first piece" >> +gdb_test_no_output "set var \$[lindex $regname 1] = 0" \ >> + "init t2, second piece" >> +gdb_test "print/d t2" " = \\{u = , x = 0, y = -1, z = >> 0\\}" \ >> + "initialized t2 from regs" >> +gdb_test_no_output "set var t2.y = 2641" >> +gdb_test_no_output "set var t2.z = -400" >> +gdb_test_no_output "set var t2.x = 200" >> +gdb_test "print t2.x + t2.y + t2.z" " = 2441" > > Thanks, > > Simon Thanks, Andreas