From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 51080 invoked by alias); 3 May 2017 13:59:34 -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 51058 invoked by uid 89); 3 May 2017 13:59:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=intermediary, realize, honest, states X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 May 2017 13:59:31 +0000 Received: by simark.ca (Postfix, from userid 33) id A78711E4B5; Wed, 3 May 2017 09:59:32 -0400 (EDT) To: Andreas Arnez Subject: Re: [PATCH 5/9] Fix issues in write_pieced_value when targeting bit-fields X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Wed, 03 May 2017 13:59:00 -0000 From: Simon Marchi Cc: gdb-patches@sourceware.org In-Reply-To: References: <1491586736-21296-1-git-send-email-arnez@linux.vnet.ibm.com> <1491586736-21296-6-git-send-email-arnez@linux.vnet.ibm.com> Message-ID: <26e7caee7bba2976bb4f54fe2b87643b@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.5 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00053.txt.bz2 On 2017-04-27 13:54, Andreas Arnez wrote: > On Fri, Apr 14 2017, Simon Marchi wrote: > >> On 2017-04-07 13:38, Andreas Arnez wrote: >>> There are multiple issues in write_pieced_value when dealing with a >>> bit-field as the target value: >>> >>> (1) The number of bits preceding the bit-field is calculated without >>> considering the relative offset of the value's parent. >> >> If others are wondering when this can happen: >> >> struct s { >> uint64_t foo; >> struct { >> uint32_t bar; >> uint32_t bf : 10; >> } baz; >> }; >> >> If "val" is a struct value representing bf: >> >> - value_offset(val) == 4 (sizeof bar) >> - val->parent represents the whole baz structure >> - value_offset(val->parent) == 8 (sizeof foo) >> >> If bf was a "standard", non-bitfield variable, its offset would be 12 >> directly. > > Right. Now that you mention it, I realize that the test case doesn't > cover this yet. So I'm enhancing it. I did not realize that, but I'm glad you did :). >> >> There are multiple places that do "value_offset (parent) + >> value_offset >> (value)" when value is a bitfield. Isn't it what we want most of the >> time? If so, I wonder if eventually value_offset shouldn't instead >> return >> the computed offset, like: >> >> LONGEST >> value_offset (const struct value *value) >> { >> if (value_bitsize (value)) >> return value->offset + value_offset (value_parent (value)); >> else >> return value->offset; >> } > > Maybe, but what would set_value_offset do then? Indeed, it's probably better not to break the symmetry. > To be honest, I don't completely understand the definition of > value_offset, so I decided not to change anything there with this > patch. > Maybe we should spend some time refactoring the value API, but I think > this is a separate task. Yes, it was just brainstorming for future enhancements. >>> type_len = value_bitsize (v); >>> } >>> else >>> @@ -1796,18 +1797,11 @@ read_pieced_value (struct value *v) >>> bits_to_skip -= this_size_bits; >>> continue; >>> } >>> - if (bits_to_skip > 0) >>> - { >>> - dest_offset_bits = 0; >>> - source_offset_bits = bits_to_skip; >>> - this_size_bits -= bits_to_skip; >>> - bits_to_skip = 0; >>> - } >>> - else >>> - { >>> - dest_offset_bits = offset; >>> - source_offset_bits = 0; >>> - } >>> + source_offset_bits = bits_to_skip; >>> + this_size_bits -= bits_to_skip; >>> + bits_to_skip = 0; >>> + dest_offset_bits = offset; >>> + >> >> Is this snippet related to one of the problems you have described? It >> seems to me like it's just simplifying the code, but it's not changing >> the >> behavior. If that's the case, I'd suggest putting it in its own patch >> (along with its equivalent in write_pieced_value). > > This is just to mirror the change in write_pieced_value. See below for > the rationale of that. > >> >>> if (this_size_bits > type_len - offset) >>> this_size_bits = type_len - offset; >>> >>> @@ -1942,8 +1936,16 @@ write_pieced_value (struct value *to, struct >>> value *from) >>> bits_to_skip = 8 * value_offset (to); >>> if (value_bitsize (to)) >>> { >>> - bits_to_skip += value_bitpos (to); >>> + bits_to_skip += (8 * value_offset (value_parent (to)) >>> + + value_bitpos (to)); >>> type_len = value_bitsize (to); >>> + /* Use the least significant bits of FROM. */ >>> + if (gdbarch_byte_order (get_type_arch (value_type (from))) >>> + == BFD_ENDIAN_BIG) >>> + { >>> + offset = 8 * TYPE_LENGTH (value_type (from)) - type_len; >>> + type_len += offset; >>> + } >> >> I guess this is related to (1) and (2). > > Right. Note that 'offset' may now be nonzero upon entering the loop. > >> >>> } >>> else >>> type_len = 8 * TYPE_LENGTH (value_type (to)); >>> @@ -1962,25 +1964,19 @@ write_pieced_value (struct value *to, struct >>> value *from) >>> bits_to_skip -= this_size_bits; >>> continue; >>> } >>> - if (bits_to_skip > 0) >>> - { >>> - dest_offset_bits = bits_to_skip; >>> - source_offset_bits = 0; >>> - this_size_bits -= bits_to_skip; >>> - bits_to_skip = 0; >>> - } >>> - else >>> - { >>> - dest_offset_bits = 0; >>> - source_offset_bits = offset; >>> - } >>> + dest_offset_bits = bits_to_skip; >>> + this_size_bits -= bits_to_skip; >>> + bits_to_skip = 0; >>> + source_offset_bits = offset; >>> + > > This is related to (2). The old version assumed that 'offset' is still > zero when there are bits to skip, so a literal zero (instead of > 'offset') could be copied into source_offset_bits. This assumption > doesn't hold any longer, now that 'offset' may start with a nonzero > value. Ok, I though it was just some simplification. The new code is equivalent to both branches of the if/else that's being removed for dest_offset_bits, this_size_bits and bits_to_skip. But you're right, for source_offset_bits it changes the behaviour. In any case, I like the new code better, less branches :). > As you can see, the patch fixes 5 different, fairly independent bugs. > Thus it could have been split into 5 patches, but I found that a bit > extreme... I think it's worth splitting it*, it will help understand each issue and corresponding fix independently. It's some non-trivial work you're doing here, so think about external observers that want to take a look at it :). Patches are cheap after all. *unless it adds to much complexity to have these intermediary states. Thanks, Simon