From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 580 invoked by alias); 29 Sep 2014 17:35:12 -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 558 invoked by uid 89); 29 Sep 2014 17:35:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.3 required=5.0 tests=AWL,BAYES_50,UNSUBSCRIBE_BODY autolearn=no version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 29 Sep 2014 17:35:09 +0000 Received: from svr-orw-fem-05.mgc.mentorg.com ([147.34.97.43]) by relay1.mentorg.com with esmtp id 1XYeqo-0001Qy-DF from Luis_Gustavo@mentor.com ; Mon, 29 Sep 2014 10:35:06 -0700 Received: from [172.30.12.34] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.3.181.6; Mon, 29 Sep 2014 10:35:05 -0700 Message-ID: <54299847.7010202@codesourcery.com> Date: Mon, 29 Sep 2014 17:35:00 -0000 From: Luis Machado Reply-To: User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Pedro Alves , "'gdb-patches@sourceware.org'" Subject: Re: [PATCH] MIPS bit field failures in gdb.base/store.exp References: <5413534F.7000705@codesourcery.com> <541C63DF.8090206@redhat.com> <541C6A43.2000200@codesourcery.com> <54246DAE.6080208@codesourcery.com> <5425835B.609@redhat.com> <5429523F.3000706@codesourcery.com> <54295EEB.30009@gmail.com> In-Reply-To: <54295EEB.30009@gmail.com> Content-Type: multipart/mixed; boundary="------------090705050805010905090801" X-IsSubscribed: yes X-SW-Source: 2014-09/txt/msg00841.txt.bz2 --------------090705050805010905090801 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3598 On 09/29/2014 10:30 AM, Pedro Alves wrote: > On 09/29/2014 01:36 PM, Luis Machado wrote: >> Hi, >> >> On 09/26/2014 12:16 PM, Pedro Alves wrote: >>> On 09/25/2014 08:31 PM, Luis Machado wrote: >>> >>>> ping! Any ideas on different approaches suitable for this problem or is >>>> the proposed fix ok (with either passing a value struct or a bit size)? >>> >>> Sorry, it's not easy to have a quick opinion without thinking this >>> through... >>> >>> So, in value_assign, the case in question, we see: >>> >>> gdbarch = get_frame_arch (frame); >>> if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type)) >>> { >>> /* If TOVAL is a special machine register requiring >>> conversion of program values to a special raw >>> format. */ >>> gdbarch_value_to_register (gdbarch, frame, >>> VALUE_REGNUM (toval), type, >>> value_contents (fromval)); >>> } >>> >>> Notice how gdbarch_value_to_register takes the fromval's contents >>> as a buffer, only, and isn't passed down anything that would make it >>> possible to find out whether it's writing to a bitfield, so that >>> the implementation could do a read-modify-write itself and >>> write to the proper bitfield offset. >>> >>> So, it seems to me that until we find an arch that needs to handle >>> bitfields especially (I'm having trouble imagining why that >>> would be necessary), we should just change value_assign's >>> lval_register handling from: >>> >>> if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type)) >>> { >>> gdbarch_value_to_register (); >>> } >>> else >>> { >>> if (value_bitsize (toval)) >>> { >>> // read-modify-write >>> } >>> else >>> { >>> put_frame_register_bytes (); >>> } >>> } >>> >>> to: >>> >>> if (value_bitsize (toval)) >>> { >>> // read-modify-write >>> } >>> else >>> { >>> if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type)) >>> { >>> gdbarch_value_to_register (); >>> } >>> else >>> { >>> put_frame_register_bytes (); >>> } >>> } >> > >> Though a bit less generic, that also seems to be a reasonable solution >> for now, and it fixes the failures i saw for MIPS. > > The proper solution for an arch that needs to treat bitfields differently > might well be to do without gdbarch_convert_register_p and change > gdbarch_value_to_register's parameters to > 'gdbarch_value_to_register(gdbarch, toval, fromval)', and rename it > to gdbarch_register_assign while at it, as it's only called by value_assign. > > Like: > > if (gdbarch_register_assign_p (gdbarch)) > { > gdbarch_register_assign (gdbarch, toval, fromval); > } > else > { > // default fallback > } > > (Or install the fallback code as fallback gdbarch_register_assign > implementation and then just call gdbarch_register_assign directly.) > > Seems unnecessary to do until we find a user that wants to treat > bitfields differently though. Or viewed another way, we're discussing > what that "default fallback" code should look like. :-) > >> Out of the top of my >> head i also don't recall a target that handles bit fields in a special >> way. Should i go with this patch for the next submission > > Yes, please. > >> or do you want to author it? > > Nope. How about the following small patch? --------------090705050805010905090801 Content-Type: text/x-patch; name="mips_bit_field.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="mips_bit_field.diff" Content-length: 3399 2014-09-29 Luis Machado gdb/ * valops.c (value_assign): Check for bit field assignments before calling architecture-specific register value conversion functions. diff --git a/gdb/valops.c b/gdb/valops.c index e1decf0..f177907 100644 --- a/gdb/valops.c +++ b/gdb/valops.c @@ -1112,52 +1112,54 @@ value_assign (struct value *toval, struct value *fromval) error (_("Value being assigned to is no longer active.")); gdbarch = get_frame_arch (frame); - if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), type)) + + if (value_bitsize (toval)) { - /* If TOVAL is a special machine register requiring - conversion of program values to a special raw - format. */ - gdbarch_value_to_register (gdbarch, frame, - VALUE_REGNUM (toval), type, - value_contents (fromval)); + struct value *parent = value_parent (toval); + int offset = value_offset (parent) + value_offset (toval); + int changed_len; + gdb_byte buffer[sizeof (LONGEST)]; + int optim, unavail; + + changed_len = (value_bitpos (toval) + + value_bitsize (toval) + + HOST_CHAR_BIT - 1) + / HOST_CHAR_BIT; + + if (changed_len > (int) sizeof (LONGEST)) + error (_("Can't handle bitfields which " + "don't fit in a %d bit word."), + (int) sizeof (LONGEST) * HOST_CHAR_BIT); + + if (!get_frame_register_bytes (frame, value_reg, offset, + changed_len, buffer, + &optim, &unavail)) + { + if (optim) + throw_error (OPTIMIZED_OUT_ERROR, + _("value has been optimized out")); + if (unavail) + throw_error (NOT_AVAILABLE_ERROR, + _("value is not available")); + } + + modify_field (type, buffer, value_as_long (fromval), + value_bitpos (toval), value_bitsize (toval)); + + put_frame_register_bytes (frame, value_reg, offset, + changed_len, buffer); } else { - if (value_bitsize (toval)) + if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval), + type)) { - struct value *parent = value_parent (toval); - int offset = value_offset (parent) + value_offset (toval); - int changed_len; - gdb_byte buffer[sizeof (LONGEST)]; - int optim, unavail; - - changed_len = (value_bitpos (toval) - + value_bitsize (toval) - + HOST_CHAR_BIT - 1) - / HOST_CHAR_BIT; - - if (changed_len > (int) sizeof (LONGEST)) - error (_("Can't handle bitfields which " - "don't fit in a %d bit word."), - (int) sizeof (LONGEST) * HOST_CHAR_BIT); - - if (!get_frame_register_bytes (frame, value_reg, offset, - changed_len, buffer, - &optim, &unavail)) - { - if (optim) - throw_error (OPTIMIZED_OUT_ERROR, - _("value has been optimized out")); - if (unavail) - throw_error (NOT_AVAILABLE_ERROR, - _("value is not available")); - } - - modify_field (type, buffer, value_as_long (fromval), - value_bitpos (toval), value_bitsize (toval)); - - put_frame_register_bytes (frame, value_reg, offset, - changed_len, buffer); + /* If TOVAL is a special machine register requiring + conversion of program values to a special raw + format. */ + gdbarch_value_to_register (gdbarch, frame, + VALUE_REGNUM (toval), type, + value_contents (fromval)); } else { --------------090705050805010905090801--